Skip to content

GODRIVER-2349 Seed all pseudorandom number generators with a crypto-secure random number. #889

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 4 commits into from
Mar 30, 2022

Conversation

matthewdale
Copy link
Collaborator

@matthewdale matthewdale commented Mar 25, 2022

GODRIVER-2349

On some versions of Go and on some operating systems, time.Now() returns a time with lower-than-expected resolution (updates every 500μs to 15ms). The Go driver uses time.Now().UnixNano() to seed some pseudo-random number generators, including the one for generating session IDs here. Due to that, it's possible, under the right conditions, to start two processes that reproduce the same sequence of session IDs if they are started at almost the same time.

To remove that possibility, read an int64 value from the "crypto/rand".Reader and use it to seed all pseudo-random number generators in the driver.

Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Thanks!

@matthewdale matthewdale force-pushed the godriver2349-use-crypto-seed branch 2 times, most recently from c6335ff to 250cfb7 Compare March 30, 2022 03:46
Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Just requesting more licensing.

The new test looks great; thank you. I only wonder if we should increase the number of goroutines? Did TestGlobalSource as is ever fail when we were seeding with time.Now()?

Copy link
Collaborator Author

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

@benjirewis Responding to your question: Yes, TestGlobalSource does fail reliably on my workstation with 1,000 goroutines using time.Now().UnixNano() as a seed value. However, the test is not deterministic because the timing depends on runtime conditions, which may be different on the Evergreen hosts. I can bump the number of goroutines to 10,000 to increase the probability that the test will expose any issues. Edit: Increasing the number of goroutines actually causes issues because of the issue described here, so I'll leave it at 1,000.

@matthewdale matthewdale requested a review from benjirewis March 30, 2022 17:38
@matthewdale matthewdale force-pushed the godriver2349-use-crypto-seed branch from 740fdd6 to a89b49a Compare March 30, 2022 18:24
Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

LGTM; thanks for filing GODRIVER-2361, too. This stuff is very interesting 🤔

@matthewdale matthewdale merged commit 0cd9fa6 into mongodb:master Mar 30, 2022
matthewdale added a commit that referenced this pull request Mar 31, 2022
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.

3 participants