web: fix panic when client_allowed_sans is used with an optional client certificate

docs/web-configuration.md

@@ -61,6 +61,10 @@ tls_server_config:
   # which is an exact match to an entry in this list, else terminate the
   # connection. SAN match can be one or multiple of the following: DNS,
   # IP, e-mail, or URI address from https://pkg.go.dev/crypto/x509#Certificate.
+  #
+  # NOTE: This requires client_auth_type to be either RequireAnyClientCert or
+  # RequireAndVerifyClientCert. The other client auth types let a client
+  # connect without sending a certificate, leaving no SAN to match against.
   [ client_allowed_sans:
     [ - <string> ] ]
 

web/testdata/web_config_auth_client_san_no_client_auth.bad.yaml

@@ -0,0 +1,5 @@
+tls_server_config:
+  cert_file: "server.crt"
+  key_file: "server.key"
+  client_allowed_sans:
+    - "one"

web/testdata/web_config_auth_client_san_optional_cert.bad.yaml

@@ -0,0 +1,7 @@
+tls_server_config:
+  cert_file: "server.crt"
+  key_file: "server.key"
+  client_auth_type: "VerifyClientCertIfGiven"
+  client_ca_file: "client2_selfsigned.pem"
+  client_allowed_sans:
+    - "one"

web/tls_config.go

