Skip to content

Better RedLock token #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 11, 2021
Merged

Better RedLock token #41

merged 1 commit into from
Jan 11, 2021

Conversation

mvorisek
Copy link
Member

improve redis token entropy and make the token hexadecimal (easier to debug)

@willemstuursma
Copy link
Contributor

Thank you. This does not improve the entropy of the token.

I'll take a PR that uses binhex() on the binary data, I understand that it is useful to have a human readable token.

@mvorisek
Copy link
Member Author

mvorisek commented Jan 6, 2021

@willemstuursma

  • random_bytes is supposed to be good enough even cryptographically safe entropy. Adding a microtime, which we READ ANYWAY a few lines before, can not worse it.
  • if binhex() on binary data is ok with you, then hashing the input to hex should be fine too

@willemstuursma
Copy link
Contributor

Hi @mvorisek.

There is no reason to add the microtime to the input of the hashing function.
There is no reason to use a hashing function instead of converting the random bytes to a human readable value directly.

A change should have a good reason behind it. Else it the change is arbitrary and I prefer my own over yours.

I already explained that a PR that does binhex() only is acceptable to me. That has a clear reason, e.g. to make the tokens human readable when debugging.

@mvorisek
Copy link
Member Author

mvorisek commented Jan 11, 2021

@willemstuursma ok, updated

@willemstuursma
Copy link
Contributor

Thanks.

It is not breaking locking compatibility between versions, so it can go in a minor release.

@willemstuursma willemstuursma merged commit 8b254ed into php-lock:master Jan 11, 2021
@mvorisek mvorisek deleted the better_lock_random branch January 11, 2021 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants