upstream: Extends the SK API to accept a set of key/value options

for all operations. These are intended to future-proof the API a little by
making it easier to specify additional fields for without having to change
the API version for each.

At present, only two options are defined: one to explicitly specify
the device for an operation (rather than accepting the middleware's
autoselection) and another to specify the FIDO2 username that may
be used when generating a resident key. These new options may be
invoked at key generation time via ssh-keygen -O

This also implements a suggestion from Markus to avoid "int" in favour
of uint32_t for the algorithm argument in the API, to make implementation
of ssh-sk-client/helper a little easier.

feedback, fixes and ok markus@

OpenBSD-Commit-ID: 973ce11704609022ab36abbdeb6bc23c8001eabc

1 files changed, 152 insertions, 42 deletions

diff --git a/sk-usbhid.c b/sk-usbhid.c
index 22a4c5df5..2e1573c48 100644
--- a/sk-usbhid.c
+++ b/sk-usbhid.c
@@ -54,7 +54,7 @@
         } while (0)
 #endif
 
-#define SK_VERSION_MAJOR        0x00030000 /* current API version */
+#define SK_VERSION_MAJOR        0x00040000 /* current API version */
 
 /* Flags */
 #define SK_USER_PRESENCE_REQD           0x01
@@ -91,12 +91,18 @@ struct sk_sign_response {
 };
 
 struct sk_resident_key {
-        uint8_t alg;
+        uint32_t alg;
         size_t slot;
         char *application;
         struct sk_enroll_response key;
 };
 
+struct sk_option {
+        char *name;
+        char *value;
+        uint8_t required;
+};
+
 /* If building as part of OpenSSH, then rename exported functions */
 #if !defined(SK_STANDALONE)
 #define sk_api_version          ssh_sk_api_version
