Skip to content

Commit

Permalink
Offer ALPN for WebSockets requests.
Browse files Browse the repository at this point in the history 
Connections made for WebSockets requests cannot offer HTTP/2, so we
currently clear the ALPN list. Instead, still offer ALPN, but with just
"http/1.1". This has no impact on the selected HTTP protocol, but it
does change behavior slightly. The server may either ignore ALPN (in
which case, we use HTTP/1.1) or negotiate ALPN and explicitly select
HTTP/1.1.

My immediate motivation is that, when we add support for HTTPS records,
we won't be able to process the records correctly if we don't know the
ALPN protocol list. However, this has two other benefits:

1. We harden against cross-protocol attacks like ALPACA. If some
   non-HTTPS TLS-based protocol also uses Web PKI certificates, an ALPN
   negotiation avoids a mixup between that protocol's and HTTPS clients
   or vice versa. However, while Chrome always offers ALPN for HTTPS,
   we missed WSS. This CL fixes this.

2. We currently gate False Start on ALPN (historically there were
   compatibility issues). This change allows TLS 1.2 WSS servers to save
   a round-trip. (IIRC, NGINX still negotiates ALPN with http/1.1 to
   pick up this optimization.)

It also matches MaybeForceHTTP11, which is part of handling
HTTP_1_1_REQUIRED (maybe also some auth cases?), as well as Firefox
and Safari behavior.

