From 62580ff12920dbc0e16c5c9db5f2aad17ecabfcc Mon Sep 17 00:00:00 2001 From: Anderson Queiroz Date: Wed, 23 Oct 2024 10:38:49 +0200 Subject: [PATCH] tlscommon: do not match host on server side verification mode 'full' --- transport/tlscommon/tls_config.go | 40 +++++++++----------------- transport/tlscommon/tls_config_test.go | 7 +++-- 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/transport/tlscommon/tls_config.go b/transport/tlscommon/tls_config.go index 424a38f9..09a2c8d9 100644 --- a/transport/tlscommon/tls_config.go +++ b/transport/tlscommon/tls_config.go @@ -113,7 +113,7 @@ func (c *TLSConfig) ToConfig() *tls.Config { Certificates: c.Certificates, RootCAs: c.RootCAs, ClientCAs: c.ClientCAs, - InsecureSkipVerify: insecure, //nolint: gosec // we are using our own verification for now + InsecureSkipVerify: insecure, //nolint:gosec // we are using our own verification for now CipherSuites: convCipherSuites(c.CipherSuites), CurvePreferences: c.CurvePreferences, Renegotiation: c.Renegotiation, @@ -123,13 +123,13 @@ func (c *TLSConfig) ToConfig() *tls.Config { } } -// BuildModuleConfig takes the TLSConfig and transform it into a `tls.Config`. +// BuildModuleClientConfig takes the TLSConfig and transform it into a `tls.Config`. func (c *TLSConfig) BuildModuleClientConfig(host string) *tls.Config { if c == nil { // use default TLS settings, if config is empty. return &tls.Config{ ServerName: host, - InsecureSkipVerify: true, //nolint: gosec // we are using our own verification for now + InsecureSkipVerify: true, //nolint:gosec // we are using our own verification for now VerifyConnection: makeVerifyConnection(&TLSConfig{ Verification: VerifyFull, ServerName: host, @@ -154,15 +154,16 @@ func (c *TLSConfig) BuildModuleClientConfig(host string) *tls.Config { return config } -// BuildServerConfig takes the TLSConfig and transform it into a `tls.Config` for server side objects. +// BuildServerConfig takes the TLSConfig and transform it into a `tls.Config` +// for server side connections. func (c *TLSConfig) BuildServerConfig(host string) *tls.Config { if c == nil { // use default TLS settings, if config is empty. return &tls.Config{ ServerName: host, - InsecureSkipVerify: true, //nolint: gosec // we are using our own verification for now + InsecureSkipVerify: true, //nolint:gosec // we are using our own verification for now VerifyConnection: makeVerifyServerConnection(&TLSConfig{ - Verification: VerifyFull, + Verification: VerifyCertificate, ServerName: host, }), } @@ -285,27 +286,13 @@ func makeVerifyConnection(cfg *TLSConfig) func(tls.ConnectionState) error { func makeVerifyServerConnection(cfg *TLSConfig) func(tls.ConnectionState) error { switch cfg.Verification { - case VerifyFull: - return func(cs tls.ConnectionState) error { - if len(cs.PeerCertificates) == 0 { - if cfg.ClientAuth == tls.RequireAndVerifyClientCert { - return ErrMissingPeerCertificate - } - return nil - } - opts := x509.VerifyOptions{ - Roots: cfg.ClientCAs, - Intermediates: x509.NewCertPool(), - KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, - } - err := verifyCertsWithOpts(cs.PeerCertificates, cfg.CASha256, opts) - if err != nil { - return err - } - return verifyHostname(cs.PeerCertificates[0], cs.ServerName) - } - case VerifyCertificate: + // VerifyFull would attempt to match 'host' (c.ServerName) that is the host + // the client is trying to connect to with a DNS, IP or the CN from the + // client's certificate. Such validation, besides making no sense on the + // server side also causes errors as the client certificate usually does not + // contain a DNS, IP or CN matching the server's hostname. + case VerifyFull, VerifyCertificate: return func(cs tls.ConnectionState) error { if len(cs.PeerCertificates) == 0 { if cfg.ClientAuth == tls.RequireAndVerifyClientCert { @@ -331,7 +318,6 @@ func makeVerifyServerConnection(cfg *TLSConfig) func(tls.ConnectionState) error } return nil - } func verifyCertsWithOpts(certs []*x509.Certificate, casha256 []string, opts x509.VerifyOptions) error { diff --git a/transport/tlscommon/tls_config_test.go b/transport/tlscommon/tls_config_test.go index 07bb6327..c2bcf2cb 100644 --- a/transport/tlscommon/tls_config_test.go +++ b/transport/tlscommon/tls_config_test.go @@ -71,14 +71,14 @@ func TestMakeVerifyServerConnection(t *testing.T) { expectedCallback: true, expectedError: x509.CertificateInvalidError{Cert: testCerts["expired"], Reason: x509.Expired}, }, - "default verification with certificates when required with incorrect server name in cert": { + "default verification with certificates when required do not verify hostname": { verificationMode: VerifyFull, clientAuth: tls.RequireAndVerifyClientCert, certAuthorities: certPool, peerCerts: []*x509.Certificate{testCerts["correct"]}, - serverName: "bad.example.com", + serverName: "some.example.com", expectedCallback: true, - expectedError: x509.HostnameError{Certificate: testCerts["correct"], Host: "bad.example.com"}, + expectedError: nil, }, "default verification with certificates when required with correct cert": { verificationMode: VerifyFull, @@ -257,6 +257,7 @@ func TestTrustRootCA(t *testing.T) { // we want to know the number of certificates in the CertPool (RootCAs), as it is not // directly available, we use this workaround of reading the number of subjects in the pool. + // nolint:staticcheck // we do not expect the system root CAs. if got, expected := len(cfg.RootCAs.Subjects()), tc.expectedRootCAsLen; got != expected { t.Fatalf("expecting cfg.RootCAs to have %d element, got %d instead", expected, got) }