Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Because it is a Drupal-provided function and components cannot call them. Since drupal_random_bytes() is a simple utility function, I propose we move *it* to a Drupal\Component\Utility\RandomBytes.php and remove the function in bootstrap.inc.
Comment | File | Size | Author |
---|---|---|---|
#56 | drupal-crypt-1929288-55.patch | 33.55 KB | ParisLiakos |
#56 | interdiff.txt | 2.7 KB | ParisLiakos |
#54 | drupal-crypt-1929288-54.patch | 33.4 KB | ParisLiakos |
#53 | drupal-crypt-1929288-53.patch | 33.4 KB | ParisLiakos |
#50 | drupal-crypt-1929288-50.patch | 33.59 KB | ParisLiakos |
Comments
Comment #1
Dave ReidThis moves drupal_random_bytes() to a new component and replaces all function calls.
Comment #2
longwavePatch appears to contain unrelated changes in core/modules/field/field.attach.inc?
Comment #3
longwaveThis is presumably redundant now our minimum version is 5.3.5, although this should probably be dealt with separately.
Comment #4
Dave ReidUnrelated changes gone. Yeah, we should probably address it as a follow-up. Just wanted to move around code only.
Comment #5
longwaveThis is simply moving code out of bootstrap.inc and into a component, looks good to me.
And if this is now a component that can be used outside of Drupal, I guess the version_compare() test should stay in place.
Comment #6
catchIt'd be good to have a follow-up for the version_compare() - while other projects might use the component it's OK for our components to have sensible PHP version requirements regardless. It'd need replacing with a comment though at least.
The reference to drupal_static() should go, it wouldn't be allowed to call that anyway.
I think the static variable/method is probably fine here at least for now. It could move to a property if we turned this into a service. Was there a specific reason not to do that or is that just a product of the straight conversion?
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedremoved the comment and rerolled
i think maybe we should keep it like this for now, since this is used in mostly procedural code..and we should also add some unit tests..but lets do the conversion first?
Comment #8
dawehnerI'm wondering whether there is any kind of way to test this class.
All of them are nitpicky.
I guess we could remove the spaces in that patch.
Should be \Contains
... that's probably an int.
empty line missing.
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentednewline is there as far as i can see:)
and did not remove the space, cause there is consistent spacing like that in this function, and its out of scope cleaning it up all
Comment #10
dawehnershould be
Comment #11
ParisLiakos CreditAttribution: ParisLiakos commentedah, thats what you meant:)
Comment #12
RobLoachSince we're using a default of 55 almost everywhere we use
::generate()
, would it make sense to introduce a default of 55 in the function?I see lots of use of generation of the session ID via:
If RandomBytes was named
Random
, then we could add more things related to::generate
in there, including the base 64 hash generator. Might be out of scope here though.Noticed that there arn't any tests for
::generate()
. I understand that it just returns a random set of bytes, but that is still test-worthy, and this seems like a great chance to introduce some new PHPUnit tests.If anyone has any objections, I'd love to put together a PHPUnit hit on this.
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedagreed on all points above, i ll try to roll a patch with them later
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedsorry, seems i wont have time till next week
Comment #15
Crell CreditAttribution: Crell commentedThis is no longer relevant as we don't support that version of PHP.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedCompletely different patch, that moves the class to a service, turns static variables into properties and injects Request since we need $_SERVER.
Also added a phpunit test.
Lets leave the hash thing out for now, i guess
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedyeah have to register it on the install bundle as well
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedsigh..this will install
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedtypo:/
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedstatus
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedyeah well for this to work Uuid needs to be a service..so we either go with #11 or postpone for #1969572: Make Uuid a service
Comment #26
Dave ReidFeels like we're adding complications by making this a service rather than a Drupal-agnostic component. Is the strategy that all these types of fuctions that don't really depend on Drupal are supposed to be services rather than components?
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedi have to agree about the complexicity
here is the patch in #11 rerolled, with the phpunit added
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedsigh, i totally miss status updating today
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedand with correct docblocks:P
Comment #30
dawehnerodd spaces. Let's fix that.
Missing @return
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedthere:)
Comment #32
dawehnerThanks!
Comment #33
pwolanin CreditAttribution: pwolanin commentedCrell asked in IRC if all these uses are security critical.
Looking at the patch, of the calls to get random bytes I see, maybe only the UUID one could get replaced with something like
uniqid(mt_rand(), TRUE)
That's probably the main one that's a performance hit anyhow.
Comment #34
pwolanin CreditAttribution: pwolanin commentedComment #35
Crell CreditAttribution: Crell commentedrootawc, pwolanin, msonnabaum, and I just had a lengthy conversation in IRC about this issue and we're going to change direction a little.
1) We don't need a RandomBytes class. We need a Cyrpt class, which would include randomBytes() as well as hashBase64() and hmacBase64(). They come as a matched set.
2) The $_SERVER here is not being used in a contextual way, just in a "we need some random crap" way; $_SERVER is available in both HTTP and CLI SAPIs in PHP, so it's safe to use for that without having to inject anything. (It has a completely different set of random crap in each case, but it does have data.)
3) That said... UUID's use of random_bytes() isn't necessary anyway since that doesn't need to be a cryptographically strong string. It can just be uniquid().
rootawc is working on a new patch that does the above 3 items. Coming soon to an issue near you!
Comment #36
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease, don't. Those should stay unpredictable, there is no way to know how they are going to be used down the line. Also, this is not in the critical path.
From the patch, we might also consider simplifying this:
... I have no idea why we are doing this really weird and broken thing. Let's just use the random bytes directly.
Comment #37
ParisLiakos CreditAttribution: ParisLiakos commentedI did not touch Uuid, since it does not bring problems now, left it as is per Damien's comment.
Besides improving
After search and replace i notice this
occurs quite often. what about randomHashedString()?
Anyway, i think besides those above, patch is in good shape, i cant come up with a way to unit test hmacBase64() and hashBase64()
credits to Crell for the docblock:)
Comment #38
xjmAre we really calling it... Crypt? :)
Comment #39
Crell CreditAttribution: Crell commentedI wouldn't object to a randomHashedString($size) method. Seems a reasonable simplification.
Comment #41
ParisLiakos CreditAttribution: ParisLiakos commentedhere is a total hack
Comment #42
ParisLiakos CreditAttribution: ParisLiakos commented..
Comment #43
ParisLiakos CreditAttribution: ParisLiakos commentedmeh, i keep missing stuff
Comment #44
ParisLiakos CreditAttribution: ParisLiakos commentedOk added randStringHashed()
Any reason:
Couldnt turn to
amd just work? or objections?
Comment #45
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's not try to fix what's not broken:
The function should be called
Crypt::randomBytes()
, and$count
should be mandatory.Speaking of... I don't understand why we do this to begin with:
Hashing a random string can really only result in losing entropy. We should really just convert the random bytes to base64.
The initial reasoning was to have "safe" and "unsafe" random ids: "safe" are generated from the strong random source, "unsafe" are generated from
mt_rand()
directly, with some slight (but completely undefined) entropy added byuniqid()
.This reasoning doesn't really hold: in PHP,
mt_rand()
Is a very weak RNG, mostly because its state is not *that* hard to guess (the generator is seeded from values - the request time, the PID of the PHP process, etc. - that are partially observable and not very random). We should (but we don't) seed the generator with strong values before using it. And at this point, the performance difference between generating random bytes frommt_rand()
and reading them (in a 8k batch) directly from/dev/urandom
is not going to make a difference for us.So, I would recommend getting rid of all the
mt_rand()
and theuniqid()
in core, and be done with it.Comment #46
ParisLiakos CreditAttribution: ParisLiakos commentedSee #37 on why i added randomStringHashed(). Just for DX
In #12
Since i was already messing with DX, i thought why not?
Also, i thought RandomString is better, since it says in the docblock
Returns a string of highly randomized bytes (over the full 8-bit range).
But i am ok with just reuploading patch in #43 and renaming it to RandomBytes.
Comment #47
Damien Tournoud CreditAttribution: Damien Tournoud commented@rootatwc: I'm looking at the why we did that *even before the patch*. I think it is useful to have a function that gets you a random URL-safe identifier, so I'm not criticizing the existence of
randomStringHashed()
at all. What I'm discussing is why we are hashing + base64-ing the random bytes instead of just base64-ing them directly. This is not changed by this patch.We use
55
everywhere, but just because we actually wantrandomStringHashed()
everywhere. The default doesn't make sense outside of this particular use-case.Comment #48
Damien Tournoud CreditAttribution: Damien Tournoud commentedAbout "String" vs "Bytes": a String in Drupal always implies a UTF-8 string. It would be unwise to break this expectation here.
Comment #49
ParisLiakos CreditAttribution: ParisLiakos commentedAh, ok, sorry for misunderstanding:)
You are right, i am gonna change the method name
Comment #50
ParisLiakos CreditAttribution: ParisLiakos commentedrestored $count as required argument and renamed randomString to randomBytes
Comment #52
ParisLiakos CreditAttribution: ParisLiakos commentedentity translation ui random failure?
#50: drupal-crypt-1929288-50.patch queued for re-testing.
Comment #53
ParisLiakos CreditAttribution: ParisLiakos commentedno longer applies, reroll
Comment #54
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #55
dawehnerThere should be also an explanation, why we an't yet.
Should all be proper type hinted, sorry.
I proper to use static if possible, as this allows you to override the class later. (you never know what this might be good for).
Comment #56
ParisLiakos CreditAttribution: ParisLiakos commentedthanks for the review!
you are absolutely right on all 3 points, attached patch fixes them:)
Comment #57
msonnabaum CreditAttribution: msonnabaum commentedLooks fine to me.
Comment #58
webchickOk, this looks fairly straight-forward.
Committed and pushed to 8.x. Thanks!
Comment #59
webchickEr. Change notice.
Comment #60
ParisLiakos CreditAttribution: ParisLiakos commentedYay!
Change notice here http://drupal.org/node/1984806
Comment #61
ParisLiakos CreditAttribution: ParisLiakos commentedfollowup #1985940: Core has no valid use-case for mt_rand() or uniqid()