noise: introduce an extension registry#453
Conversation
marten-seemann
requested review from
MarcoPolo,
elenaf9,
julian88110,
mxinden and
p-shahi
| Code points above 1024 MAY be used for experimentation. Code points up to | ||
| this value MUST be registered in this document before deployment. |
There was a problem hiding this comment.
As far as I understand we tried to use multicodec for all magic numbers in the libp2p ecosystem thus far. If I understand this proposal correctly, it runs somewhat against this convention.
I am not opposed to that, I just think it is worth pointing out in the specification. E.g. a short paragraph of why we are not using multicodec for this.
What do you think @marten-seemann?
There was a problem hiding this comment.
Good point. I'm not super familiar with the design philosophy behind multicoded. @Stebalien and @lidel, could you help us out here?
MarcoPolo
left a comment
MarcoPolo
left a comment
There was a problem hiding this comment.
Let's discuss the first message payload changes first.
marten-seemann
force-pushed
the
noise-extensions
branch
2 times, most recently
from
46594ea to
a9bb714
Compare
| responder sends theirs in message 2 (their only message). It should be stressed, | ||
| that while the second message of the handshake pattern has forward secrecy, | ||
| the sender has not authenticated the responder yet, so this payload might be | ||
| sent to any party, including an active attacker. |
There was a problem hiding this comment.
As a security precaution is it helpful to specify that the "early data" should not be processed before the handshake is fully finished? thus gives us more confidence on the data, what are your thoughts on this?
There was a problem hiding this comment.
In WebTransport, we process it immediately, and fail the handshake if the certificate hash doesn't match.
Maybe we should point out that consumers of early data need to be careful, and that delaying the processing / acting upon the early data can be one strategy to deal with this.
There was a problem hiding this comment.
Great, document that is a good point.
Processing early data before authentication runs the risk of opening an attack surface for flooding / DOS attack.
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Looks good to me once updated to #456 and multicodec comment is addressed.
Thanks @marten-seemann for driving this.
marten-seemann
force-pushed
the
noise-extensions
branch
from
a9bb714 to
a0d7e00
Compare
marten-seemann
force-pushed
the
noise-extensions
branch
from
a0d7e00 to
b0818fa
Compare
|
I rebased this PR on top of master, bumped the version number and added a Changelog. This should be good to merge now. |
marten-seemann
dismissed
MarcoPolo’s stale review
all comments were addressed
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Just one question. Otherwise this looks good to me.
| message NoiseHandshakePayload { | ||
| optional bytes identity_key = 1; | ||
| optional bytes identity_sig = 2; | ||
| optional bytes data = 3; |
There was a problem hiding this comment.
For the record, rust-libp2p does not make use of data. @marten-seemann do any of the other implementations?
There was a problem hiding this comment.
go-libp2p doesn't, and js-libp2p doesn't either (as far as I know).
There was a problem hiding this comment.
What about nim-libp2p cpp-libp2p jvm-libp2p @marten-seemann?
There was a problem hiding this comment.
I’m not familiar with their code bases. This PR was open for more than a week, that should’ve been plenty of time to chime in. Also note that this is backwards-compatible change since we didn’t reuse field 3 in the protobuf.
4 participants
Fixes #450.
This PR removes the
datafield from theNoiseHandshakePayloadprotobuf, and adds anextensionsfield.extensionsisNoiseExtensionsprotobuf, modeled after the extension registries that TLS and QUIC use.By using a new ID (
datawas 3,extensionsis 4), this change won’t break implementations that used early data (I’m not aware of any, but just in case).I’ve added code points for WebTransport, WebRTC and Early Muxer Negotiation to this PR, mostly to demonstrate how this would look like. Given that none of the specs for these are merged yet, it might make sense to define an empty
NoiseExtensionsprotobuf here, and have the respective PRs add their code point (we’ll have to deal with merge conflicts then). Let me know if I should make that change.