> On June 9, 2015, 4:03 p.m., Andrew Stitcher wrote: > > proton-c/src/platform.c, line 77 > > <https://reviews.apache.org/r/35252/diff/2/?file=981447#file981447line77> > > > > My very limited knowledge of cryptographic code tells me that the > > rand() family chould **never** be used, as they aren't very good - where > > did you get this code from? > > > > Can we have someone who know something about cryptography check this > > out? > > Flavio Percoco wrote: > You make a very good point about cryptography and we having this custom > implementation. The reason I feel comfortable with this is because it's just > being used to generate the messenger name but having it in the source code > means that someone might use it in the future. This is exactly the reason why > I think we shouldn't be using UUID's at all to generate the messenger name > but just some random number. That said, considering this is an UUID4 - as > also explicitly state by the function name itself - I think it doesn't matter > much. The UUID4 is based on truly-random or pseudo-random numbers generation > as stated in the RFC. > > http://tools.ietf.org/html/rfc4122#section-4.4 > > Kenneth Giusti wrote: > He got this code from me. > > We don't need a cryptographically secure generator - at least for our > usage. The only place this is used is messenger, and its used to pick a > name for the container IFF the application fails to provide one. > > So let's move this function to messenger.c and call it > a_somewhat_random_name(). > > Alan Conway wrote: > Why are we using UUIDs at all? Just barf if the user doesn't supply a > name, or use a simple global atomic counter to number them. What, if any, is > the actual uniqueness requirement for messenger names? Is it ever passed > outside the process? > > Andrew Stitcher wrote: > The uuid is used as the local container name I think (sent to the peer) > and so does actually need to be globally unique. > > Alan Conway wrote: > The spec says nothing (that I can find) about container-id being unique. > All it says is that it is a string and it SHOULD be used for "traceability" > and "identification". I don't think it is or can be used for any form of > automated routing as presently specified, only for human-readable debugging > purposes. In which case a UUID is the worst default. Perhaps > IP-address:PID:number would be a nice helpful thing to use instead. > > Alan Conway wrote: > Lets not forget, we are providing a *default* for a highly underspecified > and AFAIK almost entirely unused name that the user is free to set themselves > if they care about it. It really doesn't matter what we do, so it should be > simple. > > Flavio Percoco wrote: > The very very very first patch I had proposed removing UUID completely in > favor of some random, number-based, name generation... but again I preferred > to take small steps forward. As Alan said in the previous review "be > agressive, beee agressive". > > The reason I'm bringing the above up is because I think that providing > something that is unique enough to make requests traceable should be ok (and > that's certainly not just a random number but something like Alan proposed). > However, since this patch is already here and the uuid like name has been > implemented already, I'd probably give this a go first. > > The reason I'd prefer this to land instead of something in the form of > "IP-address:PID:number" is that I'd expect people to start making assumptions > on this new form and the information it carries, then some apps might even > end up relying on it for some operations that they shouldn't and that ends up > in a mess. I'd rather have a generated ID that provides no information > whatsoever about the node that it was generated in since that information > should not be used at al and it may change if users decide to provide a name. > > > That said, we could create a hash out of the format Alan's proposed... :D
The _simplest_ approach is to break the pn_messenger() method's API. If the user doesn't supply a name, fail. The best choice of a name should be left up to the application, which is the only place that can determine what 'uniqueness' and 'traceability' mean for a given application. IMHO the whole purpose of this patch is to remove what I consider a needless hard dependency. Currently, if the user does not have the uuid development library installed, they cannot build proton-c. Dead stop. Worse, the component that has the libuuid dependency is _optional_ (messenger). Folks who want to use the reactive api or the engine are subject to a hard dependency that they have to address even though they're not really subject to it. Sure, the libuuid library is pretty much ubiquitous on newer distros, but the same cannot be said for the _headers_. So a user who wishes to build proton better have these installed, either by having admin rights or by downloading and installing locally the headers. Does this inconvenience make up for the convenience of being able to call pn_messenger() without providing a well thought out name? And, in the process getting a name that has really _no_ traceability/debugability merits? I'd say break the pn_messenger() call. Make the users _think_ about what a good container name should be - such as an IP address or DNS name. They will be better off for it. - Kenneth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35252/#review87213 ----------------------------------------------------------- On June 9, 2015, 5:37 p.m., Flavio Percoco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35252/ > ----------------------------------------------------------- > > (Updated June 9, 2015, 5:37 p.m.) > > > Review request for qpid, Alan Conway, Chug Rolke, Gordon Sim, and Rafael > Schloming. > > > Repository: qpid-proton-git > > > Description > ------- > > Instead of relying on libuuid for uuid generation, let proton-c have a > built-in uuid4 generator to do this job. > > > Diffs > ----- > > proton-c/CMakeLists.txt b534e86 > proton-c/bindings/python/setup.py 79168d2 > proton-c/src/messenger/messenger.c f226f7b > proton-c/src/platform.h 6962493 > proton-c/src/platform.c 8f8ac5f > > Diff: https://reviews.apache.org/r/35252/diff/ > > > Testing > ------- > > > Thanks, > > Flavio Percoco > >
