secret: rework handling of private secrets

A secret can be marked with the "private" attribute. The intent was that
it is not possible for any libvirt client to be able to read the secret
value, it would only be accesible from within libvirtd. eg the QEMU
driver can read the value to launch a guest.

With the modular daemons, the QEMU, storage and secret drivers are all
running in separate daemons. The QEMU and storage drivers thus appear to
be normal libvirt client's from the POV of the secret driver, and thus
they are not able to read a private secret. This is unhelpful.

With the previous patches that introduced a "system token" to the
identity object, we can now distinguish APIs invoked by libvirt daemons
from those invoked by client applications.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/driver-secret.h b/src/driver-secret.h
index eb6e82478c43e602a8b861b2c0aae5b93aa694bf..1d21f62bb36a5d18bba1ce0181612c235e33999f 100644 (file)
--- a/src/driver-secret.h
+++ b/src/driver-secret.h
@@ -24,12 +24,6 @@
 # error "Don't include this file directly, only use driver.h"
 #endif
 
-enum {
-    /* This getValue call is inside libvirt, override the "private" flag.
-       This flag cannot be set by outside callers. */
-    VIR_SECRET_GET_VALUE_INTERNAL_CALL = 1 << 0,
-};
-
 typedef virSecretPtr
 (*virDrvSecretLookupByUUID)(virConnectPtr conn,
                             const unsigned char *uuid);
@@ -57,8 +51,7 @@ typedef int
 typedef unsigned char *
 (*virDrvSecretGetValue)(virSecretPtr secret,
                         size_t *value_size,
-                        unsigned int flags,
-                        unsigned int internalFlags);
+                        unsigned int flags);
 
 typedef int
 (*virDrvSecretUndefine)(virSecretPtr secret);

diff --git a/src/libvirt-secret.c b/src/libvirt-secret.c
index 75d40f53dc841995e70aa760d5e3078edfc6a6d6..a427805c7a5eb378c99fb4d32d0827cab24f2873 100644 (file)
--- a/src/libvirt-secret.c
+++ b/src/libvirt-secret.c
@@ -585,7 +585,7 @@ virSecretGetValue(virSecretPtr secret, size_t *value_size, unsigned int flags)
     if (conn->secretDriver != NULL && conn->secretDriver->secretGetValue != NULL) {
         unsigned char *ret;
 
-        ret = conn->secretDriver->secretGetValue(secret, value_size, flags, 0);
+        ret = conn->secretDriver->secretGetValue(secret, value_size, flags);
         if (ret == NULL)
             goto error;
         return ret;

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 0c72d699335493456d29442ed4af0f07c28e3b42..eed99af1278b590bf7ace2a9db2d5287e37964c1 100644 (file)
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -5382,7 +5382,7 @@ remoteDomainBuildQemuMonitorEvent(virNetClientProgram *prog G_GNUC_UNUSED,
 
 static unsigned char *
 remoteSecretGetValue(virSecretPtr secret, size_t *value_size,
-                     unsigned int flags, unsigned int internalFlags)
+                     unsigned int flags)
 {
     unsigned char *rv = NULL;
     remote_secret_get_value_args args;
@@ -5391,12 +5391,6 @@ remoteSecretGetValue(virSecretPtr secret, size_t *value_size,
 
     remoteDriverLock(priv);
 
-    /* internalFlags intentionally do not go over the wire */
-    if (internalFlags) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no internalFlags support"));
-        goto done;
-    }
-
     make_nonnull_secret(&args.secret, secret);
     args.flags = flags;
 

diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 6ea8cc8ce9cb8692ab351a4de231ff358fbcdc96..d2175de8ed7ecf440a5d57ba5847574bd1e531fe 100644 (file)
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -36,6 +36,7 @@
 #include "viruuid.h"
 #include "virerror.h"
 #include "virfile.h"
+#include "viridentity.h"
 #include "virpidfile.h"
 #include "configmake.h"
 #include "virstring.h"
@@ -352,8 +353,7 @@ secretSetValue(virSecretPtr secret,
 static unsigned char *
 secretGetValue(virSecretPtr secret,
                size_t *value_size,
-               unsigned int flags,
-               unsigned int internalFlags)
+               unsigned int flags)
 {
     unsigned char *ret = NULL;
     virSecretObj *obj;
@@ -368,11 +368,31 @@ secretGetValue(virSecretPtr secret,
     if (virSecretGetValueEnsureACL(secret->conn, def) < 0)
         goto cleanup;
 
-    if ((internalFlags & VIR_SECRET_GET_VALUE_INTERNAL_CALL) == 0 &&
-        def->isprivate) {
-        virReportError(VIR_ERR_INVALID_SECRET, "%s",
-                       _("secret is private"));
-        goto cleanup;
+    /*
+     * For historical compat we want to deny access to
+     * private secrets, even if no ACL driver is
+     * present.
+     *
+     * We need to validate the identity requesting
+     * the secret value is running as the same user
+     * credentials as this driver.
+     *
+     * ie a non-root libvirt client should not be
+     * able to request the value from privileged
+     * libvirt driver.
+     *
+     * To apply restrictions to processes running under
+     * the same user account is out of scope.
+     */
+    if (def->isprivate) {
+        int rv = virIdentityIsCurrentElevated();
+        if (rv < 0)
+            goto cleanup;
+        if (rv == 0) {
+            virReportError(VIR_ERR_INVALID_SECRET, "%s",
+                           _("secret is private"));
+            goto cleanup;
+        }
     }
 
     if (!(ret = virSecretObjGetValue(obj)))

diff --git a/src/util/virsecret.c b/src/util/virsecret.c
index 0695288229507d77421a91d073fcf449d0dd659d..604d900f77b94a069d020460de9ab02d05f757d5 100644 (file)
--- a/src/util/virsecret.c
+++ b/src/util/virsecret.c
@@ -174,8 +174,7 @@ virSecretGetSecretString(virConnectPtr conn,
         goto cleanup;
     }
 
-    *secret = conn->secretDriver->secretGetValue(sec, secret_size, 0,
-                                                 VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+    *secret = conn->secretDriver->secretGetValue(sec, secret_size, 0);
 
     if (!*secret)
         goto cleanup;

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a93d21d61a9f2f41a9960fd4b16de323a27543c6..d5e59fe47461be7037a59b0116771c55cdcfd067 100644 (file)
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -43,8 +43,7 @@ static virQEMUDriver driver;
 static unsigned char *
 fakeSecretGetValue(virSecretPtr obj G_GNUC_UNUSED,
                    size_t *value_size,
-                   unsigned int fakeflags G_GNUC_UNUSED,
-                   unsigned int internalFlags G_GNUC_UNUSED)
+                   unsigned int fakeflags G_GNUC_UNUSED)
 {
     char *secret;
     secret = g_strdup("AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A");