[RFC PATCH v2 1/5] crypto/secret: rename to secret_interface.

[RFC PATCH v2 1/5] crypto/secret: rename to secret_interface.

[Alexey Krasikov]

* Rename for future division into subclasses. Most part of the interface
  will remain in basic common class.

Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
---
 crypto/{secret.c => secret_interface.c}         | 0
 include/crypto/{secret.h => secret_interface.h} | 0
 2 files changed, 0 insertions(+), 0 deletions(-)
 rename crypto/{secret.c => secret_interface.c} (100%)
 rename include/crypto/{secret.h => secret_interface.h} (100%)

diff --git a/crypto/secret.c b/crypto/secret_interface.c
similarity index 100%
rename from crypto/secret.c
rename to crypto/secret_interface.c
diff --git a/include/crypto/secret.h b/include/crypto/secret_interface.h
similarity index 100%
rename from include/crypto/secret.h
rename to include/crypto/secret_interface.h
-- 
2.17.1

[RFC PATCH v2 2/5] crypto/secret_interface: conversion to common basic class.

[Alexey Krasikov]

* Remove individual option fields. Common field have been left.

Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
---
 crypto/Makefile.objs              |   1 +
 crypto/secret_interface.c         | 156 ++++++------------------------
 include/crypto/secret_interface.h | 119 ++++-------------------
 3 files changed, 51 insertions(+), 225 deletions(-)

diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index c2a371b0b4..3ae0dfd1a4 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_interface.o
 crypto-obj-y += secret.o
 crypto-obj-y += pbkdf.o
 crypto-obj-$(CONFIG_NETTLE) += pbkdf-nettle.o
diff --git a/crypto/secret_interface.c b/crypto/secret_interface.c
index 1cf0ad0ce8..9d8accdea3 100644
--- a/crypto/secret_interface.c
+++ b/crypto/secret_interface.c
@@ -19,7 +19,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "crypto/secret.h"
+#include "crypto/secret_interface.h"
 #include "crypto/cipher.h"
 #include "qapi/error.h"
 #include "qom/object_interfaces.h"
@@ -28,44 +28,7 @@
 #include "trace.h"
 
 
