[libvirt] [PATCH 08/10] virrandom: Make virRandomBits better

[Daniel P. Berrangé]

On Fri, Jun 01, 2018 at 02:10:56PM +0200, Martin Kletzander wrote:
> On Fri, Jun 01, 2018 at 11:25:35AM +0100, Daniel P. Berrangé wrote:
> > On Wed, May 30, 2018 at 03:43:31PM +0200, Martin Kletzander wrote:
> > > On Wed, May 30, 2018 at 02:16:08PM +0200, Michal Privoznik wrote:
> > > > On 05/29/2018 03:44 PM, Martin Kletzander wrote:
> > > > > On Tue, May 29, 2018 at 10:24:44AM +0200, Michal Privoznik wrote:
> > > > > > Now that we have strong PRNG generator implemented in
> > > > > > virRandomBytes() let's use that instead of gnulib's random_r.
> > > > > >
> > > > > > Problem with the latter is in way we seed it: current UNIX time
> > > > > > and libvirtd's PID are not that random as one might think.
> > > > > > Imagine two hosts booting at the same time. There's a fair chance
> > > > > > that those hosts spawn libvirtds at the same time and with the
> > > > > > same PID. This will result in both daemons generating the same
> > > > > > sequence of say MAC addresses [1].
> > > > > >
> > > > > > 1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
> > > > > >
> > > > > > Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> > > > > > ---
> > > > > > src/util/virrandom.c | 63
> > > > > > ++--------------------------------------------------
> > > > > > 1 file changed, 2 insertions(+), 61 deletions(-)
> > > > > >
> > > > >
> > > > > ACK to patches 1-7.  But for this one I'm "concerned" about few things.
> > > > >
> > > > > First of all, just so I don't forget it, random_r can be removed from
> > > > > bootstrap.conf after this patch, right?
> > > >
> > > > Yes, I was meaning to make that change but then I forgot.
> > > >
> > > > >
> > > > > Before this patch, and without gnutls, we wouldn't deplete the entropy
> > > > > of the
> > > > > kernel, (even though we're just using /dev/urandom and not /dev/random),
> > > > > but now
> > > > > we'd read everything from /dev/urandom.
> > > >
> > > > Unless we are built with gnutls. But I don't see much problem with that.
> > > >
> > > 
> > > Yeah, it's not that big of a deal, just an extra point for the next thing I
> > > mentioned below.
> > > 
> > > > >
> > > > > Last but not least, if we completely drop the non-gnutls variants of
> > > > > everything,
> > > > > wouldn't everything be easier anyway?  Like the worrying about entropy
> > > > > pool in
> > > > > previous point?
> > > >
> > > > Sure. But requiring gnutls (like I'm suggesting in the cover letter) is
> > > > orthogonal to these patches IMO.
> > > >
> > > 
> > > My point was that the fixes might be could be cleaner and shorter, but that "not
> > > that big of a deal" above would be fixed.  It also makes it kind of relevant.
> > > 
> > > Since /dev/urandom cannot be really exhausted in newer Linux kernels (not sure
> > > for FreeBSD and others), I don't think that's a problem.  We should ensure,
> > > however, that it is seeded properly.  It might not be when it's early during the
> > > boot for Linux (although systemd and others seed it explicitly early enough),
> > > that's what getrandom() is for as it ensures the seeding was done properly.  But
> > > that's Linux-specific.  FreeBSD will apparently not give you any data until it's
> > > seeded properly.
> > 
> > /dev/urandom does not even exist on non-Linux hosts AFAIK, so this has the
> > effect of breaking libvirt on non-Linux hosts when gnutls is disabled.
> > 
> 
> On FreeBSD it is a symlink to /dev/random (they both behave like getrandom() on
> Linux) and I guess on MacOS it is the same.  Random search showed it exists
> there.

Ah that's good to know - I guess they added the symlink due to software having
assumptions from linux world :-)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|