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/x509: Google Cloud SQL TLS connections broken in Go 1.15+ #40748

Open
kristiandrucker opened this issue Aug 13, 2020 · 5 comments
Open

crypto/x509: Google Cloud SQL TLS connections broken in Go 1.15+ #40748

kristiandrucker opened this issue Aug 13, 2020 · 5 comments

Comments

@kristiandrucker
Copy link

@kristiandrucker kristiandrucker commented Aug 13, 2020

What version of Go are you using (go version)?

$ go version
go version go1.15 darwin/amd64

Does this issue reproduce with the latest release?

Only the latest release.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/kristiandrucker/Library/Caches/go-build"
GOENV="/Users/kristiandrucker/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/kristiandrucker/go/pkg/mod"
GOOS="darwin"
GOPATH="/Users/kristiandrucker/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/kristiandrucker/sdk/go1.15"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/kristiandrucker/sdk/go1.15/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/k2/q0d7ylmn3xvb1znmhd__5yvw0000gn/T/go-build717576566=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I've tried to connect to a Google Cloud MySQL instance using TLS. This worked flawlessly on previous releases, however with Go 1.15+ I'm getting the following error x509: certificate is not valid for any names, but wanted to match project-name:db-name. Am using the same code as before as well as the same certificates. While debugging and stepping through the TLS verification I found that commonNameAsHostname function fails on validHostnamePattern which calls validHostname with the hostname string and isPattern set to true.

Since the Google Cloud SQL ServerName is expected to be in format of project-name:db-name, the validHostname function would return false because it does not consider : to be valid. This change has been done in the following commit where the : character has been removed which causes the issue.

My code to connect to the Google Cloud SQL:

package main

import (
    "crypto/tls"
    "crypto/x509"
    "database/sql"
    "github.com/go-sql-driver/mysql"
    "io/ioutil"
)

func main() {
    rootCertPool := x509.NewCertPool()

    pem, err := ioutil.ReadFile("/path/server-ca.pem")
    if err != nil {
        log.Fatal(err)
    }

    if ok := rootCertPool.AppendCertsFromPEM(pem); !ok {
        log.Fatal("Failed to append PEM.")
    }

    clientCert := make([]tls.Certificate, 0, 1)
    certs, err := tls.LoadX509KeyPair("/path/client-cert.pem",
                                  "/path/client-key.pem")
    if err != nil {
        log.Fatal(err)
    }

    clientCert = append(clientCert, certs)
    mysql.RegisterTLSConfig("custom", &tls.Config{
        RootCAs: rootCertPool,
        Certificates: clientCert,
        ServerName: "<gcp-project-id>:<cloud-sql-instance>", // hostname
    })

    db, err := sql.Open("mysql",
        "<user>:<password>@tcp(<cloud sql ip>:3306)/<db_name>?tls=custom")
    // Code to execute commands against the DB. We use [EntGo](https://entgo.io/) but it uses the sql.DB interface for command execution.
}

What did you expect to see?

A successful database connection and command execution.

What did you see instead?

x509: certificate is not valid for any names, but wanted to match project-name:db-name while trying to connect to the database with TLS on Google Cloud SQL.

@ALTree
Copy link
Member

@ALTree ALTree commented Aug 13, 2020

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Aug 13, 2020

This came up before (#24151 (comment)) and I thought it was fixed in the client (GoogleCloudPlatform/cloudsql-proxy#196) but apparently that's a component that not everyone uses. I also thought they started setting SANs.

These certificates have two problems: they use the Common Name field, and they have names which are invalid hostnames. To be fair, CN is the right place to put a name which is not a hostname. But then the client can't be expected to match it as a hostname, which is what's happening here.

This can be fixed server-side by putting the name in the SAN field, because I preserved 1:1 matching of invalid hostnames specifically for cases like this. I will reach out to the team.

If you want a workaround in the meantime, you can see the code in the cloudsql-proxy PR.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Aug 13, 2020

Actually, here's an even better way to do it correctly client-side with the new VerifyConnection callback.

&tls.Config{
	ServerName: "<gcp-project-id>:<cloud-sql-instance>",
	// Set InsecureSkipVerify to skip the default validation we are
	// replacing. This will not disable VerifyConnection.
	InsecureSkipVerify: true,
	VerifyConnection: func(cs tls.ConnectionState) error {
		commonName := cs.PeerCertificates[0].Subject.CommonName
		if commonName != cs.ServerName {
			return fmt.Errorf("invalid certificate name %q, expected %q", commonName, cs.ServerName)
		}
		opts := x509.VerifyOptions{
			Roots:         rootCertPool,
			Intermediates: x509.NewCertPool(),
		}
		for _, cert := range cs.PeerCertificates[1:] {
			opts.Intermediates.AddCert(cert)
		}
		_, err := cs.PeerCertificates[0].Verify(opts)
		return err
	},
}
@onionjake
Copy link

@onionjake onionjake commented Sep 18, 2020

@FiloSottile is use of VerifyConnection in this way future proof? The documentation for VerifyHostname says "Support for Common Name is deprecated will be entirely removed in the future." Go will always still parse CommonName and allow use like this forever right?

There are potentially hundreds of thousands of devices with certificates with a common name similar to the usage here burned into their firmware that I will need to be able to support server side for the next several years. I'm just starting to look into this and exactly what changes we need to make client and server side.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Sep 18, 2020

@FiloSottile is use of VerifyConnection in this way future proof? The documentation for VerifyHostname says "Support for Common Name is deprecated will be entirely removed in the future." Go will always still parse CommonName and allow use like this forever right?

Absolutely, the field itself is not going anywhere, what's deprecated is VerifyHostname looking at it.

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.