Skip to content

[Messenger][Notifier] use more entropy with random_bytes() #57872

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 12, 2024
Merged

[Messenger][Notifier] use more entropy with random_bytes() #57872

merged 1 commit into from
Aug 12, 2024

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Jul 29, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? no
Deprecations? no
Issues #57856 (comment)
License MIT

@xabbuh xabbuh requested a review from OskarStark as a code owner July 29, 2024 15:19
@carsonbot carsonbot added this to the 7.2 milestone Jul 29, 2024
@carsonbot carsonbot changed the title [Messenger][Notifier] switch to UUIDv7 for generating message identifiers [Messenger][Notifier]  switch to UUIDv7 for generating message identifiers Jul 29, 2024
@xabbuh
Copy link
Member Author

xabbuh commented Jul 29, 2024

based on the PR feedback I chose to pick Alexander's suggestion from #57588

@xabbuh xabbuh mentioned this pull request Jul 29, 2024
Merged
derrabus
derrabus approved these changes Jul 29, 2024
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, switching to UUIDs for truly collision-free identifiers is a good call. Thank you for this PR.

@xabbuh xabbuh force-pushed the pr-57856 branch 2 times, most recently from ab424d5 to 2eadda7 Compare July 30, 2024 05:54
@OskarStark OskarStark changed the title [Messenger][Notifier]  switch to UUIDv7 for generating message identifiers [Messenger][Notifier] switch to UUIDv7 for generating message identifiers Jul 30, 2024
@stof
Copy link
Member

stof commented Jul 30, 2024

is it actually worth introducing a dependency to generate this (mostly internal) message identifier ? To me, using bin2hex(random_bytes(...)) seems fine (still worth increasing the entropy by using 16 bytes instead of 4, matching the size of a UUID)

@derrabus
Copy link
Member

We need random and collision-free identifiers, so we're pulling a dependency that does exactly that. This sounds absolutely reasonable to me.

@stof
Copy link
Member

stof commented Jul 30, 2024

bin2hex(random_bytes(16)) provides random identifiers without any dependency (we already depend on PHP), so I'm wondering whether we need that dependency.
A UUIDv7 provides additional features (time-sorting, standard UUID format), but I don't think those places need those additional benefits.

nicolas-grekas
nicolas-grekas reviewed Aug 7, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with @stof, no need for the dep to dedup messages.
But I'd go with less entropy and a shorter hash (using 9 to generate tn trailing = with base64):
base64_encode(random_bytes(9))

@xabbuh xabbuh changed the title [Messenger][Notifier] switch to UUIDv7 for generating message identifiers [Messenger][Notifier] use more entropy with random_bytes() Aug 7, 2024
@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit c9056d0 into symfony:7.2 Aug 12, 2024
9 of 10 checks passed
@xabbuh xabbuh deleted the pr-57856 branch August 12, 2024 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@derrabus derrabus derrabus approved these changes

@chalasr chalasr chalasr approved these changes

@OskarStark OskarStark Awaiting requested review from OskarStark OskarStark is a code owner

Assignees
No one assigned
Labels
Messenger Notifier Status: Reviewed
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.
6 participants
@xabbuh @stof @derrabus @nicolas-grekas @chalasr @carsonbot