From 09fc8da8a0cd132becb105fe4cd5ab188179c684 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20R=C3=BCger?= Date: Wed, 2 Sep 2026 12:10:51 +0200 Subject: [PATCH] web: fix panic when client_allowed_sans is used with an optional client cert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VerifyPeerCertificate indexed rawCerts[0] without checking the length. crypto/tls calls VerifyPeerCertificate even when the client sent no certificate, as long as the client auth type does not require one, so combining client_allowed_sans with RequestClientCert or VerifyClientCertIfGiven panicked on every certificate-less client. The panic is recovered by net/http's per-connection handler, so the process survives, but the connection is aborted with a stack trace in the log instead of a clean TLS alert. Return an error for an empty rawCerts slice, and reject the combination at configuration load time: client_allowed_sans is only meaningful when the client is required to present a certificate. With NoClientCert the callback is never invoked at all, so the configured SANs were silently ignored, which is the more dangerous half of this. All existing client_allowed_sans test fixtures use RequireAndVerifyClientCert, which is why this went unnoticed. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Manuel Rüger --- docs/web-configuration.md | 4 + ...ig_auth_client_san_no_client_auth.bad.yaml | 5 + ...fig_auth_client_san_optional_cert.bad.yaml | 7 ++ web/tls_config.go | 20 +++- web/tls_config_test.go | 100 ++++++++++++++++++ 5 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 web/testdata/web_config_auth_client_san_no_client_auth.bad.yaml create mode 100644 web/testdata/web_config_auth_client_san_optional_cert.bad.yaml diff --git a/docs/web-configuration.md b/docs/web-configuration.md index 7e996c70..a2bc1f18 100644 --- a/docs/web-configuration.md +++ b/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: [ - ] ] diff --git a/web/testdata/web_config_auth_client_san_no_client_auth.bad.yaml b/web/testdata/web_config_auth_client_san_no_client_auth.bad.yaml new file mode 100644 index 00000000..05888cb9 --- /dev/null +++ b/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" diff --git a/web/testdata/web_config_auth_client_san_optional_cert.bad.yaml b/web/testdata/web_config_auth_client_san_optional_cert.bad.yaml new file mode 100644 index 00000000..185f4cab --- /dev/null +++ b/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" diff --git a/web/tls_config.go b/web/tls_config.go index 022a839e..0e84cd9b 100644 --- a/web/tls_config.go +++ b/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 } diff --git a/web/tls_config_test.go b/web/tls_config_test.go index ea0333d9..00dd7ca3 100644 --- a/web/tls_config_test.go +++ b/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) + } +}