src/net/http: server.go ListenAndServeTLS will overwrite TLSConfig Certificates even when they are already set

[wfotiuk]

Go version

latest

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/wfotiuk/Library/Caches/go-build'
GOENV='/Users/wfotiuk/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/wfotiuk/.gobrew/current/go/bin/pkg/mod'
GONOPROXY='none'
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/wfotiuk/.gobrew/current/go/bin'
GOPRIVATE=''
GOPROXY=''
GOROOT='/Users/wfotiuk/.gobrew/current/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/wfotiuk/.gobrew/current/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.4'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD=''
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/kk/znyj_cyn64z05m_m5gzzlhlm0000gp/T/go-build3029075827=/tmp/go-build -gno-record-gcc-switches -fno-common'

Removed some data for privacy purposes, this will not affect the bug either way

What did you do?

I set up a TLSConfig on a http.Server that had Certificates filled out. Example:

               server := &http.Server{
			Addr:      "whatever",
			TLSConfig: tlsConfig,
		}
		if err := server.ListenAndServeTLS(deviceCert, privateKey); err != nil {
			log.WithError(err).Error("https server start")
			return
		}

There is code here that seems to be checking if there is already certificates set and not to overwrite them.

        configHasCert := len(config.Certificates) > 0 || config.GetCertificate != nil
	if !configHasCert || certFile != "" || keyFile != "" {
               var err error
		config.Certificates = make([]tls.Certificate, 1)
		config.Certificates[0], err = tls.LoadX509KeyPair(certFile, keyFile)
		if err != nil {
			return err
		}
	}

The problem here is the if condition needs to be an AND not an OR. If we have already set certificates we NEVER want to overwrite them, even if the provided files are not empty. I would be happy to implement this fix just want to confirm it is indeed a problem.

Proposed fix:

if !configHasCert && (certFile != "" || keyFile != "") {

Please let me know if you need more clarification!

What did you see happen?

I see my certificates get overwritten in my TLSConfig even though there is a check for it.

What did you expect to see?

I expect my Certificates not to be overwritten regardless of the device cert file and private key file I provide. I wonder if another function that accepts noa rguments might be more clear in this case.

[gabyhelp]

Related Issues and Documentation

• net/http/httptest: Server does not check GetCertificate #63812
• net/http: Server.ListenAndServeTLS() leaks TCP sockets on invalid cert or key #23002 (closed)
• net/http: decide how users should use SetSessionTicketKeys with ServeTLS, etc #22018
• x/net/http2: `http.Server.Serve` doesn't serve http2 traffic #14374 (closed)
• net/http: make ListenAndServeTLS treat GetCertificate as a set cert too
• crypto/tls: RequireAndVerifyClientCert is not working as intended. #63122 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

I believe this is working as intended, don't pass non empty values if you don't want them to be used. Having the function ignore arguments would be more confusing.

[wfotiuk]

@seankhliao I guess my question would then be why have the check at all for certs? To me the check implies we didn't want to overwrite if there were already certs. What are your thoughts?

[seankhliao]

This forces an error if no certs are provided anywhere.

[wfotiuk]

The error would be the same with or without the cert check though. It just goes through to ServeTLS and would explode

To me that block says "If we don't have cert data and we provided cert data, set the certificates to the keypair provided". I have to imagine if we wanted to go in the conditional when we had existing cert data, we would append rather than overwrite.