Muxer selection in security handshake#446
Conversation
marten-seemann
left a comment
marten-seemann
left a comment
There was a problem hiding this comment.
This looks like a good start, thank you @julian88110!
Not sure if I was supposed to review it yet, so I just added some high-level comments.
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Solid first specification proposal @julian88110! Thank you for the write-up!
MarcoPolo
left a comment
MarcoPolo
left a comment
There was a problem hiding this comment.
Looking good! I think we should have a Go implementation before merging this, since I think that will uncover some subtleties we probably want to mention.
Also I think the exact steps in the noise handshake are still a little fuzzy for me.
Update Noise handshake section, Revise muxer string in examples.
|
@julian88110 The Noise Extension Registry (#453) was merged. You can rebase this PR now, and add an extension entry for the muxer selection. |
Merge remote-tracking branch 'origin/master' into MuxerSel-inSecHandshake
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
|
I updated spec and condensed it down a bit. @mxinden and @MarcoPolo, can I ask you for a review? |
Co-authored-by: Prithvi Shahi <50885601+p-shahi@users.noreply.github.com>
Co-authored-by: Prithvi Shahi <50885601+p-shahi@users.noreply.github.com>
Co-authored-by: Prithvi Shahi <50885601+p-shahi@users.noreply.github.com>
elenaf9
left a comment
elenaf9
left a comment
There was a problem hiding this comment.
Couple of questions. Apologies if they have already been discussed or if the answers are obvious. Parts of this are still new to me.
| this document use `"libp2p"` for their ALPN. If `"libp2p"` is the result of the | ||
| ALPN process, nodes MUST use multistream negotiation of the stream multiplexer | ||
| as described in [connections]. |
There was a problem hiding this comment.
Per connection spec, both sides can initiate the protocol negotiation.
I think the server SHOULD NOT initiate a protocol negotiation if it already knows that it doesn't support any of the client's muxers.
The client can't differentiate between whether the server doesn't support early muxer negotiation, or whether it doesn't support any of the its muxers.
In case of the latter it would do an unnecessary multistream negotiation attempt just to then still fail. We could add an extra item to signal that none of the protocols are supported. But not sure if its worth the complexity.
There was a problem hiding this comment.
Actually, in case of noise we fail if no common muxer is supported. Sorry if this is a stupid question, but why don't we also fail the TLS handshake in that case?
There was a problem hiding this comment.
This is to enable backwards-compatibility with legacy implementations. With these nodes, the result of the negotiation process will be "libp2p", so we need to do multistream.
Now we could say that we fail the handshake if there's anything other than "libp2p" in the list, but this would make it impossible to deploy a listener that speaks HTTP and libp2p without this optimization.
| According to [TLS], nodes that don't implement the optimization described in | ||
| this document use `"libp2p"` for their ALPN. If `"libp2p"` is the result of the |
There was a problem hiding this comment.
Unless I am missing something this is not true: the TLS spec does not specify what ALPN to use.
Also: if we add libp2p over http as drafted in #477 we could also have the "h2" APLN here, no?
There was a problem hiding this comment.
Yes, you could accept HTTP and libp2p at the same time. That's WebTransport does, by the way.
Negotiating HTTP though means that after completing the handshake, you're in HTTP land, not in libp2p land any more.
There was a problem hiding this comment.
Unless I am missing something this is not true: the TLS spec does not specify what ALPN to use.
Good catch. Both Go and Rust do this. @marten-seemann can you add it to the TLS spec? Either in this pull request or a separate one?
Co-authored-by: Elena Frank <elena.frank@protonmail.com>
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Apart from the comment above on documenting the default libp2p ALPN, this specification is good to merge from my end.
Thanks again for the document 🙏
(Corresponding tracking issue in rust-libp2p: libp2p/rust-libp2p#2994)
| According to [TLS], nodes that don't implement the optimization described in | ||
| this document use `"libp2p"` for their ALPN. If `"libp2p"` is the result of the |
There was a problem hiding this comment.
Unless I am missing something this is not true: the TLS spec does not specify what ALPN to use.
Good catch. Both Go and Rust do this. @marten-seemann can you add it to the TLS spec? Either in this pull request or a separate one?
6 participants
This spec describes a feature that carries out muxer selection in the security handshake process.