protocols/noise: Add `NoiseConfig::with_prologue` #2903

thomaseizinger · 2022-09-14T04:28:29Z

Description

Allows users to set the noise prologue of the handshake.

Links to any relevant issues

Open Questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger · 2022-09-14T04:29:55Z

That was surprisingly simple :)

melekes

👍

thomaseizinger · 2022-09-14T04:35:07Z

transports/noise/src/lib.rs

+    /// Set the noise prologue.
+    ///
+    /// The prologue can contain arbitrary data and will be hashed into the noise handshake.
+    /// For the handshake to succeed, both parties must set the same prologue.
+    pub fn with_prologue(&mut self, prologue: Vec<u8>) -> &mut Self {
+        self.prologue = prologue;
+        self
+    }


I made this a with_ function instead of exposing it in every constructor because it is often not needed and this way, we can keep the entire prologue functionality hidden from users that just want to use regular noise for their connections.

Having said that, if we agree to merge #2887, then most users don't need to touch NoiseConfig anyway so perhaps it should just be a regular constructor parameter?

mxinden

Thanks for looking into this!

I don't have an opinion on the concrete API.

Can you include a unit test in this pull request? Especially since this is very security critical for the WebRTC protocol.

transports/noise/src/lib.rs

mxinden · 2022-09-15T21:58:03Z

transports/noise/src/lib.rs

@@ -185,6 +199,7 @@ where
        let session = self
            .params
            .into_builder()
+            .prologue(self.prologue.as_ref())


Do I understand correctly that setting no prologue is equal to setting an empty prologue?

Yes, that is my understanding. The prologue is hashed into the protocol. Appending an empty slice vs appending nothing is not going to change the final hash.

thomaseizinger · 2022-09-16T00:50:04Z

Can you include a unit test in this pull request? Especially since this is very security critical for the WebRTC protocol.

What would you like the test to do? I am not against it but perhaps it would be better to have a test on the WebRTC level where we check that the handshake fails if we provide different certificates. That is the functionality we want. Doing that through the noise prologue is an implementation detail so it can of feels like we'd have a test there anyway, making this one redundant?

thomaseizinger · 2022-09-16T05:18:30Z

Can you include a unit test in this pull request? Especially since this is very security critical for the WebRTC protocol.

I've tried to create an abstraction in-between that tests that we are passing the prologue on correctly. While doing that, I noticed some code duplication between the various protocols. That got me thinking on why we even support so many handshake types when the specification only mentions a particular one.

What do you think of removing the others?

thomaseizinger · 2022-09-16T05:31:52Z

transports/noise/src/lib.rs

-            .build_responder()
-            .map_err(NoiseError::from);
+        let config = self.legacy;
+        let identity = self.dh_keys.clone().into_identity();


I had to clone the keys here to make the borrow-checker happy. Not ideal but it removes a bit of duplication. I am taking suggestions on how to do it better :)

How about implementing into_responder on ProtocolParams and not on NoiseConfig?

We'd have to pass the prologue and keys as an argument then. I guess we can do that :)

Unfortunately, that doesn't work well with the tests I've written. Passing the prologue in breaks the abstraction layer and the tests will no longer reflect the same usage as the code.

If we decide to merge #2909, more optimizations might be possible!

thomaseizinger · 2022-09-16T05:41:25Z

Can you include a unit test in this pull request? Especially since this is very security critical for the WebRTC protocol.

I've tried to create an abstraction in-between that tests that we are passing the prologue on correctly. While doing that, I noticed some code duplication between the various protocols. That got me thinking on why we even support so many handshake types when the specification only mentions a particular one.

What do you think of removing the others?

I am getting the impression that the abstractions within libp2p-noise are not quite ideal. For example, we have this entire layer of handshake::* functions that are all only used once (apart from rt1_initiator which is used twice) and they all almost correspond 1-to-1 to implementations of {In,Out}boundUpgrade on NoiseConfig.

I think it would make sense to simplify this, even if it means there will be a slight duplication between the IX and IK upgrades.

mxinden · 2022-09-16T13:57:33Z

That got me thinking on why we even support so many handshake types when the specification only mentions a particular one.

I think this dates back to the initial Noise specification days where it wasn't quite clear yet which handshake pattern to use. I don't have a strong opinion here, though an intuition to keep supporting them.

mxinden · 2022-09-16T14:09:42Z

Add unit tests for handshake hashes

🙏 test looks good to me.

thomaseizinger · 2022-09-19T04:52:22Z

