Re: [libvirt] [PATCH] Move the FIPS detection from capabilities

On 09/18/2014 09:52 AM, Pavel Hrdina wrote: > We are not detecting the presence of FIPS from QEMU, but from procfs and > that means it's not QEMU capability. It was decided that we will pass > this flag to QEMU even if it's not supported by old QEMU binaries. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135431 > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > > Note: The original bug is that we are not detecting whether libvirtd > binary has been updated, we detect that only for QEMU binary. So you > could update libvirt without updating QEMU and new capabilities that could > already exists in QEMU, but was recently implemented in libvirt wasn't > enabled. I'll post a patch to fix this bug. > > src/qemu/qemu_capabilities.c | 24 ------------------------ > src/qemu/qemu_command.c | 25 +++++++++++++++++++++++-- > 2 files changed, 23 insertions(+), 26 deletions(-) > I agree with moving the detection of the bit out of cached capability bit and delaying it instead to qemu startup time (although it means a stat() call for every qemu spawned, instead of the former behavior of checking only once). I also agree with the way you removed setting the bit in qemu_capabilities.c but not the bit itself (the bit is still defined and named from qemu_capabilities.h, so that if we upgrade libvirtd and parse the XML for a running qemu domain that was started from earlier libvirt, we don't mis-behave when seeing that capability bit, even if we no longer use it for anything). However, I still think you need a v2: > +++ b/src/qemu/qemu_command.c > @@ -7656,8 +7656,29 @@ qemuBuildCommandLine(virConnectPtr conn, > if (!standalone) > virCommandAddArg(cmd, "-S"); /* freeze CPU */ > > - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ENABLE_FIPS)) > - virCommandAddArg(cmd, "-enable-fips"); > + /* Qemu 1.2 and later have a binary flag -enable-fips that must be > + * used for VNC auth to obey FIPS settings; but the flag only > + * exists on Linux, and with no way to probe for it via QMP. Our > + * solution: if FIPS mode is required, then unconditionally use > + * the flag, regardless of qemu version, for the following matrix: > + * > + * old QEMU new QEMU > + * FIPS enabled doesn't start VNC auth disabled > + * FIPS disabled/missing VNC auth enabled VNC auth enabled > + * > + * Setting the flag here instead of in virQEMUCapsInitQMPMonitor > + * or virQEMUCapsInitHelp also allows the testsuite to be > + * independent of FIPS setting. > + */ > + if (virFileExists("/proc/sys/crypto/fips_enabled")) { Ouch. This will make our testsuite differ based on whether it is run on Linux in FIPS mode (where FIPS might exist) or on any other setup. I think you need to hoist the check for virFileExists() to the caller, and pass in the result as a new bool parameter to this function, so that the testsuite has full control over the boolean without regards to the current system's level of FIPS support.