[libvirt] [PATCH] tests: qemu: mock master key generation
The master key generation is using host state to get the random bytes. Currently it's not used so it doesn't break any tests, but it may become a problem in the future. Fix it by mocking qemuDomainGenerateRandomKey. This is possible after exporting it and switching to the dynamically linked qemu driver object for tests. --- This also makes the test not call any gnutls function which crashed certain gnutls versions as we did not initialize it (gnutls_global_init()). It may be also worth adding the call to virtTestMain so that tests don't run into that problem, but I'm not totaly persuaded that it's a good idea. src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 ++ tests/Makefile.am | 2 +- tests/qemuxml2argvmock.c | 21 +++++++++++++++++++++ 4 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3da0079..d955ed4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -640,7 +640,7 @@ qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv) * * Returns pointer memory containing key on success, NULL on failure */ -static uint8_t * +uint8_t * qemuDomainGenerateRandomKey(size_t nbytes) { uint8_t *key; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c711188..b116949 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -669,4 +669,6 @@ int qemuDomainSecretPrepare(virConnectPtr conn, virDomainObjPtr vm) int qemuDomainDefValidateDiskLunSource(const virStorageSource *src) ATTRIBUTE_NONNULL(1); +uint8_t *qemuDomainGenerateRandomKey(size_t nbytes); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 75efb90..ba14b8f 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -561,7 +561,7 @@ libqemutestdriver_la_LIBADD = $(qemu_LDADDS) qemuxml2argvtest_SOURCES = \ qemuxml2argvtest.c testutilsqemu.c testutilsqemu.h \ testutils.c testutils.h -qemuxml2argvtest_LDADD = $(qemu_LDADDS) $(LDADDS) $(LIBXML_LIBS) +qemuxml2argvtest_LDADD = libqemutestdriver.la $(LDADDS) $(LIBXML_LIBS) qemuxml2argvmock_la_SOURCES = \ qemuxml2argvmock.c diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 1616eed..1bad3ae 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -30,6 +30,10 @@ #include "virstring.h" #include "virtpm.h" #include "virutil.h" +#include "viralloc.h" + +#include "qemu/qemu_domain.h" + #include <time.h> #include <unistd.h> @@ -145,3 +149,20 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, { /* nada */ } + + +uint8_t * +qemuDomainGenerateRandomKey(size_t nbytes) +{ + size_t i; + uint8_t seq = 0; + uint8_t *key; + + if (VIR_ALLOC_N(key, nbytes) < 0) + return NULL; + + for (i = 0; i < nbytes; i++) + key[i] = seq++; + + return key; +} -- 2.8.2
On 05/11/2016 07:36 AM, Peter Krempa wrote:
The master key generation is using host state to get the random bytes. Currently it's not used so it doesn't break any tests, but it may become a problem in the future.
Fix it by mocking qemuDomainGenerateRandomKey. This is possible after exporting it and switching to the dynamically linked qemu driver object for tests. --- This also makes the test not call any gnutls function which crashed certain gnutls versions as we did not initialize it (gnutls_global_init()). It may be also worth adding the call to virtTestMain so that tests don't run into that problem, but I'm not totaly persuaded that it's a good idea.
src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 ++ tests/Makefile.am | 2 +- tests/qemuxml2argvmock.c | 21 +++++++++++++++++++++ 4 files changed, 25 insertions(+), 2 deletions(-)
I think I addressed this in patch 2 of my series: http://www.redhat.com/archives/libvir-list/2016-May/msg00444.html John
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3da0079..d955ed4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -640,7 +640,7 @@ qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv) * * Returns pointer memory containing key on success, NULL on failure */ -static uint8_t * +uint8_t * qemuDomainGenerateRandomKey(size_t nbytes) { uint8_t *key; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c711188..b116949 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -669,4 +669,6 @@ int qemuDomainSecretPrepare(virConnectPtr conn, virDomainObjPtr vm) int qemuDomainDefValidateDiskLunSource(const virStorageSource *src) ATTRIBUTE_NONNULL(1);
+uint8_t *qemuDomainGenerateRandomKey(size_t nbytes); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 75efb90..ba14b8f 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -561,7 +561,7 @@ libqemutestdriver_la_LIBADD = $(qemu_LDADDS) qemuxml2argvtest_SOURCES = \ qemuxml2argvtest.c testutilsqemu.c testutilsqemu.h \ testutils.c testutils.h -qemuxml2argvtest_LDADD = $(qemu_LDADDS) $(LDADDS) $(LIBXML_LIBS) +qemuxml2argvtest_LDADD = libqemutestdriver.la $(LDADDS) $(LIBXML_LIBS)
qemuxml2argvmock_la_SOURCES = \ qemuxml2argvmock.c diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 1616eed..1bad3ae 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -30,6 +30,10 @@ #include "virstring.h" #include "virtpm.h" #include "virutil.h" +#include "viralloc.h" + +#include "qemu/qemu_domain.h" + #include <time.h> #include <unistd.h>
@@ -145,3 +149,20 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, { /* nada */ } + + +uint8_t * +qemuDomainGenerateRandomKey(size_t nbytes) +{ + size_t i; + uint8_t seq = 0; + uint8_t *key; + + if (VIR_ALLOC_N(key, nbytes) < 0) + return NULL; + + for (i = 0; i < nbytes; i++) + key[i] = seq++; + + return key; +}
On Wed, May 11, 2016 at 07:53:07 -0400, John Ferlan wrote:
On 05/11/2016 07:36 AM, Peter Krempa wrote:
The master key generation is using host state to get the random bytes. Currently it's not used so it doesn't break any tests, but it may become a problem in the future.
Fix it by mocking qemuDomainGenerateRandomKey. This is possible after exporting it and switching to the dynamically linked qemu driver object for tests. --- This also makes the test not call any gnutls function which crashed certain gnutls versions as we did not initialize it (gnutls_global_init()). It may be also worth adding the call to virtTestMain so that tests don't run into that problem, but I'm not totaly persuaded that it's a good idea.
src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 ++ tests/Makefile.am | 2 +- tests/qemuxml2argvmock.c | 21 +++++++++++++++++++++ 4 files changed, 25 insertions(+), 2 deletions(-)
I think I addressed this in patch 2 of my series:
http://www.redhat.com/archives/libvir-list/2016-May/msg00444.html
I didn't notice it since it was together with other stuff. I've reviewed that series. Looking at the code I think this patch is a bit cleaner than the approach used in the linked patch. Additionally this also switches right away to the shared object qemu driver which allows to mock functions in the qemu driver directly. Peter
On 05/11/2016 08:49 AM, Peter Krempa wrote:
On Wed, May 11, 2016 at 07:53:07 -0400, John Ferlan wrote:
On 05/11/2016 07:36 AM, Peter Krempa wrote:
The master key generation is using host state to get the random bytes. Currently it's not used so it doesn't break any tests, but it may become a problem in the future.
Fix it by mocking qemuDomainGenerateRandomKey. This is possible after exporting it and switching to the dynamically linked qemu driver object for tests. --- This also makes the test not call any gnutls function which crashed certain gnutls versions as we did not initialize it (gnutls_global_init()). It may be also worth adding the call to virtTestMain so that tests don't run into that problem, but I'm not totaly persuaded that it's a good idea.
src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 ++ tests/Makefile.am | 2 +- tests/qemuxml2argvmock.c | 21 +++++++++++++++++++++ 4 files changed, 25 insertions(+), 2 deletions(-)
I think I addressed this in patch 2 of my series:
http://www.redhat.com/archives/libvir-list/2016-May/msg00444.html
I didn't notice it since it was together with other stuff. I've reviewed that series. Looking at the code I think this patch is a bit cleaner than the approach used in the linked patch.
IDC either way for the implementation... It'll adjust my series insomuch as I the 'answer' I expect in the .args file for the 'iv' and encoded masterKey, but I can deal with that. One reason I mocked virRandomBytes was based on suggestion, but also because it was used by virUUIDGenerate and that could mean we can generate output xml/args from input that doesn't have a uuid since we can predict the answer. Also, "theoretically speaking" virStorageGenerateQcowPassphrase could call virRandomBytes (with a couple of adjustments) instead of going to /dev/urandom... Anything in the future which may use virRandomBytes could also get predictable answers (while it's not written now, I'm thinking the changes that would support luks may find it useful). Basically, anything else that needs to perhaps get a "random" string of bytes could then get predictable answers if virRandomBytes is changed. I'm OK with this method, although I am wondering if it would be better to mock virRandomBytes instead? John
Additionally this also switches right away to the shared object qemu driver which allows to mock functions in the qemu driver directly.
Peter
On Wed, May 11, 2016 at 11:42:28 -0400, John Ferlan wrote:
On 05/11/2016 08:49 AM, Peter Krempa wrote:
On Wed, May 11, 2016 at 07:53:07 -0400, John Ferlan wrote:
On 05/11/2016 07:36 AM, Peter Krempa wrote:
The master key generation is using host state to get the random bytes. Currently it's not used so it doesn't break any tests, but it may become a problem in the future.
Fix it by mocking qemuDomainGenerateRandomKey. This is possible after exporting it and switching to the dynamically linked qemu driver object for tests. --- This also makes the test not call any gnutls function which crashed certain gnutls versions as we did not initialize it (gnutls_global_init()). It may be also worth adding the call to virtTestMain so that tests don't run into that problem, but I'm not totaly persuaded that it's a good idea.
src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 ++ tests/Makefile.am | 2 +- tests/qemuxml2argvmock.c | 21 +++++++++++++++++++++ 4 files changed, 25 insertions(+), 2 deletions(-)
I think I addressed this in patch 2 of my series:
http://www.redhat.com/archives/libvir-list/2016-May/msg00444.html
I didn't notice it since it was together with other stuff. I've reviewed that series. Looking at the code I think this patch is a bit cleaner than the approach used in the linked patch.
IDC either way for the implementation... It'll adjust my series insomuch as I the 'answer' I expect in the .args file for the 'iv' and encoded masterKey, but I can deal with that.
One reason I mocked virRandomBytes was based on suggestion, but also because it was used by virUUIDGenerate and that could mean we can generate output xml/args from input that doesn't have a uuid since we can predict the answer.
Hmm, that's a good call. We actually could test that UUIDs are generated in a sane way. I remember seeing some code that is actually stripping the UUID from the XML in some tests. This is yet another reason to keep it in a separate patch though.
Also, "theoretically speaking" virStorageGenerateQcowPassphrase could call virRandomBytes (with a couple of adjustments) instead of going to /dev/urandom... Anything in the future which may use virRandomBytes could also get predictable answers (while it's not written now, I'm thinking the changes that would support luks may find it useful).
Yes, virRandom bytes would really improve the quality of that function. It's not really going to be used in the qemuxml2argvtest though so the mock won't help in that case.
Basically, anything else that needs to perhaps get a "random" string of bytes could then get predictable answers if virRandomBytes is changed.
I'm OK with this method, although I am wondering if it would be better to mock virRandomBytes instead?
Mocking virRandomBytes is okay with me. I don't like the deps and conditionally compiled code that is pulled in by mocking the gnutls code, thus I decided to mock the whole function using it since it's covering both cases. One of the options would be to split out the part that is generating the random bytes from the part allocating the buffer. It's questionable wheter it's worth though. At any rate, I can make a hybrid between your version of the mock patch and mine, which will mock both qemuDomainGenerateRandomKey and virRandomBytes so that we still avoid messing with mocking gnutls. Peter
participants (2)
-
John Ferlan -
Peter Krempa