@mxinden Do you think we can merge this as is (i.e. with the .clone()) or would like me to find a different way? If so, I'd advocate for merging #2887 first because the removed indirection will make it easier to optimize the data flow.

mxinden

I am fine merging here despite the clone. I doubt this would ever be a performance issue.

Thanks @thomaseizinger for the work. Sorry @melekes for the delay for #2622.

mxinden · 2022-09-21T11:46:11Z

Interoperability test failures are due to libp2p/test-plans#41. Merging here.

thomaseizinger added 2 commits September 14, 2022 14:24

Add NoiseConfig::with_prologue

7d528bb

Bump version in manifest and update changelog

1948568

thomaseizinger requested a review from mxinden September 14, 2022 04:28

thomaseizinger mentioned this pull request Sep 14, 2022

feat: Add WebRTC transport #2622

Merged

4 tasks

thomaseizinger linked an issue Sep 14, 2022 that may be closed by this pull request

transports/noise: Expose Prologue mechanism #2791

Closed

melekes approved these changes Sep 14, 2022

View reviewed changes

thomaseizinger commented Sep 14, 2022

View reviewed changes

mxinden reviewed Sep 15, 2022

View reviewed changes

thomaseizinger added 3 commits September 16, 2022 15:03

Update docs

8f519d4

Merge branch 'master' into 2791-noise-prologue

c56be9e

Update changelog with PR number

4526980

Add unit tests for handshake hashes

5b0c4cb

thomaseizinger commented Sep 16, 2022

View reviewed changes

thomaseizinger mentioned this pull request Sep 16, 2022

protocols/noise: Inline handshake functions into upgrade traits #2909

Merged

3 tasks

melekes approved these changes Sep 16, 2022

View reviewed changes

tomaka mentioned this pull request Sep 16, 2022

Set the Noise prologue in the WebRTC handshake paritytech/smoldot#2759

Closed

thomaseizinger requested a review from mxinden September 19, 2022 04:51

mxinden approved these changes Sep 21, 2022

View reviewed changes

Merge branch 'master' into 2791-noise-prologue

6422c46

mxinden merged commit ed1b899 into master Sep 21, 2022

thomaseizinger deleted the 2791-noise-prologue branch September 21, 2022 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protocols/noise: Add `NoiseConfig::with_prologue` #2903

protocols/noise: Add `NoiseConfig::with_prologue` #2903

thomaseizinger commented Sep 14, 2022

thomaseizinger commented Sep 14, 2022

melekes left a comment

thomaseizinger Sep 14, 2022

mxinden left a comment

mxinden Sep 15, 2022

thomaseizinger Sep 16, 2022

thomaseizinger commented Sep 16, 2022

thomaseizinger commented Sep 16, 2022

thomaseizinger Sep 16, 2022

mxinden Sep 16, 2022

thomaseizinger Sep 19, 2022

thomaseizinger Sep 19, 2022

thomaseizinger commented Sep 16, 2022

mxinden commented Sep 16, 2022

mxinden commented Sep 16, 2022 •

edited

Loading

thomaseizinger commented Sep 19, 2022

mxinden left a comment

mxinden commented Sep 21, 2022

protocols/noise: Add NoiseConfig::with_prologue #2903

protocols/noise: Add NoiseConfig::with_prologue #2903

Conversation

thomaseizinger commented Sep 14, 2022

Description

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger commented Sep 14, 2022

melekes left a comment

Choose a reason for hiding this comment

thomaseizinger Sep 14, 2022

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

mxinden Sep 15, 2022

Choose a reason for hiding this comment

thomaseizinger Sep 16, 2022

Choose a reason for hiding this comment

thomaseizinger commented Sep 16, 2022

thomaseizinger commented Sep 16, 2022

thomaseizinger Sep 16, 2022

Choose a reason for hiding this comment

mxinden Sep 16, 2022

Choose a reason for hiding this comment

thomaseizinger Sep 19, 2022

Choose a reason for hiding this comment

thomaseizinger Sep 19, 2022

Choose a reason for hiding this comment

thomaseizinger commented Sep 16, 2022

mxinden commented Sep 16, 2022

mxinden commented Sep 16, 2022 • edited Loading

thomaseizinger commented Sep 19, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Sep 21, 2022

protocols/noise: Add `NoiseConfig::with_prologue` #2903

protocols/noise: Add `NoiseConfig::with_prologue` #2903

mxinden commented Sep 16, 2022 •

edited

Loading