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

Update Golang to 1.15 #7204

Merged
merged 14 commits into from
Jan 6, 2021
Merged

Update Golang to 1.15 #7204

merged 14 commits into from
Jan 6, 2021

Conversation

dkhenry
Copy link
Contributor

@dkhenry dkhenry commented Dec 19, 2020

Backport

NO

Description

Updates the version of go used by CI to 1.15, this also adds in a CI test to ensure we don't drift the bootstrap version

Impacted Areas in Vitess

  • Query Serving
  • VReplication
  • Cluster Management
  • Build

Updating golang build to 1.15 and bumping bootstrap image identifier to
reflect new build.

Signed-off-by: D.K <dan.kozlowski@gmail.com>
Signed-off-by: D.K <dan.kozlowski@gmail.com>
Signed-off-by: D.K <dan.kozlowski@gmail.com>
Signed-off-by: D.K <dan.kozlowski@gmail.com>
Signed-off-by: D.K <dan.kozlowski@gmail.com>
Signed-off-by: D.K <dan.kozlowski@gmail.com>
Signed-off-by: D.K <dan.kozlowski@gmail.com>
@dkhenry
Copy link
Contributor Author

dkhenry commented Dec 19, 2020

@systay this should take care of #7152

Signed-off-by: D.K <dan.kozlowski@gmail.com>
Signed-off-by: D.K <dan.kozlowski@gmail.com>
Signed-off-by: D.K <dan.kozlowski@gmail.com>
@dkhenry dkhenry changed the title Dk update golang Update Golang to 1.15 Dec 22, 2020
@dkhenry
Copy link
Contributor Author

dkhenry commented Dec 22, 2020

This appears to be an actual failure on ./go/test/endtoend/recovery/pitrtls

@dkhenry
Copy link
Contributor Author

dkhenry commented Dec 24, 2020

It appears the default MySQL install generates a TLS certificate that does not set the SAN

openssl x509 -text -noout -in vtdataroot/vtroot_9901/vt_0000003139/data/client-cert.pem 
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 3 (0x3)
        Signature Algorithm: sha256WithRSAEncryption
        Issuer: CN = MySQL_Server_8.0.22_Auto_Generated_CA_Certificate
        Validity
            Not Before: Dec 24 22:37:28 2020 GMT
            Not After : Dec 22 22:37:28 2030 GMT
        Subject: CN = MySQL_Server_8.0.22_Auto_Generated_Client_Certificate
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                RSA Public-Key: (2048 bit)
                Modulus:
                    00:e1:a9:e3:da:fd:06:ab:91:3e:22:d1:ab:35:68:
                    b9:0e:d3:02:ef:5c:90:af:2a:64:b0:87:94:62:55:
                    1c:bb:f6:bb:3d:77:b4:3b:89:e7:c9:09:7c:be:bf:
                    61:a0:4a:ff:45:4f:e0:0a:d6:d8:0e:a2:ed:6a:39:
                    d9:28:ce:db:d4:6b:45:10:80:55:17:a4:05:d2:50:
                    d2:5d:e2:e6:c4:f6:0d:ea:ce:28:af:3d:8a:d8:c6:
                    a1:bf:64:d6:cc:a2:1b:a5:77:3f:78:ef:51:7b:b7:
                    d8:27:10:ad:b3:9d:16:1e:56:ec:69:4c:38:c8:00:
                    5b:c6:3a:fe:c6:41:68:cd:c7:07:5d:72:29:8b:be:
                    42:4b:0b:da:8a:4e:e6:37:e6:e4:24:45:6b:2d:56:
                    32:99:e9:23:23:a4:b8:eb:0b:5d:09:3e:bd:65:1e:
                    04:54:4d:4c:7c:c2:0f:9a:24:3c:8b:78:eb:3d:19:
                    ef:50:d5:73:fa:c2:03:70:c8:05:66:9a:97:75:eb:
                    3c:c7:1c:28:4c:85:10:be:06:0b:51:0f:6b:5b:a3:
                    28:92:78:a8:49:76:ae:20:b5:60:b9:4b:94:1a:34:
                    22:0a:e1:ee:1d:a9:99:9f:84:3e:18:d4:13:90:02:
                    8e:95:7f:5b:26:51:c2:ee:65:bf:eb:91:b5:59:cf:
                    65:f1
                Exponent: 65537 (0x10001)
        X509v3 extensions:
            X509v3 Basic Constraints: critical
                CA:FALSE
    Signature Algorithm: sha256WithRSAEncryption
         ad:84:0a:ab:cd:31:10:42:18:fe:13:c4:b9:b6:b0:20:6f:f3:
         00:c2:85:2d:35:17:d4:f3:ec:cf:c1:ba:d3:4d:f7:d4:d2:b4:
         75:38:a9:43:5d:27:0e:dc:b3:75:3a:f6:d4:14:34:e2:8f:01:
         03:47:81:b1:cd:e0:5b:5c:84:d3:83:d8:b5:0e:a6:c0:db:32:
         18:2f:71:63:7d:e7:1a:46:3b:2b:8b:36:3c:8c:cf:e3:35:0d:
         7c:20:53:22:3c:e9:b8:06:1d:58:d9:fa:df:e0:ea:c4:a6:b4:
         b2:b1:54:00:e3:37:ac:e9:4c:2b:5a:25:f4:7d:e6:19:2c:77:
         6d:85:0b:29:48:e4:d0:5c:8e:09:d6:4f:a4:1a:11:fd:d0:77:
         54:31:25:a4:57:04:51:7d:a7:c9:80:2c:ac:48:21:a4:d6:e1:
         28:fc:fc:ca:11:e6:83:31:f2:38:23:00:a4:1f:df:7c:43:fb:
         af:2e:92:5b:55:f2:60:56:6e:c4:2f:1b:44:3b:ec:45:b2:60:
         9a:0f:cf:4b:70:56:31:21:74:f4:8a:fa:34:df:cf:a4:91:00:
         89:7e:83:9c:6d:89:af:7c:82:f7:26:d6:7e:3d:9c:99:6d:7c:
         8f:05:73:95:fa:f0:b3:4e:87:3c:a2:60:4c:aa:e7:da:82:f9:
         01:de:52:77

