From 1d095e587c81519f82d4b2110bd367fd2fa876f8 Mon Sep 17 00:00:00 2001 From: Shawn Wang Date: Fri, 19 Jan 2024 12:41:24 -0800 Subject: [PATCH] Do exact cert matching for leaf certs Given that NSX manager may present TLS cert with mismatched subject, this change addresses TLS validation as follows: - If CA cert, native cert verification is done - if leaf cert or a chain of certs (the first one in the chain will be a leaf cert), exact cert matching will be done. This will bypass any CN / SAN / expiration / revocation check. Above logic also aligns with VC certificate validation guideline. Signed-off-by: Shawn Wang --- pkg/nsx/cluster.go | 16 ++++--------- pkg/nsx/util/utils.go | 49 ++++++++++++++++++++++++++++---------- pkg/nsx/util/utils_test.go | 36 +++++++++++++++++++++------- 3 files changed, 68 insertions(+), 33 deletions(-) diff --git a/pkg/nsx/cluster.go b/pkg/nsx/cluster.go index 7e6991137..440b38f38 100644 --- a/pkg/nsx/cluster.go +++ b/pkg/nsx/cluster.go @@ -5,7 +5,6 @@ package nsx import ( "crypto/tls" - "crypto/x509" "errors" "fmt" "net" @@ -154,17 +153,10 @@ func (cluster *Cluster) createTransport(idle time.Duration) *Transport { return nil, err } - certPool := x509.NewCertPool() - certPool.AppendCertsFromPEM(caCert) - - config = &tls.Config{ - RootCAs: certPool, - } - - // Bypass CN / SAN verification by setting tls config ServerName to the one in cert - if cn, err := util.GetCommonNameFromLeafCert(caCert); err == nil { - log.Info("pinned common name for manager", "cn", cn, "addr", addr) - config.ServerName = cn + config, err = util.GetTLSConfigForCert(caCert) + if err != nil { + log.Error(err, "create transport", "get tls config from cert", cafile) + return nil, err } } else { thumbprint := cluster.getThumbprint(addr) diff --git a/pkg/nsx/util/utils.go b/pkg/nsx/util/utils.go index f7de7fa4c..7480254c6 100644 --- a/pkg/nsx/util/utils.go +++ b/pkg/nsx/util/utils.go @@ -4,8 +4,10 @@ package util import ( + "bytes" "crypto/sha1" "crypto/sha256" + "crypto/tls" "crypto/x509" "encoding/hex" "encoding/json" @@ -392,31 +394,54 @@ func VerifyNsxCertWithThumbprint(der []byte, thumbprint string) error { return err } -// GetCommonNameFromLeafCert returns the common name of the first (leaf) cert of -// provided pemCerts. If CA cert is passed, empty CN will be returned. +// GetTLSConfigForCert returns TLS config based on given pemCerts. +// If CA cert is passed, TLS config will do native cert check for connection. +// Otherwise, exact byte-to-byte check will be performed. // Error is returned if pem invalid or not a certificate. -func GetCommonNameFromLeafCert(pemCerts []byte) (string, error) { +func GetTLSConfigForCert(pemCerts []byte) (*tls.Config, error) { block, _ := pem.Decode(pemCerts) if block == nil { err := errors.New("decode ca file fail") - log.Error(err, "failed to get CN from cert", "pem", pemCerts) - return "", err + log.Error(err, "failed to decode cert", "pem", pemCerts) + return nil, err } if block.Type != "CERTIFICATE" || len(block.Headers) != 0 { err := errors.New("pem not certificate or header not found") - log.Error(err, "failed to get CN from cert", "pem", pemCerts) - return "", err + log.Error(err, "failed to decode cert", "pem", pemCerts) + return nil, err } cert, err := x509.ParseCertificate(block.Bytes) if err != nil { - log.Error(err, "failed to get CN from cert", "pem", pemCerts) - return "", err + log.Error(err, "failed to decode cert", "pem", pemCerts) + return nil, err } + // Native cert verification in case of CA cert if cert.IsCA { - return "", nil + log.Info("configured CA cert", "subject", cert.Subject) + certPool := x509.NewCertPool() + certPool.AddCert(cert) + return &tls.Config{RootCAs: certPool}, nil + } + + // Exact pem matching for leaf certs (certificate pinning) + config := &tls.Config{ + InsecureSkipVerify: true, + VerifyConnection: func(cs tls.ConnectionState) error { + if cs.PeerCertificates == nil || cs.PeerCertificates[0] == nil { + err := errors.New("server didn't present cert") + log.Error(err, "verify cert") + return err + } + if !bytes.Equal(cs.PeerCertificates[0].Raw, cert.Raw) { + err := errors.New("server certificate didn't match pinned leaf cert") + log.Error(err, "verify cert") + return err + } + return nil + }, } - - return cert.Subject.CommonName, nil + log.Info("configured cert pining", "subject", cert.Subject) + return config, nil } diff --git a/pkg/nsx/util/utils_test.go b/pkg/nsx/util/utils_test.go index 73b2171b4..e5de151ad 100644 --- a/pkg/nsx/util/utils_test.go +++ b/pkg/nsx/util/utils_test.go @@ -5,7 +5,10 @@ package util import ( "bytes" + "crypto/tls" + "crypto/x509" "encoding/json" + "encoding/pem" "errors" "fmt" "io" @@ -244,15 +247,15 @@ func TestVerifyNsxCertWithThumbprint(t *testing.T) { } } -func TestGetCommonNameFromCert(t *testing.T) { +func TestGetTLSConfigForCert(t *testing.T) { tests := []struct { name string pem []byte - cn string + isCA bool wantErr bool }{ { - name: "One cert", + name: "One leaf cert", pem: []byte(` -----BEGIN CERTIFICATE----- MIID5DCCAsygAwIBAgIJAIJaVMN4AJHVMA0GCSqGSIb3DQEBCwUAMIGIMTQwMgYD @@ -278,7 +281,7 @@ XkMSQJYdYDsUkiu98jNxh+oT8Cqdruwtg73pw8pP17EPltBABlHkYOEznw3dgDH3 jSy6ts7e8AND6YWulG9jLmrI1xWwjbVqAoapxJQeSRYQ6Wb/KODPlg== -----END CERTIFICATE----- `), - cn: "nsxmanager-ob-22945368-1-dev-integ-nsx-9389", + isCA: false, wantErr: false, }, { @@ -378,7 +381,7 @@ nAuknZoh8/CbCzB428Hch0P+vGOaysXCHMnHjf87ElgI5rY97HosTvuDls4MPGmH VHOkc8KT/1EQrBVUAdj8BbGJoX90g5pJ19xOe4pIb4tF9g== -----END CERTIFICATE----- `), - cn: "nsxManager.sddc-10-215-208-250.vmwarevmc.com", + isCA: false, wantErr: false, }, { @@ -415,7 +418,7 @@ exCdtTix9qrKgWRs6PLigVWXUX/hwidQosk8WwBD9lu51aX8/wdQQGcHsFXwt35u Lcw= -----END CERTIFICATE----- `), - cn: "", + isCA: true, wantErr: false, }, { @@ -426,11 +429,26 @@ Lcw= } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cn, err := GetCommonNameFromLeafCert(tt.pem) + config, err := GetTLSConfigForCert(tt.pem) if tt.wantErr { - assert.Error(t, err, "GetCommonNameFromCert expected err returned") + assert.Error(t, err, "GetTLSConfigForCert expected err returned") + return + } + + if tt.isCA { + assert.False(t, config.InsecureSkipVerify) + expected := x509.NewCertPool() + expected.AppendCertsFromPEM(tt.pem) + assert.True(t, config.RootCAs.Equal(expected)) } else { - assert.Equal(t, tt.cn, cn) + assert.True(t, config.InsecureSkipVerify) + assert.Nil(t, config.RootCAs) + assert.NotNil(t, config.VerifyConnection) + certDer, _ := pem.Decode(tt.pem) + cert, _ := x509.ParseCertificate(certDer.Bytes) + assert.NoError(t, config.VerifyConnection(tls.ConnectionState{ + PeerCertificates: []*x509.Certificate{cert}, + })) } }) }