[libvirt] [PATCH] tests: qemu: mock master key generation

[Peter Krempa]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160511/f6dff234/attachment-0001.sig>