What this means is that as of golang 1.15 we will no longer trust these certificates. There is a whole github thread about how this breaks RDS
golang/go#39568

Here is the CL that changed the default
https://go-review.googlesource.com/c/go/+/231379/5

@dkhenry
Copy link
Contributor Author

dkhenry commented Dec 24, 2020

@sougou @askdba This very much would break some systems that use TLS and use legacy certs.

@dkhenry
Copy link
Contributor Author

dkhenry commented Dec 25, 2020

Also I can confirm setting export GODEBUG="x509ignoreCN=0" before I run the tests causes them to pass.

This sets the flag to ignore 509 validation errors caused by certs not
setting a SAN and only using a CN. The default MySQL certs don't set
either to a valid hostname.

Newer versions of ETCD don't respond on the v2 endpoint so this replaces
that check with the /version endpoint which will always respond.

Signed-off-by: D.K <dan.kozlowski@gmail.com>
This uses the tlstest package to generate valid certs and injects them
into mysql for the test

Signed-off-by: D.K <dan.kozlowski@gmail.com>
Some tests use this same infrastructure, but they already plumb in SSL
certificates, this will keep those tests the same and only effect this
one test

Signed-off-by: D.K <dan.kozlowski@gmail.com>
There are quite a few places where it turns out the "VerifyURL" is
actually the base url for accessing data. Who would have thought.

Signed-off-by: D.K <dan.kozlowski@gmail.com>
@deepthi
Copy link
Member

deepthi commented Jan 5, 2021

@sougou @askdba This very much would break some systems that use TLS and use legacy certs.

@dkhenry what is the path for anyone who is doing that? Do they need to run all vitess processes with GODEBUG="x509ignoreCN=0"?

We'll need to document this in release notes.

@dkhenry
Copy link
Contributor Author

dkhenry commented Jan 5, 2021

Adding that GODEBUG env var in will enable the previous behavior for now ( the go maintainers talk like that too will be removed and everyone will need to go to using custom validators ). Also this change isn't actually in the vitess code, so if someone is pulling and building Vitess themselves then they may already be hitting this, and even after this change Vitess built with golang prior to 1.15 will still work as expected. This really only effects the official docker images.

@sougou
Copy link
Contributor

sougou commented Jan 5, 2021

I think it's important that we continue work with systems like RDS. In the spirit of continuing to move forward, we should look at hardcoding this env var in our mysql client.

However, we have to make sure we add this variable to GODEBUG if it is already set. Otherwise, we may override something else.

@dkhenry
Copy link
Contributor Author

dkhenry commented Jan 5, 2021

RDS and CloudSQL have both updated their TLS certificates, but if you have a very old RDS instance that hasn't been upgraded it would require the GODEBUG setting

https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.SSL-certificate-rotation.html

@sougou
Copy link
Contributor

