src/qemu/qemu.conf | 8 ++++++++ src/qemu/qemu_conf.c | 6 ++++++ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_tpm.c | 8 ++++++++ 4 files changed, 23 insertions(+)
Extend qemu.conf with a configration option swtpm_active_pcr_banks that
allows a user to set a comma-separated list of PCR banks to activate
during 'TPM manufacturing'. Valid PCR banks are sha1,sha256,sha384 and
sha512.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2016599
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
src/qemu/qemu.conf | 8 ++++++++
src/qemu/qemu_conf.c | 6 ++++++
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_tpm.c | 8 ++++++++
4 files changed, 23 insertions(+)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 71fd125699..7aa151ed55 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -915,6 +915,14 @@
#swtpm_user = "tss"
#swtpm_group = "tss"
+# The PCR banks to activate during 'TPM manufacturing' before a swtpm instance
+# is started the first time.
+#
+# A comma-separated list without spaces containing sha1,sha256,sha384, or
+# sha512. The default is 'sha256'.
+#
+# swtpm_active_pcr_banks = "sha256,sha384"
+
# For debugging and testing purposes it's sometimes useful to be able to disable
# libvirt behaviour based on the capabilities of the qemu process. This option
# allows to do so. DO _NOT_ use in production and beaware that the behaviour
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 0451bc70ac..a62525385e 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -384,6 +384,8 @@ static void virQEMUDriverConfigDispose(void *obj)
g_strfreev(cfg->capabilityfilters);
g_free(cfg->deprecationBehavior);
+
+ g_free(cfg->swtpmActivePcrBanks);
}
@@ -1030,6 +1032,10 @@ virQEMUDriverConfigLoadSWTPMEntry(virQEMUDriverConfig *cfg,
if (swtpm_group && virGetGroupID(swtpm_group, &cfg->swtpm_group) < 0)
return -1;
+ if (virConfGetValueString(conf, "swtpm_active_pcr_banks",
+ &cfg->swtpmActivePcrBanks) < 0)
+ return -1;
+
return 0;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 2f64e39a18..37461d9e31 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -219,6 +219,7 @@ struct _virQEMUDriverConfig {
uid_t swtpm_user;
gid_t swtpm_group;
+ char *swtpmActivePcrBanks;
char **capabilityfilters;
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index e1b08a66c5..69fd1e67e3 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -448,6 +448,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
bool privileged,
uid_t swtpm_user,
gid_t swtpm_group,
+ const char *swtpmActivePcrBanks,
const char *logfile,
const virDomainTPMVersion tpmversion,
const unsigned char *secretuuid,
@@ -512,6 +513,9 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
}
if (!incomingMigration) {
+ if (!swtpmActivePcrBanks)
+ swtpmActivePcrBanks = "sha256";
+
virCommandAddArgList(cmd,
"--tpm-state", storagepath,
"--vmid", vmid,
@@ -521,6 +525,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
"--create-platform-cert",
"--lock-nvram",
"--not-overwrite",
+ "--pcr-banks", swtpmActivePcrBanks,
NULL);
} else {
virCommandAddArgList(cmd,
@@ -568,6 +573,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
bool privileged,
uid_t swtpm_user,
gid_t swtpm_group,
+ const char *swtpmActivePcrBanks,
const char *swtpmStateDir,
const char *shortName,
bool incomingMigration)
@@ -593,6 +599,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
if (created &&
qemuTPMEmulatorRunSetup(tpm->data.emulator.storagepath, vmname, vmuuid,
privileged, swtpm_user, swtpm_group,
+ swtpmActivePcrBanks,
tpm->data.emulator.logfile, tpm->version,
secretuuid, incomingMigration) < 0)
goto error;
@@ -812,6 +819,7 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver,
driver->privileged,
cfg->swtpm_user,
cfg->swtpm_group,
+ cfg->swtpmActivePcrBanks,
cfg->swtpmStateDir, shortName,
incomingMigration)))
return -1;
--
2.31.1
Hi
On Wed, Oct 27, 2021 at 9:00 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> Extend qemu.conf with a configration option swtpm_active_pcr_banks that
> allows a user to set a comma-separated list of PCR banks to activate
> during 'TPM manufacturing'. Valid PCR banks are sha1,sha256,sha384 and
> sha512.
>
Why not put this option in swtpm_setup.conf instead?
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2016599
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
> src/qemu/qemu.conf | 8 ++++++++
> src/qemu/qemu_conf.c | 6 ++++++
> src/qemu/qemu_conf.h | 1 +
> src/qemu/qemu_tpm.c | 8 ++++++++
> 4 files changed, 23 insertions(+)
>
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 71fd125699..7aa151ed55 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -915,6 +915,14 @@
> #swtpm_user = "tss"
> #swtpm_group = "tss"
>
> +# The PCR banks to activate during 'TPM manufacturing' before a swtpm instance
> +# is started the first time.
> +#
> +# A comma-separated list without spaces containing sha1,sha256,sha384, or
> +# sha512. The default is 'sha256'.
> +#
> +# swtpm_active_pcr_banks = "sha256,sha384"
> +
> # For debugging and testing purposes it's sometimes useful to be able to disable
> # libvirt behaviour based on the capabilities of the qemu process. This option
> # allows to do so. DO _NOT_ use in production and beaware that the behaviour
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 0451bc70ac..a62525385e 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -384,6 +384,8 @@ static void virQEMUDriverConfigDispose(void *obj)
> g_strfreev(cfg->capabilityfilters);
>
> g_free(cfg->deprecationBehavior);
> +
> + g_free(cfg->swtpmActivePcrBanks);
> }
>
>
> @@ -1030,6 +1032,10 @@ virQEMUDriverConfigLoadSWTPMEntry(virQEMUDriverConfig *cfg,
> if (swtpm_group && virGetGroupID(swtpm_group, &cfg->swtpm_group) < 0)
> return -1;
>
> + if (virConfGetValueString(conf, "swtpm_active_pcr_banks",
> + &cfg->swtpmActivePcrBanks) < 0)
> + return -1;
> +
> return 0;
> }
>
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 2f64e39a18..37461d9e31 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -219,6 +219,7 @@ struct _virQEMUDriverConfig {
>
> uid_t swtpm_user;
> gid_t swtpm_group;
> + char *swtpmActivePcrBanks;
>
> char **capabilityfilters;
>
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index e1b08a66c5..69fd1e67e3 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -448,6 +448,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> bool privileged,
> uid_t swtpm_user,
> gid_t swtpm_group,
> + const char *swtpmActivePcrBanks,
> const char *logfile,
> const virDomainTPMVersion tpmversion,
> const unsigned char *secretuuid,
> @@ -512,6 +513,9 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> }
>
> if (!incomingMigration) {
> + if (!swtpmActivePcrBanks)
> + swtpmActivePcrBanks = "sha256";
> +
> virCommandAddArgList(cmd,
> "--tpm-state", storagepath,
> "--vmid", vmid,
> @@ -521,6 +525,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> "--create-platform-cert",
> "--lock-nvram",
> "--not-overwrite",
> + "--pcr-banks", swtpmActivePcrBanks,
> NULL);
> } else {
> virCommandAddArgList(cmd,
> @@ -568,6 +573,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
> bool privileged,
> uid_t swtpm_user,
> gid_t swtpm_group,
> + const char *swtpmActivePcrBanks,
> const char *swtpmStateDir,
> const char *shortName,
> bool incomingMigration)
> @@ -593,6 +599,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
> if (created &&
> qemuTPMEmulatorRunSetup(tpm->data.emulator.storagepath, vmname, vmuuid,
> privileged, swtpm_user, swtpm_group,
> + swtpmActivePcrBanks,
> tpm->data.emulator.logfile, tpm->version,
> secretuuid, incomingMigration) < 0)
> goto error;
> @@ -812,6 +819,7 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver,
> driver->privileged,
> cfg->swtpm_user,
> cfg->swtpm_group,
> + cfg->swtpmActivePcrBanks,
> cfg->swtpmStateDir, shortName,
> incomingMigration)))
> return -1;
> --
> 2.31.1
>
On 10/27/21 14:17, Marc-André Lureau wrote: > Hi > > On Wed, Oct 27, 2021 at 9:00 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >> Extend qemu.conf with a configration option swtpm_active_pcr_banks that >> allows a user to set a comma-separated list of PCR banks to activate >> during 'TPM manufacturing'. Valid PCR banks are sha1,sha256,sha384 and >> sha512. >> > Why not put this option in swtpm_setup.conf instead? That is another option but it depends on when one wants to see the effect or how one wants to control it. With newer libvirt or newer swtpm?
On Wed, Oct 27, 2021 at 05:48:19PM -0400, Stefan Berger wrote: > > On 10/27/21 14:17, Marc-André Lureau wrote: > > Hi > > > > On Wed, Oct 27, 2021 at 9:00 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > Extend qemu.conf with a configration option swtpm_active_pcr_banks that > > > allows a user to set a comma-separated list of PCR banks to activate > > > during 'TPM manufacturing'. Valid PCR banks are sha1,sha256,sha384 and > > > sha512. > > > > > Why not put this option in swtpm_setup.conf instead? > > That is another option but it depends on when one wants to see the effect or > how one wants to control it. With newer libvirt or newer swtpm? The obvious reason for putting it in swtpm_setup.conf is that it also benefits people using swtpm in a non-libvirt scenario. IMHO, we should put it in swtpm_setup.conf, and *also* have a build time option in swtpm to configure the built-in default. IOW, I'd expect RHEL-9 RPM swtpm.spec to pass %configure --default-pcr-banks=sha256 and then have the swtpm_setup.conf option to allow admins to override the distro default if they need a weaker setup on a host. On the libvirt side, I think we could have a domain XML config option for PCR banks, to allow the built-in default or admin local default to be override per-VM. 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 10/28/21 07:04, Daniel P. Berrangé wrote: > On Wed, Oct 27, 2021 at 05:48:19PM -0400, Stefan Berger wrote: >> On 10/27/21 14:17, Marc-André Lureau wrote: >>> Hi >>> >>> On Wed, Oct 27, 2021 at 9:00 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> Extend qemu.conf with a configration option swtpm_active_pcr_banks that >>>> allows a user to set a comma-separated list of PCR banks to activate >>>> during 'TPM manufacturing'. Valid PCR banks are sha1,sha256,sha384 and >>>> sha512. >>>> >>> Why not put this option in swtpm_setup.conf instead? >> That is another option but it depends on when one wants to see the effect or >> how one wants to control it. With newer libvirt or newer swtpm? > The obvious reason for putting it in swtpm_setup.conf is that it also > benefits people using swtpm in a non-libvirt scenario. > > IMHO, we should put it in swtpm_setup.conf, and *also* have a build > time option in swtpm to configure the built-in default. > > IOW, I'd expect RHEL-9 RPM swtpm.spec to pass > > %configure --default-pcr-banks=sha256 > > and then have the swtpm_setup.conf option to allow admins to override > the distro default if they need a weaker setup on a host. I now have a pending PR to swtpm that does this modulo using --enable-default-pcr-banks=sha256. The selection of the PCR banks to activate can then be done via swtpm_setup.conf active_pcr_banks = <list of PCR banks> entry, if provided, otherwise it's back to the configure line default. https://github.com/stefanberger/swtpm/pull/615 > > > On the libvirt side, I think we could have a domain XML config option > for PCR banks, to allow the built-in default or admin local default to > be override per-VM. Is there an example of an attribute that can only be set once in the domain XML and cannot be modified after? The choice of active PCR banks is limited to 'TPM manufacturing' time, which means swtpm_setup runs once only when the swtpm's state directory does not exist because later it would overwrite the entire state and erase all keys etc.. Later manipulations of the PCR banks would have to be done using the firmware menu, which exist in EDK2, SeaBIOS and SLOF. Stefan > > Regards, > Daniel
On Thu, Oct 28, 2021 at 01:51:33PM -0400, Stefan Berger wrote: > > On 10/28/21 07:04, Daniel P. Berrangé wrote: > > On Wed, Oct 27, 2021 at 05:48:19PM -0400, Stefan Berger wrote: > > > On 10/27/21 14:17, Marc-André Lureau wrote: > > > > Hi > > > > > > > > On Wed, Oct 27, 2021 at 9:00 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > > Extend qemu.conf with a configration option swtpm_active_pcr_banks that > > > > > allows a user to set a comma-separated list of PCR banks to activate > > > > > during 'TPM manufacturing'. Valid PCR banks are sha1,sha256,sha384 and > > > > > sha512. > > > > > > > > > Why not put this option in swtpm_setup.conf instead? > > > That is another option but it depends on when one wants to see the effect or > > > how one wants to control it. With newer libvirt or newer swtpm? > > The obvious reason for putting it in swtpm_setup.conf is that it also > > benefits people using swtpm in a non-libvirt scenario. > > > > IMHO, we should put it in swtpm_setup.conf, and *also* have a build > > time option in swtpm to configure the built-in default. > > > > IOW, I'd expect RHEL-9 RPM swtpm.spec to pass > > > > %configure --default-pcr-banks=sha256 > > > > and then have the swtpm_setup.conf option to allow admins to override > > the distro default if they need a weaker setup on a host. > > I now have a pending PR to swtpm that does this modulo using > --enable-default-pcr-banks=sha256. The selection of the PCR banks to > activate can then be done via swtpm_setup.conf active_pcr_banks = <list of > PCR banks> entry, if provided, otherwise it's back to the configure line > default. > > https://github.com/stefanberger/swtpm/pull/615 Great, that looks good. > > On the libvirt side, I think we could have a domain XML config option > > for PCR banks, to allow the built-in default or admin local default to > > be override per-VM. > > Is there an example of an attribute that can only be set once in the domain > XML and cannot be modified after? The choice of active PCR banks is limited > to 'TPM manufacturing' time, which means swtpm_setup runs once only when the > swtpm's state directory does not exist because later it would overwrite the > entire state and erase all keys etc.. Later manipulations of the PCR banks > would have to be done using the firmware menu, which exist in EDK2, SeaBIOS > and SLOF. Yeah, it is a little unusual, but then I guess we have the similarish with other firmware selection, where setting "secure=yes|no" determines which OVMF binary we pick to use. 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 10/28/21 14:16, Daniel P. Berrangé wrote: > On Thu, Oct 28, 2021 at 01:51:33PM -0400, Stefan Berger wrote: > >>> On the libvirt side, I think we could have a domain XML config option >>> for PCR banks, to allow the built-in default or admin local default to >>> be override per-VM. >> Is there an example of an attribute that can only be set once in the domain >> XML and cannot be modified after? The choice of active PCR banks is limited >> to 'TPM manufacturing' time, which means swtpm_setup runs once only when the >> swtpm's state directory does not exist because later it would overwrite the >> entire state and erase all keys etc.. Later manipulations of the PCR banks >> would have to be done using the firmware menu, which exist in EDK2, SeaBIOS >> and SLOF. > Yeah, it is a little unusual, but then I guess we have the similarish > with other firmware selection, where setting "secure=yes|no" determines > which OVMF binary we pick to use. I will probably add a new feature (for swtpm v0.7) to be able to reconfigure the active pcr banks. The availability of this feature can be detected by libvirt via the JSON that swtpm_setup --print-capabilities returns (as usual). Now the problems are: - What to do when an older version of swtpm package is installed regarding the contents of the XML? Reject the pcr banks one can declare in the domain XML? The other option would be to allow the XML but not to react to it at all and document that one needs swptm v0.7 or later which will probably be the case in most setups sooner or later. - How would one track changes to the XML versus the state of the swtpm? At the moment I would run the reconfigure script ever time if a set of active PCR banks was given in the XML and it would log like shown below. Should we just turn off the logging (no --log <filename> option) for when doing the '--reconfigure'? Or still log it? Or could we assume the user will remove the active PCR banks description from the XML to avoid the running of swtpm_setup every time to reconfigure (probably not)? $ swtpm_setup --tpmstate ./ --tpm2 --reconfigure --pcr-banks sha1 Starting vTPM reconfiguration as stefanb:stefanb @ Fri 29 Oct 2021 03:23:59 PM EDT TPM is listening on Unix socket. Successfully activated PCR banks sha1 among sha1,sha256,sha384,sha512. Successfully authored TPM state. Ending vTPM manufacturing @ Fri 29 Oct 2021 03:23:59 PM EDT The only concern is a log full of these messages. The alternative is to configuring the active PCR banks on the swtpm_setup level via swtpm_setup.conf and default compile-time options and leave the reconfiguration to using the firmware... Stefan
© 2016 - 2024 Red Hat, Inc.