Bug: 1287240
Change-Id: I99d8c1654c4572090350c8164d3f6c7dfd02218a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3390109
Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/main@{#963607}
  • Loading branch information
@davidben
davidben authored and Chromium LUCI CQ committed Jan 26, 2022
1 parent 7fbbdbe commit db82955
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 16 deletions.
2 changes: 1 addition & 1 deletion net/http/http_network_transaction_unittest.cc
View file
Expand Up @@ -9314,7 +9314,7 @@ TEST_F(HttpNetworkTransactionTest, NTLMOverHttp2WithWebsockets) {
session_deps_.socket_factory->AddSocketDataProvider(&data1);
SSLSocketDataProvider ssl1(ASYNC, OK);
// When creating the second connection, only HTTP/1.1 should be allowed.
ssl1.next_protos_expected_in_ssl_config = NextProtoVector{};
ssl1.next_protos_expected_in_ssl_config = NextProtoVector{kProtoHTTP11};
session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl1);

session_deps_.enable_websocket_over_http2 = true;
Expand Down
11 changes: 10 additions & 1 deletion net/http/http_stream_factory_job.cc
View file
Expand Up @@ -831,8 +831,17 @@ int HttpStreamFactory::Job::DoInitConnectionImpl() {
if (is_websocket_) {
DCHECK(request_info_.socket_tag == SocketTag());
DCHECK_EQ(SecureDnsPolicy::kAllow, request_info_.secure_dns_policy);
// Only offer HTTP/1.1 for WebSockets. Although RFC 8441 defines WebSockets
// over HTTP/2, a single WSS/HTTPS origin may support HTTP over HTTP/2
// without supporting WebSockets over HTTP/2. Offering HTTP/2 for a fresh
// connection would break such origins.
//
// However, still offer HTTP/1.1 rather than skipping ALPN entirely. While
// this will not change the application protocol (HTTP/1.1 is default), it
// provides hardens against cross-protocol attacks and allows for the False
// Start (RFC 7918) optimization.
SSLConfig websocket_server_ssl_config = server_ssl_config_;
websocket_server_ssl_config.alpn_protos.clear();
websocket_server_ssl_config.alpn_protos = {kProtoHTTP11};
return InitSocketHandleForWebSocketRequest(
destination_, request_info_.load_flags, priority_, session_,
proxy_info_, websocket_server_ssl_config, proxy_ssl_config_,
Expand Down
28 changes: 14 additions & 14 deletions net/spdy/spdy_network_transaction_unittest.cc
View file
Expand Up @@ -8719,8 +8719,8 @@ TEST_F(SpdyNetworkTransactionTest, WebSocketOpensNewConnection) {
StaticSocketDataProvider data2(reads2, writes2);

auto ssl_provider2 = std::make_unique<SSLSocketDataProvider>(ASYNC, OK);
// Test that request has empty |alpn_protos|, that is, HTTP/2 is disabled.
ssl_provider2->next_protos_expected_in_ssl_config = NextProtoVector{};
// Test that the request has HTTP/2 disabled.
ssl_provider2->next_protos_expected_in_ssl_config = {kProtoHTTP11};
// Force socket to use HTTP/1.1, the default protocol without ALPN.
ssl_provider2->next_proto = kProtoHTTP11;
ssl_provider2->ssl_info.cert =
Expand Down Expand Up @@ -8842,8 +8842,8 @@ TEST_F(SpdyNetworkTransactionTest,
SequencedSocketData data2(MockConnect(ASYNC, ERR_IO_PENDING), reads2,
writes2);
auto ssl_provider2 = std::make_unique<SSLSocketDataProvider>(ASYNC, OK);
// Test that request has empty |alpn_protos|, that is, HTTP/2 is disabled.
ssl_provider2->next_protos_expected_in_ssl_config = NextProtoVector{};
// Test that the request has HTTP/2 disabled.
ssl_provider2->next_protos_expected_in_ssl_config = {kProtoHTTP11};
// Force socket to use HTTP/1.1, the default protocol without ALPN.
ssl_provider2->next_proto = kProtoHTTP11;
helper.AddDataWithSSLSocketDataProvider(&data2, std::move(ssl_provider2));
Expand Down Expand Up @@ -9118,8 +9118,8 @@ TEST_F(SpdyNetworkTransactionTest,
&tunnel_ssl_data2);

auto ssl_provider2 = std::make_unique<SSLSocketDataProvider>(ASYNC, OK);
// Test that request has empty |alpn_protos|, that is, HTTP/2 is disabled.
ssl_provider2->next_protos_expected_in_ssl_config = NextProtoVector{};
// Test that the request has HTTP/2 disabled.
ssl_provider2->next_protos_expected_in_ssl_config = {kProtoHTTP11};
// Force socket to use HTTP/1.1, the default protocol without ALPN.
ssl_provider2->next_proto = kProtoHTTP11;
helper.AddDataWithSSLSocketDataProvider(&data2, std::move(ssl_provider2));
Expand Down Expand Up @@ -9384,8 +9384,8 @@ TEST_F(SpdyNetworkTransactionTest,
"Sec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n\r\n")};
StaticSocketDataProvider data2(reads2, writes2);
auto ssl_provider2 = std::make_unique<SSLSocketDataProvider>(ASYNC, OK);
// Test that request has empty |alpn_protos|, that is, HTTP/2 is disabled.
ssl_provider2->next_protos_expected_in_ssl_config = NextProtoVector{};
// Test that the request has HTTP/2 disabled.
ssl_provider2->next_protos_expected_in_ssl_config = {kProtoHTTP11};
// Force socket to use HTTP/1.1, the default protocol without ALPN.
ssl_provider2->next_proto = kProtoHTTP11;
ssl_provider2->ssl_info.cert =
Expand Down Expand Up @@ -9493,8 +9493,8 @@ TEST_F(SpdyNetworkTransactionTest, WebSocketNegotiatesHttp2) {
StaticSocketDataProvider data;

auto ssl_provider = std::make_unique<SSLSocketDataProvider>(ASYNC, OK);
// Test that request has empty |alpn_protos|, that is, HTTP/2 is disabled.
ssl_provider->next_protos_expected_in_ssl_config = NextProtoVector{};
// Test that the request has HTTP/2 disabled.
ssl_provider->next_protos_expected_in_ssl_config = {kProtoHTTP11};
// Force socket to use HTTP/2, which should never happen (TLS implementation
// should fail TLS handshake if server chooses HTTP/2 without client
// advertising support).
Expand Down Expand Up @@ -9584,8 +9584,8 @@ TEST_F(SpdyNetworkTransactionTest, WebSocketHttp11Required) {
"Sec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n\r\n")};
StaticSocketDataProvider data2(reads2, writes2);
auto ssl_provider2 = std::make_unique<SSLSocketDataProvider>(ASYNC, OK);
// Test that request has empty |alpn_protos|, that is, HTTP/2 is disabled.
ssl_provider2->next_protos_expected_in_ssl_config = NextProtoVector{};
// Test that the request has HTTP/2 disabled.
ssl_provider2->next_protos_expected_in_ssl_config = {kProtoHTTP11};
// Force socket to use HTTP/1.1, the default protocol without ALPN.
ssl_provider2->next_proto = kProtoHTTP11;
ssl_provider2->ssl_info.cert =
Expand Down Expand Up @@ -9757,7 +9757,7 @@ TEST_F(SpdyNetworkTransactionTest, SecureWebSocketOverHttp2Proxy) {
ssl_provider.ssl_info.cert =
ImportCertFromFile(GetTestCertsDirectory(), "wildcard.pem");
// A WebSocket request should not advertise HTTP/2 support.
ssl_provider.next_protos_expected_in_ssl_config = NextProtoVector{};
ssl_provider.next_protos_expected_in_ssl_config = {kProtoHTTP11};
// This test uses WebSocket over HTTP/1.1.
ssl_provider.next_proto = kProtoHTTP11;
helper.session_deps()->socket_factory->AddSSLSocketDataProvider(
Expand Down Expand Up @@ -9817,7 +9817,7 @@ TEST_F(SpdyNetworkTransactionTest,
ssl_provider.ssl_info.cert =
ImportCertFromFile(GetTestCertsDirectory(), "wildcard.pem");
// A WebSocket request should not advertise HTTP/2 support.
ssl_provider.next_protos_expected_in_ssl_config = NextProtoVector{};
ssl_provider.next_protos_expected_in_ssl_config = {kProtoHTTP11};
// The server should not negotiate HTTP/2 over the tunnelled connection,
// but it must be handled gracefully if it does.
ssl_provider.next_proto = kProtoHTTP2;
Expand Down

0 comments on commit db82955

Please sign in to comment.