@@ -124,6 +124,13 @@ func (t *TLSConfig) SetDirectory(dir string) {
 
 // VerifyPeerCertificate will check the SAN entries of the client cert if there is configuration for it
 func (t *TLSConfig) VerifyPeerCertificate(rawCerts [][]byte, _ [][]*x509.Certificate) error {
+	// crypto/tls calls VerifyPeerCertificate with an empty slice when the
+	// client declines to send a certificate and the client auth type does not
+	// require one.
+	if len(rawCerts) == 0 {
+		return errors.New("client did not present a certificate to match against client_allowed_sans")
+	}
+
 	// sender cert comes first, see https://www.rfc-editor.org/rfc/rfc5246#section-7.4.2
 	cert, err := x509.ParseCertificate(rawCerts[0])
 	if err != nil {
@@ -286,11 +293,6 @@ func ConfigToTLSConfig(c *TLSConfig) (*tls.Config, error) {
 		cfg.ClientCAs = clientCAPool
 	}
 
-	if c.ClientAllowedSans != nil {
-		// verify that the client cert contains an allowed SAN
-		cfg.VerifyPeerCertificate = c.VerifyPeerCertificate
-	}
-
 	switch c.ClientAuth {
 	case "RequestClientCert":
 		cfg.ClientAuth = tls.RequestClientCert
@@ -310,6 +312,14 @@ func ConfigToTLSConfig(c *TLSConfig) (*tls.Config, error) {
 		return nil, errors.New("client CA's have been configured without a Client Auth Policy")
 	}
 
+	if c.ClientAllowedSans != nil {
+		if cfg.ClientAuth != tls.RequireAnyClientCert && cfg.ClientAuth != tls.RequireAndVerifyClientCert {
+			return nil, errors.New("client_allowed_sans requires client_auth_type to be one of RequireAnyClientCert or RequireAndVerifyClientCert")
+		}
+		// verify that the client cert contains an allowed SAN
+		cfg.VerifyPeerCertificate = c.VerifyPeerCertificate
+	}
+
 	return cfg, nil
 }
 

web/tls_config_test.go

@@ -66,6 +66,10 @@ var (
 		"Certificate required": regexp.MustCompile(`certificate required`),
 		"Unknown CA":           regexp.MustCompile(`unknown certificate authority`),
 		"Too Many Requests":    regexp.MustCompile(`Too Many Requests`),
+		"SANs without required client cert": regexp.MustCompile(
+			`client_allowed_sans requires client_auth_type`),
+		"No client certificate": regexp.MustCompile(
+			`client did not present a certificate to match against client_allowed_sans`),
 	}
 )
 
@@ -187,6 +191,16 @@ func TestYAMLFiles(t *testing.T) {
 			YAMLConfigPath: "testdata/web_config_noAuth_wrongTLSVersion.bad.yml",
 			ExpectedError:  ErrorMap["Unknown TLS version"],
 		},
+		{
+			Name:           `invalid config yml (client_allowed_sans with optional client cert)`,
+			YAMLConfigPath: "testdata/web_config_auth_client_san_optional_cert.bad.yaml",
+			ExpectedError:  ErrorMap["SANs without required client cert"],
+		},
+		{
+			Name:           `invalid config yml (client_allowed_sans without client auth)`,
+			YAMLConfigPath: "testdata/web_config_auth_client_san_no_client_auth.bad.yaml",
+			ExpectedError:  ErrorMap["SANs without required client cert"],
+		},
 	}
 	for _, testInputs := range testTables {
 		t.Run("run/"+testInputs.Name, testInputs.Test)
@@ -864,3 +878,89 @@ func (b *syncBuffer) String() string {
 	defer b.mu.Unlock()
 	return b.buf.String()
 }
+
+// TestVerifyPeerCertificateWithoutClientCert checks that an empty rawCerts is an
+// error rather than a panic.
+func TestVerifyPeerCertificateWithoutClientCert(t *testing.T) {
+	c := &TLSConfig{ClientAllowedSans: []string{"one"}}
+
+	for _, tc := range []struct {
+		name     string
+		rawCerts [][]byte
+	}{
+		{name: "nil", rawCerts: nil},
+		{name: "empty", rawCerts: [][]byte{}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			err := c.VerifyPeerCertificate(tc.rawCerts, nil)
+			if err == nil {
+				t.Fatal("expected an error, got nil")
+			}
+			if !ErrorMap["No client certificate"].MatchString(err.Error()) {
+				t.Fatalf("unexpected error: %v", err)
+			}
+		})
+	}
+}
+
+// TestVerifyPeerCertificateHandshakeWithoutClientCert checks that the server
+// rejects a client that presents no certificate without panicking.
+func TestVerifyPeerCertificateHandshakeWithoutClientCert(t *testing.T) {
+	cert, err := tls.LoadX509KeyPair("testdata/server.crt", "testdata/server.key")
+	if err != nil {
+		t.Fatalf("loading server key pair: %v", err)
+	}
+
+	tlsConfig := &TLSConfig{ClientAllowedSans: []string{"one"}}
+	listener, err := tls.Listen("tcp", "127.0.0.1:0", &tls.Config{
+		Certificates: []tls.Certificate{cert},
+		// Tolerates a client that sends no certificate, leaving rawCerts empty.
+		ClientAuth:            tls.VerifyClientCertIfGiven,
+		VerifyPeerCertificate: tlsConfig.VerifyPeerCertificate,
+		MinVersion:            tls.VersionTLS12,
+	})
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	t.Cleanup(func() { _ = listener.Close() })
+
+	type result struct {
+		err     error
+		recover any
+	}
+	results := make(chan result, 1)
+	go func() {
+		var r result
+		defer func() {
+			r.recover = recover()
+			results <- r
+		}()
+		conn, err := listener.Accept()
+		if err != nil {
+			r.err = err
+			return
+		}
+		defer conn.Close()
+		r.err = conn.(*tls.Conn).Handshake()
+	}()
+
+	// Under TLS 1.3 the dial can succeed before the server rejects the
+	// handshake, so only the server side is checked.
+	clientConfig := getTLSClient("").Transport.(*http.Transport).TLSClientConfig
+	clientConfig.ServerName = "localhost"
+	clientConn, err := tls.Dial("tcp", listener.Addr().String(), clientConfig)
+	if err == nil {
+		_ = clientConn.Close()
+	}
+
+	r := <-results
+	if r.recover != nil {
+		t.Fatalf("server panicked during the handshake: %v", r.recover)
+	}
+	if r.err == nil {
+		t.Fatal("expected the server to reject a client that sent no certificate")
+	}
+	if !ErrorMap["No client certificate"].MatchString(r.err.Error()) {
+		t.Fatalf("unexpected server error: %v", r.err)
+	}
+}