Skip to content
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

crypto/tls: enforcing ALPN breaks HTTP/1.1 fallbacks on misconfigured servers #46310

Closed
FiloSottile opened this issue May 21, 2021 · 4 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker
Milestone

Comments

@FiloSottile
Copy link
Contributor

In 90d6bbb we fixed crypto/tls to require a successful ALPN negotiation if both sides support it. That is, if a client sends an ALPN extension, and the server has configured NextProtos, we reject the connection if there is no overlap between client and server protocols.

This is what the spec requires, and has a security benefit, because it protects against cross-protocol attacks.

However, it's also kind of surprising, because if a client doesn't support ALPN it will never have its connection rejected.

Moreover, we used not to enforce this, so there is a risk that servers configured a partial NextProtos list, expecting other protocols to just fallback to not negotiating ALPN.

In particular, we have anecdotal evidence of servers with NextProtos: []string{"h2"} that expect a client that tries to negotiate http/1.1 to fallback successfully. These servers will break in Go 1.17.

I searched all code on the Modules Mirror for NextProtos uses, and it looks like this is the only case in which a fallback seems to be expected.

      1 "bep/1.0"
      1 "dilithium"
      1 "gemini"
      1 "go-multicast-quic"
      1 "h2", "h2c", "h2i"
      1 "h2", "hq"
      1 "h2", "http/1.1", "http/1.0"
      1 "h2", "http/1.1", acme.ALPNProto
      1 "h3-29", "h3-32"
      1 "h3r", "h3-29"
      1 "http"
      1 "http", "something-else"
      1 "http/1.0"
      1 "http/1.1", "http/1.0", acme.ALPNProto
      1 "http/1.1", ACMETLS1Protocol
      1 "http/1.1", http2.NextProtoTLS, "h2-14"
      1 "http/2", "http/1.1"
      1 "http3-reflector-server"
      1 "https"
      1 "hybs-rtv1"
      1 "istio", "h2"
      1 "ldap"
      1 "nextProtos"
      1 "ntske/1"
      1 "pop3"
      1 "quic-matrix-ygg"
      1 "quic-vpc-sbs"
      1 "qush"
      1 "rtmp-over-quic", "wq-vvv-01"
      1 "securelink"
      1 "smtp"
      1 "spdy/3"
      1 "stream"
      1 "stream;level=2;pp=1"
      1 "x-amzn-mqtt-ca"
      1 NextProtoTLS, "h2"
      1 ProtoSSH
      1 ProtocolName
      1 SocketProxyNextProto
      1 connector.ProtocolName, authproxy.SocketProxyNextProto, http2.NextProtoTLS
      1 firstMatchProto
      1 getALPN(ietfQUICDraft24VersionNumber)
      1 getALPN(versionNumber)
      1 http2WithTLSVersionID
      1 proto
      1 protoName
      1 quicconn.ProtoSSH
      1 t.protocolName
      2 "adc", "nmdc"
      2 "bw.mux"
      2 "emqx-wormhole"
      2 "foo/bar"
      2 "go-p2p"
      2 "h2", "acme-tls/1"
      2 "h2;rate=30", "spdy/3.1;rate=50", "http/1.1"
      2 "http", "ftp"
      2 "http/1.1", "http/1.0"
      2 "http2"
      2 "imap"
      2 "irc"
      2 "mqtt"
      2 "netceptor"
      2 "orbit-simple-example"
      2 "qperf"
      2 "quic-git"
      2 "quic-hello-example"
      2 "quic-stunnel"
      2 "spdy/3.1", "http/1.1"
      2 "spdy/3.1;rate=50", "http/1.1"
      2 ACMETLS1Protocol
      2 common.KQuicProxy
      2 http2.NextProtoTLS, "h2-14"
      2 nProto
      2 param.Args.Password,"quic-echo-example"
      2 protocol
      2 protocol.ProtocolName
      2 tlsProtocolName
      3 "h1"
      3 "quick"
      3 "quicssh"
      3 acme.ACMETLS1Protocol
      3 http2.NextProtoTLS, "http/1.1"
      3 nextProto
      4 "acme-tls/1"
      4 "h2", "http/1.1", tlsalpn01.ACMETLS1Protocol
      4 "hq-29"
      4 "mixin-quic-peer"
      4 "pion-quic"
      4 KQuicProxy
      5 "SCION"
      5 "http/1.1", "h2", "h3"
      5 "relay"
      5 acmez.ACMETLS1Protocol
      6 "ftp"
      8 "http2", "http/1.1"
     12 "benchmark"
     12 "crypto-setup"
     12 "proto foo", "proto bar"
     12 "proto1"
     12 httpVersionToALPN[httpProxyVer]
     15 httpVersionToAlpn[httpProxyVer]
     16 "unhandled-proto"
     18 "dns"
     18 "protocol1"
     18 "unhandled-proto", "tls-0.9"
     25 NextProtoTLS, "foo", "bar"
     27 "h3-29"
     29 http2.NextProtoTLS
     31 alpnProtocol
     32 acme.ALPNProto
     34 "http/1.1", acme.ALPNProto
     35 acmeALPNProto
     37 "quic-echo-example"
     39 "http/1.1", "h2"
     50 "foo", "bar", NextProtoTLS
     57 "foo", "bar"
     63 "foo"
     66 alpn
     75 NextProtoTLS
    193 "http/1.1"
    271 "h2"
    353 "h2", "http/1.1"

