[PATCH v2 2/2] Add support for openssl engine based keys
James Bottomley
James.Bottomley at HansenPartnership.com
Thu Jun 11 06:52:56 AEST 2020
On Wed, 2020-06-10 at 21:23 +0200, Jakub Jelen wrote:
> Hello,
> thank you for the patch. It looks good on a fast read-through.
>
> The ssh-agent protocol modification you propose would require
> addition to the draft specification [1]. This is why I used in my
> initial implementation of PKCS#11 URI few years back the same
> messages as are used today, SSH_AGENTC_ADD_SMARTCARD_KEY, but used
> PKCS#11 URI instead of the "opaque id" as described in the specs, but
> I understand that it might be cumbersome in this case.
I can certainly propose that addition. Given that this scheme would
likely be used for the provider API beyond the engine one, should it
have a more generic name?
> Reading also through the rest of the patches, I see you added support
> for pkcs11 uris only to the ssh-agent interface. When we should
> support this, we should support also input to ssh (through command-
> line and configuration) and ssh-keygen (to get public keys from
> engine) too.
OK, I'll look at that. My use case was always taking an existing key
and moving it to an engine, but creating an engine key and using it is
also a completely valid use case even if I don't do it.
> Just out of curiosity, do the keys in agent work when after removing
> and reinserting smart card? The current implementation does not
> handle this at all [3] causing headaches for anyone interested in
> using this.
Um, I have to admit that I don't actually have any actual pkcs#11
cards. I do all my stuff using either full emulators, like softhsm2 or
a shim which can make any engine key appear as a pkcs#11 one:
https://git.kernel.org/pub/scm/linux/kernel/git/jejb/openssl-pkcs11-export.git/
However, I do believe this is solved in p11 kit which means the
solution should propagate to the engine. However, the engine does use
a global context, which the agent keeps for its lifetime so there may
be other problems.
James
> [1] https://tools.ietf.org/html/draft-miller-ssh-agent-04
> [2] https://github.com/Jakuje/openssh-portable/commits/jjelen-pkcs11
> [3] https://bugzilla.mindrot.org/show_bug.cgi?id=2890
>
> Regards,
> Jakub
>
> On Tue, 2020-06-09 at 12:36 -0700, James Bottomley wrote:
> > Engine keys are keys whose file format is understood by a specific
> > engine rather than by openssl itself. Since these keys are file
> > based, the pkcs11 interface isn't appropriate for them because they
> > don't actually represent tokens. The current most useful engine
> > for
> > openssh keys are the TPM engines, which allow all private keys to
> > be
> > stored in a form only the TPM hardware can decode, making them
> > impossible to steal. The design of engine keys is that while they
> > have to be loaded using a different openssl API, they can be used
> > as
> > standard openssl EVP_PKEYs. The only difficulty for openssh is
> > that
> > the private components can't be serialised, so a new agent command
> > is
> > introduced which interprets a message giving the location of the
> > file
> > and invokes the correct engine API from with ssh-agent.
> >
> > Since engine keys are drop in replacements for openssl keys, any
> > ssh
> > key that can be converted to openssl form can also be converted to
> > an
> > engine key. To convert an existing ssh key to engine form first
> > convert to PKCS8 and then pass that output into the engine
> > conversion
> > command. So to convert a private key stored in file rsa to TPM
> > engine
> > format, you do
> >
> > ssh-keygen -p -m PKCS8 -f rsa
> > create_tpm2_key -w rsa rsa.tpm
> >
> > Then to use the TPM key simply mv rsa.tpm rsa and openssh will be
> > able
> > to use this key using the -o option to specify the engine:
> >
> > ssh -i rsa user at host
> >
> > Note that engines usually have specific limits on the type of keys
> > they accept (so TPM 2.0 usually only does 2048 bits for RSA and
> > NIST
> > elliptic curves) so not all existing openssh keys can be converted
> > to
> > engine keys.
> >
> > Signed-off-by: James Bottomley <James.Bottomley at HansenPartnership.c
> > om
> > >
> >
> > ---
> > Makefile.in | 2 +-
> > authfd.c | 44 ++++++++++++++
> > authfd.h | 6 ++
> > ssh-add.c | 36 ++++++++++++
> > ssh-agent.c | 74 ++++++++++++++++++++++++
> > ssh-engine.c | 159
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > ssh-engine.h | 9 +++
> > 7 files changed, 329 insertions(+), 1 deletion(-)
> > create mode 100644 ssh-engine.c
> > create mode 100644 ssh-engine.h
> >
> > diff --git a/Makefile.in b/Makefile.in
> > index c9e4294d3..3260d2186 100644
> > --- a/Makefile.in
> > +++ b/Makefile.in
> > @@ -136,7 +136,7 @@ SCP_OBJS= scp.o progressmeter.o
> >
> > SSHADD_OBJS= ssh-add.o $(SKOBJS)
> >
> > -SSHAGENT_OBJS= ssh-agent.o ssh-pkcs11-client.o $(SKOBJS)
> > +SSHAGENT_OBJS= ssh-agent.o ssh-pkcs11-client.o ssh-engine.o
> > $(SKOBJS)
> >
> > SSHKEYGEN_OBJS= ssh-keygen.o sshsig.o $(SKOBJS)
> >
> > diff --git a/authfd.c b/authfd.c
> > index 4b647a628..6022a24ec 100644
> > --- a/authfd.c
> > +++ b/authfd.c
> > @@ -567,6 +567,50 @@ ssh_remove_identity(int sock, struct sshkey
> > *key)
> > return r;
> > }
> >
> > +/*
> > + * Add an engine based identity
> > + */
> > +#ifdef USE_OPENSSL_ENGINE
> > +int
> > +ssh_add_engine_key(int sock, const char *file, const char *pin,
> > + u_int lifetime, u_int confirm, u_int maxsign)
> > +{
> > + struct sshbuf *msg;
> > + int r, constrained = (lifetime || confirm);
> > + u_char type = constrained ?
> > SSH_AGENTC_ADD_ENGINE_KEY_CONSTRAINED :
> > + SSH_AGENTC_ADD_ENGINE_KEY;
> > +
> > + msg = sshbuf_new();
> > + if (!msg)
> > + return SSH_ERR_ALLOC_FAIL;
> > + r = sshbuf_put_u8(msg, type);
> > + if (r)
> > + goto out;
> > + r = sshbuf_put_cstring(msg, file);
> > + if (r)
> > + goto out;
> > + r = sshbuf_put_cstring(msg, pin);
> > + if (r)
> > + goto out;
> > + if (constrained) {
> > + r = encode_constraints(msg, lifetime, confirm,
> > maxsign,
> > NULL);
> > + if (r)
> > + goto out;
> > + }
> > + r = ssh_request_reply(sock, msg, msg);
> > + if (r)
> > + goto out;
> > + r = sshbuf_get_u8(msg, &type);
> > + if (r)
> > + goto out;
> > + r = (signed char)type;
> > + out:
> > + sshbuf_free(msg);
> > + return r;
> > +}
> > +#endif
> > +
> > +
> > /*
> > * Add/remove an token-based identity from the authentication
> > server.
> > * This call is intended only for use by ssh-add(1) and like
> > applications.
> > diff --git a/authfd.h b/authfd.h
> > index c3bf6259a..1bf42b4e6 100644
> > --- a/authfd.h
> > +++ b/authfd.h
> > @@ -38,6 +38,8 @@ int ssh_remove_identity(int sock, struct
> > sshkey
> > *key);
> > int ssh_update_card(int sock, int add, const char
> > *reader_id,
> > const char *pin, u_int life, u_int confirm);
> > int ssh_remove_all_identities(int sock, int version);
> > +int ssh_add_engine_key(int sock, const char *file, const
> > char *pin,
> > + u_int lifetime, u_int confirm, u_int
> > maxsign);
> >
> > int ssh_agent_sign(int sock, const struct sshkey *key,
> > u_char **sigp, size_t *lenp,
> > @@ -76,6 +78,10 @@ int ssh_agent_sign(int sock, const struct
> > sshkey *key,
> > #define SSH2_AGENTC_ADD_ID_CONSTRAINED 25
> > #define SSH_AGENTC_ADD_SMARTCARD_KEY_CONSTRAINED 26
> >
> > +/* engine keys */
> > +#define SSH_AGENTC_ADD_ENGINE_KEY 27
> > +#define SSH_AGENTC_ADD_ENGINE_KEY_CONSTRAINED 28
> > +
> > #define SSH_AGENT_CONSTRAIN_LIFETIME 1
> > #define SSH_AGENT_CONSTRAIN_CONFIRM 2
> > #define SSH_AGENT_CONSTRAIN_MAXSIGN 3
> > diff --git a/ssh-add.c b/ssh-add.c
> > index a40198ab5..b0ce393c7 100644
> > --- a/ssh-add.c
> > +++ b/ssh-add.c
> > @@ -110,6 +110,31 @@ clear_pass(void)
> > }
> > }
> >
> > +#ifdef USE_OPENSSL_ENGINE
> > +static int
> > +add_engine_key(int agent_fd, const char *file)
> > +{
> > + int ret;
> > + char *pin = NULL;
> > +
> > + ret = ssh_add_engine_key(agent_fd, file, NULL, lifetime,
> > confirm, maxsign);
> > + if (ret == SSH_ERR_KEY_WRONG_PASSPHRASE) {
> > + pin = read_passphrase("Enter engine key
> > passphrase:",
> > RP_ALLOW_STDIN);
> > + if (!pin)
> > + return -1;
> > + ret = ssh_add_engine_key(agent_fd, file, pin,
> > lifetime,
> > confirm, maxsign);
> > + }
> > + if (ret != SSH_AGENT_SUCCESS) {
> > + fprintf(stderr, "failed to add engine key: %s\n",
> > ssh_err(ret));
> > + } else {
> > + fprintf(stderr, "Engine Identity added: %s\n",
> > file);
> > + }
> > + if (pin)
> > + free (pin);
> > + return ret;
> > +}
> > +#endif
> > +
> > static int
> > delete_file(int agent_fd, const char *filename, int key_only, int
> > qflag)
> > {
> > @@ -235,6 +260,17 @@ add_file(int agent_fd, const char *filename,
> > int
> > key_only, int qflag,
> > /* At first, try empty passphrase */
> > if ((r = sshkey_parse_private_fileblob(keyblob, "",
> > &private,
> > &comment)) != 0 && r != SSH_ERR_KEY_WRONG_PASSPHRASE)
> > {
> > +#ifdef USE_OPENSSL_ENGINE
> > + int n_r = add_engine_key(agent_fd, filename);
> > +
> > + if (n_r == SSH_AGENT_SUCCESS) {
> > + clear_pass();
> > + sshbuf_free(keyblob);
> > + return 0;
> > + } else if (n_r != SSH_ERR_INTERNAL_ERROR) {
> > + r = n_r;
> > + }
> > +#endif
> > fprintf(stderr, "Error loading key \"%s\": %s\n",
> > filename, ssh_err(r));
> > goto fail_load;
> > diff --git a/ssh-agent.c b/ssh-agent.c
> > index e081413b8..665f7a47e 100644
> > --- a/ssh-agent.c
> > +++ b/ssh-agent.c
> > @@ -92,6 +92,10 @@
> > #include "ssh-pkcs11.h"
> > #include "sk-api.h"
> >
> > +#ifdef USE_OPENSSL_ENGINE
> > +#include "ssh-engine.h"
> > +#endif
> > +
> > #ifndef DEFAULT_PROVIDER_WHITELIST
> > # define DEFAULT_PROVIDER_WHITELIST
> > "/usr/lib*/*,/usr/local/lib*/*"
> > #endif
> > @@ -639,6 +643,70 @@ no_identities(SocketEntry *e)
> > sshbuf_free(msg);
> > }
> >
> > +#ifdef USE_OPENSSL_ENGINE
> > +static void
> > +process_add_engine_key(SocketEntry *e)
> > +{
> > + char *pin, *file, *comment;
> > + int r, confirm = 0;
> > + u_int seconds;
> > + time_t death = 0;
> > + u_char type;
> > + struct sshkey *k;
> > + Identity *id;
> > +
> > + if ((r = sshbuf_get_cstring(e->request, &file, NULL)) != 0
> > ||
> > + (r = sshbuf_get_cstring(e->request, &pin, NULL)) != 0)
> > + fatal("%s: buffer error: %s", __func__,
> > ssh_err(r));
> > +
> > + while (sshbuf_len(e->request)) {
> > + if ((r = sshbuf_get_u8(e->request, &type)) != 0)
> > + fatal("%s: buffer error: %s", __func__,
> > ssh_err(r));
> > + switch (type) {
> > + case SSH_AGENT_CONSTRAIN_LIFETIME:
> > + if ((r = sshbuf_get_u32(e->request,
> > &seconds))
> > != 0)
> > + fatal("%s: buffer error: %s",
> > + __func__, ssh_err(r));
> > + death = monotime() + seconds;
> > + break;
> > + case SSH_AGENT_CONSTRAIN_CONFIRM:
> > + confirm = 1;
> > + break;
> > + default:
> > + error("%s: Unknown constraint type %d",
> > __func__, type);
> > + goto send;
> > + }
> > + }
> > + if (lifetime && !death)
> > + death = monotime() + lifetime;
> > +
> > + if ((r = engine_process_add(file, pin, &k, &comment)) < 0)
> > + goto send;
> > +
> > + r = SSH_AGENT_SUCCESS;
> > + if (lookup_identity(k) == NULL) {
> > + id = xcalloc(1, sizeof(Identity));
> > + id->key = k;
> > + id->comment = comment;
> > + id->death = death;
> > + id->confirm = confirm;
> > + TAILQ_INSERT_TAIL(&idtab->idlist, id, next);
> > + idtab->nentries++;
> > + } else {
> > + /* key is already present, just return success */
> > + sshkey_free(k);
> > + }
> > +
> > +send:
> > + free(pin);
> > + free(file);
> > + /* open code send_status because need to return actual
> > error */
> > + if (sshbuf_put_u32(e->output, 1) != 0 ||
> > + sshbuf_put_u8(e->output, r) != 0)
> > + fatal("%s: buffer error", __func__);
> > +}
> > +#endif /* USE_OPENSSL_ENGINE */
> > +
> > #ifdef ENABLE_PKCS11
> > static void
> > process_add_smartcard_key(SocketEntry *e)
> > @@ -859,6 +927,12 @@ process_message(u_int socknum)
> > process_remove_smartcard_key(e);
> > break;
> > #endif /* ENABLE_PKCS11 */
> > +#ifdef USE_OPENSSL_ENGINE
> > + case SSH_AGENTC_ADD_ENGINE_KEY:
> > + case SSH_AGENTC_ADD_ENGINE_KEY_CONSTRAINED:
> > + process_add_engine_key(e);
> > + break;
> > +#endif /* USE_OPENSSL_ENGINE */
> > default:
> > /* Unknown message. Respond with failure. */
> > error("Unknown message %d", type);
> > diff --git a/ssh-engine.c b/ssh-engine.c
> > new file mode 100644
> > index 000000000..2263c8e7d
> > --- /dev/null
> > +++ b/ssh-engine.c
> > @@ -0,0 +1,159 @@
> > +#include "includes.h"
> > +
> > +#include <string.h>
> > +
> > +#include <openssl/engine.h>
> > +#include <openssl/evp.h>
> > +#include <openssl/ui.h>
> > +
> > +#include "authfile.h"
> > +#include "log.h"
> > +#include "ssh-engine.h"
> > +#include "sshkey.h"
> > +#include "ssherr.h"
> > +#include "xmalloc.h"
> > +
> > +#ifdef USE_OPENSSL_ENGINE
> > +
> > +struct ui_data {
> > + char *passphrase;
> > + int ret;
> > +};
> > +
> > +static int
> > +ui_read(UI *ui, UI_STRING *uis)
> > +{
> > + struct ui_data *d = UI_get0_user_data(ui);
> > + d->ret = 0;
> > +
> > + if (UI_get_string_type(uis) == UIT_PROMPT) {
> > + if (d->passphrase == NULL || d->passphrase[0] ==
> > '\0')
> > {
> > + /* we sent no passphrase but get asked for
> > one
> > + * send an interrupt event to avoid DA
> > implications */
> > + d->ret = -2;
> > + } else {
> > + UI_set_result(ui, uis, d->passphrase);
> > + d->ret = 1;
> > + }
> > + }
> > +
> > + return d->ret;
> > +}
> > +
> > +static int
> > +engine_process_add_internal(ENGINE *e, char *file, char *pin,
> > + struct sshkey **k)
> > +{
> > + EVP_PKEY *pk;
> > + int ret;
> > + UI_METHOD *ui;
> > + EVP_PKEY_CTX *ctx;
> > + char hash[SHA256_DIGEST_LENGTH], result[1024];
> > + size_t siglen;
> > + const char *engine = ENGINE_get_name(e);
> > + struct ui_data d;
> > +
> > + verbose("%s: add provider=%s, key=%s", __func__, engine,
> > file);
> > +
> > + ret = SSH_ERR_INTERNAL_ERROR;
> > +
> > + ui = UI_create_method("ssh-agent password writer");
> > + if (!ui) {
> > + verbose("%s: failed to create UI method",
> > __func__);
> > + ERR_print_errors_fp(stderr);
> > + return ret;
> > + }
> > + UI_method_set_reader(ui, ui_read);
> > +
> > + if (!ENGINE_init(e)) {
> > + verbose("%s: failed to init engine %s", __func__,
> > engine);
> > + ERR_print_errors_fp(stderr);
> > + return ret;
> > + }
> > +
> > + d.passphrase = pin;
> > + d.ret = 0;
> > + pk = ENGINE_load_private_key(e, file, ui, &d);
> > + ENGINE_finish(e);
> > +
> > + if (d.ret == -2) {
> > + ret = SSH_ERR_KEY_WRONG_PASSPHRASE;
> > + ERR_clear_error();
> > + goto err_free_pkey;
> > + }
> > +
> > + if (!pk) {
> > + verbose("%s: engine returned no key", __func__);
> > + ERR_print_errors_fp(stderr);
> > + return ret;
> > + }
> > +
> > + /* here's a nasty problem: most engines don't tell us the
> > password
> > + * was wrong until we try to use the key, so do a test to
> > see
> > */
> > + ctx = EVP_PKEY_CTX_new(pk, NULL);
> > + if (!ctx) {
> > + verbose("%s: openssl context allocation failed",
> > __func__);
> > + ERR_print_errors_fp(stderr);
> > + goto err_free_pkey;
> > + }
> > +
> > + EVP_PKEY_sign_init(ctx);
> > +
> > + siglen=sizeof(result);
> > + ret = EVP_PKEY_sign(ctx, result, &siglen, hash,
> > sizeof(hash));
> > + EVP_PKEY_CTX_free(ctx);
> > +
> > + if (ret != 1 || siglen == 0) {
> > + verbose("%s: trial signature failed with %d",
> > __func__,
> > ret);
> > + ERR_print_errors_fp(stderr);
> > + ret = SSH_ERR_KEY_WRONG_PASSPHRASE;
> > + goto err_free_pkey;
> > + }
> > +
> > + ret = sshkey_openssl_private_key(KEY_UNSPEC, pk, k, 1);
> > +
> > + err_free_pkey:
> > + EVP_PKEY_free(pk);
> > + verbose("%s: returning %d", __func__, ret);
> > + return ret;
> > +}
> > +
> > +static void
> > +engine_get_comment(char *file, char **comment)
> > +{
> > + struct sshkey *kp;
> > +
> > + if (!comment)
> > + return;
> > +
> > + if (sshkey_load_public(file, &kp, comment) < 0)
> > + *comment = xstrdup(file);
> > + else
> > + sshkey_free(kp);
> > +}
> > +
> > +int
> > +engine_process_add(char *file, char *pin, struct sshkey **k, char
> > **comment)
> > +{
> > + ENGINE *e;
> > +
> > + for (e = ENGINE_get_first(); e != NULL; e =
> > ENGINE_get_next(e))
> > {
> > + int ret;
> > +
> > + if (!ENGINE_get_load_privkey_function(e))
> > + continue;
> > +
> > + ret = engine_process_add_internal(e, file, pin,
> > k);
> > +
> > + if (ret == SSH_ERR_KEY_WRONG_PASSPHRASE) {
> > + return ret;
> > + } else if (ret == 0) {
> > + engine_get_comment(file, comment);
> > + return ret;
> > + }
> > + }
> > +
> > + return SSH_ERR_INTERNAL_ERROR;
> > +}
> > +
> > +#endif
> > diff --git a/ssh-engine.h b/ssh-engine.h
> > new file mode 100644
> > index 000000000..a1310dc90
> > --- /dev/null
> > +++ b/ssh-engine.h
> > @@ -0,0 +1,9 @@
> > +#ifndef _ENGINE_H
> > +#define _ENGINE_H
> > +
> > +#include "sshkey.h"
> > +
> > +int
> > +engine_process_add(char *file, char *pin, struct sshkey **k, char
> > **comment);
> > +
> > +#endif
More information about the openssh-unix-dev
mailing list