11:52 p.m.
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>
---
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(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 9246813..5c3778d 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3215,30 +3215,6 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
config.data.nix.path = monpath;
config.data.nix.listen = false;
- /* 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")) {
- char *buf = NULL;
-
- if (virFileReadAll("/proc/sys/crypto/fips_enabled", 10, &buf) <
0)
- goto cleanup;
- if (STREQ(buf, "1\n"))
- virQEMUCapsSet(qemuCaps, QEMU_CAPS_ENABLE_FIPS);
- VIR_FREE(buf);
- }
-
VIR_DEBUG("Try to get caps via QMP qemuCaps=%p", qemuCaps);
/*
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f2e6e5a..3532518 100644
--- a/src/qemu/qemu_command.c
+++ 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")) {
+ char *buf = NULL;
+
+ if (virFileReadAll("/proc/sys/crypto/fips_enabled", 10, &buf) <
0)
+ goto error;
+ if (STREQ(buf, "1\n"))
+ virCommandAddArg(cmd, "-enable-fips");
+ VIR_FREE(buf);
+ }
if (qemuBuildMachineArgStr(cmd, def, qemuCaps) < 0)
goto error;
--
1.8.5.5
12:29 a.m.
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>
---
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:44 a.m.
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(a)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
12:48 a.m.
On 09/18/2014 10:44 AM, Pavel Hrdina wrote:
> 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?
It's still worth testing that we can add or avoid '-enable-fips' in the
generated command line according to the boolean value we pass in.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:55 a.m.
On 09/18/2014 06:48 PM, Eric Blake wrote:
On 09/18/2014 10:44 AM, Pavel Hrdina wrote:
>> 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?
It's still worth testing that we can add or avoid '-enable-fips' in the
generated command line according to the boolean value we pass in.
Yes I agree and that's why I would like to remove the test from
qemucapabilitiestest and create a new one in qemuxml2argvtest.
1:13 a.m.
On 09/18/2014 10:55 AM, Pavel Hrdina wrote:
On 09/18/2014 06:48 PM, Eric Blake wrote:
> On 09/18/2014 10:44 AM, Pavel Hrdina wrote:
>
>>> 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?
>
> It's still worth testing that we can add or avoid '-enable-fips' in the
> generated command line according to the boolean value we pass in.
>
Yes I agree and that's why I would like to remove the test from
qemucapabilitiestest and create a new one in qemuxml2argvtest.
Yes, that makes sense.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3696
days inactive
3696
days old
5 comments
2 participants
participants (2)
-
Eric Blake -
Pavel Hrdina