-static void
-qcrypto_secret_load_data(QCryptoSecret *secret,
-                         uint8_t **output,
-                         size_t *outputlen,
-                         Error **errp)
-{
-    char *data = NULL;
-    size_t length = 0;
-    GError *gerr = NULL;
-
-    *output = NULL;
-    *outputlen = 0;
-
-    if (secret->file) {
-        if (secret->data) {
-            error_setg(errp,
-                       "'file' and 'data' are mutually exclusive");
-            return;
-        }
-        if (!g_file_get_contents(secret->file, &data, &length, &gerr)) {
-            error_setg(errp,
-                       "Unable to read %s: %s",
-                       secret->file, gerr->message);
-            g_error_free(gerr);
-            return;
-        }
-        *output = (uint8_t *)data;
-        *outputlen = length;
-    } else if (secret->data) {
-        *outputlen = strlen(secret->data);
-        *output = (uint8_t *)g_strdup(secret->data);
-    } else {
-        error_setg(errp, "Either 'file' or 'data' must be provided");
-    }
-}
-
-
-static void qcrypto_secret_decrypt(QCryptoSecret *secret,
+static void qcrypto_secret_decrypt(QCryptoSecretCommon *secret,
                                    const uint8_t *input,
                                    size_t inputlen,
                                    uint8_t **output,
@@ -178,7 +141,9 @@ qcrypto_secret_prop_set_loaded(Object *obj,
                                bool value,
                                Error **errp)
 {
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
+    QCryptoSecretCommonClass *sec_class
+                                = QCRYPTO_SECRET_COMMON_GET_CLASS(obj);
 
     if (value) {
         Error *local_err = NULL;
@@ -187,9 +152,14 @@ qcrypto_secret_prop_set_loaded(Object *obj,
         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);
+        if (sec_class->load_data) {
+            sec_class->load_data(obj, &input, &inputlen, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        } else {
+            error_setg(errp, "'load_data' metod has not been initiated");
             return;
         }
 
@@ -230,7 +200,7 @@ static bool
 qcrypto_secret_prop_get_loaded(Object *obj,
                                Error **errp G_GNUC_UNUSED)
 {
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
     return secret->data != NULL;
 }
 
@@ -240,7 +210,7 @@ qcrypto_secret_prop_set_format(Object *obj,
                                int value,
                                Error **errp G_GNUC_UNUSED)
 {
-    QCryptoSecret *creds = QCRYPTO_SECRET(obj);
+    QCryptoSecretCommon *creds = QCRYPTO_SECRET_COMMON(obj);
 
     creds->format = value;
 }
@@ -250,60 +220,18 @@ static int
 qcrypto_secret_prop_get_format(Object *obj,
                                Error **errp G_GNUC_UNUSED)
 {
-    QCryptoSecret *creds = QCRYPTO_SECRET(obj);
+    QCryptoSecretCommon *creds = QCRYPTO_SECRET_COMMON(obj);
 
     return creds->format;
 }
 
 
-static void
-qcrypto_secret_prop_set_data(Object *obj,
-                             const char *value,
-                             Error **errp)
-{
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
-
-    g_free(secret->data);
-    secret->data = g_strdup(value);
-}
-
-
-static char *
-qcrypto_secret_prop_get_data(Object *obj,
-                             Error **errp)
-{
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
-    return g_strdup(secret->data);
-}
-
-
-static void
-qcrypto_secret_prop_set_file(Object *obj,
-                             const char *value,
-                             Error **errp)
-{
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
-
-    g_free(secret->file);
-    secret->file = g_strdup(value);
-}
-
-
-static char *
-qcrypto_secret_prop_get_file(Object *obj,
-                             Error **errp)
-{
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
-    return g_strdup(secret->file);
-}
-
-
 static void
 qcrypto_secret_prop_set_iv(Object *obj,
                            const char *value,
                            Error **errp)
 {
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
 
     g_free(secret->iv);
     secret->iv = g_strdup(value);
@@ -314,7 +242,7 @@ static char *
 qcrypto_secret_prop_get_iv(Object *obj,
                            Error **errp)
 {
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
     return g_strdup(secret->iv);
 }
 
@@ -324,7 +252,7 @@ qcrypto_secret_prop_set_keyid(Object *obj,
                               const char *value,
                               Error **errp)
 {
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
 
     g_free(secret->keyid);
     secret->keyid = g_strdup(value);
@@ -335,37 +263,24 @@ static char *
 qcrypto_secret_prop_get_keyid(Object *obj,
                               Error **errp)
 {
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
     return g_strdup(secret->keyid);
 }
 
 
-static void
-qcrypto_secret_complete(UserCreatable *uc, Error **errp)
-{
-    object_property_set_bool(OBJECT(uc), true, "loaded", errp);
-}
-
-
 static void
 qcrypto_secret_finalize(Object *obj)
 {
-    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+    QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(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);
-
-    ucc->complete = qcrypto_secret_complete;
-
     object_class_property_add_bool(oc, "loaded",
                                    qcrypto_secret_prop_get_loaded,
                                    qcrypto_secret_prop_set_loaded,
@@ -376,14 +291,6 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data)
                                    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,
-                                  NULL);
-    object_class_property_add_str(oc, "file",
-                                  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,
@@ -401,7 +308,7 @@ int qcrypto_secret_lookup(const char *secretid,
                           Error **errp)
 {
     Object *obj;
-    QCryptoSecret *secret;
+    QCryptoSecretCommon *secret;
 
     obj = object_resolve_path_component(
         object_get_objects_root(), secretid);
@@ -410,9 +317,9 @@ int qcrypto_secret_lookup(const char *secretid,
         return -1;
     }
 
-    secret = (QCryptoSecret *)
+    secret = (QCryptoSecretCommon *)
         object_dynamic_cast(obj,
-                            TYPE_QCRYPTO_SECRET);
+                            TYPE_QCRYPTO_SECRET_COMMON);
     if (!secret) {
         error_setg(errp, "Object with id '%s' is not a secret",
                    secretid);
@@ -480,16 +387,13 @@ char *qcrypto_secret_lookup_as_base64(const char *secretid,
 
 
 static const TypeInfo qcrypto_secret_info = {
-    .parent = TYPE_OBJECT,
-    .name = TYPE_QCRYPTO_SECRET,
-    .instance_size = sizeof(QCryptoSecret),
+    .parent            = TYPE_OBJECT,
+    .name              = TYPE_QCRYPTO_SECRET_COMMON,
+    .instance_size     = sizeof(QCryptoSecretCommon),
     .instance_finalize = qcrypto_secret_finalize,
-    .class_size = sizeof(QCryptoSecretClass),
-    .class_init = qcrypto_secret_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { TYPE_USER_CREATABLE },
-        { }
-    }
+    .class_size        = sizeof(QCryptoSecretCommonClass),
+    .class_init        = qcrypto_secret_class_init,
+    .abstract          = true,
 };
 
 
diff --git a/include/crypto/secret_interface.h b/include/crypto/secret_interface.h
index 5e07e29bae..4cd20503d5 100644
--- a/include/crypto/secret_interface.h
+++ b/include/crypto/secret_interface.h
@@ -18,120 +18,41 @@
  *
  */
 
-#ifndef QCRYPTO_SECRET_H
-#define QCRYPTO_SECRET_H
+#ifndef QCRYPTO_SECRET_COMMON_H
+#define QCRYPTO_SECRET_COMMON_H
 
 #include "qapi/qapi-types-crypto.h"
 #include "qom/object.h"
 
-#define TYPE_QCRYPTO_SECRET "secret"
-#define QCRYPTO_SECRET(obj)                  \
-    OBJECT_CHECK(QCryptoSecret, (obj), TYPE_QCRYPTO_SECRET)
+#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 QCryptoSecret QCryptoSecret;
-typedef struct QCryptoSecretClass QCryptoSecretClass;
+typedef struct QCryptoSecretCommon QCryptoSecretCommon;
+typedef struct QCryptoSecretCommonClass QCryptoSecretCommonClass;
 
-/**
- * QCryptoSecret:
- *
- * The QCryptoSecret object provides storage of secrets,
- * which may be user passwords, encryption keys or any
- * other kind of sensitive data that is represented as
- * a sequence of bytes.
- *
- * The sensitive data associated with the secret can
- * be provided directly via the 'data' property, or
- * indirectly via the 'file' property. In the latter
- * case there is support for file descriptor passing
- * via the usual /dev/fdset/NN syntax that QEMU uses.
- *
- * The data for a secret can be provided in two formats,
- * either as a UTF-8 string (the default), or as base64
- * encoded 8-bit binary data. The latter is appropriate
- * for raw encryption keys, while the former is appropriate
- * for user entered passwords.
- *
- * The data may be optionally encrypted with AES-256-CBC,
- * and the decryption key provided by another
- * QCryptoSecret instance identified by the 'keyid'
- * property. When passing sensitive data directly
- * via the 'data' property it is strongly recommended
- * to use the AES encryption facility to prevent the
- * sensitive data being exposed in the process listing
- * or system log files.
- *
- * Providing data directly, insecurely (suitable for
- * ad hoc developer testing only)
- *
- *  $QEMU -object secret,id=sec0,data=letmein
- *
- * Providing data indirectly:
- *
- *  # printf "letmein" > password.txt
- *  # $QEMU \
- *      -object secret,id=sec0,file=password.txt
- *
- * Using a master encryption key with data.
- *
- * The master key needs to be created as 32 secure
- * random bytes (optionally base64 encoded)
- *
- *  # openssl rand -base64 32 > key.b64
- *  # KEY=$(base64 -d key.b64 | hexdump  -v -e '/1 "%02X"')
- *
- * Each secret to be encrypted needs to have a random
- * initialization vector generated. These do not need
- * to be kept secret
- *
- *  # openssl rand -base64 16 > iv.b64
- *  # IV=$(base64 -d iv.b64 | hexdump  -v -e '/1 "%02X"')
- *
- * A secret to be defined can now be encrypted
- *
- *  # SECRET=$(printf "letmein" |
- *             openssl enc -aes-256-cbc -a -K $KEY -iv $IV)
- *
- * When launching QEMU, create a master secret pointing
- * to key.b64 and specify that to be used to decrypt
- * the user password
- *
- *  # $QEMU \
- *      -object secret,id=secmaster0,format=base64,file=key.b64 \
- *      -object secret,id=sec0,keyid=secmaster0,format=base64,\
- *          data=$SECRET,iv=$(<iv.b64)
- *
- * When encrypting, the data can still be provided via an
- * external file, in which case it is possible to use either
- * raw binary data, or base64 encoded. This example uses
- * raw format
- *
- *  # printf "letmein" |
- *       openssl enc -aes-256-cbc -K $KEY -iv $IV -o pw.aes
- *  # $QEMU \
- *      -object secret,id=secmaster0,format=base64,file=key.b64 \
- *      -object secret,id=sec0,keyid=secmaster0,\
- *          file=pw.aes,iv=$(<iv.b64)
- *
- * Note that the ciphertext can be in either raw or base64
- * format, as indicated by the 'format' parameter, but the
- * plaintext resulting from decryption is expected to always
- * be in raw format.
- */
-
-struct QCryptoSecret {
+struct QCryptoSecretCommon {
     Object parent_obj;
     uint8_t *rawdata;
     size_t rawlen;
     QCryptoSecretFormat format;
-    char *data;
-    char *file;
     char *keyid;
     char *iv;
 };
 
 
-struct QCryptoSecretClass {
+struct QCryptoSecretCommonClass {
     ObjectClass parent_class;
+    void (*load_data)(Object  *obj,
+                      uint8_t **output,
+                      size_t  *outputlen,
+                      Error   **errp);
 };
 
 
@@ -144,4 +65,4 @@ extern char *qcrypto_secret_lookup_as_utf8(const char *secretid,
 extern char *qcrypto_secret_lookup_as_base64(const char *secretid,
                                              Error **errp);
 
-#endif /* QCRYPTO_SECRET_H */
+#endif /* QCRYPTO_SECRET_COMMON_H */
-- 
2.17.1

[RFC PATCH v2 3/5] crypto/secret: add secret class files.

[Alexey Krasikov]

* Add child 'secret' class from basic 'secret_common'
  with 'data' and 'file' properties.

Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
---
 crypto/secret.c         | 167 ++++++++++++++++++++++++++++++++++++++++
 include/crypto/secret.h | 133 ++++++++++++++++++++++++++++++++
 2 files changed, 300 insertions(+)
 create mode 100644 crypto/secret.c
 create mode 100644 include/crypto/secret.h

diff --git a/crypto/secret.c b/crypto/secret.c
new file mode 100644
index 0000000000..d9be0409e4
--- /dev/null
+++ b/crypto/secret.c
@@ -0,0 +1,167 @@
+/*
+ * 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.h"
+#include "qapi/error.h"
+#include "qom/object_interfaces.h"
+#include "qemu/module.h"
+#include "trace.h"
+
+
+static void
+qcrypto_secret_load_data(Object *obj,
+                         uint8_t **output,
+                         size_t *outputlen,
+                         Error **errp)
+{
+    char *data = NULL;
+    size_t length = 0;
+    GError *gerr = NULL;
+
+    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+
+    *output = NULL;
+    *outputlen = 0;
+
+    if (secret->file) {
+        if (secret->data) {
+            error_setg(errp,
+                       "'file' and 'data' are mutually exclusive");
+            return;
+        }
+        if (!g_file_get_contents(secret->file, &data, &length, &gerr)) {
+            error_setg(errp,
+                       "Unable to read %s: %s",
+                       secret->file, gerr->message);
+            g_error_free(gerr);
+            return;
+        }
+        *output = (uint8_t *)data;
+        *outputlen = length;
+    } else if (secret->data) {
+        *outputlen = strlen(secret->data);
+        *output = (uint8_t *)g_strdup(secret->data);
+    } else {
+        error_setg(errp, "Either 'file' or 'data' must be provided");
+    }
+}
+
+
+static void
+qcrypto_secret_prop_set_data(Object *obj,
+                             const char *value,
+                             Error **errp)
+{
+    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+
+    g_free(secret->data);
+    secret->data = g_strdup(value);
+}
+
+
+static char *
+qcrypto_secret_prop_get_data(Object *obj,
+                             Error **errp)
+{
+    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+    return g_strdup(secret->data);
+}
+
+
+static void
+qcrypto_secret_prop_set_file(Object *obj,
+                             const char *value,
+                             Error **errp)
+{
+    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+
+    g_free(secret->file);
+    secret->file = g_strdup(value);
+}
+
+
+static char *
+qcrypto_secret_prop_get_file(Object *obj,
+                             Error **errp)
+{
+    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+    return g_strdup(secret->file);
+}
+
+
+static void
+qcrypto_secret_complete(UserCreatable *uc, Error **errp)
+{
+    object_property_set_bool(OBJECT(uc), true, "loaded", errp);
+}
+
+
+static void
+qcrypto_secret_finalize(Object *obj)
+{
+    QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+
+    g_free(secret->file);
+    g_free(secret->data);
+}
+
+static void
+qcrypto_secret_class_init(ObjectClass *oc, void *data)
+{
+    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_str(oc, "data",
+                                  qcrypto_secret_prop_get_data,
+                                  qcrypto_secret_prop_set_data,
+                                  NULL);
+    object_class_property_add_str(oc, "file",
+                                  qcrypto_secret_prop_get_file,
+                                  qcrypto_secret_prop_set_file,
+                                  NULL);
+}
+
+
+static const TypeInfo qcrypto_secret_info = {
+    .parent = TYPE_QCRYPTO_SECRET_COMMON,
+    .name = TYPE_QCRYPTO_SECRET,
+    .instance_size = sizeof(QCryptoSecret),
+    .instance_finalize = qcrypto_secret_finalize,
+    .class_size = sizeof(QCryptoSecretClass),
+    .class_init = qcrypto_secret_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.h b/include/crypto/secret.h
new file mode 100644
index 0000000000..2ce8dcc24f
--- /dev/null
+++ b/include/crypto/secret.h
@@ -0,0 +1,133 @@
+/*
+ * 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_H
+#define QCRYPTO_SECRET_H
+
+#include "qapi/qapi-types-crypto.h"
+#include "qom/object.h"
+#include "crypto/secret_interface.h"
+
+#define TYPE_QCRYPTO_SECRET "secret"
+#define QCRYPTO_SECRET(obj) \
+    OBJECT_CHECK(QCryptoSecret, (obj), TYPE_QCRYPTO_SECRET)
+
+typedef struct QCryptoSecret QCryptoSecret;
+typedef struct QCryptoSecretClass QCryptoSecretClass;
+
+/**
+ * QCryptoSecret:
+ *
+ * The QCryptoSecret object provides storage of secrets,
+ * which may be user passwords, encryption keys or any
+ * other kind of sensitive data that is represented as
+ * a sequence of bytes.
+ *
+ * The sensitive data associated with the secret can
+ * be provided directly via the 'data' property, or
+ * indirectly via the 'file' property. In the latter
+ * case there is support for file descriptor passing
+ * via the usual /dev/fdset/NN syntax that QEMU uses.
+ *
+ * The data for a secret can be provided in two formats,
+ * either as a UTF-8 string (the default), or as base64
+ * encoded 8-bit binary data. The latter is appropriate
+ * for raw encryption keys, while the former is appropriate
+ * for user entered passwords.
+ *
+ * The data may be optionally encrypted with AES-256-CBC,
+ * and the decryption key provided by another
+ * QCryptoSecret instance identified by the 'keyid'
+ * property. When passing sensitive data directly
+ * via the 'data' property it is strongly recommended
+ * to use the AES encryption facility to prevent the
+ * sensitive data being exposed in the process listing
+ * or system log files.
+ *
+ * Providing data directly, insecurely (suitable for
+ * ad hoc developer testing only)
+ *
+ *  $QEMU -object secret,id=sec0,data=letmein
+ *
+ * Providing data indirectly:
+ *
+ *  # printf "letmein" > password.txt
+ *  # $QEMU \
+ *      -object secret,id=sec0,file=password.txt
+ *
+ * Using a master encryption key with data.
+ *
+ * The master key needs to be created as 32 secure
+ * random bytes (optionally base64 encoded)
+ *
+ *  # openssl rand -base64 32 > key.b64
+ *  # KEY=$(base64 -d key.b64 | hexdump  -v -e '/1 "%02X"')
+ *
+ * Each secret to be encrypted needs to have a random
+ * initialization vector generated. These do not need
+ * to be kept secret
+ *
+ *  # openssl rand -base64 16 > iv.b64
+ *  # IV=$(base64 -d iv.b64 | hexdump  -v -e '/1 "%02X"')
+ *
+ * A secret to be defined can now be encrypted
+ *
+ *  # SECRET=$(printf "letmein" |
+ *             openssl enc -aes-256-cbc -a -K $KEY -iv $IV)
+ *
+ * When launching QEMU, create a master secret pointing
+ * to key.b64 and specify that to be used to decrypt
+ * the user password
+ *
+ *  # $QEMU \
+ *      -object secret,id=secmaster0,format=base64,file=key.b64 \
+ *      -object secret,id=sec0,keyid=secmaster0,format=base64,\
+ *          data=$SECRET,iv=$(<iv.b64)
+ *
+ * When encrypting, the data can still be provided via an
+ * external file, in which case it is possible to use either
+ * raw binary data, or base64 encoded. This example uses
+ * raw format
+ *
+ *  # printf "letmein" |
+ *       openssl enc -aes-256-cbc -K $KEY -iv $IV -o pw.aes
+ *  # $QEMU \
+ *      -object secret,id=secmaster0,format=base64,file=key.b64 \
+ *      -object secret,id=sec0,keyid=secmaster0,\
+ *          file=pw.aes,iv=$(<iv.b64)
+ *
+ * Note that the ciphertext can be in either raw or base64
+ * format, as indicated by the 'format' parameter, but the
+ * plaintext resulting from decryption is expected to always
+ * be in raw format.
+ */
+
+struct QCryptoSecret {
+    QCryptoSecretCommon parent_obj;
+    char *data;
+    char *file;
+};
+
+
+struct QCryptoSecretClass {
+    QCryptoSecretCommonClass parent_class;
+};
+
+#endif /* QCRYPTO_SECRET_H */
-- 
2.17.1

[RFC PATCH v2 4/5] crypto/linux_keyring: add 'syskey' 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
  rights to read it.

Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
---
 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

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
 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);
+    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;
+        }
+    } else {
+      error_setg(errp, "Either 'serial' 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;
+
+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 */
-- 
2.17.1

[RFC PATCH v2 5/5] test-crypto-secret: add 'syskey' object tests.

[Alexey Krasikov]

* test_secret_seckey_bad_key_access_right() is not working yet.
  We don't know yet if this due a bag in the Linux kernel or
  whether it's normal syscall behavior.
  We've requested information from kernel maintainer.

Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
---
 tests/test-crypto-secret.c | 138 +++++++++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)

diff --git a/tests/test-crypto-secret.c b/tests/test-crypto-secret.c
index 13fc6c4c75..6b17fe3a81 100644
--- a/tests/test-crypto-secret.c
+++ b/tests/test-crypto-secret.c
@@ -22,8 +22,10 @@
 
 #include "crypto/init.h"
 #include "crypto/secret.h"
+#include "crypto/linux_keyring.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include <keyutils.h>
 
 static void test_secret_direct(void)
 {
@@ -125,6 +127,132 @@ static void test_secret_indirect_emptyfile(void)
 }
 
 
+#define DESCRIPTION "qemu_test_secret"
+#define PAYLOAD "Test Payload"
+
+
+static void test_secret_seckey_good(void)
+{
+    char key_str[16];
+    Object *sec;
+    key_serial_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_LINUX_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_seckey_revoked_key(void)
+{
+    char key_str[16];
+    Object *sec;
+    key_serial_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_LINUX_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_seckey_expired_key(void)
+{
+    char key_str[16];
+    Object *sec;
+    key_serial_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_LINUX_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_seckey_bad_serial_key(void)
+{
+    Object *sec;
+
+    sec = object_new_with_props(
+        TYPE_QCRYPTO_SECRET,
+        object_get_objects_root(),
+        "sec0",
+        NULL,
+        "serial", "1",
+        NULL);
+
+    g_assert(errno == ENOKEY);
+    g_assert(sec == NULL);
+}
+
+
+static void test_secret_seckey_bad_key_access_right(void)
+{
+    char key_str[16];
+    Object *sec;
+    key_serial_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_LINUX_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);
+}
+
+
 static void test_secret_noconv_base64_good(void)
 {
     Object *sec = object_new_with_props(
@@ -425,6 +553,16 @@ int main(int argc, char **argv)
                     test_secret_indirect_badfile);
     g_test_add_func("/crypto/secret/indirect/emptyfile",
                     test_secret_indirect_emptyfile);
+    g_test_add_func("/crypto/secret/seckey/good",
+                    test_secret_seckey_good);
+    g_test_add_func("/crypto/secret/seckey/revoked_key",
+                    test_secret_seckey_revoked_key);
+    g_test_add_func("/crypto/secret/seckey/expired_key",
+                    test_secret_seckey_expired_key);
+    g_test_add_func("/crypto/secret/seckey/bad_serial_key",
+                    test_secret_seckey_bad_serial_key);
+    g_test_add_func("/crypto/secret/seckey/bad_key_access_right",
+                    test_secret_seckey_bad_key_access_right);
 
     g_test_add_func("/crypto/secret/noconv/base64/good",
                     test_secret_noconv_base64_good);
-- 
2.17.1

Re: [RFC PATCH v2 1/5] crypto/secret: rename to secret_interface.

[Markus Armbruster]

Consider including a cover letter in the future, and use it to explain
what you're proposing and why.  The why is even more important for an
RFC.

Re: [RFC PATCH v2 1/5] crypto/secret: rename to secret_interface.

[Daniel P. Berrangé]

On Thu, Apr 16, 2020 at 01:25:21AM +0300, Alexey Krasikov wrote:
> * Rename for future division into subclasses. Most part of the interface
>   will remain in basic common class.

You don't need to put bullet points in the commit message, just
have the text.

> 
> Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
> ---
>  crypto/{secret.c => secret_interface.c}         | 0
>  include/crypto/{secret.h => secret_interface.h} | 0
>  2 files changed, 0 insertions(+), 0 deletions(-)
>  rename crypto/{secret.c => secret_interface.c} (100%)
>  rename include/crypto/{secret.h => secret_interface.h} (100%)

This breaks the build because Makefile.objs doesn't reference
the new filename, and likewise other files doing #include
don't work.

I don't think renaming actually makes sense in the first place,
because you then add the original files back again in a later
patch.

You need to just have a patch which introduces secret_interface.{ch}
without killing the original secret.{c,h} entirely.  The key point is
that QEMU must successfully compile on each individual patch in the
series, otherwise it breaks "git bisect" usage.

Also, since the object is called "SecretCommon",the filenames
should match that "secret_common.{ch}"


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

Re: [RFC PATCH v2 4/5] crypto/linux_keyring: add 'syskey' secret object.

[Daniel P. Berrangé]

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

Re: [RFC PATCH v2 5/5] test-crypto-secret: add 'syskey' object tests.

[Daniel P. Berrangé]

On Thu, Apr 16, 2020 at 01:25:25AM +0300, Alexey Krasikov wrote:
> * test_secret_seckey_bad_key_access_right() is not working yet.
>   We don't know yet if this due a bag in the Linux kernel or
>   whether it's normal syscall behavior.
>   We've requested information from kernel maintainer.
> 
> Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
> ---
>  tests/test-crypto-secret.c | 138 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 138 insertions(+)
> 
> diff --git a/tests/test-crypto-secret.c b/tests/test-crypto-secret.c
> index 13fc6c4c75..6b17fe3a81 100644
> --- a/tests/test-crypto-secret.c
> +++ b/tests/test-crypto-secret.c
> @@ -22,8 +22,10 @@
>  
>  #include "crypto/init.h"
>  #include "crypto/secret.h"
> +#include "crypto/linux_keyring.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
> +#include <keyutils.h>
>  
>  static void test_secret_direct(void)
>  {
> @@ -125,6 +127,132 @@ static void test_secret_indirect_emptyfile(void)
>  }
>  
>  
> +#define DESCRIPTION "qemu_test_secret"
> +#define PAYLOAD "Test Payload"
> +
> +
> +static void test_secret_seckey_good(void)
> +{
> +    char key_str[16];
> +    Object *sec;
> +    key_serial_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_LINUX_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_seckey_revoked_key(void)
> +{
> +    char key_str[16];
> +    Object *sec;
> +    key_serial_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_LINUX_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_seckey_expired_key(void)
> +{
> +    char key_str[16];
> +    Object *sec;
> +    key_serial_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_LINUX_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_seckey_bad_serial_key(void)
> +{
> +    Object *sec;
> +
> +    sec = object_new_with_props(
> +        TYPE_QCRYPTO_SECRET,
> +        object_get_objects_root(),
> +        "sec0",
> +        NULL,
> +        "serial", "1",
> +        NULL);
> +
> +    g_assert(errno == ENOKEY);
> +    g_assert(sec == NULL);
> +}
> +
> +
> +static void test_secret_seckey_bad_key_access_right(void)
> +{
> +    char key_str[16];
> +    Object *sec;
> +    key_serial_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_LINUX_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);
> +}
> +
> +
>  static void test_secret_noconv_base64_good(void)
>  {
>      Object *sec = object_new_with_props(
> @@ -425,6 +553,16 @@ int main(int argc, char **argv)
>                      test_secret_indirect_badfile);
>      g_test_add_func("/crypto/secret/indirect/emptyfile",
>                      test_secret_indirect_emptyfile);
> +    g_test_add_func("/crypto/secret/seckey/good",
> +                    test_secret_seckey_good);
> +    g_test_add_func("/crypto/secret/seckey/revoked_key",
> +                    test_secret_seckey_revoked_key);
> +    g_test_add_func("/crypto/secret/seckey/expired_key",
> +                    test_secret_seckey_expired_key);
> +    g_test_add_func("/crypto/secret/seckey/bad_serial_key",
> +                    test_secret_seckey_bad_serial_key);
> +    g_test_add_func("/crypto/secret/seckey/bad_key_access_right",
> +                    test_secret_seckey_bad_key_access_right);

Again, we'll need to make this all conditional based on the result
of a configure check to avoid breaking non-Linux.


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