Re: [PATCH v2 2/2] Add support for openssl engine based keys

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

 



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



[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux