Re: [RFC PATCH] crypto/secret: support fetching secrets from Linux keyring

2020-03-30 Thread Daniel P . Berrangé 
On Sat, Mar 28, 2020 at 02:40:14PM +0300, Alexey Krasikov wrote:
> Add the ability for the secret object to obtain secret data from the
> Linux in-kernel key managment and retention facility, as an extra option
> to the existing ones: reading from a file or passing directly as a
> string.
> 
> The secret is identified by the key serial number.  The upper layers
> need to instantiate the key and make sure the QEMU process has access
> rights to read it.

This looks like a generally useful concept to me, however, I think
it highlights a mistake in the original secrets design. We should
have had TYPE_QCRYPTO_SECRET be an abstract interface, and then
had subclasses  TYPE_QCRYPTO_SECRET_FILE, TYPE_QCRYPTO_SECRET_INLINE
Then we could add TYPE_QCRYPTO_SECRET_LINUX.

We can still mostly fix that mistake now though.

We can introduce a TYPE_QCRYPTO_SECRET_INTERFACE which defines
the generic interface. This interface just needs to define one
API entry point

"char *get_data(QCryptoSecretInterface *secret)"

The current TYPE_QCRYPTO_SECRET can be the current impl of that
interface that returns the rawdata field.

Then we can add the new TYPE_QCRYPTO_SECRET_LINUX for the keyring
implementation that does the Linux specific magic. This will let
us easily compile out the Linux impl on platforms where it is not
available too.



