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

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(a)redhat.com&gt; > > > > --- > > > > 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.

IMHO we should used be using getrandom() as the first fallback, and only then try /dev/urandom or /dev/random if the former doesn't exist

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 :|