[Bug 1371] Add PKCS#11 (Smartcards) support into OpenSSH

Sat Apr 26 21:29:15 EST 2008

https://bugzilla.mindrot.org/show_bug.cgi?id=1371

--- Comment #26 from Alon Bar-Lev <alon.barlev at gmail.com>  2008-04-26 21:29:11 ---

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 at gmail.com>.  All rights reserved.
> >+ * Copyright(c) 2000, 2001 Markus Friedl.  All rights reserved.
> >+ * Copyright(c) 1995 Tatu Ylonen <ylo at 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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.