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(rln): add seeded keygen #56

Merged
merged 4 commits into from
Sep 30, 2022
Merged

feat(rln): add seeded keygen #56

merged 4 commits into from
Sep 30, 2022

Conversation

s1fr0
Copy link
Contributor

@s1fr0 s1fr0 commented Sep 28, 2022

Addresses #49 by implementing in the rln module a seeded_keygen function to generate an (id_secret, id commitment) pair from a Chacha20 RNG seeded by the user.

Such API is implemented in both public and ffi crates. Tests are implemented as well.

The choice of using Chacha20 is to minimize the risk of obtaining different results among different platform and dependencies' versions.

ChaCha20 requires a seed of exactly 32 bytes: for this reason the (arbitrary long) input seed provided by the user is hashed using Keccak256 before being passed to ChaCha20 as seed. This enables to use seed phrases as well in order to generate identity pairs. Note however, that Keccak has very efficient hardware implementations and might not be the right choice to protect short seed phrases that would be then become vulnerable to brute-force attacks.

Note: since a new API is added, to access it in nwaku we need to regenerate the RLN nim bindings. This will be done in a nwaku PR once this is merged.

@s1fr0 s1fr0 added track:zerokit Zerokit track (Applied ZK/Explorations) track:rln RLN Track - (Secure Messaging/Applied ZK), relay and applications labels Sep 28, 2022
@s1fr0 s1fr0 self-assigned this Sep 28, 2022
@s1fr0 s1fr0 linked an issue Sep 28, 2022 that may be closed by this pull request
@@ -33,6 +33,7 @@ num-bigint = { version = "0.4.3", default-features = false, features = ["rand"]
num-traits = "0.2.11"
once_cell = "1.14.0"
rand = "0.8"
rand_chacha = "0.3.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am worried about the new dependency here. How does it affect the wasm blob size? I intended to add some size checking as part of the CI but it was not as straightforward as I hoped because cargo make is used instead of npm.

Cc @richard-ramos

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this also depends on how much this library delegates to the web crypto subtle API

Copy link
Member

Choose a reason for hiding this comment

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

Right now building this branch, i end up with 530KB which is way better than the 838KB of the version published https://unpkg.com/@waku/[email protected]/bundle/assets/rln_wasm_bg-0185b546.wasm

-rw-rw-r-- 1 richard richard  17K sep 24 12:15 rln_wasm_bg.js
-rw-rw-r-- 1 richard richard 530K sep 30 08:42 rln_wasm_bg.wasm
-rw-rw-r-- 1 richard richard 1.1K sep 30 08:42 rln_wasm_bg.wasm.d.ts
-rw-rw-r-- 1 richard richard 3.2K sep 30 08:42 rln_wasm.d.ts
-rw-rw-r-- 1 richard richard  20K sep 30 08:42 rln_wasm.js

Copy link
Contributor

Choose a reason for hiding this comment

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

What changed ???

Copy link
Contributor Author

@s1fr0 s1fr0 Sep 30, 2022

Choose a reason for hiding this comment

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

Probably the poseidon constants that now are generated on the fly while before were hardcoded (~300KB less)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fryorcraken if ok for you, I'll merge!

// Generates a tupe (identity_secret, id_commitment) where
// identity_secret is random and id_commitment = PoseidonHash(identity_secret)
// RNG is instantiated using 20 rounds of ChaCha seeded with the hash of the input
pub fn seeded_keygen(signal: &[u8]) -> (Fr, Fr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function can accept a 1 byte signal, which I assume, is not secure.

I'd suggest to fix the signal size (and I am guessing that it should be 32 bytes).

Copy link
Contributor Author

@s1fr0 s1fr0 Sep 30, 2022

Choose a reason for hiding this comment

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

of course it won't, although any input would be hashed to become a 32 bytes seed. If we allow users to choose the seed, we're delegating the entropy level and credential security. How does differ passing one byte from passing e.g. an all-0 32 bytes array? They're both unsecure regardless of the input size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: can also accept empty signals :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point.

@s1fr0
Copy link
Contributor Author

s1fr0 commented Sep 30, 2022

I'll merge so that I can update zerokit module in nwaku and update bindings. @fryorcraken in case you see further problems, we'll fast track them in new PRs. Thanks!

@fryorcraken
Copy link
Contributor

Moved the discussion to waku-org/nwaku#1278 on whether we should use that for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
track:rln RLN Track - (Secure Messaging/Applied ZK), relay and applications track:zerokit Zerokit track (Applied ZK/Explorations)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable library consumer to pass their entropy source
3 participants