Thomas Jarosch
2015-Jul-26 14:52 UTC
[PATCH] ssh-agent: Add support to load additional certificates
Add support to load additional certificates
for already loaded private keys. Useful
if the private key is on a PKCS#11 hardware token.
The private keys inside ssh-agent are now using a refcount
to share the private parts between "Identities".
The reason for this change was that the PKCS#11 code
might have redirected ("wrap") the RSA functions to a hardware token.
We don't want to mess with those internals.
Tested with an OpenGPG card. Patch developed against 6.9p
and applies to original 6.9, too.
Please CC: comments.
Signed-off-by: Thomas Jarosch <thomas.jarosch at intra2net.com>
diff -u -r -p openssh-6.9p1/ssh-add.1 openssh.cert_shadow/ssh-add.1
--- openssh-6.9p1/ssh-add.1 2015-07-01 04:35:31.000000000 +0200
+++ openssh.cert_shadow/ssh-add.1 2015-07-26 16:02:14.119312097 +0200
@@ -122,6 +122,10 @@ Remove keys provided by the PKCS#11 shar
.It Fl k
When loading keys into or deleting keys from the agent, process plain private
keys only and skip certificates.
+.It Fl p
+Load additional certificate for already loaded private key.
+Will refuse to load the certificate if no matching key is found.
+Useful if the private key is stored on a PKCS#11 hardware token.
.It Fl L
Lists public key parameters of all identities currently represented
by the agent.
diff -u -r -p openssh-6.9p1/ssh-add.c openssh.cert_shadow/ssh-add.c
--- openssh-6.9p1/ssh-add.c 2015-07-01 04:35:31.000000000 +0200
+++ openssh.cert_shadow/ssh-add.c 2015-07-26 15:58:06.513151180 +0200
@@ -180,6 +180,49 @@ delete_all(int agent_fd)
}
static int
+add_certificate_only(int agent_fd, const char *filename)
+{
+ struct sshkey *cert = NULL;
+ char *comment = NULL;
+ int r, ret = -1;
+
+ /* Load certificate */
+ if ((r = sshkey_load_public(filename, &cert, &comment)) != 0) {
+ if (r != SSH_ERR_SYSTEM_ERROR || errno != ENOENT)
+ error("Failed to load certificate \"%s\": %s",
+ filename, ssh_err(r));
+ goto out;
+ }
+ if (!sshkey_is_cert(cert)) {
+ error("Not a certificate: %s", filename);
+ goto out;
+ }
+
+ /* Add empty private key fields for serialization */
+ if ((r = sshkey_add_private(cert)) != 0)
+ goto out;
+
+ if ((r = ssh_add_identity_constrained(agent_fd, cert, comment,
+ lifetime, confirm)) != 0) {
+ error("Certificate %s (%s) add failed: %s", filename,
+ cert->cert->key_id, ssh_err(r));
+ goto out;
+ }
+ ret = 0;
+ fprintf(stderr, "Certificate added: %s (%s)\n", filename,
+ cert->cert->key_id);
+ if (lifetime != 0)
+ fprintf(stderr, "Lifetime set to %d seconds\n", lifetime);
+ if (confirm != 0)
+ fprintf(stderr, "The user must confirm each use of the key\n");
+ out:
+ free(comment);
+ sshkey_free(cert);
+
+ return ret;
+}
+
+static int
add_file(int agent_fd, const char *filename, int key_only)
{
struct sshkey *private, *cert;
@@ -445,13 +488,16 @@ lock_agent(int agent_fd, int lock)
}
static int
-do_file(int agent_fd, int deleting, int key_only, char *file)
+do_file(int agent_fd, int deleting, int key_only, int cert_only, char *file)
{
if (deleting) {
if (delete_file(agent_fd, file, key_only) == -1)
return -1;
} else {
- if (add_file(agent_fd, file, key_only) == -1)
+ if (cert_only) {
+ if (add_certificate_only(agent_fd, file) == -1)
+ return -1;
+ } else if (add_file(agent_fd, file, key_only) == -1)
return -1;
}
return 0;
@@ -466,6 +512,7 @@ usage(void)
fprintf(stderr, " -E hash Specify hash algorithm used for
fingerprints.\n");
fprintf(stderr, " -L List public key parameters of all
identities.\n");
fprintf(stderr, " -k Load only keys and not
certificates.\n");
+ fprintf(stderr, " -p Load additional certificate. Private key
must be loaded.\n");
fprintf(stderr, " -c Require confirmation to sign using
identities\n");
fprintf(stderr, " -t life Set lifetime (in seconds) when adding
identities.\n");
fprintf(stderr, " -d Delete identity.\n");
@@ -483,7 +530,7 @@ main(int argc, char **argv)
extern int optind;
int agent_fd;
char *pkcs11provider = NULL;
- int r, i, ch, deleting = 0, ret = 0, key_only = 0;
+ int r, i, ch, deleting = 0, ret = 0, key_only = 0, cert_only = 0;
int xflag = 0, lflag = 0, Dflag = 0;
/* Ensure that fds 0, 1 and 2 are open or directed to /dev/null */
@@ -511,7 +558,7 @@ main(int argc, char **argv)
exit(2);
}
- while ((ch = getopt(argc, argv, "klLcdDxXE:e:s:t:")) != -1) {
+ while ((ch = getopt(argc, argv, "kplLcdDxXE:e:s:t:")) != -1) {
switch (ch) {
case 'E':
fingerprint_hash = ssh_digest_alg_by_name(optarg);
@@ -519,8 +566,15 @@ main(int argc, char **argv)
fatal("Invalid hash algorithm \"%s\"", optarg);
break;
case 'k':
+ if (cert_only)
+ fatal("-k and -p are incompatible");
key_only = 1;
break;
+ case 'p':
+ if (key_only)
+ fatal("-k and -p are incompatible");
+ cert_only = 1;
+ break;
case 'l':
case 'L':
if (lflag != 0)
@@ -604,7 +658,7 @@ main(int argc, char **argv)
default_files[i]);
if (stat(buf, &st) < 0)
continue;
- if (do_file(agent_fd, deleting, key_only, buf) == -1)
+ if (do_file(agent_fd, deleting, key_only, cert_only, buf) == -1)
ret = 1;
else
count++;
@@ -613,7 +667,7 @@ main(int argc, char **argv)
ret = 1;
} else {
for (i = 0; i < argc; i++) {
- if (do_file(agent_fd, deleting, key_only,
+ if (do_file(agent_fd, deleting, key_only, cert_only,
argv[i]) == -1)
ret = 1;
}
diff -u -r -p openssh-6.9p1/ssh-agent.c openssh.cert_shadow/ssh-agent.c
--- openssh-6.9p1/ssh-agent.c 2015-07-01 04:35:31.000000000 +0200
+++ openssh.cert_shadow/ssh-agent.c 2015-07-26 14:59:53.733842195 +0200
@@ -112,9 +112,15 @@ typedef struct {
u_int sockets_alloc = 0;
SocketEntry *sockets = NULL;
+typedef struct refcountkey {
+ struct sshkey *key;
+ int count;
+} RefcountKey;
+
typedef struct identity {
TAILQ_ENTRY(identity) next;
- struct sshkey *key;
+ RefcountKey *idkey;
+ RefcountKey *shadowed_key;
char *comment;
char *provider;
time_t death;
@@ -188,16 +194,43 @@ idtab_lookup(int version)
return &idtable[version];
}
+static RefcountKey *
+refkey_new(struct sshkey *key)
+{
+ RefcountKey *ref = xcalloc(1, sizeof(RefcountKey));
+
+ ref->key = key;
+ ref->count = 1;
+
+ return ref;
+}
+
+static RefcountKey *
+refkey_addref(RefcountKey *refkey)
+{
+ ++refkey->count;
+ return refkey;
+}
+
+static void refkey_unref(RefcountKey *refkey)
+{
+ if (refkey && --refkey->count <= 0) {
+ sshkey_free(refkey->key);
+ free(refkey);
+ }
+}
+
static void
free_identity(Identity *id)
{
- sshkey_free(id->key);
+ refkey_unref(id->idkey);
+ refkey_unref(id->shadowed_key);
free(id->provider);
free(id->comment);
free(id);
}
-/* return matching private key for given public key */
+/* return matching Identity for given public key */
static Identity *
lookup_identity(struct sshkey *key, int version)
{
@@ -205,7 +238,22 @@ lookup_identity(struct sshkey *key, int
Idtab *tab = idtab_lookup(version);
TAILQ_FOREACH(id, &tab->idlist, next) {
- if (sshkey_equal(key, id->key))
+ if (sshkey_equal(key, id->idkey->key))
+ return (id);
+ }
+ return (NULL);
+}
+
+/* return matching private key for given public key */
+static Identity *
+lookup_identity_unshadowed_key(struct sshkey *key, int version)
+{
+ Identity *id;
+
+ Idtab *tab = idtab_lookup(version);
+ TAILQ_FOREACH(id, &tab->idlist, next) {
+ if (sshkey_equal_public(key, id->idkey->key) &&
+ id->shadowed_key == NULL)
return (id);
}
return (NULL);
@@ -218,7 +266,7 @@ confirm_key(Identity *id)
char *p;
int ret = -1;
- p = sshkey_fingerprint(id->key, fingerprint_hash, SSH_FP_DEFAULT);
+ p = sshkey_fingerprint(id->idkey->key, fingerprint_hash,
SSH_FP_DEFAULT);
if (p != NULL &&
ask_permission("Allow use of key %s?\nKey fingerprint %s.",
id->comment, p))
@@ -256,14 +304,14 @@ process_request_identities(SocketEntry *
(r = sshbuf_put_u32(msg, tab->nentries)) != 0)
fatal("%s: buffer error: %s", __func__, ssh_err(r));
TAILQ_FOREACH(id, &tab->idlist, next) {
- if (id->key->type == KEY_RSA1) {
+ if (id->idkey->key->type == KEY_RSA1) {
#ifdef WITH_SSH1
if ((r = sshbuf_put_u32(msg,
- BN_num_bits(id->key->rsa->n))) != 0 ||
+ BN_num_bits(id->idkey->key->rsa->n))) != 0 ||
(r = sshbuf_put_bignum1(msg,
- id->key->rsa->e)) != 0 ||
+ id->idkey->key->rsa->e)) != 0 ||
(r = sshbuf_put_bignum1(msg,
- id->key->rsa->n)) != 0)
+ id->idkey->key->rsa->n)) != 0)
fatal("%s: buffer error: %s",
__func__, ssh_err(r));
#endif
@@ -271,7 +319,7 @@ process_request_identities(SocketEntry *
u_char *blob;
size_t blen;
- if ((r = sshkey_to_blob(id->key, &blob, &blen)) != 0) {
+ if ((r = sshkey_to_blob(id->idkey->key, &blob, &blen)) != 0) {
error("%s: sshkey_to_blob: %s", __func__,
ssh_err(r));
continue;
@@ -327,7 +375,7 @@ process_authentication_challenge1(Socket
id = lookup_identity(key, 1);
if (id != NULL && (!id->confirm || confirm_key(id) == 0)) {
- struct sshkey *private = id->key;
+ struct sshkey *private = id->idkey->key;
/* Decrypt the challenge using the private key. */
if ((r = rsa_private_decrypt(challenge, challenge,
private->rsa) != 0)) {
@@ -380,7 +428,7 @@ process_sign_request2(SocketEntry *e)
u_int compat = 0, flags;
int r, ok = -1;
struct sshbuf *msg;
- struct sshkey *key;
+ struct sshkey *key, *sign_key;
struct identity *id;
if ((msg = sshbuf_new()) == NULL)
@@ -403,7 +451,12 @@ process_sign_request2(SocketEntry *e)
verbose("%s: user refused key", __func__);
goto send;
}
- if ((r = sshkey_sign(id->key, &signature, &slen,
+
+ if (id->shadowed_key)
+ sign_key = id->shadowed_key->key;
+ else
+ sign_key = id->idkey->key;
+ if ((r = sshkey_sign(sign_key, &signature, &slen,
data, dlen, compat)) != 0) {
error("%s: sshkey_sign: %s", __func__, ssh_err(ok));
goto send;
@@ -643,12 +696,38 @@ process_add_identity(SocketEntry *e, int
}
}
- success = 1;
if (lifetime && !death)
death = monotime() + lifetime;
+
+ /* handle additional certificates for an existing private key */
+ if (!sshkey_is_private(k)) {
+ id = lookup_identity_unshadowed_key(k, version);
+ /* ensure we have a private key and this cert is new */
+ if (id != NULL && lookup_identity(k, version) == NULL) {
+ Identity *certid = xcalloc(1, sizeof(Identity));
+ certid->idkey = refkey_new(k);
+ certid->shadowed_key = refkey_addref(id->idkey);
+ if (id->provider)
+ certid->provider = xstrdup(id->provider);
+ if (id->comment)
+ certid->comment = xstrdup(id->comment); /* XXX */
+ certid->death = death;
+ certid->confirm = confirm | id->confirm;
+
+ TAILQ_INSERT_TAIL(&tab->idlist, certid, next);
+ tab->nentries++;
+ success = 1;
+ } else
+ sshkey_free(k);
+
+ free(comment);
+ goto send;
+ }
+
+ success = 1;
if ((id = lookup_identity(k, version)) == NULL) {
id = xcalloc(1, sizeof(Identity));
- id->key = k;
+ id->idkey = refkey_new(k);
TAILQ_INSERT_TAIL(&tab->idlist, id, next);
/* Increment the number of identities. */
tab->nentries++;
@@ -774,7 +853,7 @@ process_add_smartcard_key(SocketEntry *e
tab = idtab_lookup(version);
if (lookup_identity(k, version) == NULL) {
id = xcalloc(1, sizeof(Identity));
- id->key = k;
+ id->idkey = refkey_new(k);
id->provider = xstrdup(provider);
id->comment = xstrdup(provider); /* XXX */
id->death = death;
diff -u -r -p openssh-6.9p1/sshkey.c openssh.cert_shadow/sshkey.c
--- openssh-6.9p1/sshkey.c 2015-07-01 04:35:31.000000000 +0200
+++ openssh.cert_shadow/sshkey.c 2015-07-26 13:55:40.978410299 +0200
@@ -324,6 +324,48 @@ sshkey_is_cert(const struct sshkey *k)
return sshkey_type_is_cert(k->type);
}
+/* TODO: Please review carefully */
+int
+sshkey_is_private(const struct sshkey *k)
+{
+ switch (k->type) {
+#ifdef WITH_OPENSSL
+ case KEY_RSA1:
+ case KEY_RSA:
+ case KEY_RSA_CERT_V00:
+ case KEY_RSA_CERT:
+ if (k->rsa && k->rsa->d && k->rsa->q
&& k->rsa->p &&
+ k->rsa->iqmp &&
+ !BN_is_zero(k->rsa->d) &&
+ !BN_is_zero(k->rsa->q) &&
+ !BN_is_zero(k->rsa->p) &&
+ !BN_is_zero(k->rsa->iqmp))
+ return 1;
+ break;
+ case KEY_DSA:
+ case KEY_DSA_CERT_V00:
+ case KEY_DSA_CERT:
+ if (k->dsa && k->dsa->priv_key)
+ return 1;
+ break;
+ case KEY_ECDSA:
+ case KEY_ECDSA_CERT:
+ if (k->ecdsa && EC_KEY_get0_private_key(k->ecdsa))
+ return 1;
+ break;
+#endif /* WITH_OPENSSL */
+ case KEY_ED25519:
+ case KEY_ED25519_CERT:
+ if (k->ed25519_sk)
+ return 1;
+ break;
+ case KEY_UNSPEC:
+ break;
+ }
+
+ return 0;
+}
+
/* Return the cert-less equivalent to a certified key type */
int
sshkey_type_plain(int type)
diff -u -r -p openssh-6.9p1/sshkey.h openssh.cert_shadow/sshkey.h
--- openssh-6.9p1/sshkey.h 2015-07-01 04:35:31.000000000 +0200
+++ openssh.cert_shadow/sshkey.h 2015-07-26 11:15:33.344024398 +0200
@@ -135,6 +135,7 @@ int sshkey_generate(int type, u_int bi
int sshkey_from_private(const struct sshkey *, struct sshkey **);
int sshkey_type_from_name(const char *);
int sshkey_is_cert(const struct sshkey *);
+int sshkey_is_private(const struct sshkey *);
int sshkey_type_is_cert(int);
int sshkey_type_plain(int);
int sshkey_to_certified(struct sshkey *, int);
Thomas Jarosch
2015-Aug-13 16:04 UTC
[PATCH] ssh-agent: Add support to load additional certificates
Hi, On Sunday, 26. July 2015 16:52:18 you wrote:> Add support to load additional certificates > for already loaded private keys. Useful > if the private key is on a PKCS#11 hardware token. > > The private keys inside ssh-agent are now using a refcount > to share the private parts between "Identities". > The reason for this change was that the PKCS#11 code > might have redirected ("wrap") the RSA functions to a hardware token. > We don't want to mess with those internals. > > Tested with an OpenGPG card. Patch developed against 6.9p > and applies to original 6.9, too. > > Please CC: comments. > > Signed-off-by: Thomas Jarosch <thomas.jarosch at intra2net.com>any comment on this? Is the concept sound or did I take the wrong turn here? If upstream considers this the way to go, I can try to split up the patch into smaller pieces like this: - sshkey.c: Add "int sshkey_is_private(const struct sshkey *)" function - ssh-agent: Transition to private key refcounting - ssh-agent: Implement private key "shadowing" - ssh-add: Add support to add plain certificates Thanks in advance, Thomas
Damien Miller
2015-Aug-17 00:33 UTC
[PATCH] ssh-agent: Add support to load additional certificates
Hi, This seems like a resonable idea. Could you please attach this to a bug at https://bugzilla.mindrot.org/ ? This will ensure it won't get lost. On Thu, 13 Aug 2015, Thomas Jarosch wrote:> Hi, > > On Sunday, 26. July 2015 16:52:18 you wrote: > > Add support to load additional certificates > > for already loaded private keys. Useful > > if the private key is on a PKCS#11 hardware token. > > > > The private keys inside ssh-agent are now using a refcount > > to share the private parts between "Identities". > > The reason for this change was that the PKCS#11 code > > might have redirected ("wrap") the RSA functions to a hardware token. > > We don't want to mess with those internals. > > > > Tested with an OpenGPG card. Patch developed against 6.9p > > and applies to original 6.9, too. > > > > Please CC: comments. > > > > Signed-off-by: Thomas Jarosch <thomas.jarosch at intra2net.com> > > any comment on this? > > Is the concept sound or did I take the wrong turn here? > > If upstream considers this the way to go, I can try > to split up the patch into smaller pieces like this: > > - sshkey.c: Add "int sshkey_is_private(const struct sshkey *)" function > - ssh-agent: Transition to private key refcounting > - ssh-agent: Implement private key "shadowing" > - ssh-add: Add support to add plain certificates > > Thanks in advance, > Thomas > > _______________________________________________ > openssh-unix-dev mailing list > openssh-unix-dev at mindrot.org > https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev >