[RFC PATCH 0/1] secret: Inhibit shutdown for ephemeral secrets
I'm kind of convinced that we want to do this, but also it's a significant change in the behaviour of the daemon, hence RFC prefix. This stemmed from a discussion with a user who wants us to use something more secure than base64 encoded secret values stored on a disk. They suggested storing the values in TPM and while that might sound like a good idea, I suggested using ephemeral secrets for the time being. Well, because of '--timeout 120', ephemeral secrets are short lived, indeed. Meanwhile, let me see if there's a library we could use to talk to TPM. Michal Prívozník (1): secret: Inhibit shutdown for ephemeral secrets src/secret/secret_driver.c | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) -- 2.38.2
Our secret driver divides secrets into two groups: ephemeral (stored only in memory) and persistent (stored on disk). Now, the aim of ephemeral secrets is to define them shortly before being used and then undefine them. But 'shortly before being used' is a very vague time frame. And since we default to socket activation and thus pass '--timeout 120' to every daemon it may happen that just defined ephemeral secret is gone among with the virtsecretd. This is no problem for persistent secrets as their definition (and value) is restored when the virtsecretd starts again, but ephemeral secrets can't be restored. Therefore, we could view ephemeral secrets as active objects that the daemon manages and thus inhibit automatic shutdown (just like hypervisor daemons do when a guest is running). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/secret/secret_driver.c | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index bd981a8ace..c38cd6f651 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -66,6 +66,10 @@ struct _virSecretDriverState { /* Immutable pointer, self-locking APIs */ virObjectEventState *secretEventState; + + /* Immutable pointers. Caller must provide locking */ + virStateInhibitCallback inhibitCallback; + void *inhibitOpaque; }; static virSecretDriverState *driver; @@ -86,6 +90,23 @@ secretObjFromSecret(virSecretPtr secret) } +static bool +secretNumOfEphemeralSecretsHelper(virConnectPtr conn G_GNUC_UNUSED, + virSecretDef *def) +{ + return def->isephemeral; +} + + +static int +secretNumOfEphemeralSecrets(void) +{ + return virSecretObjListNumOfSecrets(driver->secrets, + secretNumOfEphemeralSecretsHelper, + NULL); +} + + /* Driver functions */ static int @@ -266,6 +287,10 @@ secretDefineXML(virConnectPtr conn, cleanup: virSecretDefFree(def); virSecretObjEndAPI(&obj); + + if (secretNumOfEphemeralSecrets() > 0) + driver->inhibitCallback(true, driver->inhibitOpaque); + virObjectEventStateQueue(driver->secretEventState, event); return ret; @@ -424,6 +449,10 @@ secretUndefine(virSecretPtr secret) cleanup: virSecretObjEndAPI(&obj); + + if (secretNumOfEphemeralSecrets() == 0) + driver->inhibitCallback(false, driver->inhibitOpaque); + virObjectEventStateQueue(driver->secretEventState, event); return ret; @@ -463,8 +492,8 @@ static int secretStateInitialize(bool privileged, const char *root, bool monolithic G_GNUC_UNUSED, - virStateInhibitCallback callback G_GNUC_UNUSED, - void *opaque G_GNUC_UNUSED) + virStateInhibitCallback callback, + void *opaque) { VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); @@ -473,6 +502,8 @@ secretStateInitialize(bool privileged, driver->lockFD = -1; driver->secretEventState = virObjectEventStateNew(); driver->privileged = privileged; + driver->inhibitCallback = callback; + driver->inhibitOpaque = opaque; if (root) { driver->embeddedRoot = g_strdup(root); -- 2.38.2
On Tue, Dec 20, 2022 at 09:27:11AM +0100, Michal Privoznik wrote:
Our secret driver divides secrets into two groups: ephemeral (stored only in memory) and persistent (stored on disk). Now, the aim of ephemeral secrets is to define them shortly before being used and then undefine them. But 'shortly before being used' is a very vague time frame. And since we default to socket activation and thus pass '--timeout 120' to every daemon it may happen that just defined ephemeral secret is gone among with the virtsecretd.
This is no problem for persistent secrets as their definition (and value) is restored when the virtsecretd starts again, but ephemeral secrets can't be restored.
Therefore, we could view ephemeral secrets as active objects that the daemon manages and thus inhibit automatic shutdown (just like hypervisor daemons do when a guest is running).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/secret/secret_driver.c | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With 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 :|
On Tue, Dec 20, 2022 at 09:27:10AM +0100, Michal Privoznik wrote:
I'm kind of convinced that we want to do this, but also it's a significant change in the behaviour of the daemon, hence RFC prefix.
This stemmed from a discussion with a user who wants us to use something more secure than base64 encoded secret values stored on a disk. They suggested storing the values in TPM and while that might sound like a good idea, I suggested using ephemeral secrets for the time being. Well, because of '--timeout 120', ephemeral secrets are short lived, indeed.
Meanwhile, let me see if there's a library we could use to talk to TPM.
Storing secrets in the TPM isn't viable, as it has insufficient NVRAM for our needs. What we need todo is encrypt the secrets, with a primary key that is in turn sealed against the TPM. This sounds difficult, but its actually fairly trivial as we can receive such a primary key from systemd, using its credentials mechanism. If we assume a (encrypted) primary key in /var/lib/libvirt, then we can put a line in virtsecretd.service: LoadCredential=primary:/var/lib/libvirt/secret/primary.creds When virtsecretd runs, this will result in the plain text primary key being made available in a file under $CREDENTIALS_DIRECTORY. We can reference this directory using %d, so for example we change virtsecretd.service to use: ExecStart=/usr/sbin/virtsecretd --primary-key %d/primary $VIRTSECRETD_ARGS The problem is how do we create the original encrypted primary key. The best trick I've come up with is to use a ExecStartPre script: ExecStartPre=/usr/sbin/virtsecretd-mkcreds /var/lib/libvirt/secret/primary.creds Where virtsecretd-mkcreds contains: #!/bin/sh CREDS=$1 if test -f $CREDS then exit 0 fi dd if=/dev/urandom bs=256 count=1 status=none | systemd-creds encrypt - $CREDS exit 0 This creates a random key encrypting it, preferentially with the TPM2 if one is available. It is kinda irritating that systemd can't auto-create creds itself when a service is started, as this mkcreds script would be the same for everyone who wants this kind of functionality. With 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 :|
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik