tlscommon: do not match host on server side verification mode 'full' (#…

…242)
elastic · Oct 24, 2024 · 4babafd · 4babafd
1 parent 0e8760f
commit 4babafd
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 31 deletions.
diff --git 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
@@ -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)
 				}
@@ -763,10 +764,11 @@ func genTestCerts(t *testing.T) map[string]*x509.Certificate {
 		// We write the certificate to disk, so if the test fails the certs can
 		// be inspected/reused
 		certPEM := new(bytes.Buffer)
-		pem.Encode(certPEM, &pem.Block{
+		err = pem.Encode(certPEM, &pem.Block{
 			Type:  "CERTIFICATE",
 			Bytes: cert.Leaf.Raw,
 		})
+		require.NoErrorf(t, err, "failed to encode certificste to PEM")
 
 		serverCertFile, err := os.Create(filepath.Join(tmpDir, certName+".crt"))
 		if err != nil {