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

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