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

Expose PRNG functions #1023

Merged
merged 17 commits into from
Sep 16, 2023
Merged

Conversation

masonforest
Copy link
Contributor

@masonforest masonforest commented Jun 27, 2023

What

Expose prng_reseed, u64_in_inclusive_range and vec_shuffle in the SDK.

Why

Exposing the PRNG functions in the SDK allows users to utilize randomness provided by the host in their smart contracts.

Known limitations

N/A

Close #969


I added tests for sha256 and verify_sig_ed25519 while I was in here. Let me know if those changes should be moved to a separate PR. I considered adding a PRNG module separate from the crypto module. I decided to keep them under crypto because there are only three functions. Also this is also similar getRandomValues is a function implemented within the Web Crypto API in browsers.

Copy link
Contributor

@jayz22 jayz22 left a comment

Choose a reason for hiding this comment

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

Thanks for contributing @masonforest !
Just a couple of suggestions on env sameness check.
Overall it looks great and thanks for adding the additional tests.

soroban-sdk/src/crypto.rs Outdated Show resolved Hide resolved
soroban-sdk/src/crypto.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jayz22 jayz22 left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

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

Sorry I'm late to the game on this one. The code of this is I think probably fine (I don't see anything problematic) but it's essential that the PRNG functions not live in a module called crypto, and that they instead be very clearly marked and documented as not providing cryptographically strong randomness of the sort cryptographic applications need. The P in PRNG and the term pseudo should be prominent, ideally every function should retain prng_... as a prefix, and documentation should repeat in big blinking letters that this is non-cryptographic, non-random, pseudorandom generation based on strictly public information.

In other words, the analogy you're making to WebCrypto's getRandomValues function -- which provides access to an OS's cryptographically strong random number source -- is dangerously wrong and we should be extremely clear to users to ensure they never make that mistake -- if they do they will fatally undermine their code.

I am happy to write a block comment describing the potential contexts where these functions are appropriate, but they are very inappropriate for cryptographic uses.

Other Changes:

* Add tests for `sha256` and `verify_sig_ed25519`
These functions  do not provide cryptographically strong randomness of the sort cryptographic applications need. Therefore they should not be in the crypto module.
@masonforest
Copy link
Contributor Author

Thank you for the feedback @graydon and thank you for all you've done for the open source community.

it's essential that the PRNG functions not live in a module called crypto

I moved these functions to a new module called Prng which is more appropriate and added the prng prefix to all of the functions.

I am happy to write a block comment describing the potential contexts where these functions are appropriate, but they are very inappropriate for cryptographic uses.

Could you post that comment here? Or feel free to add it yourself before merge. If there is somewhere this is documented which I can synthesize into a block comment I'd be happy to do that as well. Otherwise, I think I've done all I'm able to do here.

soroban-sdk/src/prng.rs Outdated Show resolved Hide resolved
soroban-sdk/src/prng.rs Outdated Show resolved Hide resolved
soroban-sdk/src/prng.rs Outdated Show resolved Hide resolved
soroban-sdk/src/prng.rs Outdated Show resolved Hide resolved
soroban-sdk/src/prng.rs Outdated Show resolved Hide resolved
soroban-sdk/src/lib.rs Show resolved Hide resolved
@leighmcculloch leighmcculloch self-assigned this Sep 15, 2023
@leighmcculloch
Copy link
Member

Thanks @masonforest for leading the charge on this. I'm going to make the changes above, and some other additions, and see if we can slide this into the next release.

@leighmcculloch leighmcculloch mentioned this pull request Sep 15, 2023
github-merge-queue bot pushed a commit that referenced this pull request Sep 15, 2023
### What
Add Vec::to_vals function that converts a Vec<T> to a Vec<Val>.

### Why
It's reasonably common, at least in tests and internally, that we need
to construct Vec's of Val's. There are different ways to do this by
constructing a Vec of Val::from calls, to using tuples with always
convert to Vec<Val>. We should have a function to help us do it.

I hope to use the function in #1023.
Copy link
Contributor

@dmkozh dmkozh left a comment

Choose a reason for hiding this comment

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

A general comment on the 'not secure' comments: while I understand what these are trying to convey, I think these are somewhat misleading and might lead to users using even worse approaches to randomness (e.g. seeding with ledger seq/timestamp as they do now).

I think what the comments mostly imply is that one shouldn't try to e.g. generate private keys with this. However, the main factor for that is the observability/reproducibility, which makes such usages impossible, no matter how good initial entropy is.

So I think it's worth to at least state that while this is imperfect, is is as good for randomness, as it can get given the restrictions of running in blockchain.

soroban-sdk/src/env.rs Show resolved Hide resolved
@leighmcculloch
Copy link
Member

@graydon I'd appreciate your review of this, but I'm going to merge this now. Could circle back on it before we cut the next release?

@leighmcculloch leighmcculloch dismissed graydon’s stale review September 15, 2023 23:08

Dismissing @graydon's comments because I think we've addressed the concern of signalling loud and clear that this is not a secure RNG, but a pseudo RNG.

@leighmcculloch leighmcculloch added this pull request to the merge queue Sep 15, 2023
@leighmcculloch leighmcculloch removed this pull request from the merge queue due to a manual request Sep 15, 2023
@leighmcculloch leighmcculloch added this pull request to the merge queue Sep 16, 2023
Merged via the queue into stellar:main with commit 4efef11 Sep 16, 2023
14 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Sep 18, 2023
### What

Expose `secp256k1` and `keccak256` in the SDK.

### Why

`secp256k1` is a widely used cryptographic signature algorithm and
`keccak256` is a widely used hashing algorithm. Exposing these functions
will allow smart contract developers to utilize these algorithms in
their applications without having to implement them themselves.

### Follow on work

* ~Pending [this
discussion](#1023 (comment))
on best practices I will call `check_env` where applicable.~


### Known limitations

N/A
------
**Github Issue**: [surface new crypto functions in
SDK](#970)

---------

Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
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.

surface PRNG functionality in SDK
5 participants