diff --git a/modules/caddytls/connpolicy.go b/modules/caddytls/connpolicy.go index 182bed2cd..38bc25db0 100644 --- a/modules/caddytls/connpolicy.go +++ b/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() diff --git a/modules/caddytls/connpolicy_verifyconnection_test.go b/modules/caddytls/connpolicy_verifyconnection_test.go index 8069b9377..f1822898a 100644 --- a/modules/caddytls/connpolicy_verifyconnection_test.go +++ b/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 {