Skip to content

crypto/tls: should apply default NextProtos to the result of GetConfigForClient #70214

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

Open
xinst opened this issue Nov 6, 2024 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@xinst
Copy link

xinst commented Nov 6, 2024

Go version

go version go1.22.8 darwin/amd64

Output of go env in your module/workspace:

GO111MODULE='on'
GOARCH='arm64'
GOBIN='/Users/jack/src/bin'
GOCACHE='/Users/jack/Library/Caches/go-build'
GOENV='/Users/jack/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOMODCACHE='/Users/jack/src/pkg/mod'
GOOS='darwin'
GOPATH='/Users/jack/src'
GOPROXY='https://goproxy.cn,direct'
GOROOT='/Users/jack/go/go1.22.8'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/jack/go/go1.22.8/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.8'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'

What did you do?

I start a service with multiple domains, use GetConfigForClient function for providing TLS certificates dynamically depends on the request's servername. Start go1.6 has enable http2 by default, and it will add supported application level protocols,such as "h2","http/1.1", but the tls.Config from GetConfigForClient did not do this.

// readClientHello reads a ClientHello message and selects the protocol version.
func (c *Conn) readClientHello(ctx context.Context) (*clientHelloMsg, error) {
	// clientHelloMsg is included in the transcript, but we haven't initialized
	// it yet. The respective handshake functions will record it themselves.
	msg, err := c.readHandshake(nil)
	if err != nil {
		return nil, err
	}
	clientHello, ok := msg.(*clientHelloMsg)
	if !ok {
		c.sendAlert(alertUnexpectedMessage)
		return nil, unexpectedMessageError(clientHello, msg)
	}

	var configForClient *Config
	originalConfig := c.config
	if c.config.**GetConfigForClient** != nil {
		chi := clientHelloInfo(ctx, c, clientHello)
		if configForClient, err = c.config.**GetConfigForClient**(chi); err != nil {
			c.sendAlert(alertInternalError)
			return nil, err
		} else if configForClient != nil {
			c.config = configForClient
		}
	}

this leads to the client request offers h2,http/1.1, but server only accepted http/1.1 only

What did you see happen?

 IPv4: xx.xx.xxx.xx
*   Trying xx.xx.xxx.xx:443...
* ALPN: curl offers h2,http/1.1
} [5 bytes data]
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
} [512 bytes data]
*  CAfile: ca.pem
*  CApath: none
{ [5 bytes data]
* TLSv1.3 (IN), TLS handshake, Server hello (2):
{ [122 bytes data]
* TLSv1.3 (IN), TLS change cipher, Change cipher spec (1):
{ [1 bytes data]
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
{ [25 bytes data]
* TLSv1.3 (IN), TLS handshake, Request CERT (13):
{ [229 bytes data]
* TLSv1.3 (IN), TLS handshake, Certificate (11):
{ [1035 bytes data]
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
{ [264 bytes data]
* TLSv1.3 (IN), TLS handshake, Finished (20):
{ [52 bytes data]
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
} [1 bytes data]
* TLSv1.3 (OUT), TLS handshake, Certificate (11):
} [2087 bytes data]
* TLSv1.3 (OUT), TLS handshake, CERT verify (15):
} [264 bytes data]
* TLSv1.3 (OUT), TLS handshake, Finished (20):
} [52 bytes data]
* SSL connection using TLSv1.3 / TLS_AES_256_GCM_SHA384 / x25519 / RSASSA-PSS
* ALPN: server accepted http/1.1

What did you expect to see?

if the NextProtos of tls.Config is empty, should copy from originalConfig, It should be consistent with the default

@seankhliao
Copy link
Member

Please show a reproducer.
The following code gets me HTTP2:

package main

import (
	"crypto/tls"
	"fmt"
	"net/http"
)

func main() {
	crt, err := tls.LoadX509KeyPair("localhost.pem", "localhost-key.pem")
	if err != nil {
		panic(err)
	}

	svr := &http.Server{
		Addr: ":8443",
		Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			fmt.Fprintln(w, "hello world")
		}),
		TLSConfig: &tls.Config{
			GetCertificate: func(chi *tls.ClientHelloInfo) (*tls.Certificate, error) {
				return &crt, nil
			},
		},
	}
	panic(svr.ListenAndServeTLS("", ""))
}

curl:

$ curl -v https://localhost:8443/hello
* Couldn't find host localhost in the .netrc file; using defaults
* Host localhost:8443 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:8443...
* ALPN: curl offers h2,http/1.1
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
*  CAfile: /etc/ssl/certs/ca-certificates.crt
*  CApath: none
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.3 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_128_GCM_SHA256 / x25519 / RSASSA-PSS
* ALPN: server accepted h2
* Server certificate:
*  subject: O=mkcert development certificate; OU=user@hwaryun
*  start date: Nov  6 17:36:49 2024 GMT
*  expire date: Feb  6 17:36:49 2027 GMT
*  subjectAltName: host "localhost" matched cert's "localhost"
*  issuer: O=mkcert development CA; OU=user@hwaryun; CN=mkcert user@hwaryun
*  SSL certificate verify ok.
*   Certificate level 0: Public key type RSA (2048/112 Bits/secBits), signed using sha256WithRSAEncryption
*   Certificate level 1: Public key type RSA (3072/128 Bits/secBits), signed using sha256WithRSAEncryption
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* Connected to localhost (::1) port 8443
* using HTTP/2
* [HTTP/2] [1] OPENED stream for https://localhost:8443/hello
* [HTTP/2] [1] [:method: GET]
* [HTTP/2] [1] [:scheme: https]
* [HTTP/2] [1] [:authority: localhost:8443]
* [HTTP/2] [1] [:path: /hello]
* [HTTP/2] [1] [user-agent: curl/8.10.1]
* [HTTP/2] [1] [accept: */*]
> GET /hello HTTP/2
> Host: localhost:8443
> User-Agent: curl/8.10.1
> Accept: */*
> 
* Request completely sent off
< HTTP/2 200 
< content-type: text/plain; charset=utf-8
< content-length: 12
< date: Wed, 06 Nov 2024 17:41:15 GMT
< 
hello world
* Connection #0 to host localhost left intact

@seankhliao seankhliao added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 6, 2024
@xinst xinst changed the title crypto/tls: GetCertificate doesn't filled the NextProtos values crypto/tls: GetConfigForClient doesn't filled the NextProtos values Nov 8, 2024
@xinst
Copy link
Author

xinst commented Nov 8, 2024

sorry for misleading, the callback function is GetConfigForClient

package main

import (
	"crypto/tls"
	"fmt"
	"net/http"
)


var gTlsCfg = &tls.Config{
		ClientAuth:               tls.RequireAndVerifyClientCert,
		Certificates:             tlsCertArr,
		RootCAs:                  pool,
		ClientCAs:                pool,
		InsecureSkipVerify:       false,
		MinVersion:               tls.VersionTLS12,
	}

func getTlsConfigForClient(chi *tls.ClientHelloInfo) (*tls.Config, error) {	
	if chi.ServerName == "example.com" {	
		tlsCfg := &tls.Config{
			ClientAuth:               tls.RequireAndVerifyClientCert,
			Certificates:             tlsCertArr,
			RootCAs:                  pool,
			ClientCAs:                pool,
			InsecureSkipVerify:       false,
			MinVersion:               tls.VersionTLS13,
		}
		return tlsCfg, nil
	}
	return gTlsCfg
}

func main() {
	svr := &http.Server{
		Addr: ":8443",
		Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			fmt.Fprintln(w, "hello world")
		}),
		TLSConfig: gTlsCfg,
	}
	svr.TLSConfig.GetConfigForClient = getTlsConfigForClient
	panic(svr.ListenAndServeTLS("", ""))
}

@seankhliao seankhliao changed the title crypto/tls: GetConfigForClient doesn't filled the NextProtos values crypto/tls: should apply default NextProtos to the result of GetConfigForClient Nov 13, 2024
@seankhliao seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Nov 13, 2024
@seankhliao
Copy link
Member

cc @golang/security

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants