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 <alex-krasi...@yandex-team.ru> > --- > 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 :|