Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WebSockets: add opt-in delegation of encryption to TLS #625
base: master
Are you sure you want to change the base?
WebSockets: add opt-in delegation of encryption to TLS #625
Changes from 4 commits
17c7ccc
b680589
3c3a3a9
d9402f6
08a0665
f7837dc
76ca6ec
cc8b242
7c2f77f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can we make this less specific to websocket/TLS? E.g., one way to do this is to:
To support a better upgrade path, this method can be advertised as a new "stream muxer" (more like a "next protocol"). I.e.:
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 seems like it would add quite a lot of round trips? Part of the joy of the extensions is they arrive as part of the handshake message.
I know connection establishment with WebSockets has a bunch of round trips already but is this a reason to make it slower, particularly when we already have WebTransport-specific stuff in the extensions registry?
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.
Hm. You're right, this'll add a round trip.
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.
So despite
noise
being negotiated as the encryption protocol, no encryption is done at the libp2p level, because of thehandshake_only
which overrules it?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.
That's correct. Noise does a handshake and encryption, and here we only want the handshake since encryption is done by TLS at the transport layer. This is similar to how the WebTransport and WebRTC transports work.
We still negotiate a connection encryption protocol for backwards compatibility and to allow for alternatives to noise in the future.
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 doesn't work unless the server somehow includes their domain name in their noise handshake. Otherwise, an attacker can simply announce that some peer id
QmVictim
is available at/dns/attacker.com/...
.Furthermore, by including the domain name in the noise handshake, the server is opting-in to trusting the CA system.
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.
Note that this also doesn't work unless the server trusts all owners of the given domain name. If the server operator is the sole owner of the domain name, this is fine. But if another party controls the domain name as well, but not the private key to the peer ID, they would be able to MITM/intercept this connection.
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 points both.
I've added a further
tls_common_name
extension to the noise handshake to prevent an attacker publishing an address with a bogus domain and then simply relaying the handshake.I've added a note to this effect.
I've copied the
Security Considerations
section from #564 as it seems like they have the same problems with third party root certificates installed on the machine.Do you think this covers it?
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.
I’d include a sentence that mentions Keying Material Exporters (RFC 5705), which, in a perfect world, would be the preferred solution.
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.
I've added a sentence - please suggest an edit if you think it needs more exposition.