Some of them are from gRPC, which as far as I know legitimately requires HTTP/2, so for those all is well.

Some though look like they might unintentionally break. We should figure out how common this is, weight it against the security benefit, and maybe proactively reach out.

We should also make sure the HTTP/2 docs in Go have correct examples that include http/1.1.

/cc @golang/security

@FiloSottile FiloSottile added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels May 21, 2021
@FiloSottile FiloSottile added this to the Go1.17 milestone May 21, 2021
@FiloSottile
Copy link
Contributor Author

It looks like even x/net/http2 is doing this wrong

https://github.com/golang/net/blob/37e1c6afe02340126705deced573a85ab75209d7/http2/server.go#L262-L271

so we might need to just hardcode an exception such that if h2 is configured we keep ignoring the ALPN extension as long as it includes http/1.1, assuming a fallback is expected.

@gopherbot
Copy link

Change https://golang.org/cl/325611 mentions this issue: http2: also set "http/1.1" ALPN in ConfigureServer

@gopherbot
Copy link

Change https://golang.org/cl/325432 mentions this issue: crypto/tls: let HTTP/1.1 clients connect to servers with NextProtos "h2"

gopherbot pushed a commit to golang/net that referenced this issue Jun 10, 2021
With CL 289209, we started enforcing ALPN protocol overlap when both
sides support ALPN. This means that an "http/1.1"-only ALPN-aware client
will fail to connect to a server with only "h2" in NextProtos.
Unfortunately, that's how ConfigureServer was setting up the tls.Config.

The new behavior mirrors ConfigureTransport on the client side (but not
Transport.newTLSConfig because the latter is used to make HTTP/2-only
connections).

Updates golang/go#46310

Change-Id: Idbab7a6f873f3be78104df6f2908895f4d399bd3
Reviewed-on: https://go-review.googlesource.com/c/net/+/325611
Trust: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@howardjohn
Copy link
Contributor

This broke us and has been preventing us from upgrade http://github.com/istio/istio to golang 1.17.

Our expectation is that we can have a server that will serve any explicitly configured ALPNs with that config (ie h2 will serve HTTP 2), and anything else will fall back to http 1.1. This was the behavior on all former go versions.

dc00dc6 was an insufficient fix for us; we do not have NextProtos: h2 and want to allow http/1.1, we want to support ANY alpn.

A few alternatives we have tried:

  • Not setting NextProtos. This breaks h2 negotiation.
  • Override SupportedProtos in GetConfigForClient. This works on the serverside but breaks go clients (and possibly others) as the NegotiatedProtocol is not corrent
  • Add all ALPNs we care about to NextProtos (not really possible, since ideally we want all of them, but we can live with this), and then setup TLSNextProtos to serve http/1.1 for all those ALPNs. This seems to require completely reimplementing the HTTP/1.1 serving code which is highly undesirable.
  • Re-implement TLS ??? Clearly not a good idea.

Is there any viable workarounds? Right now we are stuck.

dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
With CL 289209, we started enforcing ALPN protocol overlap when both
sides support ALPN. This means that an "http/1.1"-only ALPN-aware client
will fail to connect to a server with only "h2" in NextProtos.
Unfortunately, that's how ConfigureServer was setting up the tls.Config.

The new behavior mirrors ConfigureTransport on the client side (but not
Transport.newTLSConfig because the latter is used to make HTTP/2-only
connections).

Updates golang/go#46310

Change-Id: Idbab7a6f873f3be78104df6f2908895f4d399bd3
Reviewed-on: https://go-review.googlesource.com/c/net/+/325611
Trust: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@golang golang locked and limited conversation to collaborators Oct 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker
Projects
None yet
Development

No branches or pull requests

3 participants