Persisting rln credentials #1037

kgrgpg · 2022-07-20T12:13:17Z

This code implements the persistence of MembershipKeyPair and MembershipIndex to the RLN membership contract. This is done by storing raw unencrypted JSON text in the files keyPair.txt and rlnIndex.txt when the first registration takes place. At a later stage, these credentials need to be stored in an encrypted format. One of the ways to implement that encryption could be similar to how keystore is implemented in Nimbus. But for now this should serve the purpose of credentials persistence saving user funds.

kaiserd

🎉 on your first nwaku PR :).

What are the security implications of printing / persisting the membership key in the clear?

Also, I left some nit comments inline.

waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

kgrgpg · 2022-07-21T07:57:37Z

@kaiserd Since the files are stored as a raw text file, it is highly susceptible to theft. The files needs some encryption to resolve this.

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.

kaiserd · 2022-07-21T09:24:45Z

Thank you for the clarification :).
I'd add comments in the code explaining the reason for both

storing the raw key on disc
printing the raw key

kgrgpg · 2022-07-21T10:13:19Z

Security warning incorporated as comments within code base.

s1fr0

Congrats for your first nwaku PR! Overall good, I left some comments.

waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

.gitignore

waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

jm-clius

Thanks, @kgrgpg! Left some comments on Nim styling and abstracting the file-specific logic and magic constants - if this is to be short-lived until we have a proper keystore, the reading/writing and file types should not be in line with the main logic of the code but abstracted to modular procs.

waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

jm-clius · 2022-07-21T12:24:02Z

waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

