From c72f9014b7ed12a67f01900c61e4fc0022cabab9 Mon Sep 17 00:00:00 2001 From: Scott Miller Date: Mon, 7 Jul 2025 10:49:27 -0500 Subject: [PATCH] Test cert equality when matching a non-CA certificate (#31210) * Test cert equality when matching a non-CA certificate * changelog --- builtin/credential/cert/backend_test.go | 147 +++++++++++++++++- builtin/credential/cert/path_login.go | 19 +-- builtin/credential/cert/path_login_test.go | 12 +- .../cert/test-fixtures/keys/cert-alt-cn.pem | 19 +++ changelog/31210.txt | 3 + 5 files changed, 178 insertions(+), 22 deletions(-) create mode 100644 builtin/credential/cert/test-fixtures/keys/cert-alt-cn.pem create mode 100644 changelog/31210.txt diff --git a/builtin/credential/cert/backend_test.go b/builtin/credential/cert/backend_test.go index f86cbb002e..40794e4930 100644 --- a/builtin/credential/cert/backend_test.go +++ b/builtin/credential/cert/backend_test.go @@ -64,7 +64,7 @@ const ( testRootCertCRL = "test-fixtures/cacert2crl" ) -func generateTestCertAndConnState(t *testing.T, template *x509.Certificate) (string, tls.ConnectionState, error) { +func generateTestCertAndConnState(t *testing.T, template *x509.Certificate, caKey *ecdsa.PrivateKey) (string, tls.ConnectionState, error) { t.Helper() tempDir, err := ioutil.TempDir("", "vault-cert-auth-test-") if err != nil { @@ -84,7 +84,10 @@ func generateTestCertAndConnState(t *testing.T, template *x509.Certificate) (str BasicConstraintsValid: true, IsCA: true, } - caKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + + if caKey == nil { + caKey, err = ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + } if err != nil { t.Fatal(err) } @@ -1341,7 +1344,7 @@ func TestBackend_dns_singleCert(t *testing.T) { NotAfter: time.Now().Add(262980 * time.Hour), } - tempDir, connState, err := generateTestCertAndConnState(t, certTemplate) + tempDir, connState, err := generateTestCertAndConnState(t, certTemplate, nil) if tempDir != "" { defer os.RemoveAll(tempDir) } @@ -1388,7 +1391,7 @@ func TestBackend_email_singleCert(t *testing.T) { NotAfter: time.Now().Add(262980 * time.Hour), } - tempDir, connState, err := generateTestCertAndConnState(t, certTemplate) + tempDir, connState, err := generateTestCertAndConnState(t, certTemplate, nil) if tempDir != "" { defer os.RemoveAll(tempDir) } @@ -1469,7 +1472,7 @@ func TestBackend_uri_singleCert(t *testing.T) { NotAfter: time.Now().Add(262980 * time.Hour), } - tempDir, connState, err := generateTestCertAndConnState(t, certTemplate) + tempDir, connState, err := generateTestCertAndConnState(t, certTemplate, nil) if tempDir != "" { defer os.RemoveAll(tempDir) } @@ -1497,6 +1500,58 @@ func TestBackend_uri_singleCert(t *testing.T) { }) } +// Test a self-signed client with URI alt names (root CA) that is trusted +func TestBackend_sameKey_differentCN(t *testing.T) { + certTemplate := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "example.com", + }, + DNSNames: []string{"example.com"}, + ExtKeyUsage: []x509.ExtKeyUsage{ + x509.ExtKeyUsageServerAuth, + x509.ExtKeyUsageClientAuth, + }, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment | x509.KeyUsageKeyAgreement, + SerialNumber: big.NewInt(mathrand.Int63()), + NotBefore: time.Now().Add(-30 * time.Second), + NotAfter: time.Now().Add(262980 * time.Hour), + } + caKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal("error generating cert keypair") + } + tempDir1, connState1, err := generateTestCertAndConnState(t, certTemplate, caKey) + if tempDir1 != "" { + defer os.RemoveAll(tempDir1) + } + cert1, err := ioutil.ReadFile(filepath.Join(tempDir1, "cert.pem")) + if err != nil { + t.Fatalf("err: %v", err) + } + + certTemplate.Subject.CommonName = "notfake.com" + tempDir2, connState2, err := generateTestCertAndConnState(t, certTemplate, caKey) + if tempDir2 != "" { + defer os.RemoveAll(tempDir2) + } + if err != nil { + t.Fatalf("error testing connection state: %v", err) + } + + logicaltest.Test(t, logicaltest.TestCase{ + CredentialBackend: testFactory(t), + Steps: []logicaltest.TestStep{ + testAccStepCert(t, "web", cert1, "foo", allowed{common_names: "example.com"}, false), + testAccStepLogin(t, connState1), + testAccStepLoginInvalid(t, connState2), + testAccStepCert(t, "web", cert1, "foo", allowed{}, false), + testAccStepLogin(t, connState1), + testAccStepLoginInvalid(t, connState2), + }, + }) +} + // Test against a collection of matching and non-matching rules func TestBackend_mixed_constraints(t *testing.T) { connState, err := testConnState("test-fixtures/keys/cert.pem", @@ -1537,6 +1592,88 @@ func TestBackend_untrusted(t *testing.T) { }) } +// Test a forged CN +func TestBackend_different_cn(t *testing.T) { + config := logical.TestBackendConfig() + storage := &logical.InmemStorage{} + config.StorageView = storage + + b, err := Factory(context.Background(), config) + if err != nil { + t.Fatal(err) + } + + connState, err := testConnState("test-fixtures/keys/cert.pem", + "test-fixtures/keys/key.pem", "test-fixtures/root/rootcacert.pem") + if err != nil { + t.Fatalf("error testing connection state: %v", err) + } + connState2, err := testConnState("test-fixtures/keys/cert-alt-cn.pem", + "test-fixtures/keys/key.pem", "test-fixtures/root/rootcacert.pem") + if err != nil { + t.Fatalf("error testing connection state: %v", err) + } + ca, err := ioutil.ReadFile("test-fixtures/keys/cert.pem") + if err != nil { + t.Fatalf("err: %v", err) + } + + name := "web" + + addCertReq := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "certs/" + name, + Data: map[string]interface{}{ + "certificate": string(ca), + "policies": "foo", + "display_name": name, + "lease": 1000, + }, + Storage: storage, + Connection: &logical.Connection{ConnState: &connState}, + } + + _, err = b.HandleRequest(context.Background(), addCertReq) + if err != nil { + t.Fatal(err) + } + + loginReq := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "login", + Unauthenticated: true, + Data: map[string]interface{}{ + "name": name, + }, + Storage: storage, + Connection: &logical.Connection{ConnState: &connState}, + } + + resp, err := b.HandleRequest(context.Background(), loginReq) + if err != nil { + t.Fatal(err.Error()) + } + if resp.IsError() { + t.Fatal(resp.Error()) + } + + loginReq = &logical.Request{ + Operation: logical.UpdateOperation, + Path: "login", + Unauthenticated: true, + Data: map[string]interface{}{ + "name": name, + }, + Storage: storage, + Connection: &logical.Connection{ConnState: &connState2}, + } + + resp, err = b.HandleRequest(context.Background(), loginReq) + if err == nil && !resp.IsError() { + t.Fatal("expected error") + } +} + func TestBackend_validCIDR(t *testing.T) { config := logical.TestBackendConfig() storage := &logical.InmemStorage{} diff --git a/builtin/credential/cert/path_login.go b/builtin/credential/cert/path_login.go index d770b1b9da..6826d3c7f4 100644 --- a/builtin/credential/cert/path_login.go +++ b/builtin/credential/cert/path_login.go @@ -4,7 +4,6 @@ package cert import ( - "bytes" "context" "crypto/tls" "crypto/x509" @@ -339,18 +338,16 @@ func (b *backend) verifyCredentials(ctx context.Context, req *logical.Request, d for _, trustedNonCA := range trustedNonCAs { tCert := trustedNonCA.Certificates[0] // Check for client cert being explicitly listed in the config (and matching other constraints) - if tCert.SerialNumber.Cmp(clientCert.SerialNumber) == 0 && - bytes.Equal(tCert.AuthorityKeyId, clientCert.AuthorityKeyId) { - pkMatch, err := certutil.ComparePublicKeysAndType(tCert.PublicKey, clientCert.PublicKey) - if err != nil { - return nil, nil, err - } - if !pkMatch { - // Someone may be trying to pass off a forged certificate as the trusted non-CA cert. Reject early. - return nil, logical.ErrorResponse("public key mismatch of a trusted leaf certificate"), nil - } + if tCert.SerialNumber.Cmp(clientCert.SerialNumber) == 0 { matches, err := b.matchesConstraints(ctx, clientCert, trustedNonCA.Certificates, trustedNonCA, verifyConf) + if matches { + if !tCert.Equal(clientCert) { + // Someone may be trying to pass off a forged certificate as the trusted non-CA cert. Reject early. + return nil, logical.ErrorResponse("certificate mismatch of a trusted leaf certificate"), nil + } + } + // matchesConstraints returns an error when OCSP verification fails, // but some other path might still give us success. Add to the // retErr multierror, but avoid duplicates. This way, if we reach a diff --git a/builtin/credential/cert/path_login_test.go b/builtin/credential/cert/path_login_test.go index 9db8e7e6e1..a91c1370b5 100644 --- a/builtin/credential/cert/path_login_test.go +++ b/builtin/credential/cert/path_login_test.go @@ -75,7 +75,7 @@ func TestCert_RoleResolve(t *testing.T) { NotAfter: time.Now().Add(262980 * time.Hour), } - tempDir, connState, err := generateTestCertAndConnState(t, certTemplate) + tempDir, connState, err := generateTestCertAndConnState(t, certTemplate, nil) if tempDir != "" { defer os.RemoveAll(tempDir) } @@ -136,7 +136,7 @@ func TestCert_RoleResolveWithoutProvidingCertName(t *testing.T) { NotAfter: time.Now().Add(262980 * time.Hour), } - tempDir, connState, err := generateTestCertAndConnState(t, certTemplate) + tempDir, connState, err := generateTestCertAndConnState(t, certTemplate, nil) if tempDir != "" { defer os.RemoveAll(tempDir) } @@ -305,7 +305,7 @@ func TestCert_RoleResolve_RoleDoesNotExist(t *testing.T) { NotAfter: time.Now().Add(262980 * time.Hour), } - tempDir, connState, err := generateTestCertAndConnState(t, certTemplate) + tempDir, connState, err := generateTestCertAndConnState(t, certTemplate, nil) if tempDir != "" { defer os.RemoveAll(tempDir) } @@ -344,7 +344,7 @@ func TestCert_RoleResolveOCSP(t *testing.T) { NotAfter: time.Now().Add(262980 * time.Hour), OCSPServer: []string{fmt.Sprintf("http://localhost:%d", ocspPort)}, } - tempDir, connState, err := generateTestCertAndConnState(t, certTemplate) + tempDir, connState, err := generateTestCertAndConnState(t, certTemplate, nil) if tempDir != "" { defer os.RemoveAll(tempDir) } @@ -366,7 +366,7 @@ func TestCert_RoleResolveOCSP(t *testing.T) { t.Fatalf("err: %v", err) } - tempDir, connState2, err := generateTestCertAndConnState(t, certTemplate) + tempDir, connState2, err := generateTestCertAndConnState(t, certTemplate, nil) if err != nil { t.Fatalf("error testing connection state: %v", err) } @@ -485,7 +485,7 @@ func TestCert_MetadataOnFailure(t *testing.T) { NotAfter: time.Now().Add(262980 * time.Hour), } - tempDir, connState, err := generateTestCertAndConnState(t, certTemplate) + tempDir, connState, err := generateTestCertAndConnState(t, certTemplate, nil) if tempDir != "" { defer os.RemoveAll(tempDir) } diff --git a/builtin/credential/cert/test-fixtures/keys/cert-alt-cn.pem b/builtin/credential/cert/test-fixtures/keys/cert-alt-cn.pem new file mode 100644 index 0000000000..06975732de --- /dev/null +++ b/builtin/credential/cert/test-fixtures/keys/cert-alt-cn.pem @@ -0,0 +1,19 @@ +-----BEGIN CERTIFICATE----- +MIIDFjCCAf6gAwIBAgIJAJIiPq+77hewMA0GCSqGSIb3DQEBCwUAMBYxFDASBgNV +BAMTC2V4YW1wbGUuY29tMCAXDTI1MDYxODE5MzEzM1oYDzIwNTAwNjE5MTkzMTMz +WjAWMRQwEgYDVQQDDAtub3RmYWtlLmNvbTCCASIwDQYJKoZIhvcNAQEBBQADggEP +ADCCAQoCggEBALGcdEr6/NmCaRaSMuHyTVuXygOgfJocUk0QFZ8oAH7XrU/Kbh35 +WNLLO2Ol9jl47FND0Z/tT7MLYr4Sovni7YFpFBfXBWBRp0oDJXemVkYTYXmYQOmX +0JnjPwRdqWuRytkgxd+pY4Iy/U/p1q67tlL9AQqB8ywJomGDIiFqZYnTJjDuTU0R +t5PtwDdE5H2pAeiVc4yxoU3F3TnFRDF5HkFaiTALhqLlOXaJ8qrZ2h7iCRrlHcue +GA51/OrcYTkIQL+q+3R182cUuKZi78PFCNa7En/xi3FFRgfufgABheYrhwtzdg05 +eMc9xXDeAm+MPaW2kO/pgs8/mp6oiX8q9p8CAwEAAaNlMGMwIQYDVR0RBBowGIIQ +Y2VydC5leGFtcGxlLmNvbYcEfwAAATAdBgNVHQ4EFgQUmuMiWgeoyPlkEyXcJ7Og +qJotlJkwHwYDVR0jBBgwFoAUncSzT/6HMexyuiU9/7EgHu+ok5swDQYJKoZIhvcN +AQELBQADggEBANYE2poNMZakvN2a8hGbVHNXCR4+j5dE/nIw0ZW2PPv4CuNJ3whN +G15MT790k9DiNNecJBP32jwAOedIWKr3aqasur4eXA8xhN1nbtJODWxXYaV3USs7 +C1Mu9DV3aRiyFhEv6Va6RL4gCnOqOZdXG1+dAkughBKrRNJAMo5Wop/VC8rkSssO +NpQBt7ROIJjyTXrHQq23HR/uVv511AE5hDMDzxaVbsslBuzhD4nHOJ/Y/LWcXHJ1 +8W6S1vgSES2lo8U5tthSE4WyYW6kH6r2hBrK4ESdpdAUfO/lVNCe5KeYYWRwzMOw +oj9OOxBsjSX5C368IWM4sAn08mYJUUmq3i4= +-----END CERTIFICATE----- diff --git a/changelog/31210.txt b/changelog/31210.txt new file mode 100644 index 0000000000..3ffa7cd6a6 --- /dev/null +++ b/changelog/31210.txt @@ -0,0 +1,3 @@ +```release-note:improvement +auth/cert: test non-CA cert equality on login matching instead of individual fields. +``` \ No newline at end of file