Skip to content

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.

3 participants