LAPI: support CRL files with multiple PEM blocks (#3002)

crowdsecurity · May 13, 2024 · e3c6a5b · e3c6a5b
1 parent e4a8d3b
commit e3c6a5b
Show file tree

Hide file tree

Showing 3 changed files with 108 additions and 72 deletions.
diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go
@@ -5,6 +5,7 @@ import (
 	"crypto"
 	"crypto/x509"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -135,31 +136,35 @@ func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, bool) {
 		return false, false
 	}
 
-	crlBinary, rest := pem.Decode(crlContent)
-	if len(rest) > 0 {
-		ta.logger.Warn("CRL file contains more than one PEM block, ignoring the rest")
-	}
+	var crlBlock *pem.Block
 
-	crl, err := x509.ParseRevocationList(crlBinary.Bytes)
-	if err != nil {
-		ta.logger.Errorf("could not parse CRL file, skipping check: %s", err)
-		return false, false
-	}
+	for {
+		crlBlock, crlContent = pem.Decode(crlContent)
+		if crlBlock == nil {
+			break // no more PEM blocks
+		}
 
-	now := time.Now().UTC()
+		crl, err := x509.ParseRevocationList(crlBlock.Bytes)
+		if err != nil {
+			ta.logger.Errorf("could not parse a PEM block in CRL file, skipping: %s", err)
+			continue
+		}
 
-	if now.After(crl.NextUpdate) {
-		ta.logger.Warn("CRL has expired, will still validate the cert against it.")
-	}
+		now := time.Now().UTC()
 
-	if now.Before(crl.ThisUpdate) {
-		ta.logger.Warn("CRL is not yet valid, will still validate the cert against it.")
-	}
+		if now.After(crl.NextUpdate) {
+			ta.logger.Warn("CRL has expired, will still validate the cert against it.")
+		}
 
-	for _, revoked := range crl.RevokedCertificateEntries {
-		if revoked.SerialNumber.Cmp(cert.SerialNumber) == 0 {
-			ta.logger.Warn("client certificate is revoked by CRL")
-			return true, true
+		if now.Before(crl.ThisUpdate) {
+			ta.logger.Warn("CRL is not yet valid, will still validate the cert against it.")
+		}
+
+		for _, revoked := range crl.RevokedCertificateEntries {
+			if revoked.SerialNumber.Cmp(cert.SerialNumber) == 0 {
+				ta.logger.Warn("client certificate is revoked by CRL")
+				return true, true
+			}
 		}
 	}
 
@@ -181,9 +186,7 @@ func (ta *TLSAuth) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) (
 	}
 
 	revokedByOCSP, cacheOCSP := ta.isOCSPRevoked(cert, issuer)
-
 	revokedByCRL, cacheCRL := ta.isCRLRevoked(cert)
-
 	revoked := revokedByOCSP || revokedByCRL
 
 	if cacheOCSP && cacheCRL {
@@ -203,8 +206,8 @@ func (ta *TLSAuth) isInvalid(cert *x509.Certificate, issuer *x509.Certificate) (
 
 	revoked, err := ta.isRevoked(cert, issuer)
 	if err != nil {
-		//Fail securely, if we can't check the revocation status, let's consider the cert invalid
-		//We may change this in the future based on users feedback, but this seems the most sensible thing to do
+		// Fail securely, if we can't check the revocation status, let's consider the cert invalid
+		// We may change this in the future based on users feedback, but this seems the most sensible thing to do
 		return true, fmt.Errorf("could not check for client certification revocation status: %w", err)
 	}
 
@@ -213,12 +216,12 @@ func (ta *TLSAuth) isInvalid(cert *x509.Certificate, issuer *x509.Certificate) (
 
 func (ta *TLSAuth) SetAllowedOu(allowedOus []string) error {
 	for _, ou := range allowedOus {
-		//disallow empty ou
+		// disallow empty ou
 		if ou == "" {
-			return fmt.Errorf("empty ou isn't allowed")
+			return errors.New("empty ou isn't allowed")
 		}
 
-		//drop & warn on duplicate ou
+		// drop & warn on duplicate ou
 		ok := true
 
 		for _, validOu := range ta.AllowedOUs {
@@ -238,11 +241,11 @@ func (ta *TLSAuth) SetAllowedOu(allowedOus []string) error {
 }
 
 func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) {
-	//Checks cert validity, Returns true + CN if client cert matches requested OU
+	// Checks cert validity, Returns true + CN if client cert matches requested OU
 	var clientCert *x509.Certificate
 
 	if c.Request.TLS == nil || len(c.Request.TLS.PeerCertificates) == 0 {
-		//do not error if it's not TLS or there are no peer certs
+		// do not error if it's not TLS or there are no peer certs
 		return false, "", nil
 	}
 
@@ -279,7 +282,7 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) {
 		return true, clientCert.Subject.CommonName, nil
 	}
 
-	return false, "", fmt.Errorf("no verified cert in request")
+	return false, "", errors.New("no verified cert in request")
 }
 
 func NewTLSAuth(allowedOus []string, crlPath string, cacheExpiration time.Duration, logger *log.Entry) (*TLSAuth, error) {

diff --git a/test/bats/11_bouncers_tls.bats b/test/bats/11_bouncers_tls.bats
@@ -13,24 +13,37 @@ setup_file() {
     CFDIR="${BATS_TEST_DIRNAME}/testdata/cfssl"
     export CFDIR
 
-    #gen the CA
+    # Generate the CA
     cfssl gencert --initca "${CFDIR}/ca.json" 2>/dev/null | cfssljson --bare "${tmpdir}/ca"
-    #gen an intermediate
+
+    # Generate an intermediate
     cfssl gencert --initca "${CFDIR}/intermediate.json" 2>/dev/null | cfssljson --bare "${tmpdir}/inter"
     cfssl sign -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" 2>/dev/null | cfssljson --bare "${tmpdir}/inter"
-    #gen server cert for crowdsec with the intermediate
+
+    # Generate server cert for crowdsec with the intermediate
     cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" 2>/dev/null | cfssljson --bare "${tmpdir}/server"
-    #gen client cert for the bouncer
+
+    # Generate client cert for the bouncer
     cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null | cfssljson --bare "${tmpdir}/bouncer"
-    #gen client cert for the bouncer with an invalid OU
+
+    # Genearte client cert for the bouncer with an invalid OU
     cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer_invalid.json" 2>/dev/null | cfssljson --bare "${tmpdir}/bouncer_bad_ou"
-    #gen client cert for the bouncer directly signed by the CA, it should be refused by crowdsec as uses the intermediate
+
+    # Generate client cert for the bouncer directly signed by the CA, it should be refused by crowdsec as uses the intermediate
     cfssl gencert -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null | cfssljson --bare "${tmpdir}/bouncer_invalid"
 
-    cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null | cfssljson --bare "${tmpdir}/bouncer_revoked"
-    serial="$(openssl x509 -noout -serial -in "${tmpdir}/bouncer_revoked.pem" | cut -d '=' -f2)"
-    echo "ibase=16; ${serial}" | bc >"${tmpdir}/serials.txt"
-    cfssl gencrl "${tmpdir}/serials.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" | base64 -d | openssl crl -inform DER -out "${tmpdir}/crl.pem"
+    # Generate revoked client certs
+    for cert_name in "revoked_1" "revoked_2"; do
+        cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null | cfssljson --bare "${tmpdir}/${cert_name}"
+        serial="$(openssl x509 -noout -serial -in "${tmpdir}/${cert_name}.pem" | cut -d '=' -f2)"
+        echo "ibase=16; ${serial}" | bc >"${tmpdir}/serials_${cert_name}.txt"
+    done
+
+    # Generate separate CRL blocks and concatenate them
+    for cert_name in "revoked_1" "revoked_2"; do
+        cfssl gencrl "${tmpdir}/serials_${cert_name}.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" | base64 -d | openssl crl -inform DER -out "${tmpdir}/crl_${cert_name}.pem"
+    done
+    cat "${tmpdir}/crl_revoked_1.pem" "${tmpdir}/crl_revoked_2.pem" >"${tmpdir}/crl.pem"
 
     cat "${tmpdir}/ca.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem"
 
@@ -90,11 +103,14 @@ teardown() {
 }
 
 @test "simulate one bouncer request with a revoked certificate" {
-    truncate_log
-    rune -0 curl -i -s --cert "${tmpdir}/bouncer_revoked.pem" --key "${tmpdir}/bouncer_revoked-key.pem" --cacert "${tmpdir}/bundle.pem" https://localhost:8080/v1/decisions\?ip=42.42.42.42
-    assert_log --partial "client certificate is revoked by CRL"
-    assert_log --partial "client certificate for CN=localhost OU=[bouncer-ou] is revoked"
-    assert_output --partial "access forbidden"
-    rune -0 cscli bouncers list -o json
-    assert_output "[]"
+    # we have two certificates revoked by different CRL blocks
+    for cert_name in "revoked_1" "revoked_2"; do
+        truncate_log
+        rune -0 curl -i -s --cert "${tmpdir}/${cert_name}.pem" --key "${tmpdir}/${cert_name}-key.pem" --cacert "${tmpdir}/bundle.pem" https://localhost:8080/v1/decisions\?ip=42.42.42.42
+        assert_log --partial "client certificate is revoked by CRL"
+        assert_log --partial "client certificate for CN=localhost OU=[bouncer-ou] is revoked"
+        assert_output --partial "access forbidden"
+        rune -0 cscli bouncers list -o json
+        assert_output "[]"
+    done
 }
diff --git a/test/bats/30_machines_tls.bats b/test/bats/30_machines_tls.bats
@@ -16,24 +16,37 @@ setup_file() {
     CFDIR="${BATS_TEST_DIRNAME}/testdata/cfssl"
     export CFDIR
 
-    #gen the CA
+    # Generate the CA
     cfssl gencert --initca "${CFDIR}/ca.json" 2>/dev/null | cfssljson --bare "${tmpdir}/ca"
-    #gen an intermediate
+
+    # Generate an intermediate
     cfssl gencert --initca "${CFDIR}/intermediate.json" 2>/dev/null | cfssljson --bare "${tmpdir}/inter"
     cfssl sign -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" 2>/dev/null | cfssljson --bare "${tmpdir}/inter"
-    #gen server cert for crowdsec with the intermediate
+
+    # Generate server cert for crowdsec with the intermediate
     cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" 2>/dev/null | cfssljson --bare "${tmpdir}/server"
-    #gen client cert for the agent
+
+    # Generate client cert for the agent
     cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null | cfssljson --bare "${tmpdir}/agent"
-    #gen client cert for the agent with an invalid OU
+
+    # Genearte client cert for the agent with an invalid OU
     cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent_invalid.json" 2>/dev/null | cfssljson --bare "${tmpdir}/agent_bad_ou"
-    #gen client cert for the agent directly signed by the CA, it should be refused by crowdsec as uses the intermediate
+
+    # Generate client cert for the bouncer directly signed by the CA, it should be refused by crowdsec as uses the intermediate
     cfssl gencert -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null | cfssljson --bare "${tmpdir}/agent_invalid"
 
-    cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null | cfssljson --bare "${tmpdir}/agent_revoked"
-    serial="$(openssl x509 -noout -serial -in "${tmpdir}/agent_revoked.pem" | cut -d '=' -f2)"
-    echo "ibase=16; ${serial}" | bc >"${tmpdir}/serials.txt"
-    cfssl gencrl "${tmpdir}/serials.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" | base64 -d | openssl crl -inform DER -out "${tmpdir}/crl.pem"
+    # Generate revoked client cert
+    for cert_name in "revoked_1" "revoked_2"; do
+        cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null | cfssljson --bare "${tmpdir}/${cert_name}"
+        serial="$(openssl x509 -noout -serial -in "${tmpdir}/${cert_name}.pem" | cut -d '=' -f2)"
+        echo "ibase=16; ${serial}" | bc >"${tmpdir}/serials_${cert_name}.txt"
+    done
+
+    # Generate separate CRL blocks and concatenate them
+    for cert_name in "revoked_1" "revoked_2"; do
+        cfssl gencrl "${tmpdir}/serials_${cert_name}.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" | base64 -d | openssl crl -inform DER -out "${tmpdir}/crl_${cert_name}.pem"
+    done
+    cat "${tmpdir}/crl_revoked_1.pem" "${tmpdir}/crl_revoked_2.pem" >"${tmpdir}/crl.pem"
 
     cat "${tmpdir}/ca.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem"
 
@@ -181,19 +194,23 @@ teardown() {
 }
 
 @test "revoked cert for agent" {
-    truncate_log
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" '
-        .ca_cert_path=strenv(tmpdir) + "/bundle.pem" |
-        .key_path=strenv(tmpdir) + "/agent_revoked-key.pem" |
-        .cert_path=strenv(tmpdir) + "/agent_revoked.pem" |
-        .url="https://127.0.0.1:8080"
-    '
-
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)'
-    ./instance-crowdsec start
-    rune -1 cscli lapi status
-    assert_log --partial "client certificate is revoked by CRL"
-    assert_log --partial "client certificate for CN=localhost OU=[agent-ou] is revoked"
-    rune -0 cscli machines list -o json
-    assert_output '[]'
+    # we have two certificates revoked by different CRL blocks
+    for cert_name in "revoked_1" "revoked_2"; do
+        truncate_log
+        cert_name="$cert_name" config_set "${CONFIG_DIR}/local_api_credentials.yaml" '
+            .ca_cert_path=strenv(tmpdir) + "/bundle.pem" |
+            .key_path=strenv(tmpdir) + "/" + strenv(cert_name) + "-key.pem" |
+            .cert_path=strenv(tmpdir) + "/" + strenv(cert_name) + ".pem" |
+            .url="https://127.0.0.1:8080"
+        '
+
+        config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)'
+        ./instance-crowdsec start
+        rune -1 cscli lapi status
+        assert_log --partial "client certificate is revoked by CRL"
+        assert_log --partial "client certificate for CN=localhost OU=[agent-ou] is revoked"
+        rune -0 cscli machines list -o json
+        assert_output '[]'
+        ./instance-crowdsec stop
+    done
 }