s4-messaging: Use generate_random() to get a unique ID for messaging clients

[Andrew Bartlett]

On Wed, 2012-04-25 at 23:12 +1000, Andrew Bartlett wrote:
> On Wed, 2012-04-25 at 08:59 -0400, simo wrote:
> > On Wed, 2012-04-25 at 22:51 +1000, Andrew Bartlett wrote: 
> > > On Wed, 2012-04-25 at 08:04 -0400, simo wrote:
> > > > On Wed, 2012-04-25 at 11:44 +0200, Andrew Bartlett wrote:
> > > > > commit b8055132b1c62dd19981fea2822ab9e1829a8ded
> > > > > Author: Andrew Bartlett <abartlet at samba.org>
> > > > > Date:   Wed Apr 25 17:53:18 2012 +1000
> > > > > 
> > > > >     s4-messaging: Use generate_random() to get a unique ID for
> > > > > messaging clients
> > > > >     
> > > > >     The call to random() resulted in duplicate values for s3fs
> > > > > configurations
> > > > >     which, due to the forked child, all started with the same random
> > > > > seed.
> > > > >     
> > > > >     A future improvement would be to move to a proven unique value.
> > > > >     
> > > > >     Andrew Bartlett
> > > > >     
> > > > >     Autobuild-User: Andrew Bartlett <abartlet at samba.org>
> > > > >     Autobuild-Date: Wed Apr 25 11:43:40 CEST 2012 on sn-devel-104
> > > > 
> > > > 
> > > > Andrew are you sure you need to use /dev/urandom here ? It doesn't look
> > > > like you need absolutelu unpredictable numbers here, just non-colliding
> > > > numbers.
> > > > You changed the code to draw from urandom, and if it is used often it
> > > > mean it will suck a lot of entropy out of the system, causing any
> > > > application that need to use /dev/random to halt.
> > > > Wouldn't it have been simpler to just run srand(time(NULL)*pid) to get a
> > > > new seed for the process ?
> > > 
> > > Simo,
> > > 
> > > You are correct to think carefully about our over-use of random entropy.
> > > In this case, it isn't used often (typically once per child process in
> > > s3fs configurations only), and we have logic in generate_random_buffer()
> > > to attempt to reduce the load on /dev/urandom. We set up our own PRNG,
> > > where a process has read more than 40 bytes, which I added with
> > > 6554433fc227baab93398576db703c91db1541f2 back in 2007.
> > 
> > This doesn't really help at process startup, as we reset the global
> > variable after fork, so for the first 40 bytes at least we do
> > use /dev/urandom, that's why I had a concern.
> 
> Certainly.  If we were using less than 40 random bytes in total per
> process, then we could be reading a little more random data.  Do you
> have a suggestion for a different heuristic?  (It was just a guess at
> the time, trying not to always pull 40 bytes per process regardless).
> 
> > > I also worked on a solution using IDR trees to allocate a server_id
> > > number (along side the pid, for the auth_samba4 use case):
> > > https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/s3-task-id-allocator however I haven't yet made it work.  If I can make it work, I would prefer to use an allocator, for more predictable behaviour under test.  
> > > 
> > > Another idea I had was to use the combination of pid and memory address
> > > of the event context or messaging context, but currently 'task' is just
> > > 32 bits.
> > > 
> > > Metze has also suggested some improvements on which fields to randomize
> > > in https://bugzilla.samba.org/show_bug.cgi?id=8872#c4 
> > 
> > Ok, seem like iot is staying on the radar, that's good enough.
> > 

I've pushed patches to autobuild which I hope you will agree addresses
this, and metze's concerns in the bug. 

Thanks for your comments!

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org