update calls to deprecated x509 methods (#2824)

.github/workflows/docker-tests.yml

@@ -50,7 +50,7 @@ jobs:
           cache-to: type=gha,mode=min
 
       - name: "Setup Python"
-        uses: actions/setup-python@v4
+        uses: actions/setup-python@v5
         with:
           python-version: "3.x"
 
@@ -61,7 +61,7 @@ jobs:
 
       - name: "Cache virtualenvs"
         id: cache-pipenv
-        uses: actions/cache@v3
+        uses: actions/cache@v4
         with:
           path: ~/.local/share/virtualenvs
           key: ${{ runner.os }}-pipenv-${{ hashFiles('**/Pipfile.lock') }}

.golangci.yml

@@ -310,10 +310,6 @@ issues:
     # Will fix,  might be trickier
     #
 
-    - linters:
-        - staticcheck
-      text: "x509.ParseCRL has been deprecated since Go 1.19: Use ParseRevocationList instead"
-
     # https://github.com/pkg/errors/issues/245
     - linters:
         - depguard

pkg/apiserver/middlewares/v1/api_key.go

@@ -66,7 +66,7 @@ func (a *APIKey) authTLS(c *gin.Context, logger *log.Entry) *ent.Bouncer {
 
 	validCert, extractedCN, err := a.TlsAuth.ValidateCert(c)
 	if !validCert {
-		logger.Errorf("invalid client certificate: %s", err)
+		logger.Error(err)
 		return nil
 	}
 

pkg/apiserver/middlewares/v1/tls_auth.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto"
 	"crypto/x509"
+	"encoding/pem"
 	"fmt"
 	"io"
 	"net/http"
@@ -19,14 +20,13 @@ import (
 type TLSAuth struct {
 	AllowedOUs      []string
 	CrlPath         string
-	revokationCache map[string]cacheEntry
+	revocationCache map[string]cacheEntry
 	cacheExpiration time.Duration
 	logger          *log.Entry
 }
 
 type cacheEntry struct {
 	revoked   bool
-	err       error
 	timestamp time.Time
 }
 
@@ -89,10 +89,12 @@ func (ta *TLSAuth) isExpired(cert *x509.Certificate) bool {
 	return false
 }
 
-func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) {
+// isOCSPRevoked checks if the client certificate is revoked by any of the OCSP servers present in the certificate.
-	if cert.OCSPServer == nil || (cert.OCSPServer != nil && len(cert.OCSPServer) == 0) {
+// It returns a boolean indicating if the certificate is revoked and a boolean indicating if the OCSP check was successful and could be cached.
+func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, bool) {
+	if cert.OCSPServer == nil || len(cert.OCSPServer) == 0 {
 		ta.logger.Infof("TLSAuth: no OCSP Server present in client certificate, skipping OCSP verification")
-		return false, nil
+		return false, true
 	}
 
 	for _, server := range cert.OCSPServer {
@@ -104,9 +106,10 @@ func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificat
 
 		switch ocspResponse.Status {
 		case ocsp.Good:
-			return false, nil
+			return false, true
 		case ocsp.Revoked:
-			return true, fmt.Errorf("client certificate is revoked by server %s", server)
+			ta.logger.Errorf("TLSAuth: client certificate is revoked by server %s", server)
+			return true, true
 		case ocsp.Unknown:
 			log.Debugf("unknow OCSP status for server %s", server)
 			continue
@@ -115,83 +118,82 @@ func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificat
 
 	log.Infof("Could not get any valid OCSP response, assuming the cert is revoked")
 
-	return true, nil
+	return true, false
 }
 
-func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, error) {
+// isCRLRevoked checks if the client certificate is revoked by the CRL present in the CrlPath.
+// It returns a boolean indicating if the certificate is revoked and a boolean indicating if the CRL check was successful and could be cached.
+func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, bool) {
 	if ta.CrlPath == "" {
-		ta.logger.Warn("no crl_path, skipping CRL check")
+		ta.logger.Info("no crl_path, skipping CRL check")
-		return false, nil
+		return false, true
 	}
 
 	crlContent, err := os.ReadFile(ta.CrlPath)
 	if err != nil {
-		ta.logger.Warnf("could not read CRL file, skipping check: %s", err)
+		ta.logger.Errorf("could not read CRL file, skipping check: %s", err)
-		return false, nil
+		return false, false
 	}
 
-	crl, err := x509.ParseCRL(crlContent)
+	crlBinary, rest := pem.Decode(crlContent)
+	if len(rest) > 0 {
+		ta.logger.Warn("CRL file contains more than one PEM block, ignoring the rest")
+	}
+
+	crl, err := x509.ParseRevocationList(crlBinary.Bytes)
 	if err != nil {
-		ta.logger.Warnf("could not parse CRL file, skipping check: %s", err)
+		ta.logger.Errorf("could not parse CRL file, skipping check: %s", err)
-		return false, nil
+		return false, false
 	}
 
-	if crl.HasExpired(time.Now().UTC()) {
+	now := time.Now().UTC()
+
+	if now.After(crl.NextUpdate) {
 		ta.logger.Warn("CRL has expired, will still validate the cert against it.")
 	}
 
-	for _, revoked := range crl.TBSCertList.RevokedCertificates {
+	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 {
-			return true, fmt.Errorf("client certificate is revoked by CRL")
+			ta.logger.Warn("client certificate is revoked by CRL")
+			return true, true
 		}
 	}
 
-	return false, nil
+	return false, true
 }
 
 func (ta *TLSAuth) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) {
 	sn := cert.SerialNumber.String()
-	if cacheValue, ok := ta.revokationCache[sn]; ok {
+	if cacheValue, ok := ta.revocationCache[sn]; ok {
 		if time.Now().UTC().Sub(cacheValue.timestamp) < ta.cacheExpiration {
-			ta.logger.Debugf("TLSAuth: using cached value for cert %s: %t | %s", sn, cacheValue.revoked, cacheValue.err)
+			ta.logger.Debugf("TLSAuth: using cached value for cert %s: %t", sn, cacheValue.revoked)
-			return cacheValue.revoked, cacheValue.err
+			return cacheValue.revoked, nil
-		} else {
-			ta.logger.Debugf("TLSAuth: cached value expired, removing from cache")
-			delete(ta.revokationCache, sn)
 		}
+
+		ta.logger.Debugf("TLSAuth: cached value expired, removing from cache")
+		delete(ta.revocationCache, sn)
 	} else {
 		ta.logger.Tracef("TLSAuth: no cached value for cert %s", sn)
 	}
 
-	revoked, err := ta.isOCSPRevoked(cert, issuer)
+	revokedByOCSP, cacheOCSP := ta.isOCSPRevoked(cert, issuer)
-	if err != nil {
+
-		ta.revokationCache[sn] = cacheEntry{
+	revokedByCRL, cacheCRL := ta.isCRLRevoked(cert)
+
+	revoked := revokedByOCSP || revokedByCRL
+
+	if cacheOCSP && cacheCRL {
+		ta.revocationCache[sn] = cacheEntry{
 			revoked:   revoked,
-			err:       err,
 			timestamp: time.Now().UTC(),
 		}
-
-		return true, err
 	}
 
-	if revoked {
+	return revoked, nil
-		ta.revokationCache[sn] = cacheEntry{
-			revoked:   revoked,
-			err:       err,
-			timestamp: time.Now().UTC(),
-		}
-
-		return true, nil
-	}
-
-	revoked, err = ta.isCRLRevoked(cert)
-	ta.revokationCache[sn] = cacheEntry{
-		revoked:   revoked,
-		err:       err,
-		timestamp: time.Now().UTC(),
-	}
-
-	return revoked, err
 }
 
 func (ta *TLSAuth) isInvalid(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) {
@@ -265,11 +267,11 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) {
 		revoked, err := ta.isInvalid(clientCert, c.Request.TLS.VerifiedChains[0][1])
 		if err != nil {
 			ta.logger.Errorf("TLSAuth: error checking if client certificate is revoked: %s", err)
-			return false, "", fmt.Errorf("could not check for client certification revokation status: %w", err)
+			return false, "", fmt.Errorf("could not check for client certification revocation status: %w", err)
 		}
 
 		if revoked {
-			return false, "", fmt.Errorf("client certificate is revoked")
+			return false, "", fmt.Errorf("client certificate for CN=%s OU=%s is revoked", clientCert.Subject.CommonName, clientCert.Subject.OrganizationalUnit)
 		}
 
 		ta.logger.Debugf("client OU %v is allowed vs required OU %v", clientCert.Subject.OrganizationalUnit, ta.AllowedOUs)
@@ -282,7 +284,7 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) {
 
 func NewTLSAuth(allowedOus []string, crlPath string, cacheExpiration time.Duration, logger *log.Entry) (*TLSAuth, error) {
 	ta := &TLSAuth{
-		revokationCache: map[string]cacheEntry{},
+		revocationCache: map[string]cacheEntry{},
 		cacheExpiration: cacheExpiration,
 		CrlPath:         crlPath,
 		logger:          logger,

test/bats/11_bouncers_tls.bats

@@ -90,7 +90,10 @@ 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 "[]"

test/bats/30_machines_tls.bats

@@ -132,13 +132,15 @@ teardown() {
     '
     config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)'
     ./instance-crowdsec start
+    rune -1 cscli lapi status
     rune -0 cscli machines list -o json
     assert_output '[]'
 }
 
 @test "revoked cert for agent" {
+    truncate_log
     config_set "${CONFIG_DIR}/local_api_credentials.yaml" '
-         .ca_cert_path=strenv(tmpdir) + "/bundle.pem" |
+        .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"
@@ -146,6 +148,9 @@ teardown() {
 
     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 '[]'
 }