Skip to content

Add secure_random functions#17399

Merged
rschlussel merged 1 commit intoprestodb:masterfrom
darrenfu:secure_random_functions
Mar 7, 2022
Merged

Add secure_random functions#17399
rschlussel merged 1 commit intoprestodb:masterfrom
darrenfu:secure_random_functions

Conversation

@darrenfu
Copy link
Copy Markdown
Contributor

@darrenfu darrenfu commented Mar 4, 2022

Currently all presto random() functions return a pseudo-random number.

Implement secure_random() functions returning a cryptographically secure random number between lower (inclusive) and upper (exclusive).

Test plan - unit tests and verifier run

Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.

== RELEASE NOTES ==

General Changes
* Add secure_random() and secure_random(lower, upper) with uniform distribution.

@darrenfu darrenfu force-pushed the secure_random_functions branch from 05ee558 to b8c6132 Compare March 4, 2022 07:46
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Mar 4, 2022

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ Darren Fu (b43ea70a2879f0e225d39d28cfdf7a2546a2a359, bcb267349ca60d5cf5b39a296735a55988a4046c, 01aacb333440e8ae2a84fa4a28c307df9b36aede, 7aa41abd39c475b4b4e7a0f6277a2a66d137e6c4)

@darrenfu darrenfu force-pushed the secure_random_functions branch from b8c6132 to 7aa41ab Compare March 4, 2022 07:48
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want to catch a noSuchAlgorithmException because it means we would basically never know if NativePRNGNonBlocking wasn't being used. It would be better to throw an internal error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing my PR!
Actually NativePRNGNonBlocking is an OS specific secure random implementation. See this post.
I will extract the os-specific secure random algorithm as a static variable then.

For exception handling, I can definitely wrap NoSuchAlgorithmException into a PrestoException and throw it.

@rschlussel
Copy link
Copy Markdown
Contributor

Also, we avoid merge commits. Can you rebase your change on master instead?

@darrenfu darrenfu force-pushed the secure_random_functions branch 2 times, most recently from f7c19a2 to ce87a31 Compare March 5, 2022 00:34
@darrenfu darrenfu force-pushed the secure_random_functions branch from ce87a31 to 8ed093e Compare March 5, 2022 01:01
@darrenfu darrenfu requested a review from rschlussel March 5, 2022 04:42
@rschlussel rschlussel merged commit 3020df1 into prestodb:master Mar 7, 2022
@varungajjala varungajjala mentioned this pull request Mar 22, 2022
9 tasks
@asjadsyed asjadsyed mentioned this pull request Mar 23, 2022
9 tasks
@asjadsyed asjadsyed mentioned this pull request Apr 1, 2022
8 tasks
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