> 
> Signed-off-by: Alexey Krasikov 
> ---
>  crypto/secret.c | 88 +++--
>  include/crypto/secret.h |  3 ++
>  2 files changed, 88 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/secret.c b/crypto/secret.c
> index 1cf0ad0ce8..2e8be6241c 100644
> --- a/crypto/secret.c
> +++ b/crypto/secret.c
> @@ -19,6 +19,8 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include 
> +#include 
>  #include "crypto/secret.h"
>  #include "crypto/cipher.h"
>  #include "qapi/error.h"
> @@ -28,6 +30,40 @@
>  #include "trace.h"
>  
>  
> +static inline
> +long keyctl_read(key_serial_t key, uint8_t *buffer, size_t buflen)
> +{
> +#ifdef __NR_keyctl
> +return syscall(__NR_keyctl, KEYCTL_READ, key, buffer, buflen, 0);
> +#else
> +errno = ENOSYS;
> +return -1;
> +#endif
> +}
> +
> +static
> +long keyctl_read_alloc(key_serial_t key, uint8_t **buffer)
> +{
> +uint8_t *loc_buf;
> +long retcode = keyctl_read(key, NULL, 0);
> +if (retcode < 0) {
> +return retcode;
> +}
> +loc_buf = g_malloc(retcode + 1);
> +retcode = keyctl_read(key, loc_buf, retcode + 1);
> +   /*
> +* We don't have key operations locks between syscalls.
> +* For example, the key could have been removed or expired.
> +*/
> +if (retcode >= 0) {
> +loc_buf[retcode] = '\0';
> +*buffer = loc_buf;
> +} else {
> +g_free(loc_buf);
> +}
> +return retcode;
> +}
> +
>  static void
>  qcrypto_secret_load_data(QCryptoSecret *secret,
>   uint8_t **output,
> @@ -41,10 +77,28 @@ qcrypto_secret_load_data(QCryptoSecret *secret,
>  *output = NULL;
>  *outputlen = 0;
>  
> -if (secret->file) {
> +if (secret->syskey) {
> +uint8_t *buffer = NULL;
> +long retcode;
> +if (secret->data || secret->file) {
> +error_setg(errp,
> +   "'syskey', 'file' and 'data' are mutually exclusive");
> +return;
> +}
> +retcode = keyctl_read_alloc(secret->syskey, );
> +if (retcode < 0) {
> +error_setg_errno(errp, errno,
> +   "Unable to read serial key %08x",
> +   secret->syskey);
> +return;
> +} else {
> +*outputlen = retcode;
> +*output = buffer;
> +}
> +} else if (secret->file) {
>  if (secret->data) {
>  error_setg(errp,
> -   "'file' and 'data' are mutually exclusive");
> +   "'syskey', 'file' and 'data' are mutually exclusive");
>  return;
>  }
>  if (!g_file_get_contents(secret->file, , , )) {
> @@ -60,7 +114,8 @@ qcrypto_secret_load_data(QCryptoSecret *secret,
>  *outputlen = strlen(secret->data);
>  *output = (uint8_t *)g_strdup(secret->data);
>  } else {
> -error_setg(errp, "Either 'file' or 'data' must be provided");
> +error_setg(errp,
> +   "Either 'syskey' or 'file' or 'data' must be provided");
>  }
>  }
>  
> @@ -298,6 +353,29 @@ qcrypto_secret_prop_get_file(Object *obj,
>  }
>  
>  
> +static void
> +qcrypto_secret_prop_set_syskey(Object *obj, Visitor *v,
> +   const char *name, void *opaque,
> +   Error **errp)
> +{
> +QCryptoSecret *secret = QCRYPTO_SECRET(obj);
> +int32_t value;
> +visit_type_int32(v, name, , errp);
> +secret->syskey = value;
> +}
> +
> +
> +static void
> +qcrypto_secret_prop_get_syskey(Object *obj, Visitor *v,
> +   const char *name, void

Re: [RFC PATCH] crypto/secret: support fetching secrets from Linux keyring

2020-03-28 Thread no-reply 
Patchew URL: 
https://patchew.org/QEMU/20200328114014.6362-1-alex-krasi...@yandex-team.ru/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  chardev/char-ringbuf.o
  CC  chardev/char-serial.o
  CC  chardev/char-socket.o
/tmp/qemu-test/src/crypto/secret.c:22:10: fatal error: asm/unistd.h: No such 
file or directory
 #include 
  ^~
compilation terminated.
make: *** [/tmp/qemu-test/src/rules.mak:69: crypto/secret.o] Error 1
make: *** Waiting for unfinished jobs
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=4961116ada114c5d93e518eb1e9d8685', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-rgq6jw7h/src/docker-src.2020-03-28-11.34.05.19902:/var/tmp/qemu:z,ro',
 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=4961116ada114c5d93e518eb1e9d8685
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-rgq6jw7h/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real1m49.176s
user0m7.677s


The full log is available at
http://patchew.org/logs/20200328114014.6362-1-alex-krasi...@yandex-team.ru/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[RFC PATCH] crypto/secret: support fetching secrets from Linux keyring

2020-03-28 Thread 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 
---
 crypto/secret.c | 88 +++--
 include/crypto/secret.h |  3 ++
 2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/crypto/secret.c b/crypto/secret.c
index 1cf0ad0ce8..2e8be6241c 100644
--- a/crypto/secret.c
+++ b/crypto/secret.c
@@ -19,6 +19,8 @@
  */
 
 #include "qemu/osdep.h"
+#include 
+#include 
 #include "crypto/secret.h"
 #include "crypto/cipher.h"
 #include "qapi/error.h"
@@ -28,6 +30,40 @@
 #include "trace.h"
 
 
+static inline
+long keyctl_read(key_serial_t key, uint8_t *buffer, size_t buflen)
+{
+#ifdef __NR_keyctl
+return syscall(__NR_keyctl, KEYCTL_READ, key, buffer, buflen, 0);
+#else
+errno = ENOSYS;
+return -1;
+#endif
+}
+
+static
+long keyctl_read_alloc(key_serial_t key, uint8_t **buffer)
+{
+uint8_t *loc_buf;
+long retcode = keyctl_read(key, NULL, 0);
+if (retcode < 0) {
+return retcode;
+}
+loc_buf = g_malloc(retcode + 1);
+retcode = keyctl_read(key, loc_buf, retcode + 1);
+   /*
+* We don't have key operations locks between syscalls.
+* For example, the key could have been removed or expired.
+*/
+if (retcode >= 0) {
+loc_buf[retcode] = '\0';
+*buffer = loc_buf;
+} else {
+g_free(loc_buf);
+}
+return retcode;
+}
+
 static void
 qcrypto_secret_load_data(QCryptoSecret *secret,
  uint8_t **output,
@@ -41,10 +77,28 @@ qcrypto_secret_load_data(QCryptoSecret *secret,
 *output = NULL;
 *outputlen = 0;
 
-if (secret->file) {
+if (secret->syskey) {
+uint8_t *buffer = NULL;
+long retcode;
+if (secret->data || secret->file) {
+error_setg(errp,
+   "'syskey', 'file' and 'data' are mutually exclusive");
+return;
+}
+retcode = keyctl_read_alloc(secret->syskey, );
+if (retcode < 0) {
+error_setg_errno(errp, errno,
+   "Unable to read serial key %08x",
+   secret->syskey);
+return;
+} else {
+*outputlen = retcode;
+*output = buffer;
+}
+} else if (secret->file) {
 if (secret->data) {
 error_setg(errp,
-   "'file' and 'data' are mutually exclusive");
+   "'syskey', 'file' and 'data' are mutually exclusive");
 return;
 }
 if (!g_file_get_contents(secret->file, , , )) {
@@ -60,7 +114,8 @@ qcrypto_secret_load_data(QCryptoSecret *secret,
 *outputlen = strlen(secret->data);
 *output = (uint8_t *)g_strdup(secret->data);
 } else {
-error_setg(errp, "Either 'file' or 'data' must be provided");
+error_setg(errp,
+   "Either 'syskey' or 'file' or 'data' must be provided");
 }
 }
 
@@ -298,6 +353,29 @@ qcrypto_secret_prop_get_file(Object *obj,
 }
 
 
+static void
+qcrypto_secret_prop_set_syskey(Object *obj, Visitor *v,
+   const char *name, void *opaque,
+   Error **errp)
+{
+QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+int32_t value;
+visit_type_int32(v, name, , errp);
+secret->syskey = value;
+}
+
+
+static void
+qcrypto_secret_prop_get_syskey(Object *obj, Visitor *v,
+   const char *name, void *opaque,
+   Error **errp)
+{
+QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+int32_t value = secret->syskey;
+visit_type_int32(v, name, , errp);
+}
+
+
 static void
 qcrypto_secret_prop_set_iv(Object *obj,
const char *value,
@@ -384,6 +462,10 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data)
   qcrypto_secret_prop_get_file,
   qcrypto_secret_prop_set_file,
   NULL);
+object_class_property_add(oc, "syskey", "key_serial_t",
+  qcrypto_secret_prop_get_syskey,
+  qcrypto_secret_prop_set_syskey,
+  NULL, NULL, NULL);
 object_class_property_add_str(oc, "keyid",
   qcrypto_secret_prop_get_keyid,
   qcrypto_secret_prop_set_keyid,
diff --git a/include/crypto/secret.h b/include/crypto/secret.h
index 5e07e29bae..9d350e35ed 100644
--- a/include/crypto/secret.h
+++ b/include/crypto/secret.h
@@ -31,6 +31,8 @@
 typedef struct QCryptoSecret QCryptoSecret;
 typedef struct