Re: [RFC PATCH] crypto/secret: support fetching secrets from Linux keyring

[Daniel P . Berrangé]

On Sat, Mar 28, 2020 at 02:40:14PM +0300, Alexey Krasikov wrote:
> Add the ability for the secret object to obtain secret data from the
> Linux in-kernel key managment and retention facility, as an extra option
> to the existing ones: reading from a file or passing directly as a
> string.
> 
> The secret is identified by the key serial number.  The upper layers
> need to instantiate the key and make sure the QEMU process has access
> rights to read it.

This looks like a generally useful concept to me, however, I think
it highlights a mistake in the original secrets design. We should
have had TYPE_QCRYPTO_SECRET be an abstract interface, and then
had subclasses  TYPE_QCRYPTO_SECRET_FILE, TYPE_QCRYPTO_SECRET_INLINE
Then we could add TYPE_QCRYPTO_SECRET_LINUX.

We can still mostly fix that mistake now though.

We can introduce a TYPE_QCRYPTO_SECRET_INTERFACE which defines
the generic interface. This interface just needs to define one
API entry point

    "char *get_data(QCryptoSecretInterface *secret)"

The current TYPE_QCRYPTO_SECRET can be the current impl of that
interface that returns the rawdata field.

Then we can add the new TYPE_QCRYPTO_SECRET_LINUX for the keyring
implementation that does the Linux specific magic. This will let
us easily compile out the Linux impl on platforms where it is not
available too.



> 
> Signed-off-by: Alexey Krasikov <address@hidden>
> ---
>  crypto/secret.c         | 88 +++++++++++++++++++++++++++++++++++++++--
>  include/crypto/secret.h |  3 ++
>  2 files changed, 88 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/secret.c b/crypto/secret.c
> index 1cf0ad0ce8..2e8be6241c 100644
> --- a/crypto/secret.c
> +++ b/crypto/secret.c
> @@ -19,6 +19,8 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include <asm/unistd.h>
> +#include <linux/keyctl.h>
>  #include "crypto/secret.h"
>  #include "crypto/cipher.h"
>  #include "qapi/error.h"
> @@ -28,6 +30,40 @@
>  #include "trace.h"
>  
>  
> +static inline
> +long keyctl_read(key_serial_t key, uint8_t *buffer, size_t buflen)
> +{
> +#ifdef __NR_keyctl
> +    return syscall(__NR_keyctl, KEYCTL_READ, key, buffer, buflen, 0);
> +#else
> +    errno = ENOSYS;
> +    return -1;
> +#endif
> +}
> +
> +static
> +long keyctl_read_alloc(key_serial_t key, uint8_t **buffer)
> +{
> +    uint8_t *loc_buf;
> +    long retcode = keyctl_read(key, NULL, 0);
> +    if (retcode < 0) {
> +        return retcode;
> +    }
> +    loc_buf = g_malloc(retcode + 1);
> +    retcode = keyctl_read(key, loc_buf, retcode + 1);
> +   /*
> +    * We don't have key operations locks between syscalls.
> +    * For example, the key could have been removed or expired.
> +    */
> +    if (retcode >= 0) {
> +        loc_buf[retcode] = '\0';
> +        *buffer = loc_buf;
> +    } else {
> +        g_free(loc_buf);
> +    }
> +    return retcode;
> +}
> +
>  static void
>  qcrypto_secret_load_data(QCryptoSecret *secret,
>                           uint8_t **output,
> @@ -41,10 +77,28 @@ qcrypto_secret_load_data(QCryptoSecret *secret,
>      *output = NULL;
>      *outputlen = 0;
>  
> -    if (secret->file) {
> +    if (secret->syskey) {
> +        uint8_t *buffer = NULL;
> +        long retcode;
> +        if (secret->data || secret->file) {
> +            error_setg(errp,
> +                       "'syskey', 'file' and 'data' are mutually exclusive");
> +            return;
> +        }
> +        retcode = keyctl_read_alloc(secret->syskey, &buffer);
> +        if (retcode < 0) {
> +            error_setg_errno(errp, errno,
> +                       "Unable to read serial key %08x",
> +                       secret->syskey);
> +            return;
> +        } else {
> +            *outputlen = retcode;
> +            *output = buffer;
> +        }
> +    } else if (secret->file) {
>          if (secret->data) {
>              error_setg(errp,
> -                       "'file' and 'data' are mutually exclusive");
> +                       "'syskey', 'file' and 'data' are mutually exclusive");
>              return;
>          }
>          if (!g_file_get_contents(secret->file, &data, &length, &gerr)) {
> @@ -60,7 +114,8 @@ qcrypto_secret_load_data(QCryptoSecret *secret,
>          *outputlen = strlen(secret->data);
>          *output = (uint8_t *)g_strdup(secret->data);
>      } else {
> -        error_setg(errp, "Either 'file' or 'data' must be provided");
> +        error_setg(errp,
> +                   "Either 'syskey' or 'file' or 'data' must be provided");
>      }
>  }
>  
> @@ -298,6 +353,29 @@ qcrypto_secret_prop_get_file(Object *obj,
>  }
>  
>  
> +static void
> +qcrypto_secret_prop_set_syskey(Object *obj, Visitor *v,
> +                               const char *name, void *opaque,
> +                               Error **errp)
> +{
> +    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
> +    int32_t value;
> +    visit_type_int32(v, name, &value, errp);
> +    secret->syskey = value;
> +}
> +
> +
> +static void
> +qcrypto_secret_prop_get_syskey(Object *obj, Visitor *v,
> +                               const char *name, void *opaque,
> +                               Error **errp)
> +{
> +    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
> +    int32_t value = secret->syskey;
> +    visit_type_int32(v, name, &value, errp);
> +}
> +
> +
>  static void
>  qcrypto_secret_prop_set_iv(Object *obj,
>                             const char *value,
> @@ -384,6 +462,10 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data)
>                                    qcrypto_secret_prop_get_file,
>                                    qcrypto_secret_prop_set_file,
>                                    NULL);
> +    object_class_property_add(oc, "syskey", "key_serial_t",
> +                                  qcrypto_secret_prop_get_syskey,
> +                                  qcrypto_secret_prop_set_syskey,
> +                                  NULL, NULL, NULL);
>      object_class_property_add_str(oc, "keyid",
>                                    qcrypto_secret_prop_get_keyid,
>                                    qcrypto_secret_prop_set_keyid,
> diff --git a/include/crypto/secret.h b/include/crypto/secret.h
> index 5e07e29bae..9d350e35ed 100644
> --- a/include/crypto/secret.h
> +++ b/include/crypto/secret.h
> @@ -31,6 +31,8 @@
>  typedef struct QCryptoSecret QCryptoSecret;
>  typedef struct QCryptoSecretClass QCryptoSecretClass;
>  
> +typedef int32_t key_serial_t;
> +
>  /**
>   * QCryptoSecret:
>   *
> @@ -125,6 +127,7 @@ struct QCryptoSecret {
>      QCryptoSecretFormat format;
>      char *data;
>      char *file;
> +    key_serial_t syskey;
>      char *keyid;
>      char *iv;
>  };
> -- 
> 2.17.1
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|