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

[Pavel Hrdina]

On 09/18/2014 06:29 PM, Eric Blake wrote:
> 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 at redhat.com>
>> ---
>>
>> 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.
>

Sigh, that's right. I've completely forget about the tests. And that 
reminds me whether we should also revert the test for enable-fips as it 
seems to be pointless?

Pavel