[Bug 1371] Add PKCS#11 (Smartcards) support into OpenSSH
bugzilla-daemon at bugzilla.mindrot.org
bugzilla-daemon at bugzilla.mindrot.org
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.
More information about the openssh-bugs
mailing list