[PATCH v3 0/3] Add secret_keyring object

[PATCH v3 0/3] Add secret_keyring object

[Alexey Krasikov]

Add the ability to store encryption keys in the Linux keyring
facility.

For that, factor out common parts from secret to a new abstract class
secret_common, and introduce new user-creatable secret_keyring class
inheriting from it.
Use '--enable-keyring/--disable-keyring' configuration parameters
to provide this feature.

Example:

$QEMU -object secret_keyring,id=sec0,serial=0x15968230

Alexey Krasikov (3):
  crypto/secret: move main logic from 'secret' to 'secret_common'.
  crypto/linux_keyring: add 'secret_keyring' secret object.
  test-crypto-secret: add 'secret_keyring' object tests.

 configure                       |  63 +++++
 crypto/Makefile.objs            |   2 +
 crypto/secret.c                 | 351 +--------------------------
 crypto/secret_common.c          | 405 ++++++++++++++++++++++++++++++++
 crypto/secret_keyring.c         | 141 +++++++++++
 include/crypto/secret.h         |  20 +-
 include/crypto/secret_common.h  |  68 ++++++
 include/crypto/secret_keyring.h |  45 ++++
 tests/Makefile.include          |   4 +
 tests/test-crypto-secret.c      | 154 ++++++++++++
 10 files changed, 892 insertions(+), 361 deletions(-)
 create mode 100644 crypto/secret_common.c
 create mode 100644 crypto/secret_keyring.c
 create mode 100644 include/crypto/secret_common.h
 create mode 100644 include/crypto/secret_keyring.h

-- 
2.17.1

[PATCH v3 1/3] crypto/secret: move main logic from 'secret' to 'secret_common'.

[Alexey Krasikov]

Create base class 'common secret'. Move common data and logic from
'secret' to 'common_secret' class. This allowed adding abstraction layer
for easier adding new 'secret' objects in future.
Convert 'secret' class to child from basic 'secret_common' with 'data'
and 'file' properties.

Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
---
 crypto/Makefile.objs           |   1 +
 crypto/secret.c                | 351 +---------------------------
 crypto/secret_common.c         | 407 +++++++++++++++++++++++++++++++++
 include/crypto/secret.h        |  20 +-
 include/crypto/secret_common.h |  68 ++++++
 5 files changed, 486 insertions(+), 361 deletions(-)
 create mode 100644 crypto/secret_common.c
 create mode 100644 include/crypto/secret_common.h

diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index c2a371b0b4..695da72dd1 100644
--- a/crypto/Makefile.objs
+++ b/crypto/Makefile.objs
@@ -18,6 +18,7 @@ crypto-obj-y += tlscredsanon.o
 crypto-obj-y += tlscredspsk.o
 crypto-obj-y += tlscredsx509.o
 crypto-obj-y += tlssession.o
+crypto-obj-y += secret_common.o
 crypto-obj-y += secret.o
 crypto-obj-y += pbkdf.o
 crypto-obj-$(CONFIG_NETTLE) += pbkdf-nettle.o
diff --git a/crypto/secret.c b/crypto/secret.c
index a846a3c87c..c1e1145232 100644
--- a/crypto/secret.c
+++ b/crypto/secret.c
@@ -20,16 +20,14 @@
 
 #include "qemu/osdep.h"
 #include "crypto/secret.h"
-#include "crypto/cipher.h"
 #include "qapi/error.h"
 #include "qom/object_interfaces.h"
-#include "qemu/base64.h"
 #include "qemu/module.h"
 #include "trace.h"
 
 
 static void
