[Bug 1371] Add PKCS#11 (Smartcards) support into OpenSSH
bugzilla-daemon at bugzilla.mindrot.org
bugzilla-daemon at bugzilla.mindrot.org
Sat Apr 26 18:47:02 EST 2008
https://bugzilla.mindrot.org/show_bug.cgi?id=1371
--- Comment #24 from Damien Miller <djm at mindrot.org> 2008-04-26 18:46:59 ---
(From update of attachment 1467)
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 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.
>+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".
--
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