Hello, PKCS#11 is a standard API interface that can be used in order to access cryptographic tokens. You can find the specification at http://www.rsasecurity.com/rsalabs/node.asp?id=2133, most smartcard and other cryptographic device vendors support PKCS#11, opensc also provides PKCS#11 interface. PKCS#11 is much more portable, standard, used standard than the current opensc implementation. The implementation is much cleaner than current smartcard support as it handles the passpharse correctly (card remove/insert), it also much easier to use as it allow adding specific keys to the agent and much more, please see: http://alon.barlev.googlepages.com/openssh-pkcs11 Many users already use this patch, with many different smartcards' providers. I believe that a security product without decent smartcard support loses much of its target. Please consider to merge. I will be glad to work with you in order to make it better and more usable. Some references: 2005-10-04: http://www.gossamer-threads.com/lists/openssh/dev/29448 2005-11-01: http://www.gossamer-threads.com/lists/openssh/dev/29599 2007-09-24: http://www.gossamer-threads.com/lists/openssh/dev/40662 In order to merge it cleanly, we should also discuss a modification for the agent protocol. As smartcards are dynamic in nature, there should be an option for the agent to ask the caller to provide information, for example "Insert token <xxx>" or "Please enter passphrase for token <xxx>". Current implementation does not modify the agent protocol but execute dialog from within the agent. Thanks!
Please attach the patches that you would like merged to this bug - it will make them easier to review and discuss.
Created attachment 1444 [details] openssh-4.7pkcs11-0.22.tar.bz2 Hello! Attached patchset. if you think it is easier to review individual patch as attachment, I will be happy to attach each patch from the tarball. Thanks!
Hello, Can I do anything to push this forward? Are there any issues I can fix? Issues I can explain? Thanks!
Could you please attach your patches *as patches*. We use bugzilla's patch review functionality to review patches. Attaching a tarball doesn't help.
Created attachment 1463 [details] 0000_README.patch [PATCH] OpenSSH and Portable OpenSSH Smartcard (PKCS#11) Support CONTENTS Patch: 1000_all_log.patch Descr: [PATCH] Add get_log_level() Status: Unrelated to PKCS#11, common with X.509 patch. Patch: 1001_all_ssh-agent-log-level.patch Descr: [PATCH] Allow ssh-agent to have more than one debug level Status: Unrelated to PKCS#11. Patch: 2000_all_pkcs11-docs.patch Descr: [PATCH] Add PKCS#11 support into OpenSSH Status: Not to be merged. Patch: 2001_all_pkcs11.patch Descr: [PATCH] Add PKCS#11 support into OpenSSH Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 2002_all_pkcs11-mans.patch Descr: [PATCH] PKCS#11 manpages updates Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 3000_ossh_pkcs11.patch Descr: [PATCH] OpenSSH PKCS#11 Additions Status: Pending to be merged into OpenSSH. Patch: 4000_possh_pkcs11.patch Descr: [PATCH] Portable OpenSSH PKCS#11 Additions Status: Pending to be merged into Portable OpenSSH. Patch: 4001_possh_pkcs11-x509.patch Descr: [PATCH] Portable OpenSSH X.509 Additions Status: Pending to be merged into Roumen Patrov's X.509 patch (after merge). Patch: 4100_possh_pkcs11-autoconf.patch Descr: [PATCH] Portable OpenSSH PKCS#11 autoconf generated files Status: Not to be merged. Patch: 4101_possh_pkcs11-x509-autoconf.patch Descr: [PATCH] Portable OpenSSH PKCS#11 X.509 autoconf generated files Status: Not to be merged. APPLY Apply to OpenSSH: for f in @@HOME@@/{1,2,3}*; do patch -p1 < $f; done Apply to Portable OpenSSH: for f in @@HOME@@/{1,2,4}*; do patch -p1 < $f; done Apply with X.509 patch: for f in @@HOME@@/{1,2,4}*; do patch -p1 < $f; done patch -R < @@HOME@@/1000_all_log.patch <apply x.509 patch>
Created attachment 1464 [details] 1000_all_log.patch [PATCH] Add get_log_level() This required so that other modules may behave according to the log level. Just noticed that the X.509 patch also have this, so this should be reverted before applying X.509 patch. Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
Created attachment 1465 [details] 1001_all_ssh-agent-log-level.patch [PATCH] Allow ssh-agent to have more than one debug level Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
Created attachment 1466 [details] 2000_all_pkcs11-docs.patch [PATCH] PKCS#11 Documentations Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
Created attachment 1467 [details] 2001_all_pkcs11.patch [PATCH] Add PKCS#11 support into OpenSSH Common for both OpenSSH and Portable OpenSSH Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
Created attachment 1468 [details] 2002_all_pkcs11-mans.patch [PATCH] PKCS#11 manpages updates Common for both OpenSSH and Portable OpenSSH Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
Created attachment 1469 [details] 3000_ossh_pkcs11.patch [PATCH] OpenSSH PKCS#11 Additions Set up for: make PKCS11=yes Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
Created attachment 1470 [details] 4000_possh_pkcs11.patch [PATCH] Portable OpenSSH PKCS#11 Additions Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
Created attachment 1471 [details] 4001_possh_pkcs11-x509.patch [PATCH] Portable OpenSSH PKCS#11 X.509 Additions Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com> This should be merged into the external X.509 patch.
Hello, Attached all patches as individual patches, sync to my patchset release 0.23. The documentation and mans are only a proposal, as I am not a native English speaker I guess it can be better. If the code is OK, I will ask for people help on the list to make documentation better. Thanks! Hoping to hear from you soon.
Hello, Any chance to set target milestone? Or even OpenSSH 4.8 as experimental feature?
4.8 is already frozen for release. Hopefully I can find some time to look at these patches once the release is done.
Created attachment 1477 [details] 2000_all_pkcs11-docs.patch Update ChangeLog.
Created attachment 1478 [details] 1001_all_ssh-agent-log-level.patch Resolve conflict with openssh-4.9 ssh-agent man page.
Hello, Just wanted to remind you guys that I am here for you... I just don't want to miss next merge window without at least some fruitful discussion. Thanks you, Alon.
Created attachment 1484 [details] 2003_all_pkcs11-scp.patch [PATCH] scp PKCS#11 parameter addition Requested by users.
Created attachment 1485 [details] 2004_all_pkcs11-scp-man.patch [PATCH] scp PKCS#11 parameter addition
Created attachment 1486 [details] 2000_all_pkcs11-docs.patch Update ChangeLog
Created attachment 1487 [details] 0000_README.patch [PATCH] OpenSSH and Portable OpenSSH Smartcard (PKCS#11) Support Update patch list.
Comment on attachment 1467 [details] 2001_all_pkcs11.patch Is there a reason behind having to add providers before adding identities? I think it would be less cumbersome to specify the provider+key in one go. The common case would be adding all the keys in a provider, right? >diff -urNp ssh.org/pkcs11.c ssh/pkcs11.c >--- ssh.org/pkcs11.c 1970-01-01 02:00:00.000000000 +0200 >+++ ssh/pkcs11.c 2008-01-09 13:26:02.000000000 +0200 >@@ -0,0 +1,944 @@ >+/* >+ * Copyright(c) 2005-2006 Alon Bar-Lev. All rights reserved. >+ * >+ * The _ssh_from_x509 is dirived of Tatu and Markus work. >+ * Copyright(c) 2006 Alon bar-Lev <alon.barlev@gmail.com>. All rights reserved. >+ * Copyright(c) 2000, 2001 Markus Friedl. All rights reserved. >+ * Copyright(c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland I don't think Tatu Ylonen ever used the BSD license. If code is derived from his sources, it is absolutely essential that his license is preserved. >+static char * >+_ssh_from_x509(X509 *); >+ >+static time_t >+__mytime(void) >+{ >+ return time(NULL); >+} >+ >+static void >+__mysleep(const unsigned long usec) >+{ >+ usleep((unsigned)usec); >+} >+ >+static int >+__mygettimeofday(struct timeval *tv) >+{ >+ return gettimeofday(tv, NULL); >+} Please do not prefix functions with underscores, declaring them static is enough. >+static void >+_pkcs11_openssh_log(void *const global_data, unsigned flags, >+ const char *const format, va_list args) >+{ >+ (void) global_data; Please do not avoid compiler warnings like this. Just add a comment "/* ARGSUSED */" before each function that does not use all its arguments. >+static PKCS11H_BOOL >+_pkcs11_ssh_pin_prompt(void *const global_data, >+ void *const user_data, const pkcs11h_token_id_t token, >+ const unsigned retry, char *const pin, const size_t pin_max) >+{ >+ char prompt[1024]; >+ char *passphrase = NULL; >+ PKCS11H_BOOL ret = FALSE; >+ >+ (void) global_data; >+ (void) user_data; >+ (void) retry; >+ >+ if (snprintf(prompt, sizeof(prompt), "Please enter PIN for token '%s': ", >+ token->display) < 0) >+ goto cleanup; >+ >+ passphrase = read_passphrase(prompt, RP_ALLOW_EOF); >+ >+ if (passphrase == NULL || strlen(passphrase) == 0 || >+ strlen(passphrase) > pin_max-1) >+ goto cleanup; >+ >+ strncpy(pin, passphrase, pin_max); Please use strlcpy here. If you need the trailing bytes of pin zeroed, then please use an explicit bzero() call. >+static int >+_pkcs11_convert_to_ssh_key(const pkcs11h_certificate_id_t certificate_id, Key **const key, >+ char **const comment, const int pin_cache_period) ... >+ if ((rv = pkcs11h_certificate_getCertificateBlob(certificate, >+ NULL, &temp)) != CKR_OK) { What is the purpose of this function call? You don't every receive a certificate blob and you don't use the blob length. Is it just to ensure that the provider is able to return a certificate? >+ if ((rv = pkcs11h_certificate_getCertificateId(certificate, >+ &certificate_id_new)) != CKR_OK) { Why duplicate the certificate ID? (Sorry if these are obvious questions, but I can't find any documentation on pkcs11-helper other than some doxygen stuff that doesn't tell much much more than the headers themselves) >+PKCS11Provider * >+pkcs11_parse_provider(const char *const info) ... >+ /* >+ * provider[:protected_authentication[:private_mode[:cert_is_private]]] >+ * string:1|0:hex:1|0 >+ */ Is this complexity needed? Under what circumstances will users need to specify more than just "provider"? >+int >+pkcs11_get_key(const PKCS11Id *const id, Key **const key, >+ char **const comment) ... >+ if (id->cert_file != NULL && id->cert_file[0] != '\x0') { Where does the cert_file come from? Is it created implicitly by the library? Does it need to be cleaned up? >+int >+pkcs11_get_keys(Key ***const keys, char ***const comments) ... >+ if((internal_keys = xmalloc((PKCS11_MAX_KEYS+1)*sizeof(*internal_keys))) == NULL) { Please use xcalloc() instead of xmalloc(x * y). >+void >+pkcs11_show_ids(void) Can some of this (perhaps just provider & DN) go into the comment, so "ssh-keygen -l" does the right thing automatically? >+static char * >+_ssh_from_x509(X509 * x509) >+{ >+#define PUT_32BIT(cp, value) ( \ >+ (cp)[0] = (unsigned char)((value) >> 24), \ >+ (cp)[1] = (unsigned char)((value) >> 16), \ >+ (cp)[2] = (unsigned char)((value) >> 8), \ >+ (cp)[3] = (unsigned char)((value) >> 0) ) Please use put_u32() in misc.[ch] if you need it at all (see below). >+ bytes_name = strlen(keyname); >+ bytes_exponent = (size_t)BN_num_bytes(pubkey->pkey.rsa->e); >+ bytes_modulus = (size_t)BN_num_bytes(pubkey->pkey.rsa->n); >+ >+ blobsize = (4 + bytes_name + 4 + ((unsigned)bytes_exponent + 1) + 4 + >+ ((unsigned)bytes_modulus + 1) + 1); >+ >+ if ((blob = (unsigned char *) xmalloc(blobsize)) == NULL) >+ goto cleanup; >+ >+ if ((buffer = (unsigned char *) xmalloc(blobsize)) == NULL) >+ goto cleanup; >+ >+ bp = blob; >+ >+ PUT_32BIT(bp, bytes_name), bp += 4; >+ memcpy(bp, keyname, bytes_name), bp += (ssize_t)bytes_name; >+ >+ BN_bn2bin(pubkey->pkey.rsa->e, buffer); >+ if (buffer[0] & 0x80) { >+ // highest bit set would indicate a negative number. >+ // to avoid this, we have to spend an extra byte: >+ PUT_32BIT(bp, bytes_exponent + 1), bp += 4; >+ *(bp++) = 0; >+ } else >+ PUT_32BIT(bp, bytes_exponent), bp += 4; >+ >+ memcpy(bp, buffer, bytes_exponent), bp += (ssize_t)bytes_exponent; >+ >+ BN_bn2bin(pubkey->pkey.rsa->n, buffer); >+ if (buffer[0] & 0x80) { >+ PUT_32BIT(bp, bytes_modulus + 1), bp += 4; >+ *(bp++) = 0; >+ } else >+ PUT_32BIT(bp, bytes_modulus), bp += 4; >+ >+ memcpy(bp, buffer, bytes_modulus), bp += (ssize_t)bytes_modulus; Please use the OpenSSH buffer.h functions here, they do all the math for you: Buffer b; buffer_init(&b); buffer_put_cstring(&b, keyname); buffer_put_bignum2(&b, pubkey->pkey.rsa->e); buffer_put_bignum2(&b, pubkey->pkey.rsa->n); if (BIO_write(bio2, buffer_ptr(&b), buffer_len(&b)) == -1) goto cleanup; >+ if ((n = BIO_read(bio, ret + (ssize_t)strlen(ret), >+ (int)(retsize - strlen(ret) - 1))) == -1) >+ goto cleanup; What are you reading back here? >diff -urNp ssh.org/pkcs11.h ssh/pkcs11.h >--- ssh.org/pkcs11.h 1970-01-01 02:00:00.000000000 +0200 >+++ ssh/pkcs11.h 2008-01-09 12:59:05.000000000 +0200 ... >+typedef struct { >+ char *provider; >+ int protected_authentication; >+ unsigned private_mode; >+ int cert_is_private; >+} PKCS11Provider; >+ >+typedef struct { >+ char *id; >+ int pin_cache_period; >+ char *cert_file; >+} PKCS11Id; Unless you need to follow some external naming convention, please use all lowercase names, e.g. "struct pkcs11_provider".
Created attachment 1488 [details] 2001_all_pkcs11.patch Updated per comments.
Thank you for reviewing this! (In reply to comment #24) > (From update of attachment 1467 [details]) > > Is there a reason behind having to add providers before adding > identities? The public key is read from the token, so it would be easier for people to use the data stored on the token. I have alternative of specifying a file containting the public key, so token is not required. > I think it would be less cumbersome to specify the > provider+key in one go. The common case would be adding all the keys in > a provider, right? I believe users would like to add only authentication keys into the agent. Adding signature keys that have some law consequences automatically would make some users fear the interface. > > >diff -urNp ssh.org/pkcs11.c ssh/pkcs11.c > >--- ssh.org/pkcs11.c 1970-01-01 02:00:00.000000000 +0200 > >+++ ssh/pkcs11.c 2008-01-09 13:26:02.000000000 +0200 > >@@ -0,0 +1,944 @@ > >+/* > >+ * Copyright(c) 2005-2006 Alon Bar-Lev. All rights reserved. > >+ * > >+ * The _ssh_from_x509 is dirived of Tatu and Markus work. > >+ * Copyright(c) 2006 Alon bar-Lev <alon.barlev@gmail.com>. All rights reserved. > >+ * Copyright(c) 2000, 2001 Markus Friedl. All rights reserved. > >+ * Copyright(c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland > > I don't think Tatu Ylonen ever used the BSD license. If code is derived > from his sources, it is absolutely essential that his license is > preserved. He did. But as per your later comments, nothing is left from his source, so I removed his copyright. > >+static char * > >+_ssh_from_x509(X509 *); > >+ > >+static time_t > >+__mytime(void) > >+{ > >+ return time(NULL); > >+} > >+ > >+static void > >+__mysleep(const unsigned long usec) > >+{ > >+ usleep((unsigned)usec); > >+} > >+ > >+static int > >+__mygettimeofday(struct timeval *tv) > >+{ > >+ return gettimeofday(tv, NULL); > >+} > > Please do not prefix functions with underscores, declaring them static > is enough. Done. > >+static void > >+_pkcs11_openssh_log(void *const global_data, unsigned flags, > >+ const char *const format, va_list args) > >+{ > >+ (void) global_data; > > Please do not avoid compiler warnings like this. Just add a comment "/* > ARGSUSED */" before each function that does not use all its arguments. Done. > >+static PKCS11H_BOOL > >+_pkcs11_ssh_pin_prompt(void *const global_data, > >+ void *const user_data, const pkcs11h_token_id_t token, > >+ const unsigned retry, char *const pin, const size_t pin_max) > >+{ > >+ char prompt[1024]; > >+ char *passphrase = NULL; > >+ PKCS11H_BOOL ret = FALSE; > >+ > >+ (void) global_data; > >+ (void) user_data; > >+ (void) retry; > >+ > >+ if (snprintf(prompt, sizeof(prompt), "Please enter PIN for token '%s': ", > >+ token->display) < 0) > >+ goto cleanup; > >+ > >+ passphrase = read_passphrase(prompt, RP_ALLOW_EOF); > >+ > >+ if (passphrase == NULL || strlen(passphrase) == 0 || > >+ strlen(passphrase) > pin_max-1) > >+ goto cleanup; > >+ > >+ strncpy(pin, passphrase, pin_max); > > Please use strlcpy here. If you need the trailing bytes of pin zeroed, > then please use an explicit bzero() call. Done. > >+static int > >+_pkcs11_convert_to_ssh_key(const pkcs11h_certificate_id_t certificate_id, Key **const key, > >+ char **const comment, const int pin_cache_period) > ... > >+ if ((rv = pkcs11h_certificate_getCertificateBlob(certificate, > >+ NULL, &temp)) != CKR_OK) { > > What is the purpose of this function call? You don't every receive a > certificate blob and you don't use the blob length. Is it just to > ensure that the provider is able to return a certificate? No, the DN is fetched at this point. So you get nice descriptive text in ssh-add -l. > >+ if ((rv = pkcs11h_certificate_getCertificateId(certificate, > >+ &certificate_id_new)) != CKR_OK) { > > Why duplicate the certificate ID? This gets the new id with descriptive name. > (Sorry if these are obvious questions, but I can't find any > documentation on pkcs11-helper other than some doxygen stuff that > doesn't tell much much more than the headers themselves) That's OK, I will be happy to improve this. BTW: There is a comment: + /* + * New certificate_id is constructed from certificate + * blob so that it will contian the proper description. + */ > >+PKCS11Provider * > >+pkcs11_parse_provider(const char *const info) > ... > >+ /* > >+ * provider[:protected_authentication[:private_mode[:cert_is_private]]] > >+ * string:1|0:hex:1|0 > >+ */ > > Is this complexity needed? Under what circumstances will users need to > specify more than just "provider"? As PKCS#11 spec may be interprated differently by different developers, there are many providers that have issues. The default values are good for 95% of best practice, the remaining 5% needs some tweeks. > >+int > >+pkcs11_get_key(const PKCS11Id *const id, Key **const key, > >+ char **const comment) > ... > >+ if (id->cert_file != NULL && id->cert_file[0] != '\x0') { > > Where does the cert_file come from? Is it created implicitly by the > library? Does it need to be cleaned up? The user can place the certificate in a file and use ssh-add -K ... -I @ID@ -1 cert_file This allows user to add key into the agent even if the token is not available at that time. It is usable during login script, adding all your identities. > >+int > >+pkcs11_get_keys(Key ***const keys, char ***const comments) > ... > >+ if((internal_keys = xmalloc((PKCS11_MAX_KEYS+1)*sizeof(*internal_keys))) == NULL) { > > Please use xcalloc() instead of xmalloc(x * y). Done. > >+void > >+pkcs11_show_ids(void) > > Can some of this (perhaps just provider & DN) go into the comment, so > "ssh-keygen -l" does the right thing automatically? Already: $ ssh-add -l 2048 86:28:63:f7:db:1b:17:7b:2c:ea:55:9b:c9:2b:02:ba /CN=Alon Bar-Lev on Alon Bar-Lev (Default) (RSA+cert) > Please use the OpenSSH buffer.h functions here, they do all the math > for you: > > Buffer b; > > buffer_init(&b); > buffer_put_cstring(&b, keyname); > buffer_put_bignum2(&b, pubkey->pkey.rsa->e); > buffer_put_bignum2(&b, pubkey->pkey.rsa->n); Took a lot of effort to clean up this function. Did not know this buffer magic. I hope now it is OK, I agree, it is much simpler! > >diff -urNp ssh.org/pkcs11.h ssh/pkcs11.h > >--- ssh.org/pkcs11.h 1970-01-01 02:00:00.000000000 +0200 > >+++ ssh/pkcs11.h 2008-01-09 12:59:05.000000000 +0200 > ... > >+typedef struct { > >+ char *provider; > >+ int protected_authentication; > >+ unsigned private_mode; > >+ int cert_is_private; > >+} PKCS11Provider; > >+ > >+typedef struct { > >+ char *id; > >+ int pin_cache_period; > >+ char *cert_file; > >+} PKCS11Id; > > Unless you need to follow some external naming convention, please use > all lowercase names, e.g. "struct pkcs11_provider". Done.
I splitted the patch to more parts to ease reviewing in this approach.
Created attachment 1489 [details] 0000_README.patch [PATCH] OpenSSH and Portable OpenSSH Smartcard (PKCS#11) Support CONTENTS Patch: 1000_all_log.patch Descr: [PATCH] Add get_log_level() Status: Unrelated to PKCS#11, common with X.509 patch. Patch: 1001_all_ssh-agent-log-level.patch Descr: [PATCH] Allow ssh-agent to have more than one debug level Status: Unrelated to PKCS#11. Patch: 2000_all_pkcs11-docs.patch Descr: [PATCH] Add PKCS#11 support into OpenSSH Status: Not to be merged. Patch: 2001_all_pkcs11-core.patch Descr: [PATCH] PKCS#11 core module Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 2003_all_pkcs11-scp.patch Descr: [PATCH] scp PKCS#11 parameter addition Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 2004_all_pkcs11-scp-man.patch Descr: [PATCH] PKCS#11 support in scp (man) Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 2005_all_pkcs11-ssh.patch Descr: [PATCH] PKCS#11 support in ssh Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 2006_all_pkcs11-ssh-man.patch Descr: [PATCH] PKCS#11 support in ssh (man) Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 2007_all_pkcs11-ssh-keygen.patch Descr: [PATCH] PKCS#11 support in ssh-keygen Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 2008_all_pkcs11-ssh-keygen-man.patch Descr: [PATCH] PKCS#11 support in ssh-keygen (man) Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 2009_all_pkcs11-agent.patch Descr: [PATCH] PKCS#11 support in agent Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 2010_all_pkcs11-agent-man.patch Descr: [PATCH] PKCS#11 support in agent (man) Status: Pending to be merged into OpenSSH and Portable OpenSSH. Patch: 3000_ossh_pkcs11.patch Descr: [PATCH] OpenSSH PKCS#11 Additions Status: Pending to be merged into OpenSSH. Patch: 4000_possh_pkcs11.patch Descr: [PATCH] Portable OpenSSH PKCS#11 Additions Status: Pending to be merged into Portable OpenSSH. Patch: 4001_possh_pkcs11-x509.patch Descr: [PATCH] Portable OpenSSH X.509 Additions Status: Pending to be merged into Roumen Patrov's X.509 patch (after merge). Patch: 4100_possh_pkcs11-autoconf.patch Descr: [PATCH] Portable OpenSSH PKCS#11 autoconf generated files Status: Not to be merged. Patch: 4101_possh_pkcs11-x509-autoconf.patch Descr: [PATCH] Portable OpenSSH PKCS#11 X.509 autoconf generated files Status: Not to be merged. APPLY Apply to OpenSSH: for f in @@HOME@@/{1,2,3}*; do patch -p1 < $f; done Apply to Portable OpenSSH: for f in @@HOME@@/{1,2,4}*; do patch -p1 < $f; done Apply with X.509 patch: for f in @@HOME@@/{1,2,4}*; do patch -p1 < $f; done patch -R < @@HOME@@/1000_all_log.patch <apply x.509 patch>
Created attachment 1490 [details] 2001_all_pkcs11-core.patch
Comment on attachment 1490 [details] 2001_all_pkcs11-core.patch [PATCH] PKCS#11 core module
Created attachment 1491 [details] 2005_all_pkcs11-ssh.patch [PATCH] PKCS#11 support in ssh
Created attachment 1492 [details] 2006_all_pkcs11-ssh-man.patch [PATCH] PKCS#11 support in ssh (man)
Created attachment 1493 [details] 2007_all_pkcs11-ssh-keygen.patch [PATCH] PKCS#11 support in ssh-keygen
Created attachment 1494 [details] 2008_all_pkcs11-ssh-keygen-man.patch [PATCH] PKCS#11 support in ssh-keygen (man)
Created attachment 1495 [details] 2009_all_pkcs11-agent.patch [PATCH] PKCS#11 support in agent
Created attachment 1496 [details] 2010_all_pkcs11-agent-man.patch [PATCH] PKCS#11 support in agent (man)
Created attachment 1497 [details] 2001_all_pkcs11-core.patch [PATCH] PKCS#11 core module Just had an idea... If you have this buffer interface, maybe you also have base64 interface, and you do! So now the ssh_from_x509() is actually readable. The pkcs11_show_ids() is opened for suggestion regarding the format of the output. Maybe you have some magic for i2a_ASN1_INTEGER() replacement.
Hello Damien, I will love to get some more work :) Regards, Alon
Hello Damien, I don't wish to bother you, I just don't want to miss next merge window. Can I cleanup the code more so that it be better and ready to be merged if you decide to do so? Thanks!
An interesting monolog I found [1]. [1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=482806
I have been thinking about this some more and am wondering whether PKCS#11 support would be better as a standalone agent. Can you think of any use-cases that this would not be able to cope with? One problem that I could think of is mixing traditional SSH keys with PKCS#11 keys. This could be solved by either adding support for such keys to the pkcs11-agent or adding support for multiple agents to ssh.
Hello, People are using smartcards without an agent. This is why I added support for agent-less PKCS#11 as parameter -#. Working in tty mode will not allow the askpass to work, although I have an ncurses askpass implementation that is working, people want to use OpenSSH without UI. I wanted to replace current smartcard implementation with standard one, without changing the way people use it. Andreas Jellinghaus was one of the people who insisted that nobody will use this unless agent-less configuration is supported. Making OpenSSH support several agents is great! People will love it, especially these who use OpenPGP smartcards and use the gnupg's scdaemon. But for this to be valid OpenSSH should provide a development environment for agents, so that it will be easy to implement and maintain an agent. For example, an agent library and headers with more or less static interface should be installed with OpenSSH. I already maintain gnupg's scdaemon replacement for PKCS#11 [1] as Werner do not agree to merge PKCS#11 into mainline. And as there is no agent library available I need to chase gnupg implementation and copy relevant parts each time. But there something to learn from gnupg... it always uses the agent, if there is none it executes one for the current session. This allows having simpler utilities and also the agent functionality without modifying the utilities. Maybe you need to do the same for OpenSSH, so that the whole private key logic will exist in one place. This and multiple agent support will allow to extend OpenSSH better. But while thinking of extending OpenSSH, a better test case for proper agent support would be to allow, for example, X.509 patch to exist as a separate agent. And maybe extend the agent interface to allow adding new authentication algorithms. Then I am sure I will be able to provide and external PKCS#11 agent implementation, as other people may provide external GSSAPI agent implementation or any other. Thanks, [1] http://gnupg-pkcs11.sourceforge.net/
Ok, supporting multiple agents is easy and I will try to get this done by the next release. Providing a support library for external agents is a little more tricky, but I think this can be accomplished relatively soon. The first steps are: 1. Documenting the agent protocol 2. Implementing a library for working with the ssh wire protocol and cryptographic primitives 3. Writing a skeleton agent that implements most of the agent protocol I had already planned to do #1 as part of my effort to document all that is undocumented in OpenSSH's implementation of the SSH protocols. I have most of #2 done already (as part of a wider project), but it still needs some work (in particular it doesn't support SSH1). #3 is as-yet unstarted, but the agent is a simple program so it wouldn't take too long once #2 is finished. I disagree that agent-only configurations are useless. The agent can communicate via X11 or other means; I think perhaps people are looking at the limited nature of the existing SSH_ASKPASS and thinking that it is all that is possible. Your ncurses UI is a great idea too.
So please at least consider the following improvement to the agent protocol: Acquire information from the user. When the agent needs passphrase or acknowledgment it will have the ability to forward it to the caller program. Two types of prompts need to be supported, string and password. This way, when an agent needs passphrase or in PKCS#11 case prompt for user to insert his token, the caller program may be used in order to prompt the user for this information. Also, please also consider adding configuration file support to the agent as well, in the same manner other OpenSSH tools use configuration files.
An alternative to providing user interaction in agent protocol is to forward $(tty) to the agent, so ncurses application can do its interaction from the origin tool terminal. What I fear is a user working in multi terminal environment, having agent running in one terminal and ssh in another terminal, the ssh is waiting for the agent, while the agent is waiting for user input at other invisible terminal. The user is not notified, and from his perspective it looks like that ssh is not responding. The current agent-less mode makes it much more simple to solve this without a change in the agent protocol. Also please expose the agent the buffer interface, and the uuencode.
As a first step, I have documented the protocol used between ssh and ssh-agent in the file PROTOCOL.agent. It will be available from: http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.agent?rev=HEAD once the mirror catches up and will be in the openssh-5.1 distribution.
I am not sure I understand what you are doing. But I do understand that I and users are going to miss another merge window. You left the current *SMARTCARD calls to the agent, while if you truly wish to provide a way for people to add new agents you need to abstract the agent, and leave only relevant required messages. Also there is the issue of providing the agent with some variables, such as active tty. I thought you are going to address this. Imaging you going to split up the ssh-agent (and ssh-add) into separate package. And review the protocol using this assumption. Calls made by ssh-add may be agent specific and should not be documented here. However, if you are going to keep current smartcard parameters in ssh command-line, the smartcard commands should be documented, but I will never be able to provide users with the same level of solution in agent only implementation. For example... Adding a new command of "set property" and add ssh configuration option "AgentProperty". Then users will be able to enter something like: ssh -o AgentProperty=smartcard-key:reader_id,pin,key_constraints host [or adding this to ssh_config] This will allow external implementation to work without modifying the protocol, for example: ssh -o AgentProperty=pkcs11-add-provider:provider host And also solve the tty issue simply as tty= attribute may automatically be set by all utilities. I think that merging PKCS#11 patches provides the best solution until the agent implementation may truly be separated.
If you are going to implement a standalone agent, then you need to understand the protocol that it will need to speak. My last message just announced that documentation for the protocol is ready. The was the first action that I described in comment #43. The goal is to enable people like yourself to write agents (other may like to, e.g. write GUI agents), not to separate ssh-agent from OpenSSH. BTW I'm not sure that I agree that you need any more message types than already exist to implement a pkcs11-agent.
Hello, I expected that part of the work of multiple agents and formalizing the protocol is reviewing the protocol in order to make it suitable for external implementations, not just documenting what you have, as this was not written in order to be external protocol. Take for example the tty issue, until agent only solution may be used in console only mode properly, there will be a need to patch the tools anyway. For agent to work properly with ssh, only the following command may be used (ssh2): SSH2_AGENTC_REQUEST_IDENTITIES (add tty field) SSH2_AGENTC_SIGN_REQUEST (add tty field) SSH2_AGENTC_REMOVE_IDENTITY SSH2_AGENTC_REMOVE_ALL_IDENTITIES SSH_AGENTC_GET_PROPERTY (new) SSH_AGENTC_SET_PROPERTY (new) All the other (add, delete) are implementation specific. All implementation specific can go into the get/set property messages. If you do this, then people may write external agents as there will be no dependencies between the client implementation and new agent features. Please review the implementation at attachment#1495 [details] and see the messages I use: SSH_AGENTC_PKCS11_ADD_PROVIDER SSH_AGENTC_PKCS11_ADD_ID SSH_AGENTC_PKCS11_REMOVE_ID The remove id may be shared, but the format of the other two is different than current messages, please tell me where I am wrong.
I don't think the protocol should be modified to accept a tty channel. The SSH agent protocol allows for forwarded operation though hosts that may not be completely trustworthy. Passing a pin though for frequent operations like listing identities or private key operations increases the likelihood that is will be exposed. Better IMO to cache the pin in the agent at the time the key is added - this is what the existing smartcard support does. Caching the pin in the agent is no additional security risk - if the agent host were compromised then an attacker could just as easily steal the pin when it was used. As for other protocol extensions - please keep it simple for now. Part of the difficulty with merging the existing pkcs#11 patch is that it touches much more than it strictly needs to. Better to start simple and add features based on clear need.
Hello, (In reply to comment #50) > I don't think the protocol should be modified to accept a tty channel. > The SSH agent protocol allows for forwarded operation though hosts that > may not be completely trustworthy. Passing a pin though for frequent > operations like listing identities or private key operations increases > the likelihood that is will be exposed. How do you propose solving the issue of console only mode without touching the client? Currently the agentless mode is the only solution for this one. > Better IMO to cache the pin in the agent at the time the key is added - > this is what the existing smartcard support does. Caching the pin in > the agent is no additional security risk - if the agent host were > compromised then an attacker could just as easily steal the pin when it > was used. Wrong. Caching smartcard PIN is none standard, unexpected and unsecure. It is part of the problem in current implementation. People implement external patches to fix this behavior [1], [2]. Smartcard usage best practice forces re-authentication after smartcard is powered off (removed and inserted), or when smartcard session duration expires. Also, implementation should allow re-authentication for each application instance/type. > As for other protocol extensions - please keep it simple for now. Part > of the difficulty with merging the existing pkcs#11 patch is that it > touches much more than it strictly needs to. Better to start simple and > add features based on clear need. I add all feature based on clear need. Hardware cryptography best practices are different than software ones. The PKCS#11 patch touches exactly the same locations of current smartcard implementation, this in order to provide full replacement and allow its removal in future. While adding support for expected behavior of re-authentication and prompt the user to insert token if needed. I will be more than happy to reduce the size of the patch! But I won't compromise on security, as the target of hardware cryptography is to improve security level of OpenSSH not provide "nice" feature to the list. [1] http://www.opensc-project.org/opensc/browser/trunk/src/openssh/README [2] http://www.opensc-project.org/opensc/browser/trunk/src/openssh/ask-for-pin.diff
(In reply to comment #51) > > Better IMO to cache the pin in the agent at the time the key is added - > > this is what the existing smartcard support does. Caching the pin in > > the agent is no additional security risk - if the agent host were > > compromised then an attacker could just as easily steal the pin when it > > was used. > > Wrong. > Caching smartcard PIN is none standard, unexpected and unsecure. It is > part of the problem in current implementation. People implement > external patches to fix this behavior [1], [2]. Can you offer a rationale for why this is insecure? I think I have given a good argument for why caching the pin gives no additional security risk, while passing it though does. > Smartcard usage best practice forces re-authentication after smartcard > is powered off (removed and inserted), or when smartcard session > duration expires. What defines a "smartcard session"? As for poweroff/removal, the cleanest way to deal with these is simply to invalidate all keys that were hosted on the card and force the user to re-add them.
(In reply to comment #52) > What defines a "smartcard session"? The provider/card derived from security constraint for key usage. For example, there are keys that may only be used once after authentication or there may be a timeout of 1 minutes for private key operations and then force re-authentication. > As for poweroff/removal, the cleanest way to deal with these is simply > to invalidate all keys that were hosted on the card and force the user > to re-add them. This is the source of the difference between hardware cryptography and software cryptography. In many cases the smartcard is also used in order to open the door to one's office. So even when you go to drink some water you have to take the smartcard with you. And if you have several computers (disconnected from each other) you need to remove the card from one computer and insert it into another to switch computers. Removing and inserting smartcard is frequent, forcing the user to take action or invalidate sessions because of it makes the complex environment to be even more difficult to handle. Just imagine that you need to re-add keys to the agent every time you return to your computer after being away from it even few steps! Specifying the PIN when you add the key into the agent will be good as long as the smartcard is not removed. It is security risk as if one find other smartcard and plug it in, he should not be able to use its resources. The behavior of dynamic "need token", "need passphrase" was successfully tested and accepted by users who use this patch, use the OpenVPN, GnuPG scd do ask passphrase correctly but fails if token is not available (but it does not have sessions), eCryptfs, QCA based (PSI, Iris and I hope soon KDE).
Well... I see I missed another merge window. And from what I understand you are not going to replace the proprietary smartcard support into standard one. Maybe I am doing something wrong. I will post a message in the mailing list requesting some help.
comment#51: Add reference of bug#608, and also bug#69 for partial vt operation.
Created attachment 1547 [details] 2005_all_pkcs11-ssh.patch [PATCH] PKCS#11 support in ssh Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
Created attachment 1557 [details] 2001_all_pkcs11-core.patch [PATCH] PKCS#11 core module Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com> Add missing uuencode.h.
Created attachment 1558 [details] 4000_possh_pkcs11.patch [PATCH] Portable OpenSSH PKCS#11 Additions Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com> Update: Error if include or lib are missing.
Hi, has there been any further progress on integrating Alon's PKCS#11 into standard OpenSSH ? I've made a donation to OpenBSD fund for this cause (in case it can help it :)
(In reply to comment #50) > Better IMO to cache the pin in the agent at the time the key is added - > this is what the existing smartcard support does. Caching the pin in > the agent is no additional security risk - if the agent host were > compromised then an attacker could just as easily steal the pin when it > was used. PIN caching (or the assumption that this is the always right thing to do) is something that usually breaks secure PIN entry (pinpads). As Alon said, caching PINs is something that requires argumentation, documentation and a really good reason in the first place. Upgrading to smart cards often translates from "we have keyfiles with passwords" to "we have smart cards with pins", which seems to nicely fit into the existing architecture/api, but totally forgets pinpad readers which is a reason they hardly ever work as expected on Linux (or OS X, or Windows ...) It has been two years since the initial post + patches, what has been/can be done to get this thing moving forward?
What is the status of this feature enhancement? Smartcard support is very important. Without hardware (smartcard reader with pinpad), there is no garantee for private certificates to stay secure. So is this not a priority for OpenSSH developers? Don't you have smartcard readers?
Support for PKCS#11 has been added by Markus: VSROOT: /cvs Module name: src Changes by: markus@cvs.openbsd.org 2010/02/08 03:50:20 Modified files: usr.bin/ssh : Makefile Makefile.inc pathnames.h readconf.c readconf.h scp.1 sftp.1 ssh-add.1 ssh-add.c ssh-agent.c ssh-keygen.1 ssh-keygen.c ssh.1 ssh.c ssh_config.5 usr.bin/ssh/lib: Makefile usr.bin/ssh/ssh-agent: Makefile Added files: usr.bin/ssh : pkcs11.h ssh-pkcs11-client.c ssh-pkcs11-helper.c ssh-pkcs11.c ssh-pkcs11.h usr.bin/ssh/ssh-pkcs11-helper: Makefile Log message: replace our obsolete smartcard code with PKCS#11. ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-11/v2-20/pkcs-11v2-20.pdf ssh(1) and ssh-keygen(1) use dlopen(3) directly to talk to a PKCS#11 provider (shared library) while ssh-agent(1) delegates PKCS#11 to a forked a ssh-pkcs11-helper process. PKCS#11 is currently a compile time option. feedback and ok djm@; inspired by patches from Alon Bar-Lev
With the release of 5.4p1, this bug is now considered closed.