Skip to content
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

protocols/noise: Add NoiseConfig::with_prologue #2903

Merged
merged 7 commits into from
Sep 21, 2022
Merged

Conversation

thomaseizinger
Copy link
Contributor

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
Copy link
Contributor Author

That was surprisingly simple :)

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

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines 92 to 99
/// 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
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@@ -185,6 +199,7 @@ where
let session = self
.params
.into_builder()
.prologue(self.prologue.as_ref())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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?

.build_responder()
.map_err(NoiseError::from);
let config = self.legacy;
let identity = self.dh_keys.clone().into_identity();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about implementing into_responder on ProtocolParams and not on NoiseConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Member

mxinden commented Sep 16, 2022

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
Copy link
Member

mxinden commented Sep 16, 2022

Add unit tests for handshake hashes

🙏 test looks good to me.

@thomaseizinger
Copy link
Contributor Author

@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.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

mxinden commented Sep 21, 2022

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

@mxinden mxinden merged commit ed1b899 into master Sep 21, 2022
@thomaseizinger thomaseizinger deleted the 2791-noise-prologue branch September 21, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transports/noise: Expose Prologue mechanism
3 participants