[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v2 4/5] crypto/linux_keyring: add 'syskey' secret object.
From: |
Daniel P . Berrangé |
Subject: |
Re: [RFC PATCH v2 4/5] crypto/linux_keyring: add 'syskey' secret object. |
Date: |
Wed, 22 Apr 2020 16:46:30 +0100 |
User-agent: |
Mutt/1.13.3 (2020-01-12) |
On Thu, Apr 16, 2020 at 01:25:24AM +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.
>
> Signed-off-by: Alexey Krasikov <address@hidden>
> ---
> crypto/Makefile.objs | 1 +
> crypto/linux_keyring.c | 140 +++++++++++++++++++++++++++++++++
> include/crypto/linux_keyring.h | 38 +++++++++
> 3 files changed, 179 insertions(+)
> create mode 100644 crypto/linux_keyring.c
> create mode 100644 include/crypto/linux_keyring.h
Can we call these secret_keyring.{c,h}
> diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
> index 3ae0dfd1a4..7fc354a8d5 100644
> --- a/crypto/Makefile.objs
> +++ b/crypto/Makefile.objs
> @@ -19,6 +19,7 @@ crypto-obj-y += tlscredspsk.o
> crypto-obj-y += tlscredsx509.o
> crypto-obj-y += tlssession.o
> crypto-obj-y += secret_interface.o
> +crypto-obj-y += linux_keyring.o
I'd expect to see a configure check for deciding whether or not
to build the keyring code, and then have $(CONFIG_SECRET_KEYRING)
variable used here.
> crypto-obj-y += secret.o
> crypto-obj-y += pbkdf.o
> crypto-obj-$(CONFIG_NETTLE) += pbkdf-nettle.o
> diff --git a/crypto/linux_keyring.c b/crypto/linux_keyring.c
> new file mode 100644
> index 0000000000..7950d4c12d
> --- /dev/null
> +++ b/crypto/linux_keyring.c
> @@ -0,0 +1,140 @@
> +#ifdef __NR_keyctl
> +
> +#include "qemu/osdep.h"
> +#include <asm/unistd.h>
> +#include <linux/keyctl.h>
> +#include "qapi/error.h"
> +#include "qom/object_interfaces.h"
> +#include "trace.h"
> +#include "crypto/linux_keyring.h"
> +
> +
> +static inline
> +long keyctl_read(key_serial_t key, uint8_t *buffer, size_t buflen)
> +{
> + return syscall(__NR_keyctl, KEYCTL_READ, key, buffer, buflen, 0);
> +}
> +
> +
> +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);
We generally prefer g_new0 to guarantee zero-initialization
> + retcode = keyctl_read(key, loc_buf, retcode);
> +
> + if (retcode >= 0) {
> + *buffer = loc_buf;
> + } else {
> + g_free(loc_buf);
> + }
> + return retcode;
> +}
> +
> +
> +static void
> +qcrypto_secret_linux_load_data(Object *obj,
> + uint8_t **output,
> + size_t *outputlen,
> + Error **errp)
> +{
> + QCryptoSecretLinuxKeyring *secret = QCRYPTO_SECRET_LINUX_KEYRING(obj);
> + uint8_t *buffer = NULL;
> + long retcode;
> +
> + *output = NULL;
> + *outputlen = 0;
> +
> + if (secret->serial) {
> + retcode = keyctl_read_alloc(secret->serial, &buffer);
> + if (retcode < 0) {
> + error_setg_errno(errp, errno,
> + "Unable to read serial key %08x",
> + secret->serial);
> + return;
> + } else {
> + *outputlen = retcode;
> + *output = buffer;
> + }
IMHO, we should just be passing outputlen & output straight
into keyctl_read_alloc, except then I think keyctl_read_alloc
is pointless. So just inline its logic into this method.
> + } else {
> + error_setg(errp, "Either 'serial' must be provided");
Indent is a bit off, and the error message text doesn't make sense
Just use
"'serial' parameter must be provided"
> + }
> +}
> +
> +
> +static void
> +qcrypto_secret_prop_set_key(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + QCryptoSecretLinuxKeyring *secret = QCRYPTO_SECRET_LINUX_KEYRING(obj);
> + int32_t value;
> + visit_type_int32(v, name, &value, errp);
> + if (!value) {
> + error_setg(errp, "The 'serial' should be not equal 0");
> + }
> + secret->serial = value;
> +}
> +
> +
> +static void
> +qcrypto_secret_prop_get_key(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + QCryptoSecretLinuxKeyring *secret = QCRYPTO_SECRET_LINUX_KEYRING(obj);
> + int32_t value = secret->serial;
> + visit_type_int32(v, name, &value, errp);
> +}
> +
> +
> +static void
> +qcrypto_secret_linux_complete(UserCreatable *uc, Error **errp)
> +{
> + object_property_set_bool(OBJECT(uc), true, "loaded", errp);
> +}
> +
> +
> +static void
> +qcrypto_secret_linux_class_init(ObjectClass *oc, void *data)
> +{
> + QCryptoSecretCommonClass *sic = QCRYPTO_SECRET_COMMON_CLASS(oc);
> + sic->load_data = qcrypto_secret_linux_load_data;
> +
> + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> + ucc->complete = qcrypto_secret_linux_complete;
> +
> + object_class_property_add(oc, "serial", "key_serial_t",
> + qcrypto_secret_prop_get_key,
> + qcrypto_secret_prop_set_key,
> + NULL, NULL, NULL);
> +}
> +
> +
> +static const TypeInfo qcrypto_secret_info = {
> + .parent = TYPE_QCRYPTO_SECRET_COMMON,
> + .name = TYPE_QCRYPTO_SECRET_LINUX_KEYRING,
> + .instance_size = sizeof(QCryptoSecretLinuxKeyring),
> + .class_size = sizeof(QCryptoSecretLinuxKeyringClass),
> + .class_init = qcrypto_secret_linux_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { TYPE_USER_CREATABLE },
> + { }
> + }
> +};
> +
> +
> +static void
> +qcrypto_secret_register_types(void)
> +{
> + type_register_static(&qcrypto_secret_info);
> +}
> +
> +
> +type_init(qcrypto_secret_register_types);
> +
> +#endif /* __NR_keyctl */
> diff --git a/include/crypto/linux_keyring.h b/include/crypto/linux_keyring.h
> new file mode 100644
> index 0000000000..2618b34444
> --- /dev/null
> +++ b/include/crypto/linux_keyring.h
> @@ -0,0 +1,38 @@
> +#ifndef QCRYPTO_SECRET_LINUX_KEYRING_H
> +#define QCRYPTO_SECRET_LINUX_KEYRING_H
> +
> +#ifdef __NR_keyctl
> +
> +#include "qapi/qapi-types-crypto.h"
> +#include "qom/object.h"
> +#include "crypto/secret_interface.h"
> +
> +#define TYPE_QCRYPTO_SECRET_LINUX_KEYRING "syskey"
> +#define QCRYPTO_SECRET_LINUX_KEYRING(obj) \
> + OBJECT_CHECK(QCryptoSecretLinuxKeyring, (obj), \
> + TYPE_QCRYPTO_SECRET_LINUX_KEYRING)
> +#define QCRYPTO_SECRET_LINUX_KEYRING_CLASS(class) \
> + OBJECT_CLASS_CHECK(QCryptoSecretLinuxKeyringClass, \
> + (class), TYPE_QCRYPTO_SECRET_LINUX_KEYRING)
> +#define QCRYPTO_SECRET_LINUX_KEYRING_GET_CLASS(class) \
> + OBJECT_GET_CLASS(QCryptoSecretLinuxKeyringClass, \
> + (class), TYPE_QCRYPTO_SECRET_LINUX_KEYRING)
> +
> +typedef struct QCryptoSecretLinux QCryptoSecretLinux;
> +typedef struct QCryptoSecretLinuxClass QCryptoSecretLinuxClass;
> +
> +typedef int32_t key_serial_t;
IMHO, just use the int32_t type inline throughout, and skip the
key_serial_t , as this typename clashes with typenames I see
in /usr/include
> +
> +typedef struct QCryptoSecretLinuxKeyring {
> + QCryptoSecretCommon parent;
> + key_serial_t serial;
> +} QCryptoSecretLinuxKeyring;
> +
> +
> +typedef struct QCryptoSecretLinuxKeyringClass {
> + QCryptoSecretCommonClass parent;
> +} QCryptoSecretLinuxKeyringClass;
> +
> +#endif /* __NR_keyctl */
> +
> +#endif /* QCRYPTO_SECRET_LINUX_KEYRING_H */
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 :|
- [RFC PATCH v2 1/5] crypto/secret: rename to secret_interface., Alexey Krasikov, 2020/04/15
- [RFC PATCH v2 5/5] test-crypto-secret: add 'syskey' object tests., Alexey Krasikov, 2020/04/15
- [RFC PATCH v2 4/5] crypto/linux_keyring: add 'syskey' secret object., Alexey Krasikov, 2020/04/15
- Re: [RFC PATCH v2 4/5] crypto/linux_keyring: add 'syskey' secret object.,
Daniel P . Berrangé <=
- [RFC PATCH v2 2/5] crypto/secret_interface: conversion to common basic class., Alexey Krasikov, 2020/04/15
- [RFC PATCH v2 3/5] crypto/secret: add secret class files., Alexey Krasikov, 2020/04/15
- Re: [RFC PATCH v2 1/5] crypto/secret: rename to secret_interface., Markus Armbruster, 2020/04/16
- Re: [RFC PATCH v2 1/5] crypto/secret: rename to secret_interface., Daniel P . Berrangé, 2020/04/22