@@ -818,7 +833,7 @@ proc mountRlnRelayDynamic*(node: WakuNode,
  node.wakuRlnRelay = rlnPeer


-proc mountRlnRelay*(node: WakuNode, conf: WakuNodeConf) {.raises: [Defect, ValueError, IOError, CatchableError].} =
+proc mountRlnRelay*(node: WakuNode, conf: WakuNodeConf) {.raises: [Defect, ValueError, IOError, CatchableError, Exception].} =


Where is the Exception raised from? Couldn't we find the exact type of Exception being raised and catch it here? Now we have a list of Defect, CatchableError and Exception which just means "everything". I know it's not always easy to catch specific exceptions from submodules. Also see: https://status-im.github.io/nim-style-guide/errors.exceptions.html

This has been opened as a new issue. See #1045

So far it seems like an OSError but raising it is still giving error that appropriate exception is not raised.

There are a few function calls in this proc that can potentially raise an exception, i.e., readFile, parseJson and
to(jsonObject, R andlnMembershipCredentials), have you inspected those to see what they raise?

Moving all discussions related to Exception handling to the new issue. Review this PR on persistence primarily. @staheri14 Your comment is linked in new issue. #1045 (comment)

jm-clius · 2022-07-21T12:35:18Z

waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

@@ -861,8 +876,32 @@ proc mountRlnRelay*(node: WakuNode, conf: WakuNodeConf) {.raises: [Defect, Value
      let memKeyPair = keyPair.toMembershipKeyPairs()[0]
      # mount the rln relay protocol in the on-chain/dynamic mode
      waitFor node.mountRlnRelayDynamic(memContractAddr = ethMemContractAddress, ethClientAddr = ethClientAddr, memKeyPair = some(memKeyPair), memIndex = some(rlnRelayIndex), ethAccAddr = ethAccountAddr, ethAccountPrivKey = ethAccountPrivKey, pubsubTopic = conf.rlnRelayPubsubTopic, contentTopic = conf.rlnRelayContentTopic)
+    elif fileExists("keyPair.txt") and fileExists("rlnIndex.txt"):


I may not understand the exact context, but if the idea is simply for this "write to cleartext file" to be a temporary stopgap until we have an integrated keystore, the logic should be abstracted to separate functions with no magic references to the .txt files in the main logic.

In other words, instead of having an elif fileExists(... here, it should be something like elif usePersistentKeys() where usePersistentKeys() are implemented elsewhere. And instead of having the logic to read the keys from the files here it should be abstracted to a readKeys() proc, etc.

Suggestion incorporated. Refer to 6236dd6

waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

status-im-auto · 2022-07-22T04:15:05Z

Jenkins Builds

Click to see older builds (6)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`dc61be0`	#1	2022-07-22 04:15:04	~27 min	`macos`	📦`bin`
❌	`dc61be0`	#1	2022-07-22 10:16:51	~32 min	`windows`	📄`log`
✔️	`dc61be0`	#1	2022-07-22 12:10:07	~19 min	`linux`	📦`bin`

✔️	`7162bfa`	#2	2022-07-24 16:13:34	~26 min	`macos`	📦`bin`
❌	`7162bfa`	#2	2022-07-24 22:11:03	~26 min	`windows`	📄`log`

❌	`135b72a`	#3	2022-07-31 04:08:04	~20 min	`macos`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`08ab7a1`	#4	2022-08-02 04:08:54	~21 min	`macos`	📦`bin`

✔️	`1ad6b4d`	#2	2022-08-28 15:59:08	~19 min	`linux`	📦`bin`

…index to files

Co-authored-by: Daniel Kaiser <[email protected]>

Co-authored-by: Hanno Cornelius <[email protected]>

Reading credentials from file abstracted away.

s1fr0 · 2022-07-28T22:20:40Z

@kgrgpg Note that tests fail and there exist some conflicts that has to be addressed before merging

/home/runner/work/nwaku/nwaku/waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim(840) readPersistentRlnCredentials
/home/runner/work/nwaku/nwaku/vendor/nimbus-build-system/vendor/Nim/lib/system/io.nim(693) readFile

    Unhandled exception: cannot open: testPath.txt [IOError]
  [FAILED] Read Persistent RLN credentials

Please ping reviewers when ready.

Abstrated writePersistentRlnCredentials usage causing error, with readPersistentRlnCredentials

kgrgpg · 2022-07-29T16:15:48Z

@s1fr0 Usage of writePersistentRlnCredentials in the test is resulting in an IOError while reading.
Even though usage of this write abstract function works correctly within waku_rln_relay_utils.nim.

When I reverted to a direct call to writeFile within the test, the reading becomes successful. Refer f10d873

Now I am perplexed, why this is the case. I read at https://nim-lang.org/docs/io.html#writeFile%2Cstring%2Cstring that writeFile closes the file after the operation is complete. I am wondering if this is failing for some reason when contained inside the abstracted function writePersistentRlnCredentials as readPersistentRlnCredentials gets called right away in the test. Do you have any clue with regards to this?

s1fr0

Do not push commits related to vendor (so revert this). Regularly run make update to update submodules.

This reverts commit 3f4dba1.

kgrgpg · 2022-07-29T16:33:27Z

Do not push commits related to vendor (so revert this). Regularly run make update to update submodules.

Thanks for this info. Make update resulted in new checkout for dnsclient. Concerned commit reverted.

..persiting credential

kgrgpg · 2022-08-01T00:46:35Z

The abstract proc writePersistentRlnCredentials is reverted in commit 08ab7a1. The reason for this is explained in #1037. Since this abstract proc is not necessary for the "persistence" implementation of the RLN credentials and the test associated with it are valid and pass without this abstraction, this is now opened as a new issue #1053 (comment)

waku/v2/protocol/waku_rln_relay/waku_rln_relay_types.nim

LNSD · 2022-08-02T09:47:38Z

waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

+  let jsonObject = parseJson(entireRlnCredentialsFile)
+  let deserializedRlnCredentials = to(jsonObject, RlnMembershipCredentials)
+
+  debug "Deserialized Rln credentials", rlnCredentials=deserializedRlnCredentials


I would prefer that we do not dump the content of the credentials through the stdout

Refer to #1037 (comment)

LNSD

LGTM ✅

Congrats on you first contribution 😁

LNSD · 2022-08-04T10:06:40Z

IMHO We need to do an overall code review for the RLN part before it becomes hard to maintain (e.g. @s1fr0 has already experienced issues with the Zerokit integration).

Right now I am fully focused on the Waku Store issue, but let's see if we can find a slot after the next nwaku's release to improve the code maintainability.

cc @staheri14 @kgrgpg @s1fr0

kgrgpg · 2022-08-04T11:50:01Z

@LNSD Thanks but technically this is my second PR :D
First one is the tutorial at #1031
And agree, always up to improve code readability and maintainability.

s1fr0

All comments where addressed except abstraction of writePersistentRlnCredentials and byte serialization/storage. These changes are tracked in #1053 and #1041 (encryption somehow implies byte serialization).

LNSD · 2022-08-05T10:37:38Z

@kgrgpg If the work in this PR is finished, could you rebase, solve the conflicts and merge the PR into master branch?

kgrgpg requested review from staheri14, oskarth, jm-clius, s1fr0 and LNSD July 20, 2022 12:13

kgrgpg mentioned this pull request Jul 20, 2022

Rln-Relay: Persisting rln credentials #1021

Closed

kgrgpg requested a review from kaiserd July 20, 2022 12:16

kaiserd reviewed Jul 20, 2022

View reviewed changes

s1fr0 suggested changes Jul 21, 2022

View reviewed changes

jm-clius reviewed Jul 21, 2022

View reviewed changes

kgrgpg force-pushed the Persisting-rln-credentials branch from dc61be0 to 5a15930 Compare July 22, 2022 21:03

kgrgpg and others added 12 commits July 23, 2022 00:47

Persisting Credentials implemented by writing json of keypair and rln…

881d593

…index to files

Removing testing files and ignores

b5533d2

Update waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

942da4e

Co-authored-by: Daniel Kaiser <[email protected]>

Update waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

f85da55

Co-authored-by: Daniel Kaiser <[email protected]>

Comments

68ee74d

Comments

15f4ee7

Security warning in comments

966eb5b

Redundant echos. Omitting unused variables.

0e5c725

Update waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim

81bb5c1

Co-authored-by: Hanno Cornelius <[email protected]>

Limit line lengths using line breaks and indents

a004b52

Membership file paths declared as const

a90e16a

Const fix

b375418

kgrgpg force-pushed the Persisting-rln-credentials branch from 5a15930 to b375418 Compare July 22, 2022 22:47

Rln Credentials Merged.

6236dd6

Reading credentials from file abstracted away.

kgrgpg force-pushed the Persisting-rln-credentials branch from 39a30ec to 6236dd6 Compare July 24, 2022 11:20

Merge branch 'master' into Persisting-rln-credentials

bdf0296

kgrgpg added 4 commits July 29, 2022 17:14

Dnsclient?

3f4dba1

Test reverted to direct call to writeFile.

f10d873

Abstrated writePersistentRlnCredentials usage causing error, with readPersistentRlnCredentials

Merge branch 'master' into Persisting-rln-credentials

0b56889

Indentation

db5fddc

s1fr0 suggested changes Jul 29, 2022

View reviewed changes

Revert "Dnsclient?"

135b72a

This reverts commit 3f4dba1.

Reverting abstraction of writing..

08ab7a1

..persiting credential

kgrgpg mentioned this pull request Aug 1, 2022

Abstraction of writing persistent RLN credentials to a modular proc #1053

Closed

kgrgpg requested a review from s1fr0 August 1, 2022 00:47

LNSD reviewed Aug 2, 2022

View reviewed changes

kgrgpg requested a review from LNSD August 2, 2022 22:25

LNSD approved these changes Aug 4, 2022

View reviewed changes

s1fr0 approved these changes Aug 4, 2022

View reviewed changes

kgrgpg added 2 commits August 5, 2022 00:57

Merge branch 'master' into Persisting-rln-credentials

a9d591f

Merge branch 'master' into Persisting-rln-credentials

1ad6b4d

oskarth removed request for oskarth, kaiserd, jm-clius and staheri14 August 5, 2022 06:00

oskarth mentioned this pull request Aug 5, 2022

Milestone: RLN-Relay testnet1 - dynamic group and on-chain vacp2p/research#126

Closed

7 tasks

kgrgpg merged commit 115982f into master Aug 5, 2022

kgrgpg deleted the Persisting-rln-credentials branch August 5, 2022 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persisting rln credentials #1037

Persisting rln credentials #1037

kgrgpg commented Jul 20, 2022

kaiserd left a comment

kgrgpg commented Jul 21, 2022

kaiserd commented Jul 21, 2022 •

edited

Loading

kgrgpg commented Jul 21, 2022

s1fr0 left a comment

jm-clius left a comment

jm-clius Jul 21, 2022

kgrgpg Jul 26, 2022

staheri14 Jul 27, 2022

kgrgpg Jul 28, 2022

jm-clius Jul 21, 2022

kgrgpg Jul 24, 2022 •

edited

Loading

status-im-auto commented Jul 22, 2022 •

edited

Loading

s1fr0 commented Jul 28, 2022

kgrgpg commented Jul 29, 2022 •

edited

Loading

s1fr0 left a comment

kgrgpg commented Jul 29, 2022

kgrgpg commented Aug 1, 2022

LNSD Aug 2, 2022

kgrgpg Aug 2, 2022

LNSD left a comment

LNSD commented Aug 4, 2022

kgrgpg commented Aug 4, 2022

s1fr0 left a comment

LNSD commented Aug 5, 2022

Persisting rln credentials #1037

Persisting rln credentials #1037

Conversation

kgrgpg commented Jul 20, 2022

kaiserd left a comment

Choose a reason for hiding this comment

kgrgpg commented Jul 21, 2022

kaiserd commented Jul 21, 2022 • edited Loading

kgrgpg commented Jul 21, 2022

s1fr0 left a comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

jm-clius Jul 21, 2022

Choose a reason for hiding this comment

kgrgpg Jul 26, 2022

Choose a reason for hiding this comment

staheri14 Jul 27, 2022

Choose a reason for hiding this comment

kgrgpg Jul 28, 2022

Choose a reason for hiding this comment

jm-clius Jul 21, 2022

Choose a reason for hiding this comment

kgrgpg Jul 24, 2022 • edited Loading

Choose a reason for hiding this comment

status-im-auto commented Jul 22, 2022 • edited Loading

Jenkins Builds

s1fr0 commented Jul 28, 2022

kgrgpg commented Jul 29, 2022 • edited Loading

s1fr0 left a comment

Choose a reason for hiding this comment

kgrgpg commented Jul 29, 2022

kgrgpg commented Aug 1, 2022

LNSD Aug 2, 2022

Choose a reason for hiding this comment

kgrgpg Aug 2, 2022

Choose a reason for hiding this comment

LNSD left a comment

Choose a reason for hiding this comment

LNSD commented Aug 4, 2022

kgrgpg commented Aug 4, 2022

s1fr0 left a comment

Choose a reason for hiding this comment

LNSD commented Aug 5, 2022

kaiserd commented Jul 21, 2022 •

edited

Loading

kgrgpg Jul 24, 2022 •

edited

Loading

status-im-auto commented Jul 22, 2022 •

edited

Loading

kgrgpg commented Jul 29, 2022 •

edited

Loading