-qcrypto_secret_load_data(QCryptoSecret *secret,
+qcrypto_secret_load_data(QCryptoSecretCommon *sec_common,
                          uint8_t **output,
                          size_t *outputlen,
                          Error **errp)
@@ -38,6 +36,8 @@ qcrypto_secret_load_data(QCryptoSecret *secret,
     size_t length = 0;
     GError *gerr = NULL;
 
+    QCryptoSecret *secret = QCRYPTO_SECRET(sec_common);
+
     *output = NULL;
     *outputlen = 0;
 
@@ -65,198 +65,6 @@ qcrypto_secret_load_data(QCryptoSecret *secret,
 }
 
 
-static void qcrypto_secret_decrypt(QCryptoSecret *secret,
-                                   const uint8_t *input,
-                                   size_t inputlen,
-                                   uint8_t **output,
-                                   size_t *outputlen,
-                                   Error **errp)
-{
-    g_autofree uint8_t *key = NULL;
-    g_autofree uint8_t *ciphertext = NULL;
-    g_autofree uint8_t *iv = NULL;
-    size_t keylen, ciphertextlen, ivlen;
-    g_autoptr(QCryptoCipher) aes = NULL;
-    g_autofree uint8_t *plaintext = NULL;
-
-    *output = NULL;
-    *outputlen = 0;
-
-    if (qcrypto_secret_lookup(secret->keyid,
-                              &key, &keylen,
-                              errp) < 0) {
-        return;
-    }
-
-    if (keylen != 32) {
-        error_setg(errp, "Key should be 32 bytes in length");
-        return;
-    }
-
-    if (!secret->iv) {
-        error_setg(errp, "IV is required to decrypt secret");
-        return;
-    }
-
-    iv = qbase64_decode(secret->iv, -1, &ivlen, errp);
-    if (!iv) {
-        return;
-    }
-    if (ivlen != 16) {
-        error_setg(errp, "IV should be 16 bytes in length not %zu",
-                   ivlen);
-        return;
-    }
-
-    aes = qcrypto_cipher_new(QCRYPTO_CIPHER_ALG_AES_256,
-                             QCRYPTO_CIPHER_MODE_CBC,
-                             key, keylen,
-                             errp);
-    if (!aes) {
-        return;
-    }
-
-    if (qcrypto_cipher_setiv(aes, iv, ivlen, errp) < 0) {
-        return;
-    }
-
-    if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
-        ciphertext = qbase64_decode((const gchar*)input,
-                                    inputlen,
-                                    &ciphertextlen,
-                                    errp);
-        if (!ciphertext) {
-            return;
-        }
-        plaintext = g_new0(uint8_t, ciphertextlen + 1);
-    } else {
-        ciphertextlen = inputlen;
-        plaintext = g_new0(uint8_t, inputlen + 1);
-    }
-    if (qcrypto_cipher_decrypt(aes,
-                               ciphertext ? ciphertext : input,
-                               plaintext,
-                               ciphertextlen,
-                               errp) < 0) {
-        return;
-    }
-
-    if (plaintext[ciphertextlen - 1] > 16 ||
-        plaintext[ciphertextlen - 1] > ciphertextlen) {
-        error_setg(errp, "Incorrect number of padding bytes (%d) "
-                   "found on decrypted data",
-                   (int)plaintext[ciphertextlen - 1]);
-        return;
-    }
-
-    /* Even though plaintext may contain arbitrary NUL
-     * ensure it is explicitly NUL terminated.
-     */
-    ciphertextlen -= plaintext[ciphertextlen - 1];
-    plaintext[ciphertextlen] = '\0';
-
-    *output = g_steal_pointer(&plaintext);
-    *outputlen = ciphertextlen;
-}
-
-
-static void qcrypto_secret_decode(const uint8_t *input,
-                                  size_t inputlen,
-                                  uint8_t **output,
-                                  size_t *outputlen,
-                                  Error **errp)
-{
-    *output = qbase64_decode((const gchar*)input,
-                             inputlen,
-                             outputlen,
-                             errp);
-}
-
-
-static void
-qcrypto_secret_prop_set_loaded(Object *obj,
-                               bool value,
-                               Error **errp)
-{
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
-
-    if (value) {
-        Error *local_err = NULL;
-        uint8_t *input = NULL;
-        size_t inputlen = 0;
-        uint8_t *output = NULL;
-        size_t outputlen = 0;
-
-        qcrypto_secret_load_data(secret, &input, &inputlen, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
-
-        if (secret->keyid) {
-            qcrypto_secret_decrypt(secret, input, inputlen,
-                                   &output, &outputlen, &local_err);
-            g_free(input);
-            if (local_err) {
-                error_propagate(errp, local_err);
-                return;
-            }
-            input = output;
-            inputlen = outputlen;
-        } else {
-            if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
-                qcrypto_secret_decode(input, inputlen,
-                                      &output, &outputlen, &local_err);
-                g_free(input);
-                if (local_err) {
-                    error_propagate(errp, local_err);
-                    return;
-                }
-                input = output;
-                inputlen = outputlen;
-            }
-        }
-
-        secret->rawdata = input;
-        secret->rawlen = inputlen;
-    } else {
-        g_free(secret->rawdata);
-        secret->rawdata = NULL;
-        secret->rawlen = 0;
-    }
-}
-
-
-static bool
-qcrypto_secret_prop_get_loaded(Object *obj,
-                               Error **errp G_GNUC_UNUSED)
-{
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
-    return secret->rawdata != NULL;
-}
-
-
-static void
-qcrypto_secret_prop_set_format(Object *obj,
-                               int value,
-                               Error **errp G_GNUC_UNUSED)
-{
-    QCryptoSecret *creds = QCRYPTO_SECRET(obj);
-
-    creds->format = value;
-}
-
-
-static int
-qcrypto_secret_prop_get_format(Object *obj,
-                               Error **errp G_GNUC_UNUSED)
-{
-    QCryptoSecret *creds = QCRYPTO_SECRET(obj);
-
-    return creds->format;
-}
-
-
 static void
 qcrypto_secret_prop_set_data(Object *obj,
                              const char *value,
@@ -299,48 +107,6 @@ qcrypto_secret_prop_get_file(Object *obj,
 }
 
 
-static void
-qcrypto_secret_prop_set_iv(Object *obj,
-                           const char *value,
-                           Error **errp)
-{
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
-
-    g_free(secret->iv);
-    secret->iv = g_strdup(value);
-}
-
-
-static char *
-qcrypto_secret_prop_get_iv(Object *obj,
-                           Error **errp)
-{
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
-    return g_strdup(secret->iv);
-}
-
-
-static void
-qcrypto_secret_prop_set_keyid(Object *obj,
-                              const char *value,
-                              Error **errp)
-{
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
-
-    g_free(secret->keyid);
-    secret->keyid = g_strdup(value);
-}
-
-
-static char *
-qcrypto_secret_prop_get_keyid(Object *obj,
-                              Error **errp)
-{
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
-    return g_strdup(secret->keyid);
-}
-
-
 static void
 qcrypto_secret_complete(UserCreatable *uc, Error **errp)
 {
@@ -353,30 +119,19 @@ qcrypto_secret_finalize(Object *obj)
 {
     QCryptoSecret *secret = QCRYPTO_SECRET(obj);
 
-    g_free(secret->iv);
     g_free(secret->file);
-    g_free(secret->keyid);
-    g_free(secret->rawdata);
     g_free(secret->data);
 }
 
 static void
 qcrypto_secret_class_init(ObjectClass *oc, void *data)
 {
-    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+    QCryptoSecretCommonClass *sic = QCRYPTO_SECRET_COMMON_CLASS(oc);
+    sic->load_data = qcrypto_secret_load_data;
 
+    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
     ucc->complete = qcrypto_secret_complete;
 
-    object_class_property_add_bool(oc, "loaded",
-                                   qcrypto_secret_prop_get_loaded,
-                                   qcrypto_secret_prop_set_loaded,
-                                   NULL);
-    object_class_property_add_enum(oc, "format",
-                                   "QCryptoSecretFormat",
-                                   &QCryptoSecretFormat_lookup,
-                                   qcrypto_secret_prop_get_format,
-                                   qcrypto_secret_prop_set_format,
-                                   NULL);
     object_class_property_add_str(oc, "data",
                                   qcrypto_secret_prop_get_data,
                                   qcrypto_secret_prop_set_data,
@@ -385,103 +140,11 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data)
                                   qcrypto_secret_prop_get_file,
                                   qcrypto_secret_prop_set_file,
                                   NULL);
-    object_class_property_add_str(oc, "keyid",
-                                  qcrypto_secret_prop_get_keyid,
-                                  qcrypto_secret_prop_set_keyid,
-                                  NULL);
-    object_class_property_add_str(oc, "iv",
-                                  qcrypto_secret_prop_get_iv,
-                                  qcrypto_secret_prop_set_iv,
-                                  NULL);
-}
-
-
-int qcrypto_secret_lookup(const char *secretid,
-                          uint8_t **data,
-                          size_t *datalen,
-                          Error **errp)
-{
-    Object *obj;
-    QCryptoSecret *secret;
-
-    obj = object_resolve_path_component(
-        object_get_objects_root(), secretid);
-    if (!obj) {
-        error_setg(errp, "No secret with id '%s'", secretid);
-        return -1;
-    }
-
-    secret = (QCryptoSecret *)
-        object_dynamic_cast(obj,
-                            TYPE_QCRYPTO_SECRET);
-    if (!secret) {
-        error_setg(errp, "Object with id '%s' is not a secret",
-                   secretid);
-        return -1;
-    }
-
-    if (!secret->rawdata) {
-        error_setg(errp, "Secret with id '%s' has no data",
-                   secretid);
-        return -1;
-    }
-
-    *data = g_new0(uint8_t, secret->rawlen + 1);
-    memcpy(*data, secret->rawdata, secret->rawlen);
-    (*data)[secret->rawlen] = '\0';
-    *datalen = secret->rawlen;
-
-    return 0;
-}
-
-
-char *qcrypto_secret_lookup_as_utf8(const char *secretid,
-                                    Error **errp)
-{
-    uint8_t *data;
-    size_t datalen;
-
-    if (qcrypto_secret_lookup(secretid,
-                              &data,
-                              &datalen,
-                              errp) < 0) {
-        return NULL;
-    }
-
-    if (!g_utf8_validate((const gchar*)data, datalen, NULL)) {
-        error_setg(errp,
-                   "Data from secret %s is not valid UTF-8",
-                   secretid);
-        g_free(data);
-        return NULL;
-    }
-
-    return (char *)data;
-}
-
-
-char *qcrypto_secret_lookup_as_base64(const char *secretid,
-                                      Error **errp)
-{
-    uint8_t *data;
-    size_t datalen;
-    char *ret;
-
-    if (qcrypto_secret_lookup(secretid,
-                              &data,
-                              &datalen,
-                              errp) < 0) {
-        return NULL;
-    }
-
-    ret = g_base64_encode(data, datalen);
-    g_free(data);
-    return ret;
 }
 
 
 static const TypeInfo qcrypto_secret_info = {
-    .parent = TYPE_OBJECT,
+    .parent = TYPE_QCRYPTO_SECRET_COMMON,
     .name = TYPE_QCRYPTO_SECRET,
     .instance_size = sizeof(QCryptoSecret),
     .instance_finalize = qcrypto_secret_finalize,
diff --git a/crypto/secret_common.c b/crypto/secret_common.c
new file mode 100644
index 0000000000..4ef15b98a2
--- /dev/null
+++ b/crypto/secret_common.c
@@ -0,0 +1,407 @@
+/*
+ * QEMU crypto secret support
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "crypto/secret_common.h"
+#include "crypto/cipher.h"
+#include "qapi/error.h"
+#include "qom/object_interfaces.h"
+#include "qemu/base64.h"
+#include "qemu/module.h"
+#include "trace.h"
+
+
+static void qcrypto_secret_decrypt(QCryptoSecretCommon  *secret,
+                                   const uint8_t        *input,
+                                   size_t               inputlen,
+                                   uint8_t              **output,
+                                   size_t               *outputlen,
+                                   Error                **errp)
+{
+    g_autofree uint8_t *iv  = NULL;
+    g_autofree uint8_t *key = NULL;
+    g_autofree uint8_t *ciphertext = NULL;
+    size_t keylen, ciphertextlen, ivlen;
+    g_autoptr(QCryptoCipher) aes  = NULL;
+    g_autofree uint8_t *plaintext = NULL;
+
+    *output = NULL;
+    *outputlen = 0;
+
+    if (qcrypto_secret_lookup(secret->keyid,
+                              &key, &keylen,
+                              errp) < 0) {
+        return;
+    }
+
+    if (keylen != 32) {
+        error_setg(errp, "Key should be 32 bytes in length");
+        return;
+    }
+
+    if (!secret->iv) {
+        error_setg(errp, "IV is required to decrypt secret");
+        return;
+    }
+
+    iv = qbase64_decode(secret->iv, -1, &ivlen, errp);
+    if (!iv) {
+        return;
+    }
+    if (ivlen != 16) {
+        error_setg(errp, "IV should be 16 bytes in length not %zu",
+                   ivlen);
+        return;
+    }
+
+    aes = qcrypto_cipher_new(QCRYPTO_CIPHER_ALG_AES_256,
+                             QCRYPTO_CIPHER_MODE_CBC,
+                             key, keylen,
+                             errp);
+    if (!aes) {
+        return;
+    }
+
+    if (qcrypto_cipher_setiv(aes, iv, ivlen, errp) < 0) {
+        return;
+    }
+
+    if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
+        ciphertext = qbase64_decode((const gchar *)input,
+                                    inputlen,
+                                    &ciphertextlen,
+                                    errp);
+        if (!ciphertext) {
+            return;
+        }
+        plaintext = g_new0(uint8_t, ciphertextlen + 1);
+    } else {
+        ciphertextlen = inputlen;
+        plaintext = g_new0(uint8_t, inputlen + 1);
+    }
+    if (qcrypto_cipher_decrypt(aes,
+                               ciphertext ? ciphertext : input,
+                               plaintext,
+                               ciphertextlen,
+                               errp) < 0) {
+        return;
+    }
+
+    if (plaintext[ciphertextlen - 1] > 16 ||
+        plaintext[ciphertextlen - 1] > ciphertextlen) {
+        error_setg(errp, "Incorrect number of padding bytes (%d) "
+                   "found on decrypted data",
+                   (int)plaintext[ciphertextlen - 1]);
+        return;
+    }
+
+    /*
+     *  Even though plaintext may contain arbitrary NUL
+     * ensure it is explicitly NUL terminated.
+     */
+    ciphertextlen -= plaintext[ciphertextlen - 1];
+    plaintext[ciphertextlen] = '\0';
+
+    *output = g_steal_pointer(&plaintext);
+    *outputlen = ciphertextlen;
+}
+
+
+static void qcrypto_secret_decode(const uint8_t  *input,
+                                  size_t         inputlen,
+                                  uint8_t        **output,
+                                  size_t         *outputlen,
+                                  Error          **errp)
+{
+    *output = qbase64_decode((const gchar *)input,
+                             inputlen,
+                             outputlen,
+                             errp);
+}
+
+
+static void
+qcrypto_secret_prop_set_loaded(Object  *obj,
+                               bool    value,
+                               Error   **errp)
+{
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
+    QCryptoSecretCommonClass *sec_class
+                                = QCRYPTO_SECRET_COMMON_GET_CLASS(obj);
+
+    if (value) {
+        Error   *local_err = NULL;
+        uint8_t *input     = NULL;
+        size_t  inputlen   = 0;
+        uint8_t *output    = NULL;
+        size_t  outputlen  = 0;
+
+        if (sec_class->load_data) {
+            sec_class->load_data(secret, &input, &inputlen, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        } else {
+            error_setg(errp, "%s provides no 'load_data' method'",
+                             object_get_typename(obj));
+            return;
+        }
+
+        if (secret->keyid) {
+            qcrypto_secret_decrypt(secret, input, inputlen,
+                                   &output, &outputlen, &local_err);
+            g_free(input);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+            input    = output;
+            inputlen = outputlen;
+        } else {
+            if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
+                qcrypto_secret_decode(input, inputlen,
+                                      &output, &outputlen, &local_err);
+                g_free(input);
+                if (local_err) {
+                    error_propagate(errp, local_err);
+                    return;
+                }
+                input    = output;
+                inputlen = outputlen;
+            }
+        }
+
+        secret->rawdata = input;
+        secret->rawlen  = inputlen;
+    } else {
+        g_free(secret->rawdata);
+        secret->rawlen = 0;
+    }
+}
+
+
+static bool
+qcrypto_secret_prop_get_loaded(Object  *obj,
+                               Error   **errp G_GNUC_UNUSED)
+{
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
+    return secret->rawdata != NULL;
+}
+
+
+static void
+qcrypto_secret_prop_set_format(Object  *obj,
+                               int     value,
+                               Error   **errp G_GNUC_UNUSED)
+{
+    QCryptoSecretCommon *creds = QCRYPTO_SECRET_COMMON(obj);
+    creds->format = value;
+}
+
+
+static int
+qcrypto_secret_prop_get_format(Object  *obj,
+                               Error   **errp G_GNUC_UNUSED)
+{
+    QCryptoSecretCommon *creds = QCRYPTO_SECRET_COMMON(obj);
+    return creds->format;
+}
+
+
+static void
+qcrypto_secret_prop_set_iv(Object      *obj,
+                           const char  *value,
+                           Error       **errp)
+{
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
+
+    g_free(secret->iv);
+    secret->iv = g_strdup(value);
+}
+
+
+static char *
+qcrypto_secret_prop_get_iv(Object  *obj,
+                           Error   **errp)
+{
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
+    return g_strdup(secret->iv);
+}
+
+
+static void
+qcrypto_secret_prop_set_keyid(Object      *obj,
+                              const char  *value,
+                              Error       **errp)
+{
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
+
+    g_free(secret->keyid);
+    secret->keyid = g_strdup(value);
+}
+
+
+static char *
+qcrypto_secret_prop_get_keyid(Object  *obj,
+                              Error   **errp)
+{
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
+    return g_strdup(secret->keyid);
+}
+
+
+static void
+qcrypto_secret_finalize(Object *obj)
+{
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
+
+    g_free(secret->iv);
+    g_free(secret->keyid);
+    g_free(secret->rawdata);
+}
+
+static void
+qcrypto_secret_class_init(ObjectClass *oc, void *data)
+{
+    object_class_property_add_bool(oc, "loaded",
+                                   qcrypto_secret_prop_get_loaded,
+                                   qcrypto_secret_prop_set_loaded,
+                                   NULL);
+    object_class_property_add_enum(oc, "format",
+                                   "QCryptoSecretFormat",
+                                   &QCryptoSecretFormat_lookup,
+                                   qcrypto_secret_prop_get_format,
+                                   qcrypto_secret_prop_set_format,
+                                   NULL);
+    object_class_property_add_str(oc, "keyid",
+                                  qcrypto_secret_prop_get_keyid,
+                                  qcrypto_secret_prop_set_keyid,
+                                  NULL);
+    object_class_property_add_str(oc, "iv",
+                                  qcrypto_secret_prop_get_iv,
+                                  qcrypto_secret_prop_set_iv,
+                                  NULL);
+}
+
+
+int qcrypto_secret_lookup(const char  *secretid,
+                          uint8_t     **data,
+                          size_t      *datalen,
+                          Error       **errp)
+{
+    Object *obj;
+    QCryptoSecretCommon *secret;
+
+    obj = object_resolve_path_component(
+        object_get_objects_root(), secretid);
+    if (!obj) {
+        error_setg(errp, "No secret with id '%s'", secretid);
+        return -1;
+    }
+
+    secret = (QCryptoSecretCommon *)
+        object_dynamic_cast(obj,
+                            TYPE_QCRYPTO_SECRET_COMMON);
+    if (!secret) {
+        error_setg(errp, "Object with id '%s' is not a secret",
+                   secretid);
+        return -1;
+    }
+
+    if (!secret->rawdata) {
+        error_setg(errp, "Secret with id '%s' has no data",
+                   secretid);
+        return -1;
+    }
+
+    *data = g_new0(uint8_t, secret->rawlen + 1);
+    memcpy(*data, secret->rawdata, secret->rawlen);
+    (*data)[secret->rawlen] = '\0';
+    *datalen = secret->rawlen;
+
+    return 0;
+}
+
+
+char *qcrypto_secret_lookup_as_utf8(const char  *secretid,
+                                    Error       **errp)
+{
+    uint8_t  *data;
+    size_t   datalen;
+
+    if (qcrypto_secret_lookup(secretid,
+                              &data,
+                              &datalen,
+                              errp) < 0) {
+        return NULL;
+    }
+
+    if (!g_utf8_validate((const gchar *)data, datalen, NULL)) {
+        error_setg(errp,
+                   "Data from secret %s is not valid UTF-8",
+                   secretid);
+        g_free(data);
+        return NULL;
+    }
+
+    return (char *)data;
+}
+
+
+char *qcrypto_secret_lookup_as_base64(const char  *secretid,
+                                      Error       **errp)
+{
+    uint8_t  *data;
+    size_t   datalen;
+    char     *ret;
+
+    if (qcrypto_secret_lookup(secretid,
+                              &data,
+                              &datalen,
+                              errp) < 0) {
+        return NULL;
+    }
+
+    ret = g_base64_encode(data, datalen);
+    g_free(data);
+    return ret;
+}
+
+
+static const TypeInfo qcrypto_secret_info = {
+    .parent            = TYPE_OBJECT,
+    .name              = TYPE_QCRYPTO_SECRET_COMMON,
+    .instance_size     = sizeof(QCryptoSecretCommon),
+    .instance_finalize = qcrypto_secret_finalize,
+    .class_size        = sizeof(QCryptoSecretCommonClass),
+    .class_init        = qcrypto_secret_class_init,
+    .abstract          = true,
+};
+
+
+static void
+qcrypto_secret_register_types(void)
+{
+    type_register_static(&qcrypto_secret_info);
+}
+
+
+type_init(qcrypto_secret_register_types);
diff --git a/include/crypto/secret.h b/include/crypto/secret.h
index 5e07e29bae..2deb461d2f 100644
--- a/include/crypto/secret.h
+++ b/include/crypto/secret.h
@@ -23,6 +23,7 @@
 
 #include "qapi/qapi-types-crypto.h"
 #include "qom/object.h"
+#include "crypto/secret_common.h"
 
 #define TYPE_QCRYPTO_SECRET "secret"
 #define QCRYPTO_SECRET(obj)                  \
@@ -119,29 +120,14 @@ typedef struct QCryptoSecretClass QCryptoSecretClass;
  */
 
 struct QCryptoSecret {
-    Object parent_obj;
-    uint8_t *rawdata;
-    size_t rawlen;
-    QCryptoSecretFormat format;
+    QCryptoSecretCommon parent_obj;
     char *data;
     char *file;
-    char *keyid;
-    char *iv;
 };
 
 
 struct QCryptoSecretClass {
-    ObjectClass parent_class;
+    QCryptoSecretCommonClass parent_class;
 };
 
-
-extern int qcrypto_secret_lookup(const char *secretid,
-                                 uint8_t **data,
-                                 size_t *datalen,
-                                 Error **errp);
-extern char *qcrypto_secret_lookup_as_utf8(const char *secretid,
-                                           Error **errp);
-extern char *qcrypto_secret_lookup_as_base64(const char *secretid,
-                                             Error **errp);
-
 #endif /* QCRYPTO_SECRET_H */
diff --git a/include/crypto/secret_common.h b/include/crypto/secret_common.h
new file mode 100644
index 0000000000..41c06b5391
--- /dev/null
+++ b/include/crypto/secret_common.h
@@ -0,0 +1,68 @@
+/*
+ * QEMU crypto secret support
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef QCRYPTO_SECRET_COMMON_H
+#define QCRYPTO_SECRET_COMMON_H
+
+#include "qapi/qapi-types-crypto.h"
+#include "qom/object.h"
+
+#define TYPE_QCRYPTO_SECRET_COMMON "secret_common"
+#define QCRYPTO_SECRET_COMMON(obj) \
+    OBJECT_CHECK(QCryptoSecretCommon, (obj), TYPE_QCRYPTO_SECRET_COMMON)
+#define QCRYPTO_SECRET_COMMON_CLASS(class) \
+    OBJECT_CLASS_CHECK(QCryptoSecretCommonClass, \
+                       (class), TYPE_QCRYPTO_SECRET_COMMON)
+#define QCRYPTO_SECRET_COMMON_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(QCryptoSecretCommonClass, \
+                     (obj), TYPE_QCRYPTO_SECRET_COMMON)
+
+typedef struct QCryptoSecretCommon QCryptoSecretCommon;
+typedef struct QCryptoSecretCommonClass QCryptoSecretCommonClass;
+
+struct QCryptoSecretCommon {
+    Object               parent_obj;
+    uint8_t              *rawdata;
+    size_t               rawlen;
+    QCryptoSecretFormat  format;
+    char                 *keyid;
+    char                 *iv;
+};
+
+
+struct QCryptoSecretCommonClass {
+    ObjectClass parent_class;
+    void (*load_data)(QCryptoSecretCommon *secret,
+                      uint8_t             **output,
+                      size_t              *outputlen,
+                      Error               **errp);
+};
+
+
+extern int qcrypto_secret_lookup(const char  *secretid,
+                                 uint8_t     **data,
+                                 size_t      *datalen,
+                                 Error       **errp);
+extern char *qcrypto_secret_lookup_as_utf8(const char  *secretid,
+                                           Error       **errp);
+extern char *qcrypto_secret_lookup_as_base64(const char  *secretid,
+                                             Error       **errp);
+
+#endif /* QCRYPTO_SECRET_COMMON_H */
-- 
2.17.1

Re: [PATCH v3 1/3] crypto/secret: move main logic from 'secret' to 'secret_common'.

[Daniel P. Berrangé]

On Mon, May 18, 2020 at 11:28:02PM +0300, Alexey Krasikov wrote:
> Create base class 'common secret'. Move common data and logic from
> 'secret' to 'common_secret' class. This allowed adding abstraction layer
> for easier adding new 'secret' objects in future.
> Convert 'secret' class to child from basic 'secret_common' with 'data'
> and 'file' properties.
> 
> Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
> ---
>  crypto/Makefile.objs           |   1 +
>  crypto/secret.c                | 351 +---------------------------
>  crypto/secret_common.c         | 407 +++++++++++++++++++++++++++++++++
>  include/crypto/secret.h        |  20 +-
>  include/crypto/secret_common.h |  68 ++++++
>  5 files changed, 486 insertions(+), 361 deletions(-)
>  create mode 100644 crypto/secret_common.c
>  create mode 100644 include/crypto/secret_common.h
> 
> diff --git a/crypto/secret_common.c b/crypto/secret_common.c
> new file mode 100644
> index 0000000000..4ef15b98a2
> --- /dev/null
> +++ b/crypto/secret_common.c

> +static void qcrypto_secret_decrypt(QCryptoSecretCommon  *secret,
> +                                   const uint8_t        *input,
> +                                   size_t               inputlen,
> +                                   uint8_t              **output,
> +                                   size_t               *outputlen,
> +                                   Error                **errp)

This has reformatted the original code to vertically line up parameter
names. This is not something we do, so please remove all this extra
whitespace alignment. The is present in other funtions and patches
too, all of which need fixes.


> +static void
> +qcrypto_secret_class_init(ObjectClass *oc, void *data)
> +{
> +    object_class_property_add_bool(oc, "loaded",
> +                                   qcrypto_secret_prop_get_loaded,
> +                                   qcrypto_secret_prop_set_loaded,
> +                                   NULL);
> +    object_class_property_add_enum(oc, "format",
> +                                   "QCryptoSecretFormat",
> +                                   &QCryptoSecretFormat_lookup,
> +                                   qcrypto_secret_prop_get_format,
> +                                   qcrypto_secret_prop_set_format,
> +                                   NULL);
> +    object_class_property_add_str(oc, "keyid",
> +                                  qcrypto_secret_prop_get_keyid,
> +                                  qcrypto_secret_prop_set_keyid,
> +                                  NULL);
> +    object_class_property_add_str(oc, "iv",
> +                                  qcrypto_secret_prop_get_iv,
> +                                  qcrypto_secret_prop_set_iv,
> +                                  NULL);

A patch recently merged which removed the "ERror **errp" parameter
which causes merge conflicts on this patch. You'll just need to
drop the last "NULL" to fix it.


> diff --git a/include/crypto/secret_common.h b/include/crypto/secret_common.h
> new file mode 100644
> index 0000000000..41c06b5391
> --- /dev/null
> +++ b/include/crypto/secret_common.h
> @@ -0,0 +1,68 @@
> +/*
> + * QEMU crypto secret support
> + *
> + * Copyright (c) 2015 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef QCRYPTO_SECRET_COMMON_H
> +#define QCRYPTO_SECRET_COMMON_H
> +
> +#include "qapi/qapi-types-crypto.h"
> +#include "qom/object.h"
> +
> +#define TYPE_QCRYPTO_SECRET_COMMON "secret_common"
> +#define QCRYPTO_SECRET_COMMON(obj) \
> +    OBJECT_CHECK(QCryptoSecretCommon, (obj), TYPE_QCRYPTO_SECRET_COMMON)
> +#define QCRYPTO_SECRET_COMMON_CLASS(class) \
> +    OBJECT_CLASS_CHECK(QCryptoSecretCommonClass, \
> +                       (class), TYPE_QCRYPTO_SECRET_COMMON)
> +#define QCRYPTO_SECRET_COMMON_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(QCryptoSecretCommonClass, \
> +                     (obj), TYPE_QCRYPTO_SECRET_COMMON)
> +
> +typedef struct QCryptoSecretCommon QCryptoSecretCommon;
> +typedef struct QCryptoSecretCommonClass QCryptoSecretCommonClass;
> +
> +struct QCryptoSecretCommon {
> +    Object               parent_obj;
> +    uint8_t              *rawdata;
> +    size_t               rawlen;
> +    QCryptoSecretFormat  format;
> +    char                 *keyid;
> +    char                 *iv;
> +};

Don't vertically align struct fields either.


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 :|

[PATCH v3 2/3] crypto/linux_keyring: add 'secret_keyring' secret object.

[Alexey Krasikov]

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
permissions to read it.

Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
---
 configure                       |  38 ++++++++
 crypto/Makefile.objs            |   1 +
 crypto/secret_keyring.c         | 148 ++++++++++++++++++++++++++++++++
 include/crypto/secret_keyring.h |  52 +++++++++++
 4 files changed, 239 insertions(+)
 create mode 100644 crypto/secret_keyring.c
 create mode 100644 include/crypto/secret_keyring.h

diff --git a/configure b/configure
index 0d69c360c0..1bae5ec0a1 100755
--- a/configure
+++ b/configure
@@ -509,6 +509,7 @@ libpmem=""
 default_devices="yes"
 plugins="no"
 fuzzing="no"
+secret_keyring="yes"
 
 supported_cpu="no"
 supported_os="no"
@@ -1601,6 +1602,10 @@ for opt do
   ;;
   --gdb=*) gdb_bin="$optarg"
   ;;
+  --enable-keyring) secret_keyring="yes"
+  ;;
+  --disable-keyring) secret_keyring="no"
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -6250,6 +6255,34 @@ case "$slirp" in
     ;;
 esac
 
+##########################################
+# check for usable __NR_keyctl syscall
+
+if test "$linux" = "yes" ; then
+
+    have_keyring=no
+    cat > $TMPC << EOF
+#include <errno.h>
+#include <asm/unistd.h>
+#include <linux/keyctl.h>
+#include <unistd.h>
+int main(void) {
+    return syscall(__NR_keyctl, KEYCTL_READ, 0, NULL, NULL, 0);
+}
+EOF
+    if compile_prog "" "" ; then
+        have_keyring=yes
+    fi
+fi
+if test "$secret_keyring" = "yes"
+then
+    if test "$have_keyring" != "yes"
+    then
+    error_exit "syscall __NR_keyctl requested, \
+but not implemented on your system"
+    fi
+fi
+
 
 ##########################################
 # End of CC checks
@@ -6733,6 +6766,7 @@ echo "default devices   $default_devices"
 echo "plugin support    $plugins"
 echo "fuzzing support   $fuzzing"
 echo "gdb               $gdb_bin"
+echo "Linux keyring support $secret_keyring"
 
 if test "$supported_cpu" = "no"; then
     echo
@@ -7614,6 +7648,10 @@ if test -n "$gdb_bin" ; then
     echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
 fi
 
+if test "$secret_keyring" = "yes" ; then
+  echo "CONFIG_SECRET_KEYRING=y" >> $config_host_mak
+fi
+
 if test "$tcg_interpreter" = "yes"; then
   QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
 elif test "$ARCH" = "sparc64" ; then
diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index 695da72dd1..872c928ac0 100644
--- a/crypto/Makefile.objs
+++ b/crypto/Makefile.objs
@@ -20,6 +20,7 @@ crypto-obj-y += tlscredsx509.o
 crypto-obj-y += tlssession.o
 crypto-obj-y += secret_common.o
 crypto-obj-y += secret.o
+crypto-obj-$(CONFIG_SECRET_KEYRING) += secret_keyring.o
 crypto-obj-y += pbkdf.o
 crypto-obj-$(CONFIG_NETTLE) += pbkdf-nettle.o
 crypto-obj-$(if $(CONFIG_NETTLE),n,$(CONFIG_GCRYPT)) += pbkdf-gcrypt.o
diff --git a/crypto/secret_keyring.c b/crypto/secret_keyring.c
new file mode 100644
index 0000000000..8f256ee3b8
--- /dev/null
+++ b/crypto/secret_keyring.c
@@ -0,0 +1,148 @@
+/*
+ * QEMU crypto secret support
+ *
+ * Copyright 2020 Yandex N.V.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#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/secret_keyring.h"
+
+
+static inline
+long keyctl_read(int32_t key, uint8_t *buffer, size_t buflen)
+{
+    return syscall(__NR_keyctl, KEYCTL_READ, key, buffer, buflen, 0);
+}
+
+
+static void
+qcrypto_secret_keyring_load_data(QCryptoSecretCommon *sec_common,
+                                 uint8_t             **output,
+                                 size_t              *outputlen,
+                                 Error               **errp)
+{
+    QCryptoSecretKeyring *secret = QCRYPTO_SECRET_KEYRING(sec_common);
+    uint8_t  *buffer = NULL;
+    long     retcode;
+
+    *output    = NULL;
+    *outputlen = 0;
+
+    if (!secret->serial) {
+        error_setg(errp, "'serial' parameter must be provided");
+        return;
+    }
+
+    retcode = keyctl_read(secret->serial, NULL, 0);
+    if (retcode <= 0) {
+        goto keyctl_error;
+    }
+
+    buffer = g_new0(uint8_t, retcode);
+
+    retcode = keyctl_read(secret->serial, buffer, retcode);
+    if (retcode < 0) {
+        g_free(buffer);
+        goto keyctl_error;
+    }
+
+    *outputlen = retcode;
+    *output    = buffer;
+    return;
+
+keyctl_error:
+    error_setg_errno(errp, errno,
+                     "Unable to read serial key %08x",
+                     secret->serial);
+}
+
+
+static void
+qcrypto_secret_prop_set_key(Object     *obj,   Visitor *v,
+                            const char *name,  void    *opaque,
+                            Error      **errp)
+{
+    QCryptoSecretKeyring *secret = QCRYPTO_SECRET_KEYRING(obj);
+    int32_t value;
+    visit_type_int32(v, name, &value, errp);
+    if (!value) {
+        error_setg(errp, "'serial' should not be equal to 0");
+    }
+    secret->serial = value;
+}
+
+
+static void
+qcrypto_secret_prop_get_key(Object     *obj,   Visitor *v,
+                            const char *name,  void    *opaque,
+                            Error      **errp)
+{
+    QCryptoSecretKeyring *secret = QCRYPTO_SECRET_KEYRING(obj);
+    int32_t value = secret->serial;
+    visit_type_int32(v, name, &value, errp);
+}
+
+
+static void
+qcrypto_secret_keyring_complete(UserCreatable *uc, Error **errp)
+{
+    object_property_set_bool(OBJECT(uc), true, "loaded", errp);
+}
+
+
+static void
+qcrypto_secret_keyring_class_init(ObjectClass *oc, void *data)
+{
+    QCryptoSecretCommonClass *sic = QCRYPTO_SECRET_COMMON_CLASS(oc);
+    sic->load_data = qcrypto_secret_keyring_load_data;
+
+    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+    ucc->complete = qcrypto_secret_keyring_complete;
+
+    object_class_property_add(oc, "serial", "int32_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_KEYRING,
+    .instance_size = sizeof(QCryptoSecretKeyring),
+    .class_size    = sizeof(QCryptoSecretKeyringClass),
+    .class_init    = qcrypto_secret_keyring_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);
diff --git a/include/crypto/secret_keyring.h b/include/crypto/secret_keyring.h
new file mode 100644
index 0000000000..9406d1fedc
--- /dev/null
+++ b/include/crypto/secret_keyring.h
@@ -0,0 +1,52 @@
+/*
+ * QEMU crypto secret support
+ *
+ * Copyright 2020 Yandex N.V.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef QCRYPTO_SECRET_KEYRING_H
+#define QCRYPTO_SECRET_KEYRING_H
+
+#include "qapi/qapi-types-crypto.h"
+#include "qom/object.h"
+#include "crypto/secret_common.h"
+
+#define TYPE_QCRYPTO_SECRET_KEYRING "secret_keyring"
+#define QCRYPTO_SECRET_KEYRING(obj) \
+    OBJECT_CHECK(QCryptoSecretKeyring, (obj), \
+                 TYPE_QCRYPTO_SECRET_KEYRING)
+#define QCRYPTO_SECRET_KEYRING_CLASS(class) \
+    OBJECT_CLASS_CHECK(QCryptoSecretKeyringClass, \
+                       (class), TYPE_QCRYPTO_SECRET_KEYRING)
+#define QCRYPTO_SECRET_KEYRING_GET_CLASS(class) \
+    OBJECT_GET_CLASS(QCryptoSecretKeyringClass, \
+                     (class), TYPE_QCRYPTO_SECRET_KEYRING)
+
+typedef struct QCryptoSecretKeyring QCryptoSecretKeyring;
+typedef struct QCryptoSecretKeyringClass QCryptoSecretKeyringClass;
+
+typedef struct QCryptoSecretKeyring {
+    QCryptoSecretCommon  parent;
+    int32_t              serial;
+} QCryptoSecretKeyring;
+
+
+typedef struct QCryptoSecretKeyringClass {
+    QCryptoSecretCommonClass  parent;
+} QCryptoSecretKeyringClass;
+
+#endif /* QCRYPTO_SECRET_KEYRING_H */
-- 
2.17.1

Re: [PATCH v3 2/3] crypto/linux_keyring: add 'secret_keyring' secret object.

[Daniel P. Berrangé]

On Mon, May 18, 2020 at 11:28:03PM +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
> permissions to read it.
> 
> Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
> ---
>  configure                       |  38 ++++++++
>  crypto/Makefile.objs            |   1 +
>  crypto/secret_keyring.c         | 148 ++++++++++++++++++++++++++++++++
>  include/crypto/secret_keyring.h |  52 +++++++++++
>  4 files changed, 239 insertions(+)
>  create mode 100644 crypto/secret_keyring.c
>  create mode 100644 include/crypto/secret_keyring.h
> 
> diff --git a/configure b/configure
> index 0d69c360c0..1bae5ec0a1 100755
> --- a/configure
> +++ b/configure

>  ##########################################
>  # End of CC checks
> @@ -6733,6 +6766,7 @@ echo "default devices   $default_devices"
>  echo "plugin support    $plugins"
>  echo "fuzzing support   $fuzzing"
>  echo "gdb               $gdb_bin"
> +echo "Linux keyring support $secret_keyring"

Remove the word "support" here and vertically align the value
with the line above, otherwise the configure output summary
looks odd.

With that fixed, 

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

[PATCH v3 3/3] test-crypto-secret: add 'secret_keyring' object tests.

[Alexey Krasikov]

Add tests:
  test_secret_keyring_good;
  test_secret_keyring_revoked_key;
  test_secret_keyring_expired_key;
  test_secret_keyring_bad_serial_key;
  test_secret_keyring_bad_key_access_right;

Added tests require libkeyutils. The absence of this library is not
critical, because these tests will be skipped in this case.

Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
---
 configure                  |  25 ++++++
 tests/Makefile.include     |   4 +
 tests/test-crypto-secret.c | 154 +++++++++++++++++++++++++++++++++++++
 3 files changed, 183 insertions(+)

diff --git a/configure b/configure
index 1bae5ec0a1..2ab7d2961c 100755
--- a/configure
+++ b/configure
@@ -6283,6 +6283,28 @@ but not implemented on your system"
     fi
 fi
 
+##########################################
+# check for usable keyutils.h
+
+if test "$linux" = "yes" ; then
+
+    have_keyutils=no
+    cat > $TMPC << EOF
+#include <errno.h>
+#include <asm/unistd.h>
+#include <linux/keyctl.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <keyutils.h>
+int main(void) {
+    return request_key("user", NULL, NULL, 0);
+}
+EOF
+    if compile_prog "" "-lkeyutils"; then
+        have_keyutils=yes
+    fi
+fi
+
 
 ##########################################
 # End of CC checks
@@ -7650,6 +7672,9 @@ fi
 
 if test "$secret_keyring" = "yes" ; then
   echo "CONFIG_SECRET_KEYRING=y" >> $config_host_mak
+  if test "$have_keyutils" = "yes" ; then
+    echo "CONFIG_TEST_SECRET_KEYRING=y" >> $config_host_mak
+  fi
 fi
 
 if test "$tcg_interpreter" = "yes"; then
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 03a74b60f6..de13908701 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -538,6 +538,10 @@ tests/benchmark-crypto-cipher$(EXESUF): tests/benchmark-crypto-cipher.o $(test-c
 tests/test-crypto-secret$(EXESUF): tests/test-crypto-secret.o $(test-crypto-obj-y)
 tests/test-crypto-xts$(EXESUF): tests/test-crypto-xts.o $(test-crypto-obj-y)
 
+ifeq ($(CONFIG_TEST_SECRET_KEYRING),y)
+tests/test-crypto-secret.o-libs := -lkeyutils
+endif
+
 tests/crypto-tls-x509-helpers.o-cflags := $(TASN1_CFLAGS)
 tests/crypto-tls-x509-helpers.o-libs := $(TASN1_LIBS)
 tests/pkix_asn1_tab.o-cflags := $(TASN1_CFLAGS)
diff --git a/tests/test-crypto-secret.c b/tests/test-crypto-secret.c
index 13fc6c4c75..34f5ed75ff 100644
--- a/tests/test-crypto-secret.c
+++ b/tests/test-crypto-secret.c
@@ -24,6 +24,10 @@
 #include "crypto/secret.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#ifdef CONFIG_TEST_SECRET_KEYRING
+#include "crypto/secret_keyring.h"
+#include <keyutils.h>
+#endif
 
 static void test_secret_direct(void)
 {
@@ -124,6 +128,143 @@ static void test_secret_indirect_emptyfile(void)
     g_free(fname);
 }
 
+#ifdef CONFIG_TEST_SECRET_KEYRING
+
+#define DESCRIPTION "qemu_test_secret"
+#define PAYLOAD "Test Payload"
+
+
+static void test_secret_keyring_good(void)
+{
+    char key_str[16];
+    Object *sec;
+    int32_t key = add_key("user", DESCRIPTION, PAYLOAD,
+                          strlen(PAYLOAD), KEY_SPEC_PROCESS_KEYRING);
+
+    g_assert(key >= 0);
+
+    snprintf(key_str, sizeof(key_str), "0x%08x", key);
+    sec = object_new_with_props(
+        TYPE_QCRYPTO_SECRET_KEYRING,
+        object_get_objects_root(),
+        "sec0",
+        &error_abort,
+        "serial", key_str,
+        NULL);
+
+    assert(0 <= keyctl_unlink(key, KEY_SPEC_PROCESS_KEYRING));
+    char *pw = qcrypto_secret_lookup_as_utf8("sec0",
+                                             &error_abort);
+    g_assert_cmpstr(pw, ==, PAYLOAD);
+
+    object_unparent(sec);
+    g_free(pw);
+}
+
+
+static void test_secret_keyring_revoked_key(void)
+{
+    char key_str[16];
+    Object *sec;
+    int32_t key = add_key("user", DESCRIPTION, PAYLOAD,
+                          strlen(PAYLOAD), KEY_SPEC_PROCESS_KEYRING);
+    g_assert(key >= 0);
+    g_assert_false(keyctl_revoke(key));
+
+    snprintf(key_str, sizeof(key_str), "0x%08x", key);
+    sec = object_new_with_props(
+        TYPE_QCRYPTO_SECRET_KEYRING,
+        object_get_objects_root(),
+        "sec0",
+        NULL,
+        "serial", key_str,
+        NULL);
+
+    g_assert(errno == EKEYREVOKED);
+    g_assert(sec == NULL);
+
+    keyctl_unlink(key, KEY_SPEC_PROCESS_KEYRING);
+}
+
+
+static void test_secret_keyring_expired_key(void)
+{
+    char key_str[16];
+    Object *sec;
+    int32_t key = add_key("user", DESCRIPTION, PAYLOAD,
+                          strlen(PAYLOAD), KEY_SPEC_PROCESS_KEYRING);
+    g_assert(key >= 0);
+    g_assert_false(keyctl_set_timeout(key, 1));
+    sleep(1);
+
+    snprintf(key_str, sizeof(key_str), "0x%08x", key);
+    sec = object_new_with_props(
+        TYPE_QCRYPTO_SECRET_KEYRING,
+        object_get_objects_root(),
+        "sec0",
+        NULL,
+        "serial", key_str,
+        NULL);
+
+    g_assert(errno == EKEYEXPIRED);
+    g_assert(sec == NULL);
+
+    keyctl_unlink(key, KEY_SPEC_PROCESS_KEYRING);
+}
+
+
+static void test_secret_keyring_bad_serial_key(void)
+{
+    Object *sec;
+
+    sec = object_new_with_props(
+        TYPE_QCRYPTO_SECRET_KEYRING,
+        object_get_objects_root(),
+        "sec0",
+        NULL,
+        "serial", "1",
+        NULL);
+
+    g_assert(errno == ENOKEY);
+    g_assert(sec == NULL);
+}
+
+/*
+ * TODO
+ * test_secret_keyring_bad_key_access_right() is not working yet.
+ * We don't know yet if this due a bug in the Linux kernel or
+ * whether it's normal syscall behavior.
+ * We've requested information from kernel maintainers.
+ * See: <https://www.spinics.net/lists/keyrings/index.html>
+ * Thread: 'security/keys: remove possessor verify after key permission check'
+ */
+
+static void test_secret_keyring_bad_key_access_right(void)
+{
+    char key_str[16];
+    Object *sec;
+    int32_t key = add_key("user", DESCRIPTION, PAYLOAD,
+                          strlen(PAYLOAD), KEY_SPEC_PROCESS_KEYRING);
+    g_assert(key >= 0);
+    g_assert_false(keyctl_setperm(key, KEY_POS_ALL & (~KEY_POS_READ)));
+
+    snprintf(key_str, sizeof(key_str), "0x%08x", key);
+
+    sec = object_new_with_props(
+        TYPE_QCRYPTO_SECRET_KEYRING,
+        object_get_objects_root(),
+        "sec0",
+        NULL,
+        "serial", key_str,
+        NULL);
+
+    g_assert(errno == EACCES);
+    g_assert(sec == NULL);
+
+    keyctl_unlink(key, KEY_SPEC_PROCESS_KEYRING);
+}
+
+#endif /* CONFIG_TEST_SECRET_KEYRING */
 
 static void test_secret_noconv_base64_good(void)
 {
@@ -426,6 +567,19 @@ int main(int argc, char **argv)
     g_test_add_func("/crypto/secret/indirect/emptyfile",
                     test_secret_indirect_emptyfile);
 
+#ifdef CONFIG_TEST_SECRET_KEYRING
+    g_test_add_func("/crypto/secret/keyring/good",
+                    test_secret_keyring_good);
+    g_test_add_func("/crypto/secret/keyring/revoked_key",
+                    test_secret_keyring_revoked_key);
+    g_test_add_func("/crypto/secret/keyring/expired_key",
+                    test_secret_keyring_expired_key);
+    g_test_add_func("/crypto/secret/keyring/bad_serial_key",
+                    test_secret_keyring_bad_serial_key);
+    g_test_add_func("/crypto/secret/keyring/bad_key_access_right",
+                    test_secret_keyring_bad_key_access_right);
+#endif /* CONFIG_TEST_SECRET_KEYRING */
+
     g_test_add_func("/crypto/secret/noconv/base64/good",
                     test_secret_noconv_base64_good);
     g_test_add_func("/crypto/secret/noconv/base64/bad",
-- 
2.17.1

Re: [PATCH v3 3/3] test-crypto-secret: add 'secret_keyring' object tests.

[Daniel P. Berrangé]

On Mon, May 18, 2020 at 11:28:04PM +0300, Alexey Krasikov wrote:
> Add tests:
>   test_secret_keyring_good;
>   test_secret_keyring_revoked_key;
>   test_secret_keyring_expired_key;
>   test_secret_keyring_bad_serial_key;
>   test_secret_keyring_bad_key_access_right;
> 
> Added tests require libkeyutils. The absence of this library is not
> critical, because these tests will be skipped in this case.
> 
> Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
> ---
>  configure                  |  25 ++++++
>  tests/Makefile.include     |   4 +
>  tests/test-crypto-secret.c | 154 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 183 insertions(+)
> 
> diff --git a/configure b/configure
> index 1bae5ec0a1..2ab7d2961c 100755
> --- a/configure
> +++ b/configure
> @@ -6283,6 +6283,28 @@ but not implemented on your system"
>      fi
>  fi
>  
> +##########################################
> +# check for usable keyutils.h
> +
> +if test "$linux" = "yes" ; then
> +
> +    have_keyutils=no
> +    cat > $TMPC << EOF
> +#include <errno.h>
> +#include <asm/unistd.h>
> +#include <linux/keyctl.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <keyutils.h>
> +int main(void) {
> +    return request_key("user", NULL, NULL, 0);
> +}
> +EOF
> +    if compile_prog "" "-lkeyutils"; then
> +        have_keyutils=yes
> +    fi
> +fi

On my Fedora 32 system this always fails despite having keyutils-libs-devel
installed. Looking at config.log,  the linux/keyctl.h file defines structs
with names that clash with those in keyutils.h.  Removing the linux/keyctl.h
include here makes this test succeed, but.....


> +/*
> + * TODO
> + * test_secret_keyring_bad_key_access_right() is not working yet.
> + * We don't know yet if this due a bug in the Linux kernel or
> + * whether it's normal syscall behavior.
> + * We've requested information from kernel maintainers.
> + * See: <https://www.spinics.net/lists/keyrings/index.html>
> + * Thread: 'security/keys: remove possessor verify after key permission check'
> + */
> +
> +static void test_secret_keyring_bad_key_access_right(void)
> +{
> +    char key_str[16];
> +    Object *sec;
> +    int32_t key = add_key("user", DESCRIPTION, PAYLOAD,
> +                          strlen(PAYLOAD), KEY_SPEC_PROCESS_KEYRING);
> +    g_assert(key >= 0);
> +    g_assert_false(keyctl_setperm(key, KEY_POS_ALL & (~KEY_POS_READ)));
> +
> +    snprintf(key_str, sizeof(key_str), "0x%08x", key);
> +
> +    sec = object_new_with_props(
> +        TYPE_QCRYPTO_SECRET_KEYRING,
> +        object_get_objects_root(),
> +        "sec0",
> +        NULL,
> +        "serial", key_str,
> +        NULL);
> +
> +    g_assert(errno == EACCES);
> +    g_assert(sec == NULL);
> +
> +    keyctl_unlink(key, KEY_SPEC_PROCESS_KEYRING);
> +}

This causes the entire test suite to abort

ERROR:tests/test-crypto-secret.c:261:test_secret_keyring_bad_key_access_right: assertion failed: (errno == EACCES)
Bail out! ERROR:tests/test-crypto-secret.c:261:test_secret_keyring_bad_key_access_right: assertion failed: (errno == EACCES)

Either this test needs a temporary  "g_test_skip()" at the start of the
method to stop executing (if we expect Linux to be fixed), or it just 
needs deleting (if this is intentional Linux behaviour)

> +
> +#endif /* CONFIG_TEST_SECRET_KEYRING */
>  
>  static void test_secret_noconv_base64_good(void)
>  {
> @@ -426,6 +567,19 @@ int main(int argc, char **argv)
>      g_test_add_func("/crypto/secret/indirect/emptyfile",
>                      test_secret_indirect_emptyfile);
>  
> +#ifdef CONFIG_TEST_SECRET_KEYRING
> +    g_test_add_func("/crypto/secret/keyring/good",
> +                    test_secret_keyring_good);
> +    g_test_add_func("/crypto/secret/keyring/revoked_key",
> +                    test_secret_keyring_revoked_key);
> +    g_test_add_func("/crypto/secret/keyring/expired_key",
> +                    test_secret_keyring_expired_key);
> +    g_test_add_func("/crypto/secret/keyring/bad_serial_key",
> +                    test_secret_keyring_bad_serial_key);
> +    g_test_add_func("/crypto/secret/keyring/bad_key_access_right",
> +                    test_secret_keyring_bad_key_access_right);
> +#endif /* CONFIG_TEST_SECRET_KEYRING */
> +

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 :|