-
Notifications
You must be signed in to change notification settings - Fork 274
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: docs/add-websockets-spec
Are you sure you want to change the base?
Changes from all commits
b680589
3c3a3a9
d9402f6
08a0665
f7837dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -221,6 +221,8 @@ syntax = "proto2"; | |
message NoiseExtensions { | ||
repeated bytes webtransport_certhashes = 1; | ||
repeated string stream_muxers = 2; | ||
optional bool handshake_only = 3; | ||
optional string tls_common_name = 4; | ||
Comment on lines
+224
to
+225
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Hm. You're right, this'll add a round trip. |
||
} | ||
|
||
message NoiseHandshakePayload { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,9 +50,41 @@ WebSockets have no built in authentication mechanism. Server-side processes list | |
|
||
## Encryption | ||
|
||
At the time of writing, the negotiated authentication mechanism should also be used to encrypt all traffic sent over the WebSocket even if TLS certificates are also used at the transport layer. | ||
Server-side processes listening on WebSocket addresses should use TLS certificates to secure transmitted data at the transport level. | ||
|
||
A mechanism to avoid this but also maintain backwards compatibility with existing server-side processes will be specified in a future revision to this spec. | ||
This does not provide any assurance that the remote peer possesses the private key that corresponds to their public key, so an additional handshake is necessary. | ||
|
||
During connection establishment over WebSockets, before the connection is made available to the rest of the application, if all of the following criteria are met: | ||
|
||
1. `noise` is negotiated as the connection encryption protocol | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So despite There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
1. An initial handshake is performed with: | ||
1. the `handshake_only` boolean extension set to true | ||
1. a `tls_common_name` extension value that matches the domain name being connected to | ||
1. The transport layer is secured by TLS | ||
|
||
Then all subsequent data is sent without encrypting it at the libp2p level, instead relying on TLS encryption at the transport layer. | ||
|
||
If this is the case, the server MUST complete the handshake before reading or sending any application data, and MUST abort the connection if the handshake fails. | ||
|
||
If any of the above is not true, all data is encrypted with the negotiated connection encryption method before sending. | ||
|
||
This prevents double-encryption but only when both ends opt-in to ensure backwards compatibility with existing deployments. | ||
|
||
Note that by opting-in to single encryption, peers are also opting-in to trusting the [CA](https://en.wikipedia.org/wiki/Certificate_authority) system. | ||
|
||
### MITM mitigation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Good points both.
I've added a further
I've added a note to this effect.
I've copied the Do you think this covers it? |
||
|
||
The TLS certificate used should be signed by a trusted certificate authority, the host name should correspond to the common name contained within the certificate, and the domain being connected to should match the common name sent as part of the noise handshake. | ||
|
||
This requires trusting the certificate authority to issue correct certificates, but is necessary due to limitations of certain user agents, namely web browsers which do not allow use of self-signed certificates that could be otherwise be verified via preshared certificate fingerprints. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
|
||
### Security Considerations | ||
|
||
Protection against man-in-the-middle (MITM) type attacks is through Web [PKI](https://en.wikipedia.org/wiki/Public_key_infrastructure). If the client is in an environment where Web PKI can not be fully trusted (e.g. an enterprise network with a custom enterprise root CA installed on the client), then this authentication scheme can not protect the client from a MITM attack. | ||
|
||
This authentication scheme is also not secure in cases where you do not own your domain name or the certificate. If someone else can get a valid certificate for your domain, you may be vulnerable to a MITM attack. | ||
|
||
Another solution would be to use Keying Material Exporters [RFC 5705](https://www.rfc-editor.org/info/rfc5705) which would remove the need to add data to the noise handshake, however whether this would be exposed as part of browser APIs is unclear at this point. | ||
|
||
## Addressing | ||
|
||
|
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.
nit: update revision to
r6, 2024-09-10