@@ -109,17 +115,18 @@ struct sk_resident_key {
 uint32_t sk_api_version(void);
 
 /* Enroll a U2F key (private key generation) */
-int sk_enroll(int alg, const uint8_t *challenge, size_t challenge_len,
+int sk_enroll(uint32_t alg, const uint8_t *challenge, size_t challenge_len,
     const char *application, uint8_t flags, const char *pin,
-    struct sk_enroll_response **enroll_response);
+    struct sk_option **options, struct sk_enroll_response **enroll_response);
 
 /* Sign a challenge */
-int sk_sign(int alg, const uint8_t *message, size_t message_len,
+int sk_sign(uint32_t alg, const uint8_t *message, size_t message_len,
     const char *application, const uint8_t *key_handle, size_t key_handle_len,
-    uint8_t flags, const char *pin, struct sk_sign_response **sign_response);
+    uint8_t flags, const char *pin, struct sk_option **options,
+    struct sk_sign_response **sign_response);
 
 /* Load resident keys */
-int sk_load_resident_keys(const char *pin,
+int sk_load_resident_keys(const char *pin, struct sk_option **options,
     struct sk_resident_key ***rks, size_t *nrks);
 
 static void skdebug(const char *func, const char *fmt, ...)
@@ -235,15 +242,27 @@ try_device(fido_dev_t *dev, const uint8_t *message, size_t message_len,
 
 /* Iterate over configured devices looking for a specific key handle */
 static fido_dev_t *
-find_device(const uint8_t *message, size_t message_len, const char *application,
-    const uint8_t *key_handle, size_t key_handle_len)
+find_device(const char *path, const uint8_t *message, size_t message_len,
+    const char *application, const uint8_t *key_handle, size_t key_handle_len)
 {
         fido_dev_info_t *devlist = NULL;
         fido_dev_t *dev = NULL;
         size_t devlist_len = 0, i;
-        const char *path;
         int r;
 
+        if (path != NULL) {
+                if ((dev = fido_dev_new()) == NULL) {
+                        skdebug(__func__, "fido_dev_new failed");
+                        return NULL;
+                }
+                if ((r = fido_dev_open(dev, path)) != FIDO_OK) {
+                        skdebug(__func__, "fido_dev_open failed");
+                        fido_dev_free(&dev);
+                        return NULL;
+                }
+                return dev;
+        }
+
         if ((devlist = fido_dev_info_new(MAX_FIDO_DEVICES)) == NULL) {
                 skdebug(__func__, "fido_dev_info_new failed");
                 goto out;
@@ -402,7 +421,7 @@ pack_public_key_ed25519(const fido_cred_t *cred,
 }
 
 static int
-pack_public_key(int alg, const fido_cred_t *cred,
+pack_public_key(uint32_t alg, const fido_cred_t *cred,
     struct sk_enroll_response *response)
 {
         switch(alg) {
@@ -431,10 +450,45 @@ fidoerr_to_skerr(int fidoerr)
         }
 }
 
+static int
+check_enroll_options(struct sk_option **options, char **devicep,
+    uint8_t *user_id, size_t user_id_len)
+{
+        size_t i;
+
+        if (options == NULL)
+                return 0;
+        for (i = 0; options[i] != NULL; i++) {
+                if (strcmp(options[i]->name, "device") == 0) {
+                        if ((*devicep = strdup(options[i]->value)) == NULL) {
+                                skdebug(__func__, "strdup device failed");
+                                return -1;
+                        }
+                        skdebug(__func__, "requested device %s", *devicep);
+                } if (strcmp(options[i]->name, "user") == 0) {
+                        if (strlcpy(user_id, options[i]->value, user_id_len) >=
+                            user_id_len) {
+                                skdebug(__func__, "user too long");
+                                return -1;
+                        }
+                        skdebug(__func__, "requested user %s",
+                            (char *)user_id);
+                } else {
+                        skdebug(__func__, "requested unsupported option %s",
+                            options[i]->name);
+                        if (options[i]->required) {
+                                skdebug(__func__, "unknown required option");
+                                return -1;
+                        }
+                }
+        }
+        return 0;
+}
+
 int
-sk_enroll(int alg, const uint8_t *challenge, size_t challenge_len,
+sk_enroll(uint32_t alg, const uint8_t *challenge, size_t challenge_len,
     const char *application, uint8_t flags, const char *pin,
-    struct sk_enroll_response **enroll_response)
+    struct sk_option **options, struct sk_enroll_response **enroll_response)
 {
         fido_cred_t *cred = NULL;
         fido_dev_t *dev = NULL;
@@ -454,6 +508,11 @@ sk_enroll(int alg, const uint8_t *challenge, size_t challenge_len,
                 skdebug(__func__, "enroll_response == NULL");
                 goto out;
         }
+        memset(user_id, 0, sizeof(user_id));
+        if (check_enroll_options(options, &device,
+            user_id, sizeof(user_id)) != 0)
+                goto out; /* error already logged */
+
         *enroll_response = NULL;
         switch(alg) {
 #ifdef WITH_OPENSSL
@@ -468,7 +527,7 @@ sk_enroll(int alg, const uint8_t *challenge, size_t challenge_len,
                 skdebug(__func__, "unsupported key type %d", alg);
                 goto out;
         }
-        if ((device = pick_first_device()) == NULL) {
+        if (device == NULL && (device = pick_first_device()) == NULL) {
                 skdebug(__func__, "pick_first_device failed");
                 goto out;
         }
@@ -477,7 +536,6 @@ sk_enroll(int alg, const uint8_t *challenge, size_t challenge_len,
                 skdebug(__func__, "fido_cred_new failed");
                 goto out;
         }
-        memset(user_id, 0, sizeof(user_id));
         if ((r = fido_cred_set_type(cred, cose_alg)) != FIDO_OK) {
                 skdebug(__func__, "fido_cred_set_type: %s", fido_strerr(r));
                 goto out;
@@ -654,7 +712,8 @@ pack_sig_ed25519(fido_assert_t *assert, struct sk_sign_response *response)
 }
 
 static int
-pack_sig(int alg, fido_assert_t *assert, struct sk_sign_response *response)
+pack_sig(uint32_t  alg, fido_assert_t *assert,
+    struct sk_sign_response *response)
 {
         switch(alg) {
 #ifdef WITH_OPENSSL
@@ -668,13 +727,42 @@ pack_sig(int alg, fido_assert_t *assert, struct sk_sign_response *response)
         }
 }
 
+/* Checks sk_options for sk_sign() and sk_load_resident_keys() */
+static int
+check_sign_load_resident_options(struct sk_option **options, char **devicep)
+{
+        size_t i;
+
+        if (options == NULL)
+                return 0;
+        for (i = 0; options[i] != NULL; i++) {
+                if (strcmp(options[i]->name, "device") == 0) {
+                        if ((*devicep = strdup(options[i]->value)) == NULL) {
+                                skdebug(__func__, "strdup device failed");
+                                return -1;
+                        }
+                        skdebug(__func__, "requested device %s", *devicep);
+                } else {
+                        skdebug(__func__, "requested unsupported option %s",
+                            options[i]->name);
+                        if (options[i]->required) {
+                                skdebug(__func__, "unknown required option");
+                                return -1;
+                        }
+                }
+        }
+        return 0;
+}
+
 int
-sk_sign(int alg, const uint8_t *message, size_t message_len,
+sk_sign(uint32_t alg, const uint8_t *message, size_t message_len,
     const char *application,
     const uint8_t *key_handle, size_t key_handle_len,
-    uint8_t flags, const char *pin, struct sk_sign_response **sign_response)
+    uint8_t flags, const char *pin, struct sk_option **options,
+    struct sk_sign_response **sign_response)
 {
         fido_assert_t *assert = NULL;
+        char *device = NULL;
         fido_dev_t *dev = NULL;
         struct sk_sign_response *response = NULL;
         int ret = SSH_SK_ERR_GENERAL;
@@ -689,8 +777,10 @@ sk_sign(int alg, const uint8_t *message, size_t message_len,
                 goto out;
         }
         *sign_response = NULL;
-        if ((dev = find_device(message, message_len, application, key_handle,
-            key_handle_len)) == NULL) {
+        if (check_sign_load_resident_options(options, &device) != 0)
+                goto out; /* error already logged */
+        if ((dev = find_device(device, message, message_len,
+            application, key_handle, key_handle_len)) == NULL) {
                 skdebug(__func__, "couldn't find device for key handle");
                 goto out;
         }
@@ -737,6 +827,7 @@ sk_sign(int alg, const uint8_t *message, size_t message_len,
         response = NULL;
         ret = 0;
  out:
+        free(device);
         if (response != NULL) {
                 free(response->sig_r);
                 free(response->sig_s);
@@ -789,6 +880,7 @@ read_rks(const char *devpath, const char *pin,
                 }
                 skdebug(__func__, "get metadata for %s failed: %s",
                     devpath, fido_strerr(r));
+                ret = fidoerr_to_skerr(r);
                 goto out;
         }
         skdebug(__func__, "existing %llu, remaining %llu",
@@ -904,7 +996,7 @@ read_rks(const char *devpath, const char *pin,
 }
 
 int
-sk_load_resident_keys(const char *pin,
+sk_load_resident_keys(const char *pin, struct sk_option **options,
     struct sk_resident_key ***rksp, size_t *nrksp)
 {
         int ret = SSH_SK_ERR_GENERAL, r = -1;
@@ -912,39 +1004,57 @@ sk_load_resident_keys(const char *pin,
         size_t i, ndev = 0, nrks = 0;
         const fido_dev_info_t *di;
         struct sk_resident_key **rks = NULL;
+        char *device = NULL;
         *rksp = NULL;
         *nrksp = 0;
 
-        if ((devlist = fido_dev_info_new(MAX_FIDO_DEVICES)) == NULL) {
-                skdebug(__func__, "fido_dev_info_new failed");
-                goto out;
-        }
-        if ((r = fido_dev_info_manifest(devlist,
-            MAX_FIDO_DEVICES, &ndev)) != FIDO_OK) {
-                skdebug(__func__, "fido_dev_info_manifest failed: %s",
-                    fido_strerr(r));
-                goto out;
-        }
-        for (i = 0; i < ndev; i++) {
-                if ((di = fido_dev_info_ptr(devlist, i)) == NULL) {
-                        skdebug(__func__, "no dev info at %zu", i);
-                        continue;
-                }
-                skdebug(__func__, "trying %s", fido_dev_info_path(di));
-                if ((r = read_rks(fido_dev_info_path(di), pin,
-                    &rks, &nrks)) != 0) {
+        if (check_sign_load_resident_options(options, &device) != 0)
+                goto out; /* error already logged */
+        if (device != NULL) {
+                skdebug(__func__, "trying %s", device);
+                if ((r = read_rks(device, pin, &rks, &nrks)) != 0) {
                         skdebug(__func__, "read_rks failed for %s",
                             fido_dev_info_path(di));
-                        continue;
+                        ret = r;
+                        goto out;
+                }
+        } else {
+                /* Try all devices */
+                if ((devlist = fido_dev_info_new(MAX_FIDO_DEVICES)) == NULL) {
+                        skdebug(__func__, "fido_dev_info_new failed");
+                        goto out;
+                }
+                if ((r = fido_dev_info_manifest(devlist,
+                    MAX_FIDO_DEVICES, &ndev)) != FIDO_OK) {
+                        skdebug(__func__, "fido_dev_info_manifest failed: %s",
+                            fido_strerr(r));
+                        goto out;
+                }
+                for (i = 0; i < ndev; i++) {
+                        if ((di = fido_dev_info_ptr(devlist, i)) == NULL) {
+                                skdebug(__func__, "no dev info at %zu", i);
+                                continue;
+                        }
+                        skdebug(__func__, "trying %s", fido_dev_info_path(di));
+                        if ((r = read_rks(fido_dev_info_path(di), pin,
+                            &rks, &nrks)) != 0) {
+                                skdebug(__func__, "read_rks failed for %s",
+                                    fido_dev_info_path(di));
+                                /* remember last error */
+                                ret = r;
+                                continue;
+                        }
                 }
         }
-        /* success */
-        ret = 0;
+        /* success, unless we have no keys but a specific error */
+        if (nrks > 0 || ret == SSH_SK_ERR_GENERAL)
+                ret = 0;
         *rksp = rks;
         *nrksp = nrks;
         rks = NULL;
         nrks = 0;
  out:
+        free(device);
         for (i = 0; i < nrks; i++) {
                 free(rks[i]->application);
                 freezero(rks[i]->key.public_key, rks[i]->key.public_key_len);