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.

Sign up for GitHub

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: add keyfile support for RLN credentials secure storage #1285

Merged
merged 28 commits into from
Oct 28, 2022
Merged

feat: add keyfile support for RLN credentials secure storage #1285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
7abe2fd
feat(utils): add keyfile + test
s1fr0 Oct 20, 2022
a6bf2be
feat(keyfile): make id and version field optional
s1fr0 Oct 21, 2022
31a9787
feat(rln): enable input password for keyfile encryption
s1fr0 Oct 22, 2022
1d79fbf
refactor(keyfile): store multiple keyfiles in single file
s1fr0 Oct 22, 2022
0ded30f
refactor(rln): write and read rln credentials proc using keyfiles + t…
s1fr0 Oct 22, 2022
2a6464b
feat(keyfile): allow skip successful decryption to iterate among mult…
s1fr0 Oct 22, 2022
fc5f48f
Merge branch 'master' into rln-keystore
s1fr0 Oct 22, 2022
7dc2469
fix(rln): add rln-relay-cred-password in wakunode2 config
s1fr0 Oct 22, 2022
eb744d3
Merge branch 'rln-keystore' of https://github.com/status-im/nwaku int…
s1fr0 Oct 22, 2022
7c2c1f9
chore(keyfile): remove unnencessary echo
s1fr0 Oct 22, 2022
589ee0d
Merge branch 'master' into rln-keystore
s1fr0 Oct 24, 2022
517c3b1
refactor(rln/kesyore): address reviewers' comments
s1fr0 Oct 24, 2022
5a47ec3
refactor(rln/keyfile): use defer instead of try/finally
s1fr0 Oct 25, 2022
e74cc7e
Merge branch 'master' into rln-keystore
s1fr0 Oct 25, 2022
168612e
feat(rln): address reviewers comments; return all successful decryptions
s1fr0 Oct 26, 2022
08be67c
chore(keyfile): add comments; changed name to loadKeyfiles
s1fr0 Oct 26, 2022
d814f17
feat(keyfile): chmod 600 for keyfiles
s1fr0 Oct 26, 2022
4ff8a7f
Merge branch 'master' into rln-keystore
s1fr0 Oct 26, 2022
01ebc54
fix(rln): fix errors raised by readRlnCredentials
s1fr0 Oct 26, 2022
9b6850e
feat(keyfile): add nim-eth tests; address reviewers comments
s1fr0 Oct 27, 2022
6aa7805
fix(keyfile/rln): better error handling
s1fr0 Oct 27, 2022
edab924
refactor(keyfile): refactoring nim-eth tests
s1fr0 Oct 27, 2022
75c6ff6
fix(keyfile): fix wrong index
s1fr0 Oct 27, 2022
112de23
fix(keyfile): fix wrong password generation in test
s1fr0 Oct 27, 2022
239113c
feat(keyfile): add wrong mac test
s1fr0 Oct 27, 2022
ffad08b
Merge branch 'master' into rln-keystore
s1fr0 Oct 27, 2022
78f2151
Merge branch 'master' into rln-keystore
s1fr0 Oct 27, 2022
def10d0
Merge branch 'master' into rln-keystore
rymnc Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions apps/chat2/config_chat2.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,11 @@ type
desc: "Address of membership contract on an Ethereum testnet",
defaultValue: ""
name: "rln-relay-eth-contract-address" }: string

rlnRelayCredentialsPassword* {.
desc: "Password for encrypting RLN credentials",
defaultValue: ""
name: "rln-relay-cred-password" }: string
Comment on lines +294 to +297
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this configuration option be within a when defined(rln): compilation guard?

The rest of the RLN conf options should also be under the when compilation guard; if this is allowed by Nim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible, but the full Chat2Conf/WakuNodeConf object has to be redefined under/outside the flag, i.e. we duplicate all the remaining field definition. I agree that it should be the case, i.e. not have fields unless supported, but nim seems a bit unhandy for these tasks. No problem for me, but maybe not the right PR. Wdyt?

Copy link
Contributor

@LNSD LNSD Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm ok moving this to another PR. Not a blocker on this PR, but certainly, in the near future, we should put a when guard to RLN-specific configuration options (the same way we have it in the implementation).

cc @rymnc @staheri14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking issue #1294

Copy link
Contributor

Choose a reason for hiding this comment

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

One more relevant issue about the visibility of config options is #999 where it was suggested to use sub-commands to group all the rln-relay-related configs and make them available only when the rln-relay config option is set to true. Would that address your comment @LNSD ?

Copy link
Contributor

Choose a reason for hiding this comment

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

but nim seems a bit unhandy for these tasks.

Now I see why you said that. It is an issue in nim-confutils macros 😫 Then, let's leave it as it is until we have a better way of doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I thought was just not possible in nim (i.e. have object fields defined at compilation-time), but indeed with some macros it might be possible to achieve the same goal.

I agree that this has to be done in followup PRs! Thanks for pointing it out!

Copy link
Contributor

Choose a reason for hiding this comment

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

@LNSD I see your point, that makes sense.

On a side note, I don't understand how subcommands will help here. We are not using subcommands in the nwaku node app (aka wakunode2). And I don't see any use case for them in wakunode2, at least in the near future.

It was suggested in one of the PRs and my understanding was that it is part of the plan to use subcommands: #992 (comment) @jm-clius

Copy link
Contributor

Choose a reason for hiding this comment

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

Please let us know if the plan re subcommands has changed so that we deprioritize that issue i.e., #999 cc: @jm-clius @LNSD

Copy link
Contributor

@staheri14 staheri14 Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand how subcommands will help here.

@LNSD
I'd say all the rln-relay-related config options will get encapsulated (and only accessible) under the rln-relay command/subcommand, hence the CLI interface becomes less confusing for the users (I assume this is the desirable feature we are looking for)


# NOTE: Keys are different in nim-libp2p
proc parseCmdArg*(T: type crypto.PrivateKey, p: TaintedString): T =
Expand Down
7 changes: 6 additions & 1 deletion apps/wakunode2/config.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,12 @@ type
desc: "Address of membership contract on an Ethereum testnet",
defaultValue: ""
name: "rln-relay-eth-contract-address" }: string


rlnRelayCredentialsPassword* {.
desc: "Password for encrypting RLN credentials",
defaultValue: ""
name: "rln-relay-cred-password" }: string
Comment on lines +165 to +168
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this configuration option be within a when defined(rln): compilation guard?

The rest of the RLN conf options should also be under the when compilation guard; if this is allowed by Nim.

Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment: #1285 (comment)


staticnodes* {.
desc: "Peer multiaddr to directly connect with. Argument may be repeated."
name: "staticnode" }: seq[string]
Expand Down
4 changes: 3 additions & 1 deletion tests/all_tests_v2.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ import
./v2/test_enr_utils,
./v2/test_peer_exchange,
./v2/test_waku_noise,
./v2/test_waku_noise_sessions
./v2/test_waku_noise_sessions,
# Utils
./v2/test_utils_keyfile

when defined(rln) or defined(rlnzerokit):
import
Expand Down
Loading