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

[kristiandrucker]

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]

cc @FiloSottile

[FiloSottile]

This came up before (https://github.com/golang/go/issues/24151#issuecomment-415975207) and I thought it was fixed in the client (https://github.com/GoogleCloudPlatform/cloudsql-proxy/pull/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]

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]

@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]

@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.