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

TLS does not enforce ALPN protocol #434

Closed
carl-mastrangelo opened this issue Nov 6, 2015 · 11 comments · Fixed by #7184
Closed

TLS does not enforce ALPN protocol #434

carl-mastrangelo opened this issue Nov 6, 2015 · 11 comments · Fixed by #7184

Comments

@carl-mastrangelo
Copy link
Contributor

According to https://tools.ietf.org/html/rfc7540#section-3.3 connections over TLS use the "h2" application protocol identifier. Attempting to use another protocol identifier, such as "h2c", should fail the connection. Currently, the Grpc go server accepts using this invalid identifier when establishing a TLS connection.

Here is the test that fails:

https://github.com/grpc/grpc/blob/master/tools/http2_interop/http2interop.go#L235

@jroper
Copy link
Contributor

jroper commented Dec 1, 2023

Crazy that this issue hasn't been fixed 8 years on. Here's a work around that can be used:

import (
	"context"
	"crypto/tls"
	"fmt"
	"google.golang.org/grpc/credentials"
	"net"
)

func NewAlpnEnforcingTLSCredentials(config *tls.Config) credentials.TransportCredentials {
	return &alpnEnforcer{
		delegate: credentials.NewTLS(config),
	}
}

type alpnEnforcer struct {
	delegate credentials.TransportCredentials
}

func (a *alpnEnforcer) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) {
	conn, authInfo, err := a.delegate.ClientHandshake(ctx, authority, rawConn)
	if err != nil {
		return nil, nil, err
	}

	tlsInfo, ok := authInfo.(credentials.TLSInfo)
	if !ok {
		_ = conn.Close()
		return nil, nil, fmt.Errorf("TLS credentials did not return a TLS info")
	}
	if tlsInfo.State.NegotiatedProtocol == "" {
		_ = conn.Close()
		return nil, nil, fmt.Errorf("peer does not support ALPN and therefore can't support HTTP/2")
	}
	if tlsInfo.State.NegotiatedProtocol != "h2" {
		_ = conn.Close()
		return nil, nil, fmt.Errorf("negotiated protocol with peer that is not HTTP/2")
	}
	return conn, authInfo, err
}

func (a *alpnEnforcer) ServerHandshake(conn net.Conn) (net.Conn, credentials.AuthInfo, error) {
	return a.delegate.ServerHandshake(conn)
}

func (a *alpnEnforcer) Info() credentials.ProtocolInfo {
	return a.delegate.Info()
}

func (a *alpnEnforcer) Clone() credentials.TransportCredentials {
	return &alpnEnforcer{
		delegate: a.delegate.Clone(),
	}
}

func (a *alpnEnforcer) OverrideServerName(s string) error {
	return a.delegate.OverrideServerName(s)
}

@prakrit55
Copy link

@jroper, I would like to fix it then

@jroper
Copy link
Contributor

jroper commented Dec 28, 2023

One thing, the original bug posted was about server side, but I think client side validation is a bigger concern because this results is confusing errors when connecting to servers that don't support ALPN, particularly in corporate environments where transparent decrypting proxies such as zscaler are in use. I would create a separate issue for the client side, except #2742 was already created for this, but was closed by @dfawley as a duplicate of this issue.

@arvindbr8
Copy link
Member

@prakrit55 -- thanks for the interest. which one would you like to pick up first?

@arvindbr8
Copy link
Member

Sorry for the delay in getting back to this.

Looking at the current state of things every time a new connection is made over TLS, an h2 string is appended to NextProto in tls.config

the Go library itself handles negotiating ALPN supported by the server and client. See https://cs.opensource.google/go/go/+/refs/tags/go1.21.6:src/crypto/tls/handshake_server.go;l=224

@jroper -- It seems like this case is already handled by gRPC-go? I'm curious to know how did you face this issue?

@jroper
Copy link
Contributor

jroper commented Jan 23, 2024

Yes, h2 is always added, and the Go library itself does handle negotiating ALPN. However, if the server being connected to doesn't support ALPN, then the returned NegotiatedProtocol will be empty, see the docs here:

        // NextProtos is a list of supported application level protocols, in
	// order of preference. If both peers support ALPN, the selected
	// protocol will be one from this list, and the connection will fail
	// if there is no mutually supported protocol. If NextProtos is empty
	// or the peer doesn't support ALPN, the connection will succeed and
	// ConnectionState.NegotiatedProtocol will be empty.
	NextProtos []string

That feature is by design - when making ordinary HTTP/2 connections, an HTTP client must check ConnectionState.NegotiatedProtocol, and if it's not set, it will know that the server doesn't support ALPN and therefore can't support HTTP/2, and so use HTTP/1.1 instead. However, grpc-go does not do this check, instead it proceeds to try and use HTTP/2 when that hasn't been negotiated. That's the bug I encountered, and can be reproduced by connecting to an HTTPS server that doesn't support ALPN. Of course, since it doesn't support ALPN, the connection will eventually fail anyway when the server receives the HTTP/2 connection and doesn't know how to handle it, and respond to the client in HTTP/1.1, which the client doesn't know how to handle. But that's a very messy way to fail, much better to detect it up front and fail gracefully with meaningful error message, like "server does not support ALPN and therefore HTTP/2 could not be negotiated".

@prakrit55
Copy link

hey @arvindbr8 @jroper, can I try to fix it up, I understand it and wd like to give it a try

@arvindbr8
Copy link
Member

@jroper -- thanks for the clarification. You are right, gRPC-Go does not check for ConnectionState.NegotiatedProtocol and err if nil

@prakrit55 -- Sure you can pick this up. Let me assign it you and please use this issue to track the issue. Also please feel free to ask any questions.

@arvindbr8 arvindbr8 assigned prakrit55 and unassigned jroper Jan 23, 2024
@arvindbr8
Copy link
Member

@prakrit55 -- this is potentially a breaking change for some existing users. The change needs to be carefully rolled out. Let's have the implementation flag protected using an env variable and keep the default to keep the check turned off.

Once we make sure that this change does break existing users, we can flip the flag.

@arjan-bal
Copy link
Contributor

@prakrit55 if you don't plan to raise a PR soon, I can pick up this issue.

@prakrit55
Copy link

prakrit55 commented May 2, 2024

@prakrit55 if you don't plan to raise a PR soon, I can pick up this issue

Sure: )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

11 participants