-
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(noise): add Noise Handshake processing and after-handshake encryption #934
Conversation
Jenkins BuildsClick to see older builds (8)
|
@staheri14 @jm-clius @kaiserd ready for review! Thanks! |
|
||
################################# | ||
## Utilities |
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.
This is a very long module. Any way this can be split up? noise_utils.nim, noise_types.nim, noise.nim could be a start, although the latter could perhaps be modularised even more between e.g. noise_state_machine.nim and noise_handshakes.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.
@jm-clius @staheri14 After internal discussion, we decided to not proceed (at the moment) with the split of the module in smaller submodules. Reasons for this include:
- the necessity to make the majority of object private fields public;
- most of already merged code will appear in the PR diff, thus making identifying and reviewing new parts difficult.
After all Noise features are implemented and merged, we can proceed with splitting the module in smaller submodules. By that time we might support already import foo {.all.} directive, that will ease such refactoring.
WIP PR for splitting the current Noise module in submodules: #979
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.
LGTM :) Really good work.
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.
Good job! I have added some comments mostly covering what we have discussed in our 1:1 (just as a placeholder).
@@ -321,7 +320,7 @@ procSuite "Waku Noise": | |||
######################################## | |||
|
|||
# We generate random input key material and we execute mixKey | |||
var inputKeyMaterial = randomChaChaPolyKey(rng[]) | |||
var inputKeyMaterial = randomSeqByte(rng[], rand(1..128)) |
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.
Why this change? what has been the issue with the randomChaChaPolyKey
proc?
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.
No, this was a bug coming from libp2p noise implementation, where it is still present. Input key material can be arbitrary input and not a only a ChaChaPolyKey.
Also, the macOS CI checks are failing. @s1fr0 |
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.
LGTM!
This PR introduces data structures and procedures required to execute Noise Handshakes between Waku users according to Noise specification.
Specifically, it adds:
processPreMessagePatternTokens
,processMessagePatternPayload
,processMessagePatternTokens
). To add support to a new handshake, it suffices to add its definition inNoiseHandshakePatterns
and add for it a new protocol ID inPayloadV2ProtocolIDs
;stepHandshake
): given current user's state, processes the next handshake message.finalizeHandshake
);writeMessage
,readMessage
);This PR extends #933 (which in turn extends #932, #931 and #930) and completes all the non-optional points of #881.