James Bottomley
2020-Jan-30 15:24 UTC
[PATCH 1/2] Add support for openssl engine based keys
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.
To convert an existing ssh key to engine form, the ssh private key
usually has to be converted first to a form openssl can understand,
like PKCS8 and then passed 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 -o tpm2 -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.com>
---
Makefile.in | 2 +-
authfd.c | 46 ++++++++++++++++++
authfd.h | 7 +++
ssh-add.c | 41 ++++++++++++++--
ssh-agent.c | 83 ++++++++++++++++++++++++++++++++
ssh-engine.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
ssh-engine.h | 10 ++++
7 files changed, 338 insertions(+), 5 deletions(-)
create mode 100644 ssh-engine.c
create mode 100644 ssh-engine.h
diff --git a/Makefile.in b/Makefile.in
index e7549470c..aa4be9612 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 05fd45401..7cd22044e 100644
--- a/authfd.c
+++ b/authfd.c
@@ -569,6 +569,52 @@ ssh_remove_identity(int sock, struct sshkey *key)
return r;
}
+/*
+ * Add an engine based identity
+ */
+int
+ssh_add_engine_key(int sock, const char *file, const char *engine,
+ 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, engine);
+ 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;
+}
+
+
/*
* 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..1dd1070cb 100644
--- a/authfd.h
+++ b/authfd.h
@@ -38,6 +38,9 @@ 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 *engine,
+ 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 +79,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 f3b666c93..e988023a7 100644
--- a/ssh-add.c
+++ b/ssh-add.c
@@ -111,6 +111,29 @@ clear_pass(void)
}
}
+static int
+add_engine_key(int agent_fd, const char *file, const char *engine)
+{
+ int ret;
+ char *pin = NULL;
+
+ ret = ssh_add_engine_key(agent_fd, file, engine, 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, engine, 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;
+}
+
static int
delete_file(int agent_fd, const char *filename, int key_only, int qflag)
{
@@ -609,6 +632,9 @@ usage(void)
#ifdef WITH_XMSS
" [-M maxsign] [-m minleft]\n"
#endif
+#ifdef USE_OPENSSL_ENGINE
+" [-o engine]\n"
+#endif
" [file ...]\n"
" ssh-add -s pkcs11\n"
" ssh-add -e pkcs11\n"
@@ -622,7 +648,7 @@ main(int argc, char **argv)
extern char *optarg;
extern int optind;
int agent_fd;
- char *pkcs11provider = NULL, *skprovider = NULL;
+ char *pkcs11provider = NULL, *skprovider = NULL, *opensslengine = NULL;
int r, i, ch, deleting = 0, ret = 0, key_only = 0, do_download = 0;
int xflag = 0, lflag = 0, Dflag = 0, qflag = 0, Tflag = 0;
SyslogFacility log_facility = SYSLOG_FACILITY_AUTH;
@@ -653,7 +679,7 @@ main(int argc, char **argv)
skprovider = getenv("SSH_SK_PROVIDER");
- while ((ch = getopt(argc, argv, "vkKlLcdDTxXE:e:M:m:qs:S:t:")) !=
-1) {
+ while ((ch = getopt(argc, argv, "vkKlLcdDTxXE:e:M:m:qs:S:t:o:")) !=
-1) {
switch (ch) {
case 'v':
if (log_level == SYSLOG_LEVEL_INFO)
@@ -732,6 +758,9 @@ main(int argc, char **argv)
case 'T':
Tflag = 1;
break;
+ case 'o':
+ opensslengine = optarg;
+ break;
default:
usage();
ret = 1;
@@ -771,9 +800,13 @@ main(int argc, char **argv)
ret = r == 0 ? 0 : 1;
goto done;
}
- if (pkcs11provider != NULL) {
+ if (opensslengine != NULL) {
+ if (add_engine_key(agent_fd, argv[0], opensslengine) < 0)
+ ret = 1;
+ goto done;
+ } else if (pkcs11provider != NULL) {
if (update_card(agent_fd, !deleting, pkcs11provider,
- qflag) == -1)
+ qflag) == -1)
ret = 1;
goto done;
}
diff --git a/ssh-agent.c b/ssh-agent.c
index 5c9a9de60..4997b50e4 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -89,9 +89,14 @@
#include "msg.h"
#include "ssherr.h"
#include "pathnames.h"
+#include "authfile.h"
#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
@@ -640,6 +645,78 @@ no_identities(SocketEntry *e)
sshbuf_free(msg);
}
+#ifdef USE_OPENSSL_ENGINE
+static void
+process_add_engine_key(SocketEntry *e)
+{
+ char *engine, *pin, *file, *comment;
+ int r, confirm = 0;
+ u_int seconds;
+ time_t death = 0;
+ u_char type;
+ struct sshkey *k, *kp;
+ Identity *id;
+
+ if ((r = sshbuf_get_cstring(e->request, &engine, NULL)) != 0 ||
+ (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(engine, file, pin, &k)) < 0)
+ goto send;
+
+ if (sshkey_load_public(file, &kp, &comment) < 0)
+ comment = xstrdup(file);
+ else
+ sshkey_free(kp);
+
+ r = SSH_AGENT_SUCCESS;
+ if (lookup_identity(k) == NULL) {
+ id = xcalloc(1, sizeof(Identity));
+ id->key = k;
+ id->provider = xstrdup(engine);
+ 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(engine);
+ 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)
@@ -860,6 +937,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..90150c543
--- /dev/null
+++ b/ssh-engine.c
@@ -0,0 +1,154 @@
+#include "includes.h"
+
+#include <string.h>
+
+#include <openssl/engine.h>
+#include <openssl/evp.h>
+#include <openssl/ui.h>
+
+#include "log.h"
+#include "ssh-engine.h"
+#include "sshkey.h"
+#include "ssherr.h"
+#include "xmalloc.h"
+
+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;
+}
+
+int
+engine_process_add(char *engine, char *file, char *pin,
+ struct sshkey **k)
+{
+ EVP_PKEY *pk;
+ ENGINE *e;
+ struct sshkey *key;
+ int ret;
+ UI_METHOD *ui;
+ EVP_PKEY_CTX *ctx;
+ char hash[SHA256_DIGEST_LENGTH], result[1024];
+ size_t siglen;
+ struct ui_data d;
+
+ verbose("%s: add provider=%s, key=%s", __func__, engine, file);
+
+ ret = SSH_ERR_INTERNAL_ERROR;
+ e = ENGINE_by_id(engine);
+ if (!e) {
+ verbose("%s: failed to get engine %s", __func__, engine);
+ ERR_print_errors_fp(stderr);
+ return ret;
+ }
+
+ 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;
+ pk = ENGINE_load_private_key(e, file, ui, &d);
+ ENGINE_finish(e);
+
+ if (d.ret == -2)
+ return SSH_ERR_KEY_WRONG_PASSPHRASE;
+
+ 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 = SSH_ERR_ALLOC_FAIL;
+
+ key = sshkey_new(KEY_UNSPEC);
+ key->flags |= SSHKEY_FLAG_EXT;
+ if (!key)
+ goto err_free_pkey;
+
+ switch (EVP_PKEY_id(pk)) {
+ case EVP_PKEY_RSA:
+ key->type = KEY_RSA;
+ key->rsa = EVP_PKEY_get1_RSA(pk);
+ break;
+ case EVP_PKEY_DSA:
+ key->type = KEY_DSA;
+ key->dsa = EVP_PKEY_get1_DSA(pk);
+ break;
+#ifdef OPENSSL_HAS_ECC
+ case EVP_PKEY_EC:
+ key->type = KEY_ECDSA;
+ key->ecdsa = EVP_PKEY_get1_EC_KEY(pk);
+ key->ecdsa_nid = sshkey_ecdsa_key_to_nid(key->ecdsa);
+ if (key->ecdsa_nid == -1 ||
+ sshkey_curve_nid_to_name(key->ecdsa_nid) == NULL)
+ goto err_free_sshkey;
+ break;
+#endif
+ default:
+ verbose("%s: Unrecognised key type %d\n", __func__,
EVP_PKEY_id(pk));
+ ret = SSH_ERR_INVALID_FORMAT;
+ goto err_free_sshkey;
+ }
+ *k = key;
+ key = NULL;
+ ret = 1;
+ err_free_sshkey:
+ if (key)
+ sshkey_free(key);
+ err_free_pkey:
+ EVP_PKEY_free(pk);
+ verbose("%s: returning %d", __func__, ret);
+ return ret;
+}
diff --git a/ssh-engine.h b/ssh-engine.h
new file mode 100644
index 000000000..b5e85f44d
--- /dev/null
+++ b/ssh-engine.h
@@ -0,0 +1,10 @@
+#ifndef _ENGINE_H
+#define _ENGINE_H
+
+#include "sshkey.h"
+
+int
+engine_process_add(char *engine, char *file, char *pin,
+ struct sshkey **k);
+
+#endif
--
2.16.4
James Bottomley
2020-Jan-30 15:24 UTC
[PATCH 2/2] engine: add "any" engine mechanism and make it the default
If openssl cannot load the private key, chances are it's an engine key
(or a corrupt file), so try processing it via all the available
openssl engines first before concluding corruption. The reason for
doing this is to make the -o <engine> specifier optional, which means
that gnome-keyring-daemon can treat engine based openssh keys in
exactly the same way as it does ordinary ones. Not specifying the
engine does incur some overhead in finding exactly which engine
handles the key, but most installations only have a small number of
engines available, so it's not really noticeable.
Signed-off-by: James Bottomley <James.Bottomley at HansenPartnership.com>
---
ssh-add.c | 8 ++++++++
ssh-engine.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
2 files changed, 49 insertions(+), 10 deletions(-)
diff --git a/ssh-add.c b/ssh-add.c
index e988023a7..92c324740 100644
--- a/ssh-add.c
+++ b/ssh-add.c
@@ -259,6 +259,14 @@ 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) {
+ int n_r = add_engine_key(agent_fd, filename, "any");
+ if (n_r == SSH_AGENT_SUCCESS) {
+ clear_pass();
+ sshbuf_free(keyblob);
+ return 0;
+ } else if (n_r != SSH_ERR_INTERNAL_ERROR) {
+ r = n_r;
+ }
fprintf(stderr, "Error loading key \"%s\": %s\n",
filename, ssh_err(r));
goto fail_load;
diff --git a/ssh-engine.c b/ssh-engine.c
index 90150c543..b13f9e298 100644
--- a/ssh-engine.c
+++ b/ssh-engine.c
@@ -37,29 +37,23 @@ ui_read(UI *ui, UI_STRING *uis)
return d->ret;
}
-int
-engine_process_add(char *engine, char *file, char *pin,
- struct sshkey **k)
+static int
+engine_process_add_internal(ENGINE *e, char *file, char *pin,
+ struct sshkey **k)
{
EVP_PKEY *pk;
- ENGINE *e;
struct sshkey *key;
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;
- e = ENGINE_by_id(engine);
- if (!e) {
- verbose("%s: failed to get engine %s", __func__, engine);
- ERR_print_errors_fp(stderr);
- return ret;
- }
ui = UI_create_method("ssh-agent password writer");
if (!ui) {
@@ -152,3 +146,40 @@ engine_process_add(char *engine, char *file, char *pin,
verbose("%s: returning %d", __func__, ret);
return ret;
}
+
+int
+engine_process_add(char *engine, char *file, char *pin,
+ struct sshkey **k)
+{
+ ENGINE *e;
+
+ if (strcmp(engine, "any") != 0) {
+ int ret;
+
+ e = ENGINE_by_id(engine);
+ if (!e) {
+ verbose("%s: failed to get engine %s", __func__, engine);
+ ERR_print_errors_fp(stderr);
+ return SSH_ERR_INTERNAL_ERROR;
+ }
+ ret = engine_process_add_internal(e, file, pin, k);
+ ENGINE_free(e);
+ return ret;
+ }
+
+ /* this is the any engine case */
+
+ 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 == 1 || ret == SSH_ERR_KEY_WRONG_PASSPHRASE)
+ return ret;
+ }
+
+ return SSH_ERR_INTERNAL_ERROR;
+}
--
2.16.4
On Thu, 30 Jan 2020, 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.I think this is similar enough to the FIDO key support that we recently added to OpenSSH that it would be best to reuse those interfaces for these keys. FIDO keys are file based as well - the enrollment/generation process returns a "key handle" that we bundle up in a private key and that needs to be supplied when signing. Have a look at regress/misc/sk-dummy/sk-dummy.c in portable OpenSSH for a dummy version of the API that just calls out to libcrypto. -d
On Thu, 2020-01-30 at 16:24 +0100, 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.There is already tpm2-pkcs11 module which addresses the same use case in a standard way for TPM2: https://github.com/tpm2-software/tpm2-pkcs11 I do not think all the applications that want support for TPM2/engines should need to implement support for engines. Especially when the engines are to be replaced by a new providers interface in future OpenSSL releases: https://www.openssl.org/docs/OpenSSLStrategicArchitecture.html Regards, -- Jakub Jelen Senior Software Engineer Security Technologies Red Hat, Inc.
James Bottomley
2020-Feb-01 04:32 UTC
[PATCH 1/2] Add support for openssl engine based keys
On Fri, 2020-01-31 at 18:43 +0100, Jakub Jelen wrote:> On Thu, 2020-01-30 at 16:24 +0100, 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. > > There is already tpm2-pkcs11 module which addresses the same use case > in a standard way for TPM2: > > https://github.com/tpm2-software/tpm2-pkcs11Well, there's no need for anything that specific. This project can provide a pkcs11 provider for any engine: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/openssl-pkcs11-export.git I wrote it to make engines work with NSS since there are some things that don't have a pkcs11 module. But the more fundamental problem is pkcs11 is not a good interface for file based keys ... as I think you know, otherwise the openssl U2F support would be implemented via pkcs11. pkcs11 is a great interface if the key resides within the token, as it does for a lot of them because it's got a search function and a URI expression for the token (if you use p11-kit). However, when the key is file based it's not good because the URI doesn't have an expression for the file location meaning you have to add extra configuration glue to get the pkcs11 emulation to work. Essentially the glue tells the engine where to find the key files and how to export them. Fundamentally the ssh user experience is file based: you have a public and a private key file in the .ssh directory that are your identity. The engine paradigm matches that exactly, so it's far easier for consumers simply to convert the openssl private key to an engine one and use it in the same way than it is for them to construct the two layers of indirection necessary to use the key via a pkcs11 token interface. It's also consonant with our job of making security simpler for the users not more complex.> I do not think all the applications that want support for > TPM2/engines should need to implement support for engines.Well, yes, what should happen is that openssl should try all the import formats it knows about instead of having one API for PEM, one for DER and one for engines ... however, I lost that battle a long time ago.> Especially when the engines are to be replaced by a new providers > interface in future OpenSSL releases: > > https://www.openssl.org/docs/OpenSSLStrategicArchitecture.htmlA provider is simply a fancy name for an engine, there's no huge architectural difference. I and several other people who lost the "why three different file loading interfaces" battle the last time around will be nudging openssl to couple the provider and the store, so that if a provider exists, the key file gets loaded through a single API ... if that happens, the engine code in openssl can go away, assuming we win the battle this time around ... but I wouldn't rule out that the engine file API will get reborn as the provider file API and the same problem will exist. James
James Bottomley
2020-Feb-01 04:32 UTC
[PATCH 1/2] Add support for openssl engine based keys
On Fri, 2020-01-31 at 10:02 +1100, Damien Miller wrote:> On Thu, 30 Jan 2020, 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. > > I think this is similar enough to the FIDO key support that we > recently added to OpenSSH that it would be best to reuse those > interfaces for these keys. FIDO keys are file based as well - the > enrollment/generation process returns a "key handle" that we bundle > up in a private key and that needs to be supplied when signing. > > Have a look at regress/misc/sk-dummy/sk-dummy.c in portable OpenSSH > for a dummy version of the API that just calls out to libcrypto.Will do ... the U2F key file is pretty similar to the engine key file. James
James Bottomley
2020-May-25 18:47 UTC
[PATCH 1/2] Add support for openssl engine based keys
On Fri, 2020-01-31 at 10:02 +1100, Damien Miller wrote:> On Thu, 30 Jan 2020, 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. > > I think this is similar enough to the FIDO key support that we > recently added to OpenSSH that it would be best to reuse those > interfaces for these keys. FIDO keys are file based as well - the > enrollment/generation process returns a "key handle" that we bundle > up in a private key and that needs to be supplied when signing. > > Have a look at regress/misc/sk-dummy/sk-dummy.c in portable OpenSSH > for a dummy version of the API that just calls out to libcrypto.OK, I did look at this. However, engine keys and FIDO keys are completely different. An engine key is simply a regular openssl key that can only be loaded and handled by an engine. This means that most ordinary openssh keys can be converted to engine keys and the public key (and any authorized_keys entries) remains the same. FIDO keys are completely new: they require a new file format and a new public key, so you can't just seamlessly load an old private key into one. The other thing is the FIDO key does carry all the components in the keyhandle, so it can be serialised. I think the ultimate goal of engine keys should be as seamless as possible. The only thing required of openssl is to fallback to the engine API if the PEM loader fails and to add an extra SSH_ADD_AGENTC_ command for them because they can't be transmitted as public/private components as openssh otherwise assumes for standard RSA/ECDSA keys. However, this conclusion does mean that the user of ssh should never have to know if an engine is being used, so the '-o' flag is redundant. I'll redo the patch set to unify all of this into the PEM loader path. James