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@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@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 _______________________________________________ openssh-unix-dev mailing list openssh-unix-dev@xxxxxxxxxxx https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
