feat: add keyfile support for RLN credentials secure storage #1285
Conversation
s1fr0
added
enhancement
s1fr0
changed the title
Jenkins BuildsClick to see older builds (41)
|
…iple keyfiles + test
s1fr0
changed the title
| # retrieve rln-relay credential | ||
| credentials = some(readPersistentRlnCredentials(rlnRelayCredPath)) | ||
| credentials = readRlnCredentials(rlnRelayCredPath, conf.rlnRelayCredentialsPassword) |
There was a problem hiding this comment.
Here you are accessing a conf field. Using the conf object everywhere makes refactoring/decoupling work harder.
Can you specify a parameter in this function named rlnRelayCredentialsPassword and use it here?
There was a problem hiding this comment.
I'm not sure I got the suggestion correctly: conf is passed as parameter to the mount function which in turn calls readRlnCredentials. If I add rlnRelayCredentialsPassword to mount I'll get no advantage. I got it wrong?
There was a problem hiding this comment.
The advantage is that you decouple the mount() procedure from the WakuConf type (char2 or wakunode2 conf type), which will ease the task of decoupling the RLN module from the chat2 and wakunode2 configuration types.
There was a problem hiding this comment.
I understand, but currently the WakuConf is passed to mount(), and other conf options are used for other purposes. Unless you're thinking (in the long term) to move all these conf parameters used as parameters to mount?
If so does it make sense to do it in this PR or better a separate "decoupling" one?
| @@ -1256,8 +1281,8 @@ proc mount(node: WakuNode, | |||
| # persist rln credential | |||
| credentials = some(RlnMembershipCredentials(rlnIndex: node.wakuRlnRelay.membershipIndex, | |||
| membershipKeyPair: node.wakuRlnRelay.membershipKeyPair)) | |||
| writeFile(rlnRelayCredPath, pretty(%credentials.get())) | |||
|
|
|||
| if writeRlnCredentials(rlnRelayCredPath, credentials.get(), conf.rlnRelayCredentialsPassword).isErr(): | |||
There was a problem hiding this comment.
Same here:
Can you specify a parameter in this function named
rlnRelayCredentialsPasswordand use it here?
| rlnRelayCredentialsPassword* {. | ||
| desc: "Password for encrypting RLN credentials", | ||
| defaultValue: "" | ||
| name: "rln-relay-cred-password" }: string |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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)
| rlnRelayCredentialsPassword* {. | ||
| desc: "Password for encrypting RLN credentials", | ||
| defaultValue: "" | ||
| name: "rln-relay-cred-password" }: string |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
See my comment: #1285 (comment)
|
|
||
| # We generate 6 different secrets and we encrypt them using 3 different passwords, and we store the obtained keystore | ||
|
|
||
| let secret1 = randomSeqByte(rng[], 300) |
There was a problem hiding this comment.
Since there is a clear pattern here, perhaps just use a for loop?
for pass in @[pass1, passn]:
let secret = randomSeqByte(rng[], 300)
let keyfile = createKeyFileJson(secret, pass)
check:
keyfile.isOk()
saveKeyFile(filepath, keyfile.get()).isOk()you can even randomize the password for each iteration, or even include %,Aa-? and weird stuff just in case?
There was a problem hiding this comment.
Sure, but then later checks on number of encryption under same password, the decrypted values and so one will get slightly more complicated. I would have been more open to make it more general in case we needed it in nwaku or needed to test hundreds of cases, but this is just a test and I don't see it as a valuable effort for just a stylistic advantage (which is unclear given the second part of the test).
There was a problem hiding this comment.
lgtm! thank you for addressing the comments. I left this one open as a nice to have but not blocking.
| "r": 8, | ||
| "salt": "247797c7a357b707a3bdbfaa55f4c553756bca09fec20ddc938e7636d21e4a20", | ||
| }, | ||
| "mac": "5a3ba5bebfda2c384586eda5fcda9c8397d37c9b0cc347fea86525cf2ea3a468", |
There was a problem hiding this comment.
Can we have a test for decrypting a key file with a right password but with a tampered/wrong mac?
There was a problem hiding this comment.
Sure. Test added in 239113c
tests/v2/test_utils_keyfile.nim
Outdated
| expectedSecret = randomSeqByte(rng[], 300) | ||
| password = "miawmiawcat" | ||
|
|
||
| # By default keyfiles are created using PBKDF2. Here we test keyfiles created using scrypt |
There was a problem hiding this comment.
By keyfiles are created, you mean the encryption key is derived from a supplied password using PBKDF2?
There was a problem hiding this comment.
Yes. Clarified in 239113c
This PR adds support to keyfile defined according to Web3 Secret Storage Definition but extended in order to allow
The keyfile implementation is adapted from nim-eth for this purpose. In order to allow flexibility/compatibility with
go-wakuimplementation, the keyfile fieldsidandversionare omitted by default, but can be re-enabled by changing their respective flags.Since the main purpose of keyfile introduction is to encrypt RLN credentials, this PR further implements their storage through keyfiles (re)definining the procs
writeRlnCredentialsandreadRlnCredentials. Given that keyfiles process RLN credentials as arbitrary byte data, the format of RLN credentials can be changed anytime with minor modification to the code. Here a related comment.In order to set a password to encrypt/decrypt RLN credentials, it suffices to pass the
--rln-relay-cred-password=<PASSWORD>flag to the chat2 or wakunode2 instances. When this flag is not passed, the encryption/decryption password is set to "" (empty string).This PR addresses and solve with respect to
nwakuandgo-wakucross-clients credentials the issue #1278Note: keyfile.nim, by being a code adaptation coming from nim-eth doesn't seem to follow rigorously our nim guidelines, e.g. note use the
resultvariable. Let me know if it is worth to align it to guidelines (would require some effort though).