diff --git a/authorize/evaluator/functions.go b/authorize/evaluator/functions.go index 76f631361..fa0ea75d3 100644 --- a/authorize/evaluator/functions.go +++ b/authorize/evaluator/functions.go @@ -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 - } - - // 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) - } - - // 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) - } - - // 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 + // 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 } } + // 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 +} + +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 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 { diff --git a/authorize/evaluator/functions_test.go b/authorize/evaluator/functions_test.go index f87ec953a..ea3b271f6 100644 --- a/authorize/evaluator/functions_test.go +++ b/authorize/evaluator/functions_test.go @@ -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") + }) } diff --git a/authorize/evaluator/gen-test-certs.go b/authorize/evaluator/gen-test-certs.go index 171473509..edbe4e17c 100644 --- a/authorize/evaluator/gen-test-certs.go +++ b/authorize/evaluator/gen-test-certs.go @@ -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 + "`" + ` ) `) } diff --git a/authorize/evaluator/headers_evaluator_test.go b/authorize/evaluator/headers_evaluator_test.go index 9a5af5683..86cf2973a 100644 --- a/authorize/evaluator/headers_evaluator_test.go +++ b/authorize/evaluator/headers_evaluator_test.go @@ -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")) })