-
Notifications
You must be signed in to change notification settings - Fork 53
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: add keyfile support for RLN credentials secure storage #1285
Changes from 10 commits
7abe2fd
a6bf2be
31a9787
1d79fbf
0ded30f
2a6464b
fc5f48f
7dc2469
eb744d3
7c2c1f9
589ee0d
517c3b1
5a47ec3
e74cc7e
168612e
08be67c
d814f17
4ff8a7f
01ebc54
9b6850e
6aa7805
edab924
75c6ff6
112de23
239113c
ffad08b
78f2151
def10d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -174,7 +174,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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this configuration option be within a The rest of the RLN conf options should also be under the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
{.used.} | ||
LNSD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
import | ||
std/[json, os], | ||
testutils/unittests, chronos, chronicles, | ||
eth/keys, | ||
../../waku/v2/utils/keyfile, | ||
../test_helpers | ||
LNSD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
from ../../waku/v2/protocol/waku_noise/noise_utils import randomSeqByte | ||
|
||
suite "KeyFile test suite": | ||
|
||
let rng = newRng() | ||
|
||
test "Create/Save/Load single keyfile": | ||
|
||
let password = "randompassword" | ||
let filepath = "./test.keyfile" | ||
|
||
var secret = randomSeqByte(rng[], 300) | ||
let keyfile = createKeyFileJson(secret, password) | ||
|
||
check: | ||
keyfile.isOk() | ||
saveKeyFile(filepath, keyfile.get()).isOk() | ||
|
||
var decodedSecret = loadKeyFile("test.keyfile", password) | ||
|
||
check: | ||
decodedSecret.isOk() | ||
secret == decodedSecret.get() | ||
|
||
removeFile(filepath) | ||
alrevuelta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
test "Create/Save/Load multiple keyfiles in same file": | ||
|
||
let password1 = "password1" | ||
let password2 = "password2" | ||
let password3 = "password3" | ||
let filepath = "./test.keyfile" | ||
var keyfile: KfResult[JsonNode] | ||
|
||
let secret1 = randomSeqByte(rng[], 300) | ||
keyfile = createKeyFileJson(secret1, password1) | ||
check: | ||
keyfile.isOk() | ||
saveKeyFile(filepath, keyfile.get()).isOk() | ||
|
||
let secret2 = randomSeqByte(rng[], 300) | ||
keyfile = createKeyFileJson(secret2, password2) | ||
check: | ||
keyfile.isOk() | ||
saveKeyFile(filepath, keyfile.get()).isOk() | ||
|
||
let secret3 = randomSeqByte(rng[], 300) | ||
keyfile = createKeyFileJson(secret3, password3) | ||
check: | ||
keyfile.isOk() | ||
saveKeyFile(filepath, keyfile.get()).isOk() | ||
|
||
# We encrypt secret4 with password3 | ||
let secret4 = randomSeqByte(rng[], 300) | ||
keyfile = createKeyFileJson(secret4, password3) | ||
check: | ||
keyfile.isOk() | ||
saveKeyFile(filepath, keyfile.get()).isOk() | ||
|
||
# We encrypt secret5 with password1 | ||
let secret5 = randomSeqByte(rng[], 300) | ||
keyfile = createKeyFileJson(secret5, password1) | ||
check: | ||
keyfile.isOk() | ||
saveKeyFile(filepath, keyfile.get()).isOk() | ||
|
||
# We encrypt secret5 with password1 | ||
let secret6 = randomSeqByte(rng[], 300) | ||
keyfile = createKeyFileJson(secret6, password1) | ||
check: | ||
keyfile.isOk() | ||
saveKeyFile(filepath, keyfile.get()).isOk() | ||
|
||
# Now there are 5 keyfiles stored in filepath encrypted with 3 different passwords | ||
# We decode with the respective passwords | ||
|
||
var decodedSecret1 = loadKeyFile("test.keyfile", password1) | ||
check: | ||
decodedSecret1.isOk() | ||
secret1 == decodedSecret1.get() | ||
|
||
var decodedSecret2 = loadKeyFile("test.keyfile", password2) | ||
check: | ||
decodedSecret2.isOk() | ||
secret2 == decodedSecret2.get() | ||
|
||
var decodedSecret3 = loadKeyFile("test.keyfile", password3) | ||
check: | ||
decodedSecret3.isOk() | ||
secret3 == decodedSecret3.get() | ||
|
||
# Since we have 2 keyfiles encrypted with same password3, to obtain the secret we skip the first successful decryption, i.e. the keyfile for secret3 | ||
var decodedSecret4 = loadKeyFile("test.keyfile", password3, skip = 1) | ||
check: | ||
decodedSecret4.isOk() | ||
secret4 == decodedSecret4.get() | ||
|
||
# Since we have 3 keyfiles encrypted with same password1, to obtain the secret we skip the first successful decryption, i.e. the keyfile for secret1 | ||
var decodedSecret5 = loadKeyFile("test.keyfile", password1, skip = 1) | ||
check: | ||
decodedSecret5.isOk() | ||
secret5 == decodedSecret5.get() | ||
|
||
# Since we have 3 keyfiles encrypted with same password1, to obtain the secret we skip the first 2 successful decryptions, i.e. the keyfile for secret1 and secret5 | ||
var decodedSecret6 = loadKeyFile("test.keyfile", password1, skip = 2) | ||
check: | ||
decodedSecret6.isOk() | ||
secret6 == decodedSecret6.get() | ||
|
||
removeFile(filepath) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ import | |
waku_rln_relay_constants, | ||
waku_rln_relay_types, | ||
waku_rln_relay_metrics, | ||
../../utils/time, | ||
../../utils/[time, keyfile], | ||
LNSD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
../../node/waku_node, | ||
../../../../../apps/wakunode2/config, ## TODO: Decouple the protocol code from the app configuration | ||
../../../../../apps/chat2/config_chat2, ## TODO: Decouple the protocol code from the app configuration | ||
|
@@ -38,6 +38,18 @@ contract(MembershipContract): | |
# proc withdraw(secret: Uint256, pubkeyIndex: Uint256, receiver: Address) | ||
# proc withdrawBatch( secrets: seq[Uint256], pubkeyIndex: seq[Uint256], receiver: seq[Address]) | ||
|
||
# Note: works only for non empty input | ||
proc toString(bytes: openArray[byte]): string = | ||
var output = newString(bytes.len) | ||
copyMem(output[0].addr, bytes[0].unsafeAddr, bytes.len) | ||
return output | ||
LNSD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Note: works only for non empty input | ||
proc toSeqByte(str: string): seq[byte] = | ||
var output = newSeq[byte](str.len) | ||
copyMem(output[0].addr, str[0].unsafeAddr, str.len) | ||
return output | ||
LNSD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
proc toBuffer*(x: openArray[byte]): Buffer = | ||
## converts the input to a Buffer object | ||
## the Buffer object is used to communicate data with the rln lib | ||
|
@@ -1145,19 +1157,31 @@ proc mountRlnRelayDynamic*(node: WakuNode, | |
node.wakuRlnRelay = rlnPeer | ||
return ok(true) | ||
|
||
proc readPersistentRlnCredentials*(path: string) : RlnMembershipCredentials {.raises: [Defect, OSError, IOError, Exception].} = | ||
info "Rln credentials exist in file" | ||
proc writeRlnCredentials*(path: string, credentials: RlnMembershipCredentials, password: string) : KfResult[void] {.raises: [Defect, OSError, IOError, Exception].} = | ||
LNSD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
info "Storing RLN credentials" | ||
var jsonString: string | ||
jsonString.toUgly(%credentials) | ||
let keyfile = createKeyFileJson(toSeqByte(jsonString), password) | ||
if keyfile.isErr(): | ||
return err(keyfile.error) | ||
return saveKeyFile(path, keyfile.get()) | ||
|
||
proc readRlnCredentials*(path: string, password: string) : Option[RlnMembershipCredentials] {.raises: [Defect, OSError, IOError, Exception].} = | ||
LNSD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
info "Reading RLN credentials" | ||
# With regards to printing the keys, it is purely for debugging purposes so that the user becomes explicitly aware of the current keys in use when nwaku is started. | ||
# Note that this is only until the RLN contract being used is the one deployed on Goerli testnet. | ||
# These prints need to omitted once RLN contract is deployed on Ethereum mainnet and using valuable funds for staking. | ||
waku_rln_membership_credentials_import_duration_seconds.nanosecondTime: | ||
let entireRlnCredentialsFile = readFile(path) | ||
|
||
let jsonObject = parseJson(entireRlnCredentialsFile) | ||
let deserializedRlnCredentials = to(jsonObject, RlnMembershipCredentials) | ||
|
||
debug "Deserialized Rln credentials", rlnCredentials=deserializedRlnCredentials | ||
return deserializedRlnCredentials | ||
let entireRlnCredentialsFile = loadKeyFile(path, password) | ||
if entireRlnCredentialsFile.isOk(): | ||
let jsonObject = parseJson(toString(entireRlnCredentialsFile.get())) | ||
let deserializedRlnCredentials = to(jsonObject, RlnMembershipCredentials) | ||
debug "Deserialized RLN credentials", rlnCredentials=deserializedRlnCredentials | ||
return some(deserializedRlnCredentials) | ||
else: | ||
debug "Unable to decrypt RLN credentials with provided password. ", error=entireRlnCredentialsFile.error | ||
echo "Unable to decrypt RLN credentials with provided password. ", entireRlnCredentialsFile.error | ||
LNSD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return none(RlnMembershipCredentials) | ||
|
||
proc mount(node: WakuNode, | ||
conf: WakuNodeConf|Chat2Conf, | ||
|
@@ -1220,9 +1244,10 @@ proc mount(node: WakuNode, | |
let rlnRelayCredPath = joinPath(conf.rlnRelayCredPath, RlnCredentialsFilename) | ||
debug "rln-relay credential path", rlnRelayCredPath | ||
# check if there is an rln-relay credential file in the supplied path | ||
if fileExists(rlnRelayCredPath): | ||
if fileExists(rlnRelayCredPath): | ||
info "A RLN credential file exists in provided path" | ||
LNSD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# retrieve rln-relay credential | ||
credentials = some(readPersistentRlnCredentials(rlnRelayCredPath)) | ||
credentials = readRlnCredentials(rlnRelayCredPath, conf.rlnRelayCredentialsPassword) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here you are accessing a Can you specify a parameter in this function named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I got the suggestion correctly: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The advantage is that you decouple the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand, but currently the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tracking issue #1294 |
||
|
||
else: # there is no credential file available in the supplied path | ||
# mount the rln-relay protocol leaving rln-relay credentials arguments unassigned | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tracking issue #1294 |
||
return err("error in storing rln credentials") | ||
|
||
else: | ||
# do not persist or use a persisted rln-relay credential | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking issue #1294
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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!
There was a problem hiding this comment.
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.
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.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)