authorize: check CRLs only for leaf certificates (#4480)

Set the Envoy option only_verify_leaf_cert_crl, to avoid a bug where
CRLs cannot be used in combination with an intermediate CA trust root.
Update the client certificate validation logic in the authorize service
to match this behavior.
This commit is contained in:
Kenneth Jenkins 2023-08-23 09:07:32 -07:00 committed by GitHub
parent 3e330bb76a
commit c95f1695ec
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 27 additions and 39 deletions

View file

@ -175,38 +175,29 @@ func validateClientCertificateChain(
return err return err
} }
// Consult CRLs for all CAs in the chain (that is, all certificates except // Consult CRLs only for the first CA in the chain, to match Envoy's
// for the first one). To match Envoy's behavior, if a CRL is provided for // behavior when the only_verify_leaf_cert_crl option is set (see
// 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). // https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/common.proto).
var anyIssuerHasCRL bool if len(chain) < 2 {
var lastIssuerWithoutCRL *x509.Certificate return nil
for i := 0; i < len(chain)-1; i++ { }
cert, issuer := chain[i], chain[i+1] cert, issuer := chain[0], chain[1]
crl := crls[string(issuer.RawSubject)] crl := crls[string(issuer.RawSubject)]
if crl == nil { if crl == nil {
lastIssuerWithoutCRL = issuer return nil
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 { // Is the CRL signature itself valid?
return fmt.Errorf("no CRL provided for issuer %q", lastIssuerWithoutCRL.Subject) 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)
}
} }
return nil return nil

View file

@ -238,16 +238,6 @@ func Test_isValidClientCertificate(t *testing.T) {
assert.NoError(t, err, "should not return an error") assert.NoError(t, err, "should not return an error")
assert.True(t, valid, "should return true") 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,
}, noConstraints)
assert.NoError(t, err, "should not return an error")
assert.False(t, valid, "should return false")
})
t.Run("chain too deep", func(t *testing.T) { t.Run("chain too deep", func(t *testing.T) {
valid, err := isValidClientCertificate(testCA, "", ClientCertificateInfo{ valid, err := isValidClientCertificate(testCA, "", ClientCertificateInfo{
Presented: true, Presented: true,

View file

@ -553,6 +553,7 @@ func (b *Builder) buildDownstreamValidationContext(
TrustedCa: b.filemgr.BytesDataSource("client-ca.pem", clientCA), TrustedCa: b.filemgr.BytesDataSource("client-ca.pem", clientCA),
MatchTypedSubjectAltNames: make([]*envoy_extensions_transport_sockets_tls_v3.SubjectAltNameMatcher, MatchTypedSubjectAltNames: make([]*envoy_extensions_transport_sockets_tls_v3.SubjectAltNameMatcher,
0, len(cfg.Options.DownstreamMTLS.MatchSubjectAltNames)), 0, len(cfg.Options.DownstreamMTLS.MatchSubjectAltNames)),
OnlyVerifyLeafCertCrl: true,
} }
for i := range cfg.Options.DownstreamMTLS.MatchSubjectAltNames { for i := range cfg.Options.DownstreamMTLS.MatchSubjectAltNames {
vc.MatchTypedSubjectAltNames = append(vc.MatchTypedSubjectAltNames, vc.MatchTypedSubjectAltNames = append(vc.MatchTypedSubjectAltNames,

View file

@ -119,6 +119,7 @@ func Test_buildDownstreamTLSContext(t *testing.T) {
"alpnProtocols": ["h2", "http/1.1"], "alpnProtocols": ["h2", "http/1.1"],
"validationContext": { "validationContext": {
"maxVerifyDepth": 1, "maxVerifyDepth": 1,
"onlyVerifyLeafCertCrl": true,
"trustChainVerification": "ACCEPT_UNTRUSTED", "trustChainVerification": "ACCEPT_UNTRUSTED",
"trustedCa": { "trustedCa": {
"filename": "`+clientCAFileName+`" "filename": "`+clientCAFileName+`"
@ -151,6 +152,7 @@ func Test_buildDownstreamTLSContext(t *testing.T) {
"alpnProtocols": ["h2", "http/1.1"], "alpnProtocols": ["h2", "http/1.1"],
"validationContext": { "validationContext": {
"maxVerifyDepth": 1, "maxVerifyDepth": 1,
"onlyVerifyLeafCertCrl": true,
"trustedCa": { "trustedCa": {
"filename": "`+clientCAFileName+`" "filename": "`+clientCAFileName+`"
} }
@ -186,6 +188,7 @@ func Test_buildDownstreamTLSContext(t *testing.T) {
"alpnProtocols": ["h2", "http/1.1"], "alpnProtocols": ["h2", "http/1.1"],
"validationContext": { "validationContext": {
"maxVerifyDepth": 1, "maxVerifyDepth": 1,
"onlyVerifyLeafCertCrl": true,
"trustChainVerification": "ACCEPT_UNTRUSTED", "trustChainVerification": "ACCEPT_UNTRUSTED",
"trustedCa": { "trustedCa": {
"filename": "`+clientCAFileName+`" "filename": "`+clientCAFileName+`"
@ -209,6 +212,7 @@ func Test_buildDownstreamTLSContext(t *testing.T) {
require.NoError(t, err) require.NoError(t, err)
testutil.AssertProtoJSONEqual(t, `{ testutil.AssertProtoJSONEqual(t, `{
"maxVerifyDepth": 10, "maxVerifyDepth": 10,
"onlyVerifyLeafCertCrl": true,
"trustChainVerification": "ACCEPT_UNTRUSTED", "trustChainVerification": "ACCEPT_UNTRUSTED",
"trustedCa": { "trustedCa": {
"filename": "`+clientCAFileName+`" "filename": "`+clientCAFileName+`"
@ -220,6 +224,7 @@ func Test_buildDownstreamTLSContext(t *testing.T) {
b.buildDownstreamTLSContextMulti(context.Background(), config, nil) b.buildDownstreamTLSContextMulti(context.Background(), config, nil)
require.NoError(t, err) require.NoError(t, err)
testutil.AssertProtoJSONEqual(t, `{ testutil.AssertProtoJSONEqual(t, `{
"onlyVerifyLeafCertCrl": true,
"trustChainVerification": "ACCEPT_UNTRUSTED", "trustChainVerification": "ACCEPT_UNTRUSTED",
"trustedCa": { "trustedCa": {
"filename": "`+clientCAFileName+`" "filename": "`+clientCAFileName+`"
@ -281,6 +286,7 @@ func Test_buildDownstreamTLSContext(t *testing.T) {
"sanType": "URI" "sanType": "URI"
} }
], ],
"onlyVerifyLeafCertCrl": true,
"trustChainVerification": "ACCEPT_UNTRUSTED", "trustChainVerification": "ACCEPT_UNTRUSTED",
"trustedCa": { "trustedCa": {
"filename": "`+clientCAFileName+`" "filename": "`+clientCAFileName+`"