don't cache cert verification errors

pkg/apiserver/middlewares/v1/tls_auth.go

@@ -27,7 +27,6 @@ type TLSAuth struct {
 
 type cacheEntry struct {
 	revoked   bool
-	err       error
 	timestamp time.Time
 }
 
@@ -90,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) {
-	if cert.OCSPServer == nil || (cert.OCSPServer != nil && len(cert.OCSPServer) == 0) {
+// isOCSPRevoked checks if the client certificate is revoked by any of the OCSP servers present in the certificate.
+// 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 {
@@ -105,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
@@ -116,31 +118,32 @@ 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")
-		return false, nil
+		ta.logger.Info("no crl_path, skipping CRL check")
+		return false, true
 	}
 
 	crlContent, err := os.ReadFile(ta.CrlPath)
 	if err != nil {
-		ta.logger.Warnf("could not read CRL file, skipping check: %s", err)
-		return false, nil
+		ta.logger.Errorf("could not read CRL file, skipping check: %s", err)
+		return false, false
 	}
 
 	crlBinary, rest := pem.Decode(crlContent)
 	if len(rest) > 0 {
-		ta.logger.Warn("CRL file contains more than one PEM block, skipping check")
-		return false, nil
+		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.Errorf("could not parse CRL file, skipping check: %s", err)
-		return false, nil
+		return false, false
 	}
 
 	now := time.Now().UTC()
@@ -155,56 +158,42 @@ func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, error) {
 
 	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 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)
-			return cacheValue.revoked, cacheValue.err
-		} else {
-			ta.logger.Debugf("TLSAuth: cached value expired, removing from cache")
-			delete(ta.revokationCache, sn)
+			ta.logger.Debugf("TLSAuth: using cached value for cert %s: %t", sn, cacheValue.revoked)
+			return cacheValue.revoked, nil
 		}
+
+		ta.logger.Debugf("TLSAuth: cached value expired, removing from cache")
+		delete(ta.revokationCache, sn)
 	} else {
 		ta.logger.Tracef("TLSAuth: no cached value for cert %s", sn)
 	}
 
-	revoked, err := ta.isOCSPRevoked(cert, issuer)
-	if err != nil {
+	revokedByOCSP, cacheOCSP := ta.isOCSPRevoked(cert, issuer)
+
+	revokedByCRL, cacheCRL := ta.isCRLRevoked(cert)
+
+	revoked := revokedByOCSP || revokedByCRL
+
+	if cacheOCSP && cacheCRL {
 		ta.revokationCache[sn] = cacheEntry{
 			revoked:   revoked,
-			err:       err,
 			timestamp: time.Now().UTC(),
 		}
-
-		return true, err
 	}
 
-	if revoked {
-		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
+	return revoked, nil
 }
 
 func (ta *TLSAuth) isInvalid(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) {

test/bats/30_machines_tls.bats

@@ -132,13 +132,14 @@ teardown() {
     '
     config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)'
     ./instance-crowdsec start
+    rune -0 cscli lapi status
     rune -0 cscli machines list -o json
     assert_output '[]'
 }
 
 @test "revoked cert for agent" {
     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"