caddytls: require a trust pool for client cert verification modes (#8394)

* caddytls: require a trust pool for client cert verification modes

Reject verify_if_given and require_and_verify without a trust pool so they stop falling back to system roots.

* Apply suggestion from @francislavoie

Signed-off-by: Francis Lavoie <lavofr@gmail.com>

---------

Signed-off-by: Francis Lavoie <lavofr@gmail.com>
Co-authored-by: Francis Lavoie <lavofr@gmail.com>

modules/caddytls/connpolicy.go

@@ -653,9 +653,12 @@ type ClientAuthentication struct {
 	// `verify_if_given` | Ask clients for a certificate; allow even if there isn't one, but verify it if there is
 	// `require_and_verify` | Require clients to present a valid certificate that is verified
 	//
-	// The default mode is `require_and_verify` if any
-	// TrustedCACerts or TrustedCACertPEMFiles or TrustedLeafCerts
-	// are provided; otherwise, the default mode is `require`.
+	// The `verify_if_given` and `require_and_verify` modes need a
+	// trust pool (`ca`). To verify against the system's root
+	// certificates, use the `tls.ca_pool.source.system` module.
+	//
+	// The default mode is `require_and_verify` if a trust pool is
+	// provided; otherwise, the default mode is `require`.
 	Mode string `json:"mode,omitempty"`
 
 	existingVerifyPeerCert func([][]byte, [][]*x509.Certificate) error
@@ -672,7 +675,8 @@ type ClientAuthentication struct {
 //	}
 //
 // If `mode` is not provided, it defaults to `require_and_verify` if `trust_pool` is provided.
-// Otherwise, it defaults to `require`.
+// Otherwise, it defaults to `require`. The `verify_if_given` and `require_and_verify` modes
+// need a `trust_pool`; use `trust_pool system` to verify against the system roots.
 func (ca *ClientAuthentication) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 	for d.NextArg() {
 		// consume any tokens on the same line, if any.
@@ -878,7 +882,6 @@ func (clientauth *ClientAuthentication) ConfigureTLSConfig(cfg *tls.Config) erro
 		// otherwise, set a safe default mode
 		if len(clientauth.TrustedCACerts) > 0 ||
 			len(clientauth.TrustedCACertPEMFiles) > 0 ||
-			len(clientauth.TrustedLeafCerts) > 0 ||
 			clientauth.CARaw != nil || clientauth.ca != nil {
 			cfg.ClientAuth = tls.RequireAndVerifyClientCert
 		} else {
@@ -886,6 +889,12 @@ func (clientauth *ClientAuthentication) ConfigureTLSConfig(cfg *tls.Config) erro
 		}
 	}
 
+	// without ClientCAs, Go verifies client certs against the system roots
+	verifyMode := cfg.ClientAuth == tls.VerifyClientCertIfGiven || cfg.ClientAuth == tls.RequireAndVerifyClientCert
+	if verifyMode && clientauth.ca == nil {
+		return fmt.Errorf("client auth modes verify_if_given and require_and_verify need a trust pool; use the 'system' trust pool to verify against system roots")
+	}
+
 	// enforce CA verification by adding CA certs to the ClientCAs pool
 	if clientauth.ca != nil {
 		cfg.ClientCAs = clientauth.ca.CertPool()

modules/caddytls/connpolicy_verifyconnection_test.go

@@ -1,6 +1,7 @@
 package caddytls
 
 import (
+	"context"
 	"crypto"
 	"crypto/ecdsa"
 	"crypto/elliptic"
@@ -8,12 +9,15 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"crypto/x509/pkix"
+	"encoding/json"
 	"errors"
 	"math/big"
 	"net"
 	"reflect"
 	"testing"
 	"time"
+
+	"github.com/caddyserver/caddy/v2"
 )
 
 type testClientCertificateVerifier struct {
@@ -146,6 +150,96 @@ func TestClientAuthenticationModeWithLeafVerifier(t *testing.T) {
 	}
 }
 
+func TestClientAuthenticationVerifyModesNeedTrustPool(t *testing.T) {
+	ctx, cancel := caddy.NewContext(caddy.Context{Context: context.Background()})
+	defer cancel()
+
+	// verifiers are loaded by the connection policy, so set both fields like a provisioned config
+	verifiersRaw := []json.RawMessage{json.RawMessage(`{"verifier":"leaf"}`)}
+	verifiers := []ClientCertificateVerifier{LeafCertClientAuth{trustedLeafCerts: []*x509.Certificate{issueTestCert(t, nil, 0).Leaf}}}
+	customPool := json.RawMessage(`{"provider":"inline","trusted_ca_certs":["` + test_der_1 + `"]}`)
+	systemPool := json.RawMessage(`{"provider":"system"}`)
+
+	for i, tc := range []struct {
+		clientauth ClientAuthentication
+		want       tls.ClientAuthType
+		wantErr    bool
+	}{
+		{clientauth: ClientAuthentication{Mode: "verify_if_given"}, wantErr: true},
+		{clientauth: ClientAuthentication{Mode: "require_and_verify"}, wantErr: true},
+		{clientauth: ClientAuthentication{Mode: "require_and_verify", VerifiersRaw: verifiersRaw, verifiers: verifiers}, wantErr: true},
+		{clientauth: ClientAuthentication{Mode: "verify_if_given", CARaw: systemPool}, want: tls.VerifyClientCertIfGiven},
+		{clientauth: ClientAuthentication{Mode: "require_and_verify", CARaw: systemPool}, want: tls.RequireAndVerifyClientCert},
+		{clientauth: ClientAuthentication{Mode: "verify_if_given", CARaw: customPool}, want: tls.VerifyClientCertIfGiven},
+		{clientauth: ClientAuthentication{Mode: "require_and_verify", CARaw: customPool}, want: tls.RequireAndVerifyClientCert},
+		{clientauth: ClientAuthentication{CARaw: customPool}, want: tls.RequireAndVerifyClientCert},
+		{clientauth: ClientAuthentication{Mode: "request"}, want: tls.RequestClientCert},
+		{clientauth: ClientAuthentication{Mode: "require"}, want: tls.RequireAnyClientCert},
+		{clientauth: ClientAuthentication{VerifiersRaw: verifiersRaw, verifiers: verifiers}, want: tls.RequireAnyClientCert},
+		{clientauth: ClientAuthentication{TrustedLeafCerts: []string{test_der_1}}, want: tls.RequireAnyClientCert},
+	} {
+		if err := tc.clientauth.provision(ctx); err != nil {
+			t.Fatalf("Test %d: provisioning: %v", i, err)
+		}
+		cfg := new(tls.Config)
+		err := tc.clientauth.ConfigureTLSConfig(cfg)
+		if tc.wantErr {
+			if err == nil {
+				t.Errorf("Test %d: expected error but got none", i)
+			}
+			continue
+		}
+		if err != nil {
+			t.Errorf("Test %d: unexpected error: %v", i, err)
+			continue
+		}
+		if cfg.ClientAuth != tc.want {
+			t.Errorf("Test %d: expected %v, got %v", i, tc.want, cfg.ClientAuth)
+		}
+		if tc.clientauth.CARaw != nil && cfg.ClientCAs == nil {
+			t.Errorf("Test %d: expected ClientCAs to be set", i)
+		}
+	}
+}
+
+func TestClientAuthenticationVerifierOnlyHandshake(t *testing.T) {
+	ca := issueTestCert(t, nil, 0)
+	serverCert := issueTestCert(t, &ca, x509.ExtKeyUsageServerAuth)
+	trusted := issueTestCert(t, &ca, x509.ExtKeyUsageClientAuth)
+	untrusted := issueTestCert(t, &ca, x509.ExtKeyUsageClientAuth)
+
+	for _, tc := range []struct {
+		cert      *tls.Certificate
+		wantError bool
+	}{
+		{cert: nil, wantError: true},
+		{cert: &trusted, wantError: false},
+		{cert: &untrusted, wantError: true},
+	} {
+		// no mode and no trust pool, so only the verifier checks the cert
+		clientauth := &ClientAuthentication{
+			VerifiersRaw: []json.RawMessage{json.RawMessage(`{"verifier":"leaf"}`)},
+			verifiers:    []ClientCertificateVerifier{LeafCertClientAuth{trustedLeafCerts: []*x509.Certificate{trusted.Leaf}}},
+		}
+		serverConfig := &tls.Config{Certificates: []tls.Certificate{serverCert}}
+		if err := clientauth.ConfigureTLSConfig(serverConfig); err != nil {
+			t.Fatalf("configuring TLS: %v", err)
+		}
+		clientConfig := &tls.Config{InsecureSkipVerify: true}
+		if tc.cert != nil {
+			clientConfig.Certificates = []tls.Certificate{*tc.cert}
+		}
+
+		err := serverHandshake(t, serverConfig, clientConfig)
+		if tc.wantError && err == nil {
+			t.Errorf("cert %v: expected handshake error but got none", tc.cert != nil)
+		}
+		if !tc.wantError && err != nil {
+			t.Errorf("cert %v: unexpected handshake error: %v", tc.cert != nil, err)
+		}
+	}
+}
+
 // serverHandshake runs one TLS handshake over loopback TCP and returns
 // the server's handshake error.
 func serverHandshake(t *testing.T, serverConfig, clientConfig *tls.Config) error {