sougou commented Jan 5, 2021

In that case, we should just document this and move on. No need to add any hardcoding.

@dkhenry
Copy link
Contributor Author

dkhenry commented Jan 5, 2021

As to a custom fix this comment lists how to do it client side ( i.e. what we would need to change vttls.go to have )

golang/go#40748 (comment)

@dkhenry
Copy link
Contributor Author

dkhenry commented Jan 6, 2021

If it turns out that we do need client side overrides here is what that looks like

diff --git a/go/vt/vttls/vttls.go b/go/vt/vttls/vttls.go
index 65c8724d9..982ba699a 100644
--- a/go/vt/vttls/vttls.go
+++ b/go/vt/vttls/vttls.go
@@ -57,6 +57,38 @@ func newTLSConfig() *tls.Config {
 
 var onceByKeys = sync.Map{}
 
+func constructCustomVerifier(pool *x509.CertPool) func(cs tls.ConnectionState) error {
+       return func(cs tls.ConnectionState) error {
+               opts := x509.VerifyOptions{
+                       Roots:         pool,
+                       Intermediates: x509.NewCertPool(),
+               }
+               // If there is no SAN, then fallback to using the CommonName
+               hasSanExtension := func(cert *x509.Certificate) bool {
+                       // oid taken from crypt/x509/x509.go
+                       var oidExtensionSubjectAltName = []int{2, 5, 29, 17}
+                       for _, e := range cert.Extensions {
+                               if e.Id.Equal(oidExtensionSubjectAltName) {
+                                       return true
+                               }
+                       }
+                       return false
+               }
+               if !hasSanExtension(cs.PeerCertificates[0]) {
+                       if !strings.EqualFold(cs.ServerName, cs.PeerCertificates[0].Subject.CommonName) {
+                               return x509.HostnameError{cs.PeerCertificates[0], cs.ServerName}
+                       }
+               } else {
+                       opts.DNSName = cs.ServerName
+               }
+               for _, cert := range cs.PeerCertificates[1:] {
+                       opts.Intermediates.AddCert(cert)
+               }
+               _, err := cs.PeerCertificates[0].Verify(opts)
+               return err
+       }
+}
+
 // ClientConfig returns the TLS config to use for a client to
 // connect to a server with the provided parameters.
 func ClientConfig(cert, key, ca, name string) (*tls.Config, error) {
@@ -89,6 +121,10 @@ func ClientConfig(cert, key, ca, name string) (*tls.Config, error) {
                config.ServerName = name
        }
 
+       // Use custom TLS client config to accept depricated CN Names
+       config.InsecureSkipVerify = true
+       config.VerifyConnection = constructCustomVerifier(config.RootCAs)
+
        return config, nil
 }
 
@@ -118,6 +154,9 @@ func ServerConfig(cert, key, ca string) (*tls.Config, error) {
                config.ClientAuth = tls.RequireAndVerifyClientCert
        }
 
+       config.InsecureSkipVerify = true
+       config.VerifyConnection = constructCustomVerifier(config.ClientCAs)
+
        return config, nil
 }

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@deepthi deepthi merged commit 9b5a2a5 into vitessio:master Jan 6, 2021
rohit-nayak-ps added a commit to planetscale/vitess that referenced this pull request Jan 7, 2021
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@askdba askdba added this to the v9.0 milestone Jan 7, 2021
harshit-gangal pushed a commit to planetscale/vitess that referenced this pull request Jun 21, 2021
Signed-off-by: Rohit Nayak <rohit@planetscale.com>

CI: update top level name for unit tests

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

CI: try different naming convention

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

CI: try different naming convention

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

CI: split cluster matrix into individual tests

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Add action to Makefile to generate the yaml files

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Change go version to reflect PR vitessio#7204

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Regenerate files with go-version 1.15

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

gofmt-ed

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Minor refactor, initial README

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Add descriptive names to vrep shards. Update test generator script

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

gofmt-ed

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Address review comment

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>
harshit-gangal pushed a commit to planetscale/vitess that referenced this pull request Jun 22, 2021
Signed-off-by: Rohit Nayak <rohit@planetscale.com>

CI: update top level name for unit tests

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

CI: try different naming convention

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

CI: try different naming convention

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

CI: split cluster matrix into individual tests

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Add action to Makefile to generate the yaml files

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Change go version to reflect PR vitessio#7204

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Regenerate files with go-version 1.15

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

gofmt-ed

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Minor refactor, initial README

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Add descriptive names to vrep shards. Update test generator script

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

gofmt-ed

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Address review comment

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants