Skip to content
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

feat: use SecureRandom instead of Random to reduce the chance of auto-id collisions #156

Merged
merged 2 commits into from Mar 27, 2020
Merged

Conversation

dconeybe
Copy link
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #155 ☕️

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 27, 2020
@codecov
Copy link

codecov bot commented Mar 27, 2020

Codecov Report

Merging #156 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #156   +/-   ##
=========================================
  Coverage     71.62%   71.62%           
  Complexity      969      969           
=========================================
  Files            62       62           
  Lines          5222     5222           
  Branches        579      579           
=========================================
  Hits           3740     3740           
  Misses         1302     1302           
  Partials        180      180           
Impacted Files Coverage Δ Complexity Δ
...java/com/google/cloud/firestore/FirestoreImpl.java 82.35% <100.00%> (ø) 24.00 <1.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ca0ea8...d8f74ff. Read the comment docs.

@dconeybe dconeybe changed the title Use SecureRandom instead of Random to reduce the chance of auto-id collisions feat: use SecureRandom instead of Random to reduce the chance of auto-id collisions Mar 27, 2020
@BenWhitehead
Copy link
Collaborator

Do we know if users have ran into any issues with this sort of switch due to SecureRandom blocking to return values until there is enough entropy?

Note: Depending on the implementation, the generateSeed and nextBytes methods may block as entropy is being gathered, for example, if they need to read from /dev/random on various Unix-like operating systems. [1]

[1] https://docs.oracle.com/javase/8/docs/api/java/security/SecureRandom.html

@dconeybe
Copy link
Contributor Author

@BenWhitehead I do not know if this would be an issue. I'll look into it though. I could see it potentially being an issue with the Android SDK (where the change to SecureRandom has already occurred), where blocking the main thread is strongly discouraged; however, for a server-side SDK I wouldn't anticipate it being an issue.

@BenWhitehead
Copy link
Collaborator

The main location I'd worry about it in the server side SDK would be in some of our "lean" environments, i.e. Cloud Functions, App Engine etc

@BenWhitehead
Copy link
Collaborator

I've sent a question to several folks on my team to see if they've ran into anything around entropy in those environments and will report back if I hear anything.

@dconeybe dconeybe self-assigned this Mar 27, 2020
@dconeybe
Copy link
Contributor Author

@BenWhitehead: @wilhuff pointed out that SecureRandom will not block in the way that it is used in this PR: https://tersesystems.com/blog/2015/12/17/the-right-way-to-use-securerandom/. At least, it won't block on Mac or Linux. In Windows, it's unclear if it will block or not. But do those "lean environments" you referred to include Windows machines?

@BenWhitehead
Copy link
Collaborator

I believe all the lean environments are linux/container based so /dev/urandom is probably a safe assumption, I'll go ahead an approve and merge this.

@BenWhitehead BenWhitehead merged commit 0088ee7 into googleapis:master Mar 27, 2020
@dconeybe dconeybe deleted the SecureRandom branch March 28, 2020 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use SecureRandom instead of Random to reduce the chance of auto-id collisions
3 participants