Test cert equality when matching a non-CA certificate (#31210)

* Test cert equality when matching a non-CA certificate * changelog
2026-02-03 20:40:45 -05:00 · 2025-07-07 10:49:27 -05:00 · 2025-07-07 10:49:27 -05:00 · c72f9014b7
commit c72f9014b7
parent e9b04fc0df
5 changed files with 178 additions and 22 deletions
--- 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{}
--- 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
--- 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)
 	}
--- a/builtin/credential/cert/test-fixtures/keys/cert-alt-cn.pem
+++ 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-----
--- a/changelog/31210.txt
+++ 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.
+```