pomerium · kenjenkins · Aug 10, 2023 · Jul 18, 2023
@@ -12,7 +12,7 @@ import (
 	"github.com/pomerium/pomerium/internal/log"
 )
 
-var isValidClientCertificateCache, _ = lru.New2Q[[3]string, bool](100)
+var isValidClientCertificateCache, _ = lru.New2Q[[4]string, bool](100)
 
 func isValidClientCertificate(ca, crl string, certInfo ClientCertificateInfo) (bool, error) {
 	// when ca is the empty string, client certificates are not required
@@ -21,22 +21,24 @@ func isValidClientCertificate(ca, crl string, certInfo ClientCertificateInfo) (b
 	}
 
 	cert := certInfo.Leaf
+	intermediates := certInfo.Intermediates
 
 	if cert == "" {
 		return false, nil
 	}
 
-	cacheKey := [3]string{ca, crl, cert}
+	cacheKey := [4]string{ca, crl, cert, intermediates}
 
 	value, ok := isValidClientCertificateCache.Get(cacheKey)
 	if ok {
 		return value, nil
 	}
 
-	roots, err := parseCertificates([]byte(ca))
-	if err != nil {
-		return false, err
-	}
+	roots := x509.NewCertPool()
+	roots.AppendCertsFromPEM([]byte(ca))
+
+	intermediatesPool := x509.NewCertPool()
+	intermediatesPool.AppendCertsFromPEM([]byte(intermediates))
 
 	xcert, err := parseCertificate(cert)
 	if err != nil {
@@ -48,7 +50,7 @@ func isValidClientCertificate(ca, crl string, certInfo ClientCertificateInfo) (b
 		return false, err
 	}
 
-	verifyErr := verifyClientCertificate(xcert, roots, crls)
+	verifyErr := verifyClientCertificate(xcert, roots, intermediatesPool, crls)
 	valid := verifyErr == nil
 
 	if verifyErr != nil {
@@ -60,52 +62,73 @@ func isValidClientCertificate(ca, crl string, certInfo ClientCertificateInfo) (b
 	return valid, nil
 }
 
-var errCertificateRevoked = errors.New("certificate revoked")
-
 func verifyClientCertificate(
 	cert *x509.Certificate,
-	roots map[string]*x509.Certificate,
+	roots *x509.CertPool,
+	intermediates *x509.CertPool,
 	crls map[string]*x509.RevocationList,
 ) error {
-	rootPool := x509.NewCertPool()
-	for _, root := range roots {
-		rootPool.AddCert(root)
-	}
-
-	if _, err := cert.Verify(x509.VerifyOptions{
-		Roots:     rootPool,
-		KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
-	}); err != nil {
+	chains, err := cert.Verify(x509.VerifyOptions{
+		Roots:         roots,
+		Intermediates: intermediates,
+		KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
+	})
+	if err != nil {
 		return err
 	}
 
-	// Consult any CRL for the presented certificate's Issuer.
-	issuer := string(cert.RawIssuer)
-	crl := crls[issuer]
-	if crl == nil {
-		return nil
+	// At least one of the verified chains must also pass revocation checking.
+	err = errors.New("internal error: no verified chains")
+	for _, chain := range chains {
+		err = validateClientCertificateChain(chain, crls)
+		if err == nil {
+			return nil
+		}
 	}
 
-	// Do we have a corresponding trusted CA certificate?
-	root, ok := roots[issuer]
-	if !ok {
-		return fmt.Errorf("could not check CRL: no matching trusted CA for issuer %s",
-			cert.Issuer)
-	}
+	// Return an error from one of the chains that did not validate.
+	// (In the common case there will be at most one verified chain.)
+	return err
+}
 
-	// Is the CRL signature itself valid?
-	if err := crl.CheckSignatureFrom(root); err != nil {
-		return fmt.Errorf("could not check CRL for issuer %s: signature verification "+
-			"error: %w", cert.Issuer, err)
-	}
+func validateClientCertificateChain(
+	chain []*x509.Certificate,
+	crls map[string]*x509.RevocationList,
+) error {
+	// Consult CRLs for all CAs in the chain (that is, all certificates except
+	// for the first one). To match Envoy's behavior, if a CRL is provided for
+	// any CA in the chain, CRLs must be provided for all CAs in the chain (see
+	// https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/common.proto).
+	var anyIssuerHasCRL bool
+	var lastIssuerWithoutCRL *x509.Certificate
+	for i := 0; i < len(chain)-1; i++ {
+		cert, issuer := chain[i], chain[i+1]
+		crl := crls[string(issuer.RawSubject)]
+		if crl == nil {
+			lastIssuerWithoutCRL = issuer
+			continue
+		}
+
+		anyIssuerHasCRL = true
 
-	// Is the client certificate listed as revoked in this CRL?
-	for i := range crl.RevokedCertificates {
-		if cert.SerialNumber.Cmp(crl.RevokedCertificates[i].SerialNumber) == 0 {
-			return errCertificateRevoked
+		// Is the CRL signature itself valid?
+		if err := crl.CheckSignatureFrom(issuer); err != nil {
+			return fmt.Errorf("CRL signature verification failed for issuer %q: %w",
+				issuer.Subject, err)
+		}
+
+		// Is the certificate listed as revoked in the CRL?
+		for i := range crl.RevokedCertificates {
+			if cert.SerialNumber.Cmp(crl.RevokedCertificates[i].SerialNumber) == 0 {
+				return fmt.Errorf("certificate %q was revoked", cert.Subject)
+			}
 		}
 	}
 
+	if anyIssuerHasCRL && lastIssuerWithoutCRL != nil {
+		return fmt.Errorf("no CRL provided for issuer %q", lastIssuerWithoutCRL.Subject)
+	}
+
 	return nil
 }
 
@@ -120,25 +143,6 @@ func parseCertificate(pemStr string) (*x509.Certificate, error) {
 	return x509.ParseCertificate(block.Bytes)
 }
 
-func parseCertificates(certs []byte) (map[string]*x509.Certificate, error) {
-	m := make(map[string]*x509.Certificate)
-	for {
-		var block *pem.Block
-		block, certs = pem.Decode(certs)
-		if block == nil {
-			return m, nil
-		}
-		if block.Type != "CERTIFICATE" {
-			continue
-		}
-		cert, err := x509.ParseCertificate(block.Bytes)
-		if err != nil {
-			return nil, err
-		}
-		m[string(cert.RawSubject)] = cert
-	}
-}
-
 func parseCRLs(crl []byte) (map[string]*x509.RevocationList, error) {
 	m := make(map[string]*x509.RevocationList)
 	for {

@@ -14,60 +14,85 @@ import (
 const (
 	testCA = `
 -----BEGIN CERTIFICATE-----
-MIIBZzCCAQ6gAwIBAgICEAAwCgYIKoZIzj0EAwIwGjEYMBYGA1UEAxMPVHJ1c3Rl
-ZCBSb290IENBMCAYDzAwMDEwMTAxMDAwMDAwWhcNMzMwNzMxMTUzMzE5WjAaMRgw
+MIIBaTCCAQ6gAwIBAgICEAAwCgYIKoZIzj0EAwIwGjEYMBYGA1UEAxMPVHJ1c3Rl
+ZCBSb290IENBMCAYDzAwMDEwMTAxMDAwMDAwWhcNMzMwODA2MjExMzI0WjAaMRgw
 FgYDVQQDEw9UcnVzdGVkIFJvb3QgQ0EwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNC
-AARGMVCBvgbkVB3OPltnBHAy9s9rtog2rlnzZ4BKzPBbLEM0uPYTOZa0LLxSMtCj
-N+Bu3wfGPgHU6/pJ2uEky7/Uo0IwQDAOBgNVHQ8BAf8EBAMCAQYwDwYDVR0TAQH/
-BAUwAwEB/zAdBgNVHQ4EFgQUXep6D8FTP6+5ZdR/HjP3pYfmxkwwCgYIKoZIzj0E
-AwIDRwAwRAIgSS5J6ii/n0gf2/UAMFb+UVG8n0nb1dcBCG55fSlWlVECIENVK+X3
-6SfUhfYSVBvOdS08AzMVvOM7aZbWaY9UirIf
+AATcFCe6i6IqnevuUoR8nTrka8fikGYB3ciKfRyS0NUfm27MGsbuU2ribMYjhuz2
+K4/nU7A2hcu393JNKriXgwoyo0IwQDAOBgNVHQ8BAf8EBAMCAQYwDwYDVR0TAQH/
+BAUwAwEB/zAdBgNVHQ4EFgQU0SX0xv9f6gYAcLC926z+Bt9vTAMwCgYIKoZIzj0E
+AwIDSQAwRgIhAJIFP4r9Gbo0D2MM/fzPx5wtXsjH1IoQMpn0aw+G1WkmAiEAi56g
+gO7l3bJj1YZtBv3tEkZPzaZ+xL3Nllcjv1K12Ac=
 -----END CERTIFICATE-----
 `
 	testValidCert = `
 -----BEGIN CERTIFICATE-----
 MIIBYTCCAQigAwIBAgICEAEwCgYIKoZIzj0EAwIwGjEYMBYGA1UEAxMPVHJ1c3Rl
-ZCBSb290IENBMCAYDzAwMDEwMTAxMDAwMDAwWhcNMzMwNzMxMTUzMzE5WjAeMRww
+ZCBSb290IENBMCAYDzAwMDEwMTAxMDAwMDAwWhcNMzMwODA2MjExMzI0WjAeMRww
 GgYDVQQDExN0cnVzdGVkIGNsaWVudCBjZXJ0MFkwEwYHKoZIzj0CAQYIKoZIzj0D
-AQcDQgAEfAYP3ZwiKJgk9zXpR/CMHYlAxjweJaMJihIS2FTA5gb0xBcTEe5AGpNF
-CHWPk4YCB25VeHg9GmY9Q1+qDD1hdqM4MDYwEwYDVR0lBAwwCgYIKwYBBQUHAwIw
-HwYDVR0jBBgwFoAUXep6D8FTP6+5ZdR/HjP3pYfmxkwwCgYIKoZIzj0EAwIDRwAw
-RAIgProROtxpvKS/qjrjonSvacnhdU0JwoXj2DgYvF/qjrUCIAXlHkdEzyXmTLuu
-/YxuOibV35vlaIzj21GRj4pYmVR1
+AQcDQgAE3xjPIJPp9v+qu89DNHnqcIfkOSkLb8irRAnFmAYdJJLRqWRNRjmzRHtZ
+htT4TWvhEw6VsFbRlsd510+dEASm9aM4MDYwEwYDVR0lBAwwCgYIKwYBBQUHAwIw
+HwYDVR0jBBgwFoAU0SX0xv9f6gYAcLC926z+Bt9vTAMwCgYIKoZIzj0EAwIDRwAw
+RAIgYZaCYXhfBBR8w/AfqUm9MVLYrgkz78mndNFFjz+YvpwCIFsfyIjft/vRcuaU
+xlJFtrmFMSt4x1TecZfJsWDA0M55
 -----END CERTIFICATE-----
 `
 	testUntrustedCert = `
 -----BEGIN CERTIFICATE-----
 MIIBZzCCAQygAwIBAgICEAEwCgYIKoZIzj0EAwIwHDEaMBgGA1UEAxMRVW50cnVz
-dGVkIFJvb3QgQ0EwIBgPMDAwMTAxMDEwMDAwMDBaFw0zMzA3MzExNTMzMTlaMCAx
+dGVkIFJvb3QgQ0EwIBgPMDAwMTAxMDEwMDAwMDBaFw0zMzA4MDYyMTEzMjRaMCAx
 HjAcBgNVBAMTFXVudHJ1c3RlZCBjbGllbnQgY2VydDBZMBMGByqGSM49AgEGCCqG
-SM49AwEHA0IABBG2Qo/l0evcNKjwaJsi04BJJh7ec064lRiKaRMNRUK+UxkKmfbn
-0FobVtlioTmzeWCX8OJFPfO7y7/VLMiGVr+jODA2MBMGA1UdJQQMMAoGCCsGAQUF
-BwMCMB8GA1UdIwQYMBaAFCd2l26OflZF3LTFUEBB54ZQV3AUMAoGCCqGSM49BAMC
-A0kAMEYCIQCYEk3D4nHevIlKFg6f7O2/GdptzKU6F05pz4B3Aa8ahAIhAJcBGUNm
-cqQQJNOelJJmMeFOzmmTk7oNFxCGEC00wlGn
+SM49AwEHA0IABCLf7g3vlTE+xuIDttn/FYMpAAzlCKqr37rzBnBtuEhCDypW6Y/Y
+MbIboFx9o2M9FYLzDJCS7Bj3cp2qd145HdqjODA2MBMGA1UdJQQMMAoGCCsGAQUF
+BwMCMB8GA1UdIwQYMBaAFGGTTkLU8r+tXUCs+nMR5Xm3KkgLMAoGCCqGSM49BAMC
+A0kAMEYCIQCELFkfQak8X+Yvc7H95j+shSnVgwUuOYT1Lv2NLT1qPAIhAMEvKiwc
+ZkWK0F4PJLpSt1wsbnVtfK7kXwHxdp2Z8yy7
 -----END CERTIFICATE-----
 `
 	testRevokedCert = `
 -----BEGIN CERTIFICATE-----
-MIIBYzCCAQigAwIBAgICEAIwCgYIKoZIzj0EAwIwGjEYMBYGA1UEAxMPVHJ1c3Rl
-ZCBSb290IENBMCAYDzAwMDEwMTAxMDAwMDAwWhcNMzMwNzMxMTUzMzE5WjAeMRww
+MIIBYTCCAQigAwIBAgICEAIwCgYIKoZIzj0EAwIwGjEYMBYGA1UEAxMPVHJ1c3Rl
+ZCBSb290IENBMCAYDzAwMDEwMTAxMDAwMDAwWhcNMzMwODA2MjExMzI0WjAeMRww
 GgYDVQQDExNyZXZva2VkIGNsaWVudCBjZXJ0MFkwEwYHKoZIzj0CAQYIKoZIzj0D
-AQcDQgAEoN/gKhZgyKhTmiC3qLHDQ54TIpgXBTvGKrdIRHO616XMkzj0lFZMHG5u
-LGK3qo8wJtyoalOFTkSck0kl3PD/9qM4MDYwEwYDVR0lBAwwCgYIKwYBBQUHAwIw
-HwYDVR0jBBgwFoAUXep6D8FTP6+5ZdR/HjP3pYfmxkwwCgYIKoZIzj0EAwIDSQAw
-RgIhAK6/oLtzvrK2Vrt1MRZJ6aGU2Cz28X0Y/4TOwFSvCK9AAiEAm4XPQXy6L0PE
-vfXoV8RW/RnndDhf8iDALvAaAuS82fU=
+AQcDQgAENFshpve+if3UmlNyPi5sVz8A03F9AAt6u1LxqiR5cMO6eU+L91Bey/XC
+XYrqgpJzbRgTuC4LCFx6cwwl5ff/4KM4MDYwEwYDVR0lBAwwCgYIKwYBBQUHAwIw
+HwYDVR0jBBgwFoAU0SX0xv9f6gYAcLC926z+Bt9vTAMwCgYIKoZIzj0EAwIDRwAw
+RAIgappua0SnpXDzp9nml4iHqKtYAHTn/rg0w405ahdqBQwCIHulKmPGFNLDw1dq
+1bZyKsG1t58DfFsO9G27sRssvCgV
 -----END CERTIFICATE-----
 `
 	testCRL = `
 -----BEGIN X509 CRL-----
-MIHfMIGFAgEBMAoGCCqGSM49BAMCMBoxGDAWBgNVBAMTD1RydXN0ZWQgUm9vdCBD
-QRgPMDAwMTAxMDEwMDAwMDBaMBUwEwICEAIXDTIzMDgwMzE1MzMxOVqgMDAuMB8G
-A1UdIwQYMBaAFF3qeg/BUz+vuWXUfx4z96WH5sZMMAsGA1UdFAQEAgIgADAKBggq
-hkjOPQQDAgNJADBGAiEApMG/hJxlMe9QNF8cCVjOFyTfVVBkfKtrFQDmElO46x4C
-IQCX9SYteNaaW+NVmGED6QfHXRWnDqHnXfe/mLxmnPVWzA==
+MIHeMIGFAgEBMAoGCCqGSM49BAMCMBoxGDAWBgNVBAMTD1RydXN0ZWQgUm9vdCBD
+QRgPMDAwMTAxMDEwMDAwMDBaMBUwEwICEAIXDTIzMDgwOTIxMTMyNFqgMDAuMB8G
+A1UdIwQYMBaAFNEl9Mb/X+oGAHCwvdus/gbfb0wDMAsGA1UdFAQEAgIgADAKBggq
+hkjOPQQDAgNIADBFAiEA1QoleqO9qKhxSxKUc+SOQlFTG9sTbs3ztniUhi0CxhAC
+IBElm/lXpVVuWrt0PJhcQHhHqbOxnfkx3HUxVEBWMOzX
 -----END X509 CRL-----
+`
+	testIntermediateCA = `
+-----BEGIN CERTIFICATE-----
+MIIBkjCCATegAwIBAgICEAMwCgYIKoZIzj0EAwIwGjEYMBYGA1UEAxMPVHJ1c3Rl
+ZCBSb290IENBMCAYDzAwMDEwMTAxMDAwMDAwWhcNMzMwODA2MjExMzI0WjAiMSAw
+HgYDVQQDExdUcnVzdGVkIEludGVybWVkaWF0ZSBDQTBZMBMGByqGSM49AgEGCCqG
+SM49AwEHA0IABJyxF8EUBMMh/avAul6M8AjoKstuIULPIHOvjYftT/hSqGHNYM6g
+0NIBW1g2QX/fnHG9tBy45ReTkVY5HMoO2wujYzBhMA4GA1UdDwEB/wQEAwIBBjAP
+BgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBRe0Fh8zjBKzxms/xpbS/vHY5hqujAf
+BgNVHSMEGDAWgBTRJfTG/1/qBgBwsL3brP4G329MAzAKBggqhkjOPQQDAgNJADBG
+AiEAsnob34JrBGbJjoTZS84mfno2Vb+QPJ1xy3U7AbgyYM4CIQCL3P2A3w1Z87Nr
+0A/i8rXw+kiGP1OHbs4k85ZIg6FAtQ==
+-----END CERTIFICATE-----
+`
+	testValidIntermediateCert = `
+-----BEGIN CERTIFICATE-----
+MIIBdTCCARqgAwIBAgICEAAwCgYIKoZIzj0EAwIwIjEgMB4GA1UEAxMXVHJ1c3Rl
+ZCBJbnRlcm1lZGlhdGUgQ0EwIBgPMDAwMTAxMDEwMDAwMDBaFw0zMzA4MDYyMTEz
+MjRaMCgxJjAkBgNVBAMTHWNsaWVudCBjZXJ0IGZyb20gaW50ZXJtZWRpYXRlMFkw
+EwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEvv9cmqtcuPuSXv6Qup0ycveDtx4PYC4V
+UKp5BU1B/1h/IupoIlX165rERkNCxyDXjfw5zkcHXsP1qRbc3LSXT6M4MDYwEwYD
+VR0lBAwwCgYIKwYBBQUHAwIwHwYDVR0jBBgwFoAUXtBYfM4wSs8ZrP8aW0v7x2OY
+arowCgYIKoZIzj0EAwIDSQAwRgIhAJJwdjSiC3avGDc1KZo/AZ1cMPDcFZkI92E6
+BVAnH/e8AiEAjy8cP1msG62BeDaAVU5NcU9RAXDw1Oz4HkpELXQWqK8=
+-----END CERTIFICATE-----
 `
 )
 
@@ -90,6 +115,32 @@ func Test_isValidClientCertificate(t *testing.T) {
 		assert.NoError(t, err, "should not return an error")
 		assert.True(t, valid, "should return true")
 	})
+	t.Run("valid cert with intermediate", func(t *testing.T) {
+		valid, err := isValidClientCertificate(testCA, "", ClientCertificateInfo{
+			Presented:     true,
+			Leaf:          testValidIntermediateCert,
+			Intermediates: testIntermediateCA,
+		})
+		assert.NoError(t, err, "should not return an error")
+		assert.True(t, valid, "should return true")
+	})
+	t.Run("valid cert missing intermediate", func(t *testing.T) {
+		valid, err := isValidClientCertificate(testCA, "", ClientCertificateInfo{
+			Presented:     true,
+			Leaf:          testValidIntermediateCert,
+			Intermediates: "",
+		})
+		assert.NoError(t, err, "should not return an error")
+		assert.False(t, valid, "should return false")
+	})
+	t.Run("intermediate CA as root", func(t *testing.T) {
+		valid, err := isValidClientCertificate(testIntermediateCA, "", ClientCertificateInfo{
+			Presented: true,
+			Leaf:      testValidIntermediateCert,
+		})
+		assert.NoError(t, err, "should not return an error")
+		assert.True(t, valid, "should return true")
+	})
 	t.Run("unsigned cert", func(t *testing.T) {
 		valid, err := isValidClientCertificate(testCA, "", ClientCertificateInfo{
 			Presented: true,
@@ -129,4 +180,14 @@ func Test_isValidClientCertificate(t *testing.T) {
 		assert.NoError(t, err, "should not return an error")
 		assert.True(t, valid, "should return true")
 	})
+	t.Run("missing CRL", func(t *testing.T) {
+		// If a CRL is provided for any CA, it must be provided for all CAs.
+		valid, err := isValidClientCertificate(testCA, testCRL, ClientCertificateInfo{
+			Presented:     true,
+			Leaf:          testValidIntermediateCert,
+			Intermediates: testIntermediateCA,
+		})
+		assert.NoError(t, err, "should not return an error")
+		assert.False(t, valid, "should return false")
+	})
 }
@@ -90,6 +90,26 @@ func main() {
 		ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
 	}, rootCA, rootKey)
 
+	intermediatePEM, intermediateCA, intermediateKey := newCertificate(&x509.Certificate{
+		SerialNumber: big.NewInt(0x1003),
+		Subject: pkix.Name{
+			CommonName: "Trusted Intermediate CA",
+		},
+		NotAfter:              notAfter,
+		KeyUsage:              x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
+		BasicConstraintsValid: true,
+		IsCA:                  true,
+	}, rootCA, rootKey)
+
+	trustedClientCert2PEM, _, _ := newCertificate(&x509.Certificate{
+		SerialNumber: big.NewInt(0x1000),
+		Subject: pkix.Name{
+			CommonName: "client cert from intermediate",
+		},
+		NotAfter:    notAfter,
+		ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
+	}, intermediateCA, intermediateKey)
+
 	_, untrustedCA, untrustedCAKey := newSelfSignedCertificate(&x509.Certificate{
 		SerialNumber: big.NewInt(0x1000),
 		Subject: pkix.Name{
@@ -135,6 +155,8 @@ const (
 	testUntrustedCert = ` + "`\n" + untrustedClientCertPEM + "`" + `
 	testRevokedCert = ` + "`\n" + revokedClientCertPEM + "`" + `
 	testCRL = ` + "`\n" + crlPEM + "`" + `
+	testIntermediateCA = ` + "`\n" + intermediatePEM + "`" + `
+	testValidIntermediateCert = ` + "`\n" + trustedClientCert2PEM + "`" + `
 )
 `)
 }
@@ -204,7 +204,7 @@ func TestHeadersEvaluator(t *testing.T) {
 		assert.Equal(t, "CUSTOM_VALUE", output.Headers.Get("X-Custom-Header"))
 		assert.Equal(t, "ID_TOKEN", output.Headers.Get("X-ID-Token"))
 		assert.Equal(t, "ACCESS_TOKEN", output.Headers.Get("X-Access-Token"))
-		assert.Equal(t, "17859273e8a980631d367b2d5a6a6635412b0f22835f69e47b3f65624546a704",
+		assert.Equal(t, "f0c7dc2ca5e4b792935bcdb61a8b8f31b6521c686ffd8a6edb414a1e64ab8eb5",
 			output.Headers.Get("Client-Cert-Fingerprint"))
 	})