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

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(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. > 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.