[PATCH 0/4] Protect secret values stored on disk with TPM
Secret values are stored effectively in plaintext on a disk and we rely on file perms to secure them. But with systemd-cred we can use system's TPM chip and encrypt them. Such secrets won't be transferable to another system by simply copying files stored on disk, but: a) that's not recommended way anyway, b) one can argue secrets shouldn't be migrated anyway. Future work consists of encrypting secret values even when stored in memory, as it's now possible to obtain secrets by dumping memory of virsecretd. Though, to dump a memory admin rights are required at which point users can just read values stored on disk (which is not true for ephemeral secrets). Michal Prívozník (4): virsecret: Introduce APIs to talk to systemd-cred conf: Introduce @tpm attribute to <secret/> virsecretobj: Encrypt/decrypt secrets using TPM NEWS: Document new virSecret TPM feature NEWS.rst | 6 + docs/formatsecret.rst | 8 +- src/conf/schemas/secret.rng | 5 + src/conf/secret_conf.c | 17 +++ src/conf/secret_conf.h | 2 + src/conf/virsecretobj.c | 32 ++++- src/libvirt_private.syms | 3 + src/secret/secret_driver.c | 7 + src/util/virsecret.c | 170 +++++++++++++++++++++++ src/util/virsecret.h | 10 ++ tests/secretxml2xmlin/usage-tpm-vtpm.xml | 7 + tests/secretxml2xmltest.c | 1 + 12 files changed, 263 insertions(+), 5 deletions(-) create mode 100644 tests/secretxml2xmlin/usage-tpm-vtpm.xml -- 2.43.0
The systemd-cred offers a convenient way to talk to host's TPM. While its original intent is to be used in systemd unit files, it offers two additional commands: encrypt and decrypt that can be used independent of the rest of systemd. And these are the ones we need. They offer a convenient way to encrypt/decrypt plaintext using a key that's stored in host's TPM (when --with-key=tpm is passed). In addition, there's 'has-tpm2' command which allows us to detect whether host and the OS have everything needed to utilize the TPM. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 3 + src/util/virsecret.c | 170 +++++++++++++++++++++++++++++++++++++++ src/util/virsecret.h | 10 +++ 3 files changed, 183 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 396bd80844..1d1bba915a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3362,6 +3362,9 @@ virSecretLookupDefClear; virSecretLookupDefCopy; virSecretLookupFormatSecret; virSecretLookupParseSecret; +virSecretTPMAvailable; +virSecretTPMDecrypt; +virSecretTPMEncrypt; # util/virsecureerase.h diff --git a/src/util/virsecret.c b/src/util/virsecret.c index 8a220a37ec..11aa146b6d 100644 --- a/src/util/virsecret.c +++ b/src/util/virsecret.c @@ -23,10 +23,13 @@ #include "datatypes.h" #include "viralloc.h" +#include "vircommand.h" #include "virerror.h" #include "virlog.h" #include "virsecret.h" +#include "virutil.h" #include "viruuid.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -176,3 +179,170 @@ virSecretGetSecretString(virConnectPtr conn, return 0; } + + +/** + * virSecretTPMAvailable: + * + * Checks whether systemd-creds is available and whether host supports + * TPM. Use this prior calling other virSecretTPM*() APIs. + * + * Returns: 1 in case TPM is available, + * 0 in case TPM is NOT available, + * -1 otherwise. + */ +int +virSecretTPMAvailable(void) +{ + g_autoptr(virCommand) cmd = NULL; + g_autofree char *out = NULL; + int exitstatus; + int rc; + + cmd = virCommandNewArgList("systemd-creds", "has-tpm2", NULL); + + virCommandSetOutputBuffer(cmd, &out); + + rc = virCommandRun(cmd, &exitstatus); + + if (rc < 0) { + /* systemd-creds not available */ + return -1; + } + + if (!out || !*out) { + /* systemd-creds reported nothing. Ouch. */ + return 0; + } + + /* systemd-creds can report one of these: + * + * yes - TPM is available and recognized by FW, kernel, etc. + * no - TPM is not available or not recognized + * partial - otherwise + */ + + if (STRPREFIX(out, "yes\n")) + return 1; + + return 0; +} + + +/** + * virSecretTPMEncrypt: + * @name: credential name + * @value: credential value + * @value_size: size of @value + * @base64: encrypted @value, base64 encoded + * + * Encrypts given plaintext (@value) with a secret key automatically + * derived from the system's TPM2 chip. Ciphertext is base64 encoded and + * stored into @base64. Pass the same @name to virSecretTPMDecrypt(). + * + * Returns: 0 on success, + * -1 otherwise (with error reported). + */ +int +virSecretTPMEncrypt(const char *name, + unsigned const char *value, + size_t value_size, + char **base64) +{ + g_autoptr(virCommand) cmd = NULL; + g_autofree char *errBuf = NULL; + int pipeFD[2] = { -1, -1 }; + int ret = -1; + + if (virPipe(pipeFD) < 0) + return -1; + + if (virSetCloseExec(pipeFD[1]) < 0) { + virReportSystemError(errno, "%s", + _("Unable to set cloexec flag")); + goto cleanup; + } + + cmd = virCommandNewArgList("systemd-creds", "encrypt", + "--with-key=tpm2", NULL); + virCommandAddArgFormat(cmd, "--name=%s", name); + virCommandAddArgList(cmd, "-", "-", NULL); + + virCommandSetInputFD(cmd, pipeFD[0]); + virCommandSetOutputBuffer(cmd, base64); + virCommandSetErrorBuffer(cmd, &errBuf); + virCommandDoAsyncIO(cmd); + + if (virCommandRunAsync(cmd, NULL) < 0) + goto cleanup; + + if (safewrite(pipeFD[1], value, value_size) != value_size) { + virReportSystemError(errno, "%s", + _("Unable to pass secret value to systemd-cred")); + goto cleanup; + } + + if (VIR_CLOSE(pipeFD[1]) < 0) { + virReportSystemError(errno, "%s", _("unable to close pipe")); + goto cleanup; + } + + if (virCommandWait(cmd, NULL) < 0) { + if (errBuf && *errBuf) { + VIR_WARN("systemd-creds reported: %s", errBuf); + } + goto cleanup; + } + + ret = 0; + cleanup: + virCommandAbort(cmd); + VIR_FORCE_CLOSE(pipeFD[0]); + VIR_FORCE_CLOSE(pipeFD[1]); + return ret; +} + + +/** + * virSecretTPMDecrypt: + * @name: credential name + * @base64: encrypted @value, base64 encoded + * @value: credential value, + * @value_size: size of @value + * + * Decrypts given ciphertext, base64 encoded (@base64) and stores + * plaintext into @value and its size into @value_size. + * + * Returns: 0 on success, + * -1 otherwise (with error reported). + */ +int +virSecretTPMDecrypt(const char *name, + const char *base64, + unsigned char **value, + size_t *value_size) +{ + g_autoptr(virCommand) cmd = NULL; + g_autofree char *out = NULL; + g_autofree char *errBuf = NULL; + + cmd = virCommandNewArgList("systemd-creds", "decrypt", + "--transcode=base64", NULL); + virCommandAddArgFormat(cmd, "--name=%s", name); + virCommandAddArgList(cmd, "-", "-", NULL); + + virCommandSetInputBuffer(cmd, base64); + virCommandSetOutputBuffer(cmd, &out); + virCommandSetErrorBuffer(cmd, &errBuf); + + if (virCommandRun(cmd, NULL) < 0) { + if (errBuf && *errBuf) { + VIR_WARN("systemd-creds reported: %s", errBuf); + } + return -1; + } + + *value = g_base64_decode(out, value_size); + + return 0; +} diff --git a/src/util/virsecret.h b/src/util/virsecret.h index c803f0fe33..a5ca764bb7 100644 --- a/src/util/virsecret.h +++ b/src/util/virsecret.h @@ -62,3 +62,13 @@ int virSecretGetSecretString(virConnectPtr conn, size_t *ret_secret_size) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) G_GNUC_WARN_UNUSED_RESULT; + +int virSecretTPMAvailable(void); +int virSecretTPMEncrypt(const char *name, + unsigned const char *value, + size_t value_size, + char **base64); +int virSecretTPMDecrypt(const char *name, + const char *base64, + unsigned char **value, + size_t *value_size); -- 2.43.0
On Tue, Feb 13, 2024 at 05:16:05PM +0100, Michal Privoznik wrote:
The systemd-cred offers a convenient way to talk to host's TPM. While its original intent is to be used in systemd unit files, it offers two additional commands: encrypt and decrypt that can be used independent of the rest of systemd. And these are the ones we need. They offer a convenient way to encrypt/decrypt plaintext using a key that's stored in host's TPM (when --with-key=tpm is passed).
This is one way we could do it, but what I don't like about this is that it ties the secret driver to system. I think it would be better to have an intermediate key, that is protected by credentials and use that for encryption of the individual secrets. Roughly speaking.... * Make virtsecretd accept a '--encryption-key /some/path' option * In ExecPreStart in virtsecretd.service, if no key already exists, we dd from /dev/random enough bytes for a AES256 key, and then use systemd-creds encrypt to store it * In ExecStart we set the --encryption-key arg to point to the credentials that systemd will decrypt * The secret driver can now use this master key to AES encrypt/decrypt individual secrets The benefit of this is that only encryption of the intermediate key is tied to the TPM. If someone needs to move everything to a new host, then can copy over all the secret data files, and then just re-seal the intermediate key against the new machine's TPM. The also lets people setup the --encryption-key for virtsecretd with some arbitrary non-systemd mechanism. eg could use clevis/tang, or $whatever. The secret driver does not need to know about systemd at all, all the credentials logic is done with a minimal tweak to the .service file.
In addition, there's 'has-tpm2' command which allows us to detect whether host and the OS have everything needed to utilize the TPM.
FWIW, systemd-creds does not require the use of a TPM2, it is quite happy running without one, so libvirt should not really describe this as TPM based, nor require a TPM. Without a TPM systemd just generates a random key and stores it in /var. That is less secure by default of course, but still reasonable, as in theory you could way to inject the master key to /var in some manner.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 3 + src/util/virsecret.c | 170 +++++++++++++++++++++++++++++++++++++++ src/util/virsecret.h | 10 +++ 3 files changed, 183 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 396bd80844..1d1bba915a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3362,6 +3362,9 @@ virSecretLookupDefClear; virSecretLookupDefCopy; virSecretLookupFormatSecret; virSecretLookupParseSecret; +virSecretTPMAvailable; +virSecretTPMDecrypt; +virSecretTPMEncrypt;
# util/virsecureerase.h diff --git a/src/util/virsecret.c b/src/util/virsecret.c index 8a220a37ec..11aa146b6d 100644 --- a/src/util/virsecret.c +++ b/src/util/virsecret.c @@ -23,10 +23,13 @@
#include "datatypes.h" #include "viralloc.h" +#include "vircommand.h" #include "virerror.h" #include "virlog.h" #include "virsecret.h" +#include "virutil.h" #include "viruuid.h" +#include "virfile.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -176,3 +179,170 @@ virSecretGetSecretString(virConnectPtr conn,
return 0; } + + +/** + * virSecretTPMAvailable: + * + * Checks whether systemd-creds is available and whether host supports + * TPM. Use this prior calling other virSecretTPM*() APIs. + * + * Returns: 1 in case TPM is available, + * 0 in case TPM is NOT available, + * -1 otherwise. + */ +int +virSecretTPMAvailable(void) +{ + g_autoptr(virCommand) cmd = NULL; + g_autofree char *out = NULL; + int exitstatus; + int rc; + + cmd = virCommandNewArgList("systemd-creds", "has-tpm2", NULL); + + virCommandSetOutputBuffer(cmd, &out); + + rc = virCommandRun(cmd, &exitstatus); + + if (rc < 0) { + /* systemd-creds not available */ + return -1; + } + + if (!out || !*out) { + /* systemd-creds reported nothing. Ouch. */ + return 0; + } + + /* systemd-creds can report one of these: + * + * yes - TPM is available and recognized by FW, kernel, etc. + * no - TPM is not available or not recognized + * partial - otherwise + */ + + if (STRPREFIX(out, "yes\n")) + return 1; + + return 0; +} + + +/** + * virSecretTPMEncrypt: + * @name: credential name + * @value: credential value + * @value_size: size of @value + * @base64: encrypted @value, base64 encoded + * + * Encrypts given plaintext (@value) with a secret key automatically + * derived from the system's TPM2 chip. Ciphertext is base64 encoded and + * stored into @base64. Pass the same @name to virSecretTPMDecrypt(). + * + * Returns: 0 on success, + * -1 otherwise (with error reported). + */ +int +virSecretTPMEncrypt(const char *name, + unsigned const char *value, + size_t value_size, + char **base64) +{ + g_autoptr(virCommand) cmd = NULL; + g_autofree char *errBuf = NULL; + int pipeFD[2] = { -1, -1 }; + int ret = -1; + + if (virPipe(pipeFD) < 0) + return -1; + + if (virSetCloseExec(pipeFD[1]) < 0) { + virReportSystemError(errno, "%s", + _("Unable to set cloexec flag")); + goto cleanup; + } + + cmd = virCommandNewArgList("systemd-creds", "encrypt", + "--with-key=tpm2", NULL); + virCommandAddArgFormat(cmd, "--name=%s", name); + virCommandAddArgList(cmd, "-", "-", NULL); + + virCommandSetInputFD(cmd, pipeFD[0]); + virCommandSetOutputBuffer(cmd, base64); + virCommandSetErrorBuffer(cmd, &errBuf); + virCommandDoAsyncIO(cmd); + + if (virCommandRunAsync(cmd, NULL) < 0) + goto cleanup; + + if (safewrite(pipeFD[1], value, value_size) != value_size) { + virReportSystemError(errno, "%s", + _("Unable to pass secret value to systemd-cred")); + goto cleanup; + } + + if (VIR_CLOSE(pipeFD[1]) < 0) { + virReportSystemError(errno, "%s", _("unable to close pipe")); + goto cleanup; + } + + if (virCommandWait(cmd, NULL) < 0) { + if (errBuf && *errBuf) { + VIR_WARN("systemd-creds reported: %s", errBuf); + } + goto cleanup; + } + + ret = 0; + cleanup: + virCommandAbort(cmd); + VIR_FORCE_CLOSE(pipeFD[0]); + VIR_FORCE_CLOSE(pipeFD[1]); + return ret; +} + + +/** + * virSecretTPMDecrypt: + * @name: credential name + * @base64: encrypted @value, base64 encoded + * @value: credential value, + * @value_size: size of @value + * + * Decrypts given ciphertext, base64 encoded (@base64) and stores + * plaintext into @value and its size into @value_size. + * + * Returns: 0 on success, + * -1 otherwise (with error reported). + */ +int +virSecretTPMDecrypt(const char *name, + const char *base64, + unsigned char **value, + size_t *value_size) +{ + g_autoptr(virCommand) cmd = NULL; + g_autofree char *out = NULL; + g_autofree char *errBuf = NULL; + + cmd = virCommandNewArgList("systemd-creds", "decrypt", + "--transcode=base64", NULL); + virCommandAddArgFormat(cmd, "--name=%s", name); + virCommandAddArgList(cmd, "-", "-", NULL); + + virCommandSetInputBuffer(cmd, base64); + virCommandSetOutputBuffer(cmd, &out); + virCommandSetErrorBuffer(cmd, &errBuf); + + if (virCommandRun(cmd, NULL) < 0) { + if (errBuf && *errBuf) { + VIR_WARN("systemd-creds reported: %s", errBuf); + } + return -1; + } + + *value = g_base64_decode(out, value_size); + + return 0; +} diff --git a/src/util/virsecret.h b/src/util/virsecret.h index c803f0fe33..a5ca764bb7 100644 --- a/src/util/virsecret.h +++ b/src/util/virsecret.h @@ -62,3 +62,13 @@ int virSecretGetSecretString(virConnectPtr conn, size_t *ret_secret_size) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) G_GNUC_WARN_UNUSED_RESULT; + +int virSecretTPMAvailable(void); +int virSecretTPMEncrypt(const char *name, + unsigned const char *value, + size_t value_size, + char **base64); +int virSecretTPMDecrypt(const char *name, + const char *base64, + unsigned char **value, + size_t *value_size); -- 2.43.0 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
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 :|
This attribute exists next to @ephemeral and @private attributes and controls whether the secret value is encrypted using system's TPM chip before stored on disk. Obviously, it's mutually exclusive with @ephemeral which forces us to keep the secret value in memory only. In the long run, we can even encrypt secret values that are kept in memory (so they can't be obtained by dumping virtsecretd's memory). But that's not what is being implemented here. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatsecret.rst | 8 ++++++-- src/conf/schemas/secret.rng | 5 +++++ src/conf/secret_conf.c | 17 +++++++++++++++++ src/conf/secret_conf.h | 2 ++ tests/secretxml2xmlin/usage-tpm-vtpm.xml | 7 +++++++ tests/secretxml2xmltest.c | 1 + 6 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 tests/secretxml2xmlin/usage-tpm-vtpm.xml diff --git a/docs/formatsecret.rst b/docs/formatsecret.rst index d0c37ff165..c7910aecce 100644 --- a/docs/formatsecret.rst +++ b/docs/formatsecret.rst @@ -10,14 +10,18 @@ Secret XML ---------- Secrets stored by libvirt may have attributes associated with them, using the -``secret`` element. The ``secret`` element has two optional attributes, each -with values '``yes``' and '``no``', and defaulting to '``no``': +``secret`` element. The ``secret`` element has the following optional +attributes, each with values '``yes``' and '``no``', and defaulting to +'``no``': ``ephemeral`` This secret must only be kept in memory, never stored persistently. ``private`` The value of the secret must not be revealed to any caller of libvirt, nor to any other node. +``tpm`` + The value of the secret is stored using a key that's derived from the + system's TPM2 chip. This is mutually exclusive with ``ephemeral``. The top-level ``secret`` element may contain the following elements: diff --git a/src/conf/schemas/secret.rng b/src/conf/schemas/secret.rng index c90e2eb81f..59d825bf91 100644 --- a/src/conf/schemas/secret.rng +++ b/src/conf/schemas/secret.rng @@ -19,6 +19,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="tpm"> + <ref name="virYesNo"/> + </attribute> + </optional> <interleave> <optional> <element name="uuid"> diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 966536599e..1ab6cf5cd4 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -130,6 +130,7 @@ virSecretParseXML(xmlXPathContext *ctxt) g_autoptr(virSecretDef) def = NULL; g_autofree char *ephemeralstr = NULL; g_autofree char *privatestr = NULL; + g_autofree char *tpmstr = NULL; g_autofree char *uuidstr = NULL; def = g_new0(virSecretDef, 1); @@ -150,6 +151,17 @@ virSecretParseXML(xmlXPathContext *ctxt) } } + if (virXMLPropTristateBool(ctxt->node, "tpm", + VIR_XML_PROP_NONE, &def->tpm) < 0) { + return NULL; + } + + if (def->tpm == VIR_TRISTATE_BOOL_YES && def->isephemeral) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ephemeral and tpm are mutually exclusive")); + return NULL; + } + uuidstr = virXPathString("string(./uuid)", ctxt); if (!uuidstr) { if (virUUIDGenerate(def->uuid) < 0) { @@ -248,6 +260,11 @@ virSecretDefFormat(const virSecretDef *def) def->isephemeral ? "yes" : "no", def->isprivate ? "yes" : "no"); + if (def->tpm != VIR_TRISTATE_BOOL_ABSENT) { + virBufferAsprintf(&attrBuf, " tpm='%s'", + virTristateBoolTypeToString(def->tpm)); + } + virUUIDFormat(def->uuid, uuidstr); virBufferEscapeString(&childBuf, "<uuid>%s</uuid>\n", uuidstr); if (def->description != NULL) diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 8f8f47933a..8a9e382c1e 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -21,11 +21,13 @@ #pragma once #include "internal.h" +#include "virenum.h" typedef struct _virSecretDef virSecretDef; struct _virSecretDef { bool isephemeral; bool isprivate; + virTristateBool tpm; unsigned char uuid[VIR_UUID_BUFLEN]; char *description; /* May be NULL */ virSecretUsageType usage_type; diff --git a/tests/secretxml2xmlin/usage-tpm-vtpm.xml b/tests/secretxml2xmlin/usage-tpm-vtpm.xml new file mode 100644 index 0000000000..b70785113c --- /dev/null +++ b/tests/secretxml2xmlin/usage-tpm-vtpm.xml @@ -0,0 +1,7 @@ +<secret ephemeral='no' private='yes' tpm='yes'> + <uuid>46b96412-fffc-46a3-bf3d-c371f776cadb</uuid> + <description>vTPM secret</description> + <usage type='vtpm'> + <name>vTPMvTPMvTPM</name> + </usage> +</secret> diff --git a/tests/secretxml2xmltest.c b/tests/secretxml2xmltest.c index eb4d3e143c..cdd34546b2 100644 --- a/tests/secretxml2xmltest.c +++ b/tests/secretxml2xmltest.c @@ -69,6 +69,7 @@ mymain(void) DO_TEST("usage-iscsi"); DO_TEST("usage-tls"); DO_TEST("usage-vtpm"); + DO_TEST("usage-tpm-vtpm"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.43.0
On Tue, Feb 13, 2024 at 05:16:06PM +0100, Michal Privoznik wrote:
This attribute exists next to @ephemeral and @private attributes and controls whether the secret value is encrypted using system's TPM chip before stored on disk. Obviously, it's mutually exclusive with @ephemeral which forces us to keep the secret value in memory only.
In the long run, we can even encrypt secret values that are kept in memory (so they can't be obtained by dumping virtsecretd's memory). But that's not what is being implemented here.
I'm not really convinced we need a new XML attribute. The storage backend for the secret driver is an impl detail, and we should just always use the best option we have available. If we ever do have multiple storage backends for secrets, then it feels more like an /etc/libvirt/secret.conf setting to sepcify which backend to use, as a host admin choice.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatsecret.rst | 8 ++++++-- src/conf/schemas/secret.rng | 5 +++++ src/conf/secret_conf.c | 17 +++++++++++++++++ src/conf/secret_conf.h | 2 ++ tests/secretxml2xmlin/usage-tpm-vtpm.xml | 7 +++++++ tests/secretxml2xmltest.c | 1 + 6 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 tests/secretxml2xmlin/usage-tpm-vtpm.xml
diff --git a/docs/formatsecret.rst b/docs/formatsecret.rst index d0c37ff165..c7910aecce 100644 --- a/docs/formatsecret.rst +++ b/docs/formatsecret.rst @@ -10,14 +10,18 @@ Secret XML ----------
Secrets stored by libvirt may have attributes associated with them, using the -``secret`` element. The ``secret`` element has two optional attributes, each -with values '``yes``' and '``no``', and defaulting to '``no``': +``secret`` element. The ``secret`` element has the following optional +attributes, each with values '``yes``' and '``no``', and defaulting to +'``no``':
``ephemeral`` This secret must only be kept in memory, never stored persistently. ``private`` The value of the secret must not be revealed to any caller of libvirt, nor to any other node. +``tpm`` + The value of the secret is stored using a key that's derived from the + system's TPM2 chip. This is mutually exclusive with ``ephemeral``.
The top-level ``secret`` element may contain the following elements:
diff --git a/src/conf/schemas/secret.rng b/src/conf/schemas/secret.rng index c90e2eb81f..59d825bf91 100644 --- a/src/conf/schemas/secret.rng +++ b/src/conf/schemas/secret.rng @@ -19,6 +19,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="tpm"> + <ref name="virYesNo"/> + </attribute> + </optional> <interleave> <optional> <element name="uuid"> diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 966536599e..1ab6cf5cd4 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -130,6 +130,7 @@ virSecretParseXML(xmlXPathContext *ctxt) g_autoptr(virSecretDef) def = NULL; g_autofree char *ephemeralstr = NULL; g_autofree char *privatestr = NULL; + g_autofree char *tpmstr = NULL; g_autofree char *uuidstr = NULL;
def = g_new0(virSecretDef, 1); @@ -150,6 +151,17 @@ virSecretParseXML(xmlXPathContext *ctxt) } }
+ if (virXMLPropTristateBool(ctxt->node, "tpm", + VIR_XML_PROP_NONE, &def->tpm) < 0) { + return NULL; + } + + if (def->tpm == VIR_TRISTATE_BOOL_YES && def->isephemeral) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ephemeral and tpm are mutually exclusive")); + return NULL; + } + uuidstr = virXPathString("string(./uuid)", ctxt); if (!uuidstr) { if (virUUIDGenerate(def->uuid) < 0) { @@ -248,6 +260,11 @@ virSecretDefFormat(const virSecretDef *def) def->isephemeral ? "yes" : "no", def->isprivate ? "yes" : "no");
+ if (def->tpm != VIR_TRISTATE_BOOL_ABSENT) { + virBufferAsprintf(&attrBuf, " tpm='%s'", + virTristateBoolTypeToString(def->tpm)); + } + virUUIDFormat(def->uuid, uuidstr); virBufferEscapeString(&childBuf, "<uuid>%s</uuid>\n", uuidstr); if (def->description != NULL) diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 8f8f47933a..8a9e382c1e 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -21,11 +21,13 @@ #pragma once
#include "internal.h" +#include "virenum.h"
typedef struct _virSecretDef virSecretDef; struct _virSecretDef { bool isephemeral; bool isprivate; + virTristateBool tpm; unsigned char uuid[VIR_UUID_BUFLEN]; char *description; /* May be NULL */ virSecretUsageType usage_type; diff --git a/tests/secretxml2xmlin/usage-tpm-vtpm.xml b/tests/secretxml2xmlin/usage-tpm-vtpm.xml new file mode 100644 index 0000000000..b70785113c --- /dev/null +++ b/tests/secretxml2xmlin/usage-tpm-vtpm.xml @@ -0,0 +1,7 @@ +<secret ephemeral='no' private='yes' tpm='yes'> + <uuid>46b96412-fffc-46a3-bf3d-c371f776cadb</uuid> + <description>vTPM secret</description> + <usage type='vtpm'> + <name>vTPMvTPMvTPM</name> + </usage> +</secret> diff --git a/tests/secretxml2xmltest.c b/tests/secretxml2xmltest.c index eb4d3e143c..cdd34546b2 100644 --- a/tests/secretxml2xmltest.c +++ b/tests/secretxml2xmltest.c @@ -69,6 +69,7 @@ mymain(void) DO_TEST("usage-iscsi"); DO_TEST("usage-tls"); DO_TEST("usage-vtpm"); + DO_TEST("usage-tpm-vtpm");
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.43.0 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
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 :|
If user requests their virSecret value to be encrypted using hosts' TPM we can now honour such request as we have all the APIs ready. The value is still stored in a file (obj->base64File) but because it was encrypted by TPM it's not readable (even though it's still base64 encoded). And since we can detect usability of host's TPM, let's do that when a virSecret is defined and TPM is requested. This avoids unpleasant surprises later on. Resolves: https://issues.redhat.com/browse/RHEL-7125 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virsecretobj.c | 32 +++++++++++++++++++++++++++++--- src/secret/secret_driver.c | 7 +++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 455798d414..b77d69649c 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -24,12 +24,13 @@ #include <sys/stat.h> #include "datatypes.h" -#include "virsecretobj.h" #include "viralloc.h" #include "virerror.h" #include "virfile.h" #include "virhash.h" #include "virlog.h" +#include "virsecret.h" +#include "virsecretobj.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -689,7 +690,19 @@ virSecretObjSaveData(virSecretObj *obj) if (!obj->value) return 0; - base64 = g_base64_encode(obj->value, obj->value_size); + if (obj->def->tpm == VIR_TRISTATE_BOOL_YES) { + char uuidStr[VIR_UUID_STRING_BUFLEN] = { 0 }; + + virUUIDFormat(obj->def->uuid, uuidStr); + + if (virSecretTPMEncrypt(uuidStr, + obj->value, obj->value_size, + &base64) < 0) { + return -1; + } + } else { + base64 = g_base64_encode(obj->value, obj->value_size); + } if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0) return -1; @@ -847,7 +860,20 @@ virSecretLoadValue(virSecretObj *obj) VIR_FORCE_CLOSE(fd); - obj->value = g_base64_decode(contents, &obj->value_size); + if (obj->def->tpm == VIR_TRISTATE_BOOL_YES) { + char uuidStr[VIR_UUID_STRING_BUFLEN] = { 0 }; + + virUUIDFormat(obj->def->uuid, uuidStr); + + if (virSecretTPMDecrypt(uuidStr, + contents, + &obj->value, + &obj->value_size) < 0) { + goto cleanup; + } + } else { + obj->value = g_base64_decode(contents, &obj->value_size); + } ret = 0; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index c7bd65b4e9..116d645243 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -234,6 +234,13 @@ secretDefineXML(virConnectPtr conn, if (virSecretDefineXMLEnsureACL(conn, def) < 0) goto cleanup; + if (def->tpm == VIR_TRISTATE_BOOL_YES && + virSecretTPMAvailable() != 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TPM is unavailable or unusable on this host")); + goto cleanup; + } + if (!(obj = virSecretObjListAdd(driver->secrets, &def, driver->configDir, &backup))) goto cleanup; -- 2.43.0
On a Tuesday in 2024, Michal Privoznik wrote:
If user requests their virSecret value to be encrypted using hosts' TPM we can now honour such request as we have all the APIs ready. The value is still stored in a file (obj->base64File) but because it was encrypted by TPM it's not readable (even though it's still base64 encoded).
And since we can detect usability of host's TPM, let's do that when a virSecret is defined and TPM is requested. This avoids unpleasant surprises later on.
That link is private so it does not belong in an upstream commit message. Jano
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virsecretobj.c | 32 +++++++++++++++++++++++++++++--- src/secret/secret_driver.c | 7 +++++++ 2 files changed, 36 insertions(+), 3 deletions(-)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- NEWS.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 15b0da31b6..9c0e4b4b65 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -30,6 +30,12 @@ v10.1.0 (unreleased) to resolve names of the connected guests using the name server started for this network. + * virsecretobj: Encrypt/decrypt secrets using TPM + + When defining new ``<secret/>`` user can set ``tpm='yes'`` which then makes + libvirt encrypt/decrypt the secret value using host's TPM before stored on + a disk. + * **Improvements** * **Bug fixes** -- 2.43.0
On Tue, Feb 13, 2024 at 05:16:04PM +0100, Michal Privoznik wrote:
Secret values are stored effectively in plaintext on a disk and we rely on file perms to secure them. But with systemd-cred we can use system's TPM chip and encrypt them.
Such secrets won't be transferable to another system by simply copying files stored on disk, but: a) that's not recommended way anyway, b) one can argue secrets shouldn't be migrated anyway.
Future work consists of encrypting secret values even when stored in memory, as it's now possible to obtain secrets by dumping memory of virsecretd. Though, to dump a memory admin rights are required at which point users can just read values stored on disk (which is not true for ephemeral secrets).
We should not read the secret values into memory durnig startup at all. They ought to remain on disk only, except during execution of the virSecretGetValue API call. That way we don't need to have encryption of values in memory. We just need to scrub memory used by GetValue.
Michal Prívozník (4): virsecret: Introduce APIs to talk to systemd-cred conf: Introduce @tpm attribute to <secret/> virsecretobj: Encrypt/decrypt secrets using TPM NEWS: Document new virSecret TPM feature
NEWS.rst | 6 + docs/formatsecret.rst | 8 +- src/conf/schemas/secret.rng | 5 + src/conf/secret_conf.c | 17 +++ src/conf/secret_conf.h | 2 + src/conf/virsecretobj.c | 32 ++++- src/libvirt_private.syms | 3 + src/secret/secret_driver.c | 7 + src/util/virsecret.c | 170 +++++++++++++++++++++++ src/util/virsecret.h | 10 ++ tests/secretxml2xmlin/usage-tpm-vtpm.xml | 7 + tests/secretxml2xmltest.c | 1 + 12 files changed, 263 insertions(+), 5 deletions(-) create mode 100644 tests/secretxml2xmlin/usage-tpm-vtpm.xml
-- 2.43.0 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
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 (3)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Privoznik