improved tls middleware revocation checks (#3034)

.golangci.yml

@@ -462,10 +462,6 @@ issues:
       path: pkg/hubtest/hubtest_item.go
       text: "cyclomatic: .*RunWithLogFile"
 
-    - linters:
-        - canonicalheader
-      path: pkg/apiserver/middlewares/v1/tls_auth.go
-
     # tolerate complex functions in tests for now
     - linters:
         - maintidx

docker/test/tests/test_tls.py

@@ -281,7 +281,7 @@ def test_tls_client_ou(crowdsec, flavor, certs_dir):
         lapi.wait_for_http(8080, '/health', want_status=None)
         with cs_agent as agent:
             lapi.wait_for_log([
-                "*client certificate OU (?custom-client-ou?) doesn't match expected OU (?agent-ou?)*",
+                "*client certificate OU ?custom-client-ou? doesn't match expected OU ?agent-ou?*",
             ])
 
     lapi_env['AGENTS_ALLOWED_OU'] = 'custom-client-ou'

pkg/apiclient/alerts_service.go

@@ -10,8 +10,6 @@ import (
 	"github.com/crowdsecurity/crowdsec/pkg/models"
 )
 
-// type ApiAlerts service
-
 type AlertsService service
 
 type AlertsListOpts struct {

pkg/apiclient/auth_service.go

@@ -8,8 +8,6 @@ import (
 	"github.com/crowdsecurity/crowdsec/pkg/models"
 )
 
-// type ApiAlerts service
-
 type AuthService service
 
 // Don't add it to the models, as they are used with LAPI, but the enroll endpoint is specific to CAPI

pkg/apiserver/middlewares/v1/api_key.go

@@ -60,18 +60,13 @@ func HashSHA512(str string) string {
 
 func (a *APIKey) authTLS(c *gin.Context, logger *log.Entry) *ent.Bouncer {
 	if a.TlsAuth == nil {
-		logger.Error("TLS Auth is not configured but client presented a certificate")
-		return nil
-	}
-
-	validCert, extractedCN, err := a.TlsAuth.ValidateCert(c)
-	if !validCert {
-		logger.Error(err)
+		logger.Warn("TLS Auth is not configured but client presented a certificate")
 		return nil
 	}
 
+	extractedCN, err := a.TlsAuth.ValidateCert(c)
 	if err != nil {
-		logger.Error(err)
+		logger.Warn(err)
 		return nil
 	}
 
@@ -148,6 +143,7 @@ func (a *APIKey) MiddlewareFunc() gin.HandlerFunc {
 		}
 
 		if bouncer == nil {
+			// XXX: StatusUnauthorized?
 			c.JSON(http.StatusForbidden, gin.H{"message": "access forbidden"})
 			c.Abort()
 

pkg/apiserver/middlewares/v1/cache.go

@@ -0,0 +1,99 @@
+package v1
+
+import (
+	"crypto/x509"
+	"sync"
+	"time"
+
+	log "github.com/sirupsen/logrus"
+)
+
+type cacheEntry struct {
+	err       error		// if nil, the certificate is not revocated
+	timestamp time.Time
+}
+
+type RevocationCache struct {
+	mu         sync.RWMutex
+	cache      map[string]cacheEntry
+	expiration time.Duration
+	lastPurge  time.Time
+	logger     *log.Entry
+}
+
+func NewRevocationCache(expiration time.Duration, logger *log.Entry) *RevocationCache {
+	return &RevocationCache{
+		cache:      make(map[string]cacheEntry),
+		expiration: expiration,
+		lastPurge:  time.Now(),
+		logger:     logger,
+	}
+}
+
+func (*RevocationCache) generateKey(cert *x509.Certificate) string {
+	return cert.SerialNumber.String() + "-" + cert.Issuer.String()
+}
+
+// purge removes expired entries from the cache
+func (rc *RevocationCache) purgeExpired() {
+	// we don't keep a separate interval for the full sweep, we'll just double the expiration
+	if time.Since(rc.lastPurge) < rc.expiration {
+		return
+	}
+
+	rc.mu.Lock()
+	defer rc.mu.Unlock()
+
+	for key, entry := range rc.cache {
+		if time.Since(entry.timestamp) > rc.expiration {
+			rc.logger.Debugf("purging expired entry for cert %s", key)
+			delete(rc.cache, key)
+		}
+	}
+}
+
+func (rc *RevocationCache) Get(cert *x509.Certificate) (error, bool) { //nolint:revive
+	rc.purgeExpired()
+	key := rc.generateKey(cert)
+	rc.mu.RLock()
+	entry, exists := rc.cache[key]
+	rc.mu.RUnlock()
+
+	if !exists {
+		rc.logger.Tracef("no cached value for cert %s", key)
+		return nil, false
+	}
+
+	// Upgrade to write lock to potentially modify the cache
+	rc.mu.Lock()
+	defer rc.mu.Unlock()
+
+	if entry.timestamp.Add(rc.expiration).Before(time.Now()) {
+		rc.logger.Debugf("cached value for %s expired, removing from cache", key)
+		delete(rc.cache, key)
+
+		return nil, false
+	}
+
+	rc.logger.Debugf("using cached value for cert %s: %v", key, entry.err)
+
+	return entry.err, true
+}
+
+func (rc *RevocationCache) Set(cert *x509.Certificate, err error) {
+	key := rc.generateKey(cert)
+
+	rc.mu.Lock()
+	defer rc.mu.Unlock()
+
+	rc.cache[key] = cacheEntry{
+		err:       err,
+		timestamp: time.Now(),
+	}
+}
+
+func (rc *RevocationCache) Empty() {
+	rc.mu.Lock()
+	defer rc.mu.Unlock()
+	rc.cache = make(map[string]cacheEntry)
+}

pkg/apiserver/middlewares/v1/crl.go

@@ -0,0 +1,145 @@
+package v1
+
+import (
+	"crypto/x509"
+	"encoding/pem"
+	"fmt"
+	"os"
+	"sync"
+	"time"
+
+	log "github.com/sirupsen/logrus"
+)
+
+type CRLChecker struct {
+	path     string			// path to the CRL file
+	fileInfo os.FileInfo		// last stat of the CRL file
+	crls     []*x509.RevocationList	// parsed CRLs
+	logger   *log.Entry
+	mu       sync.RWMutex
+	lastLoad time.Time		// time when the CRL file was last read successfully
+	onLoad	 func()			// called when the CRL file changes (and is read successfully)
+}
+
+func NewCRLChecker(crlPath string, onLoad func(), logger *log.Entry) (*CRLChecker, error) {
+	cc := &CRLChecker{
+		path:   crlPath,
+		logger: logger,
+		onLoad: onLoad,
+	}
+
+	err := cc.refresh()
+	if err != nil {
+		return nil, err
+	}
+
+	return cc, nil
+}
+
+func (*CRLChecker) decodeCRLs(content []byte) ([]*x509.RevocationList, error) {
+	var crls []*x509.RevocationList
+
+	for {
+		block, rest := pem.Decode(content)
+		if block == nil {
+			break // no more PEM blocks
+		}
+
+		content = rest
+
+		crl, err := x509.ParseRevocationList(block.Bytes)
+		if err != nil {
+			// invalidate the whole CRL file so we can still use the previous version
+			return nil, fmt.Errorf("could not parse file: %w", err)
+		}
+
+		crls = append(crls, crl)
+	}
+
+	return crls, nil
+}
+
+// refresh() reads the CRL file if new or changed since the last time
+func (cc *CRLChecker) refresh() error {
+	// noop if lastLoad is less than 5 seconds ago
+	if time.Since(cc.lastLoad) < 5*time.Second {
+		return nil
+	}
+
+	cc.mu.Lock()
+	defer cc.mu.Unlock()
+
+	cc.logger.Debugf("loading CRL file from %s", cc.path)
+
+	fileInfo, err := os.Stat(cc.path)
+	if err != nil {
+		return fmt.Errorf("could not access CRL file: %w", err)
+	}
+
+	// noop if the file didn't change
+	if cc.fileInfo != nil && fileInfo.ModTime().Equal(cc.fileInfo.ModTime()) && fileInfo.Size() == cc.fileInfo.Size() {
+		return nil
+	}
+
+	// the encoding/pem package wants bytes, not io.Reader
+	crlContent, err := os.ReadFile(cc.path)
+	if err != nil {
+		return fmt.Errorf("could not read CRL file: %w", err)
+	}
+
+	cc.crls, err = cc.decodeCRLs(crlContent)
+	if err != nil {
+		return err
+	}
+
+	cc.fileInfo = fileInfo
+	cc.lastLoad = time.Now()
+	cc.onLoad()
+
+	return nil
+}
+
+// isRevoked checks if the client certificate is revoked by any of the CRL blocks
+// 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 (cc *CRLChecker) isRevokedBy(cert *x509.Certificate, issuer *x509.Certificate) (bool, bool) {
+	if cc == nil {
+		return false, true
+	}
+
+	err := cc.refresh()
+	if err != nil {
+		// we can't quit obviously, so we just log the error and continue
+		// but we can assume we have loaded a CRL, or it would have quit the first time
+		cc.logger.Errorf("while refreshing CRL: %s - will keep using CRL file read at %s", err,
+			cc.lastLoad.Format(time.RFC3339))
+	}
+
+	now := time.Now().UTC()
+
+	cc.mu.RLock()
+	defer cc.mu.RUnlock()
+
+	for _, crl := range cc.crls {
+		if err := crl.CheckSignatureFrom(issuer); err != nil {
+			continue
+		}
+
+		if now.After(crl.NextUpdate) {
+			cc.logger.Warn("CRL has expired, will still validate the cert against it.")
+		}
+
+		if now.Before(crl.ThisUpdate) {
+			cc.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 {
+				cc.logger.Warn("client certificate is revoked by CRL")
+				return true, true
+			}
+		}
+	}
+
+	return false, true
+}

pkg/apiserver/middlewares/v1/jwt.go

@@ -4,7 +4,6 @@ import (
 	"crypto/rand"
 	"errors"
 	"fmt"
-	"net/http"
 	"os"
 	"strings"
 	"time"
@@ -59,27 +58,19 @@ func (j *JWT) authTLS(c *gin.Context) (*authInput, error) {
 	ret := authInput{}
 
 	if j.TlsAuth == nil {
-		c.JSON(http.StatusForbidden, gin.H{"message": "access forbidden"})
-		c.Abort()
+		err := errors.New("tls authentication required")
+		log.Warn(err)
 
-		return nil, errors.New("TLS auth is not configured")
+		return nil, err
 	}
 
-	validCert, extractedCN, err := j.TlsAuth.ValidateCert(c)
+	extractedCN, err := j.TlsAuth.ValidateCert(c)
 	if err != nil {
-		log.Error(err)
-		c.JSON(http.StatusForbidden, gin.H{"message": "access forbidden"})
-		c.Abort()
-
-		return nil, fmt.Errorf("while trying to validate client cert: %w", err)
+		log.Warn(err)
+		return nil, err
 	}
 
-	if !validCert {
-		c.JSON(http.StatusForbidden, gin.H{"message": "access forbidden"})
-		c.Abort()
-
-		return nil, errors.New("failed cert authentication")
-	}
+	logger := log.WithField("ip", c.ClientIP())
 
 	ret.machineID = fmt.Sprintf("%s@%s", extractedCN, c.ClientIP())
 
@@ -88,14 +79,12 @@ func (j *JWT) authTLS(c *gin.Context) (*authInput, error) {
 		First(j.DbClient.CTX)
 	if ent.IsNotFound(err) {
 		// Machine was not found, let's create it
-		log.Infof("machine %s not found, create it", ret.machineID)
+		logger.Infof("machine %s not found, create it", ret.machineID)
 		// let's use an apikey as the password, doesn't matter in this case (generatePassword is only available in cscli)
 		pwd, err := GenerateAPIKey(dummyAPIKeySize)
 		if err != nil {
-			log.WithFields(log.Fields{
-				"ip": c.ClientIP(),
-				"cn": extractedCN,
-			}).Errorf("error generating password: %s", err)
+			logger.WithField("cn", extractedCN).
+				Errorf("error generating password: %s", err)
 
 			return nil, errors.New("error generating password")
 		}

pkg/apiserver/middlewares/v1/ocsp.go

@@ -0,0 +1,100 @@
+package v1
+
+import (
+	"bytes"
+	"crypto"
+	"crypto/x509"
+	"io"
+	"net/http"
+	"net/url"
+
+	log "github.com/sirupsen/logrus"
+	"golang.org/x/crypto/ocsp"
+)
+
+type OCSPChecker struct {
+	logger *log.Entry
+}
+
+func NewOCSPChecker(logger *log.Entry) *OCSPChecker {
+	return &OCSPChecker{
+		logger: logger,
+	}
+}
+
+func (oc *OCSPChecker) query(server string, cert *x509.Certificate, issuer *x509.Certificate) (*ocsp.Response, error) {
+	req, err := ocsp.CreateRequest(cert, issuer, &ocsp.RequestOptions{Hash: crypto.SHA256})
+	if err != nil {
+		oc.logger.Errorf("TLSAuth: error creating OCSP request: %s", err)
+		return nil, err
+	}
+
+	httpRequest, err := http.NewRequest(http.MethodPost, server, bytes.NewBuffer(req))
+	if err != nil {
+		oc.logger.Error("TLSAuth: cannot create HTTP request for OCSP")
+		return nil, err
+	}
+
+	ocspURL, err := url.Parse(server)
+	if err != nil {
+		oc.logger.Error("TLSAuth: cannot parse OCSP URL")
+		return nil, err
+	}
+
+	httpRequest.Header.Add("Content-Type", "application/ocsp-request")
+	httpRequest.Header.Add("Accept", "application/ocsp-response")
+	httpRequest.Header.Add("Host", ocspURL.Host)
+
+	httpClient := &http.Client{}
+
+	// XXX: timeout, context?
+	httpResponse, err := httpClient.Do(httpRequest)
+	if err != nil {
+		oc.logger.Error("TLSAuth: cannot send HTTP request to OCSP")
+		return nil, err
+	}
+	defer httpResponse.Body.Close()
+
+	output, err := io.ReadAll(httpResponse.Body)
+	if err != nil {
+		oc.logger.Error("TLSAuth: cannot read HTTP response from OCSP")
+		return nil, err
+	}
+
+	ocspResponse, err := ocsp.ParseResponseForCert(output, cert, issuer)
+
+	return ocspResponse, err
+}
+
+// isRevokedBy checks if the client certificate is revoked by the issuer via 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 (oc *OCSPChecker) isRevokedBy(cert *x509.Certificate, issuer *x509.Certificate) (bool, bool) {
+	if cert.OCSPServer == nil || len(cert.OCSPServer) == 0 {
+		oc.logger.Infof("TLSAuth: no OCSP Server present in client certificate, skipping OCSP verification")
+		return false, true
+	}
+
+	for _, server := range cert.OCSPServer {
+		ocspResponse, err := oc.query(server, cert, issuer)
+		if err != nil {
+			oc.logger.Errorf("TLSAuth: error querying OCSP server %s: %s", server, err)
+			continue
+		}
+
+		switch ocspResponse.Status {
+		case ocsp.Good:
+			return false, true
+		case ocsp.Revoked:
+			oc.logger.Errorf("TLSAuth: client certificate is revoked by server %s", server)
+			return true, true
+		case ocsp.Unknown:
+			log.Debugf("unknown OCSP status for server %s", server)
+			continue
+		}
+	}
+
+	log.Infof("Could not get any valid OCSP response, assuming the cert is revoked")
+
+	return true, false
+}

pkg/apiserver/middlewares/v1/tls_auth.go

@@ -1,79 +1,24 @@
 package v1
 
 import (
-	"bytes"
-	"crypto"
 	"crypto/x509"
-	"encoding/pem"
 	"errors"
 	"fmt"
-	"io"
-	"net/http"
-	"net/url"
-	"os"
+	"slices"
 	"time"
 
 	"github.com/gin-gonic/gin"
 	log "github.com/sirupsen/logrus"
-	"golang.org/x/crypto/ocsp"
 )
 
 type TLSAuth struct {
 	AllowedOUs      []string
-	CrlPath         string
-	revocationCache map[string]cacheEntry
-	cacheExpiration time.Duration
+	crlChecker      *CRLChecker
+	ocspChecker     *OCSPChecker
+	revocationCache *RevocationCache
 	logger          *log.Entry
 }
 
-type cacheEntry struct {
-	revoked   bool
-	timestamp time.Time
-}
-
-func (ta *TLSAuth) ocspQuery(server string, cert *x509.Certificate, issuer *x509.Certificate) (*ocsp.Response, error) {
-	req, err := ocsp.CreateRequest(cert, issuer, &ocsp.RequestOptions{Hash: crypto.SHA256})
-	if err != nil {
-		ta.logger.Errorf("TLSAuth: error creating OCSP request: %s", err)
-		return nil, err
-	}
-
-	httpRequest, err := http.NewRequest(http.MethodPost, server, bytes.NewBuffer(req))
-	if err != nil {
-		ta.logger.Error("TLSAuth: cannot create HTTP request for OCSP")
-		return nil, err
-	}
-
-	ocspURL, err := url.Parse(server)
-	if err != nil {
-		ta.logger.Error("TLSAuth: cannot parse OCSP URL")
-		return nil, err
-	}
-
-	httpRequest.Header.Add("Content-Type", "application/ocsp-request")
-	httpRequest.Header.Add("Accept", "application/ocsp-response")
-	httpRequest.Header.Add("host", ocspURL.Host)
-
-	httpClient := &http.Client{}
-
-	httpResponse, err := httpClient.Do(httpRequest)
-	if err != nil {
-		ta.logger.Error("TLSAuth: cannot send HTTP request to OCSP")
-		return nil, err
-	}
-	defer httpResponse.Body.Close()
-
-	output, err := io.ReadAll(httpResponse.Body)
-	if err != nil {
-		ta.logger.Error("TLSAuth: cannot read HTTP response from OCSP")
-		return nil, err
-	}
-
-	ocspResponse, err := ocsp.ParseResponseForCert(output, cert, issuer)
-
-	return ocspResponse, err
-}
-
 func (ta *TLSAuth) isExpired(cert *x509.Certificate) bool {
 	now := time.Now().UTC()
 
@@ -90,211 +35,147 @@ func (ta *TLSAuth) isExpired(cert *x509.Certificate) bool {
 	return false
 }
 
-// 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, true
-	}
+// checkRevocationPath checks a single chain against OCSP and CRL
+func (ta *TLSAuth) checkRevocationPath(chain []*x509.Certificate) (error, bool) { //nolint:revive
+	// if we ever fail to check OCSP or CRL, we should not cache the result
+	couldCheck := true
 
-	for _, server := range cert.OCSPServer {
-		ocspResponse, err := ta.ocspQuery(server, cert, issuer)
-		if err != nil {
-			ta.logger.Errorf("TLSAuth: error querying OCSP server %s: %s", server, err)
-			continue
+	// starting from the root CA and moving towards the leaf certificate,
+	// check for revocation of intermediates too
+	for i := len(chain) - 1; i > 0; i-- {
+		cert := chain[i-1]
+		issuer := chain[i]
+
+		revokedByOCSP, checkedByOCSP := ta.ocspChecker.isRevokedBy(cert, issuer)
+		couldCheck = couldCheck && checkedByOCSP
+
+		if revokedByOCSP && checkedByOCSP {
+			return errors.New("certificate revoked by OCSP"), couldCheck
 		}
 
-		switch ocspResponse.Status {
-		case ocsp.Good:
-			return false, true
-		case ocsp.Revoked:
-			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
+		revokedByCRL, checkedByCRL := ta.crlChecker.isRevokedBy(cert, issuer)
+		couldCheck = couldCheck && checkedByCRL
+
+		if revokedByCRL && checkedByCRL {
+			return errors.New("certificate revoked by CRL"), couldCheck
 		}
 	}
 
-	log.Infof("Could not get any valid OCSP response, assuming the cert is revoked")
-
-	return true, false
+	return nil, couldCheck
 }
 
-// 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.Info("no crl_path, skipping CRL check")
-		return false, true
-	}
+func (ta *TLSAuth) setAllowedOu(allowedOus []string) error {
+	uniqueOUs := make(map[string]struct{})
 
-	crlContent, err := os.ReadFile(ta.CrlPath)
-	if err != nil {
-		ta.logger.Errorf("could not read CRL file, skipping check: %s", err)
-		return false, false
-	}
-
-	var crlBlock *pem.Block
-
-	for {
-		crlBlock, crlContent = pem.Decode(crlContent)
-		if crlBlock == nil {
-			break // no more PEM blocks
-		}
-
-		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
-		}
-
-		now := time.Now().UTC()
-
-		if now.After(crl.NextUpdate) {
-			ta.logger.Warn("CRL has expired, will still validate the cert against it.")
-		}
-
-		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
-			}
-		}
-	}
-
-	return false, true
-}
-
-func (ta *TLSAuth) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) {
-	sn := cert.SerialNumber.String()
-	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", sn, cacheValue.revoked)
-			return cacheValue.revoked, nil
-		}
-
-		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)
-	}
-
-	revokedByOCSP, cacheOCSP := ta.isOCSPRevoked(cert, issuer)
-	revokedByCRL, cacheCRL := ta.isCRLRevoked(cert)
-	revoked := revokedByOCSP || revokedByCRL
-
-	if cacheOCSP && cacheCRL {
-		ta.revocationCache[sn] = cacheEntry{
-			revoked:   revoked,
-			timestamp: time.Now().UTC(),
-		}
-	}
-
-	return revoked, nil
-}
-
-func (ta *TLSAuth) isInvalid(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) {
-	if ta.isExpired(cert) {
-		return true, nil
-	}
-
-	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
-		return true, fmt.Errorf("could not check for client certification revocation status: %w", err)
-	}
-
-	return revoked, nil
-}
-
-func (ta *TLSAuth) SetAllowedOu(allowedOus []string) error {
 	for _, ou := range allowedOus {
 		// disallow empty ou
 		if ou == "" {
-			return errors.New("empty ou isn't allowed")
+			return errors.New("allowed_ou configuration contains invalid empty string")
 		}
 
-		// drop & warn on duplicate ou
-		ok := true
-
-		for _, validOu := range ta.AllowedOUs {
-			if validOu == ou {
-				ta.logger.Warningf("dropping duplicate ou %s", ou)
-
-				ok = false
-			}
+		if _, exists := uniqueOUs[ou]; exists {
+			ta.logger.Warningf("dropping duplicate ou %s", ou)
+			continue
 		}
 
-		if ok {
-			ta.AllowedOUs = append(ta.AllowedOUs, ou)
-		}
+		uniqueOUs[ou] = struct{}{}
+
+		ta.AllowedOUs = append(ta.AllowedOUs, ou)
 	}
 
 	return nil
 }
 
-func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) {
+func (ta *TLSAuth) checkAllowedOU(ous []string) error {
+	for _, ou := range ous {
+		if slices.Contains(ta.AllowedOUs, ou) {
+			return nil
+		}
+	}
+
+	return fmt.Errorf("client certificate OU %v doesn't match expected OU %v", ous, ta.AllowedOUs)
+}
+
+func (ta *TLSAuth) ValidateCert(c *gin.Context) (string, error) {
 	// Checks cert validity, Returns true + CN if client cert matches requested OU
-	var clientCert *x509.Certificate
+	var leaf *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
-		return false, "", nil
+		return "", errors.New("no certificate in request")
 	}
 
-	if len(c.Request.TLS.VerifiedChains) > 0 {
-		validOU := false
-		clientCert = c.Request.TLS.VerifiedChains[0][0]
-
-		for _, ou := range clientCert.Subject.OrganizationalUnit {
-			for _, allowedOu := range ta.AllowedOUs {
-				if allowedOu == ou {
-					validOU = true
-					break
-				}
-			}
-		}
-
-		if !validOU {
-			return false, "", fmt.Errorf("client certificate OU (%v) doesn't match expected OU (%v)",
-				clientCert.Subject.OrganizationalUnit, ta.AllowedOUs)
-		}
-
-		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 revocation status: %w", err)
-		}
-
-		if 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)
-
-		return true, clientCert.Subject.CommonName, nil
+	if len(c.Request.TLS.VerifiedChains) == 0 {
+		return "", errors.New("no verified cert in request")
 	}
 
-	return false, "", errors.New("no verified cert in request")
+	// although there can be multiple chains, the leaf certificate is the same
+	// we take the first one
+	leaf = c.Request.TLS.VerifiedChains[0][0]
+
+	if err := ta.checkAllowedOU(leaf.Subject.OrganizationalUnit); err != nil {
+		return "", err
+	}
+
+	if ta.isExpired(leaf) {
+		return "", errors.New("client certificate is expired")
+	}
+
+	if validErr, cached := ta.revocationCache.Get(leaf); cached {
+		if validErr != nil {
+			return "", fmt.Errorf("(cache) %w", validErr)
+		}
+
+		return leaf.Subject.CommonName, nil
+	}
+
+	okToCache := true
+
+	var validErr error
+
+	var couldCheck bool
+
+	for _, chain := range c.Request.TLS.VerifiedChains {
+		validErr, couldCheck = ta.checkRevocationPath(chain)
+		okToCache = okToCache && couldCheck
+
+		if validErr != nil {
+			break
+		}
+	}
+
+	if okToCache {
+		ta.revocationCache.Set(leaf, validErr)
+	}
+
+	if validErr != nil {
+		return "", validErr
+	}
+
+	return leaf.Subject.CommonName, nil
 }
 
 func NewTLSAuth(allowedOus []string, crlPath string, cacheExpiration time.Duration, logger *log.Entry) (*TLSAuth, error) {
+	var err error
+
+	cache := NewRevocationCache(cacheExpiration, logger)
+
 	ta := &TLSAuth{
-		revocationCache: map[string]cacheEntry{},
-		cacheExpiration: cacheExpiration,
-		CrlPath:         crlPath,
+		revocationCache: cache,
+		ocspChecker:     NewOCSPChecker(logger),
 		logger:          logger,
 	}
 
-	err := ta.SetAllowedOu(allowedOus)
-	if err != nil {
+	switch crlPath {
+	case "":
+		logger.Info("no crl_path, skipping CRL checks")
+	default:
+		ta.crlChecker, err = NewCRLChecker(crlPath, cache.Empty, logger)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	if err := ta.setAllowedOu(allowedOus); err != nil {
 		return nil, err
 	}
 

test/bats/11_bouncers_tls.bats

@@ -3,6 +3,19 @@
 
 set -u
 
+# root: root CA
+# inter: intermediate CA
+# inter_rev: intermediate CA revoked by root (CRL3)
+# leaf: valid client cert
+# leaf_rev1: client cert revoked by inter (CRL1)
+# leaf_rev2: client cert revoked by inter (CRL2)
+# leaf_rev3: client cert (indirectly) revoked by root
+#
+# CRL1: inter revokes leaf_rev1
+# CRL2: inter revokes leaf_rev2
+# CRL3: root revokes inter_rev
+# CRL4: root revokes leaf, but is ignored
+
 setup_file() {
     load "../lib/setup_file.sh"
     ./instance-data load
@@ -10,43 +23,96 @@ setup_file() {
     tmpdir="$BATS_FILE_TMPDIR"
     export tmpdir
 
-    CFDIR="${BATS_TEST_DIRNAME}/testdata/cfssl"
+    CFDIR="$BATS_TEST_DIRNAME/testdata/cfssl"
     export CFDIR
 
-    # Generate the CA
-    cfssl gencert --initca "${CFDIR}/ca.json" 2>/dev/null | cfssljson --bare "${tmpdir}/ca"
+    # Root CA
+    cfssl gencert -loglevel 2 \
+        --initca "$CFDIR/ca_root.json" \
+        | cfssljson --bare "$tmpdir/root"
 
-    # 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"
+    # Intermediate CAs (valid or revoked)
+    for cert in "inter" "inter_rev"; do
+        cfssl gencert -loglevel 2 \
+            --initca "$CFDIR/ca_intermediate.json" \
+            | cfssljson --bare "$tmpdir/$cert"
 
-    # 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"
-
-    # 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"
-
-    # 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"
-
-    # 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"
-
-    # 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}"
-        cfssl certinfo -cert "${tmpdir}/${cert_name}.pem" | jq -r '.serial_number' > "${tmpdir}/serials_${cert_name}.txt"
+        cfssl sign -loglevel 2 \
+            -ca "$tmpdir/root.pem" -ca-key "$tmpdir/root-key.pem" \
+            -config "$CFDIR/profiles.json" -profile intermediate_ca "$tmpdir/$cert.csr" \
+            | cfssljson --bare "$tmpdir/$cert"
     done
 
-    # Generate separate CRL blocks and concatenate them
-    for cert_name in "revoked_1" "revoked_2"; do
-        echo '-----BEGIN X509 CRL-----' > "${tmpdir}/crl_${cert_name}.pem"
-        cfssl gencrl "${tmpdir}/serials_${cert_name}.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" >> "${tmpdir}/crl_${cert_name}.pem"
-        echo '-----END X509 CRL-----' >> "${tmpdir}/crl_${cert_name}.pem"
-    done
-    cat "${tmpdir}/crl_revoked_1.pem" "${tmpdir}/crl_revoked_2.pem" >"${tmpdir}/crl.pem"
+    # Server cert for crowdsec with the intermediate
+    cfssl gencert -loglevel 2 \
+        -ca "$tmpdir/inter.pem" -ca-key "$tmpdir/inter-key.pem" \
+        -config "$CFDIR/profiles.json" -profile=server "$CFDIR/server.json" \
+        | cfssljson --bare "$tmpdir/server"
 
-    cat "${tmpdir}/ca.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem"
+    # Client certs (valid or revoked)
+    for cert in "leaf" "leaf_rev1" "leaf_rev2"; do
+        cfssl gencert -loglevel 3 \
+            -ca "$tmpdir/inter.pem" -ca-key "$tmpdir/inter-key.pem" \
+            -config "$CFDIR/profiles.json" -profile=client \
+            "$CFDIR/bouncer.json" \
+            | cfssljson --bare "$tmpdir/$cert"
+    done
+
+    # Client cert (by revoked inter)
+    cfssl gencert -loglevel 3 \
+        -ca "$tmpdir/inter_rev.pem" -ca-key "$tmpdir/inter_rev-key.pem" \
+        -config "$CFDIR/profiles.json" -profile=client \
+        "$CFDIR/bouncer.json" \
+        | cfssljson --bare "$tmpdir/leaf_rev3"
+
+    # Bad client cert (invalid OU)
+    cfssl gencert -loglevel 3 \
+        -ca "$tmpdir/inter.pem" -ca-key "$tmpdir/inter-key.pem" \
+        -config "$CFDIR/profiles.json" -profile=client \
+        "$CFDIR/bouncer_invalid.json" \
+        | cfssljson --bare "$tmpdir/leaf_bad_ou"
+
+    # Bad client cert (directly signed by the CA, it should be refused by crowdsec as it uses the intermediate)
+    cfssl gencert -loglevel 3 \
+        -ca "$tmpdir/root.pem" -ca-key "$tmpdir/root-key.pem" \
+        -config "$CFDIR/profiles.json" -profile=client \
+        "$CFDIR/bouncer.json" \
+        | cfssljson --bare "$tmpdir/leaf_invalid"
+
+    truncate -s 0 "$tmpdir/crl.pem"
+
+    # Revoke certs
+    {
+        echo '-----BEGIN X509 CRL-----'
+        cfssl gencrl \
+            <(cert_serial_number "$tmpdir/leaf_rev1.pem") \
+            "$tmpdir/inter.pem" \
+            "$tmpdir/inter-key.pem"
+        echo '-----END X509 CRL-----'
+
+        echo '-----BEGIN X509 CRL-----'
+        cfssl gencrl \
+            <(cert_serial_number "$tmpdir/leaf_rev2.pem") \
+            "$tmpdir/inter.pem" \
+            "$tmpdir/inter-key.pem"
+        echo '-----END X509 CRL-----'
+
+        echo '-----BEGIN X509 CRL-----'
+        cfssl gencrl \
+            <(cert_serial_number "$tmpdir/inter_rev.pem") \
+            "$tmpdir/root.pem" \
+            "$tmpdir/root-key.pem"
+        echo '-----END X509 CRL-----'
+
+        echo '-----BEGIN X509 CRL-----'
+        cfssl gencrl \
+            <(cert_serial_number "$tmpdir/leaf.pem") \
+            "$tmpdir/root.pem" \
+            "$tmpdir/root-key.pem"
+        echo '-----END X509 CRL-----'
+    } >> "$tmpdir/crl.pem"
+
+    cat "$tmpdir/root.pem" "$tmpdir/inter.pem" > "$tmpdir/bundle.pem"
 
     config_set '
         .api.server.tls.cert_file=strenv(tmpdir) + "/server.pem" |
@@ -79,8 +145,12 @@ teardown() {
     assert_output "[]"
 }
 
-@test "simulate one bouncer request with a valid cert" {
-    rune -0 curl -s --cert "${tmpdir}/bouncer.pem" --key "${tmpdir}/bouncer-key.pem" --cacert "${tmpdir}/bundle.pem" https://localhost:8080/v1/decisions\?ip=42.42.42.42
+@test "simulate a bouncer request with a valid cert" {
+    rune -0 curl -f -s \
+        --cert "$tmpdir/leaf.pem" \
+        --key "$tmpdir/leaf-key.pem" \
+        --cacert "$tmpdir/bundle.pem" \
+        https://localhost:8080/v1/decisions\?ip=42.42.42.42
     assert_output "null"
     rune -0 cscli bouncers list -o json
     rune -0 jq '. | length' <(output)
@@ -91,27 +161,54 @@ teardown() {
     rune cscli bouncers delete localhost@127.0.0.1
 }
 
-@test "simulate one bouncer request with an invalid cert" {
-    rune curl -s --cert "${tmpdir}/bouncer_invalid.pem" --key "${tmpdir}/bouncer_invalid-key.pem" --cacert "${tmpdir}/ca-key.pem" https://localhost:8080/v1/decisions\?ip=42.42.42.42
+@test "simulate a bouncer request with an invalid cert" {
+    rune -77 curl -f -s \
+        --cert "$tmpdir/leaf_invalid.pem" \
+        --key "$tmpdir/leaf_invalid-key.pem" \
+        --cacert "$tmpdir/root-key.pem" \
+        https://localhost:8080/v1/decisions\?ip=42.42.42.42
     rune -0 cscli bouncers list -o json
     assert_output "[]"
 }
 
-@test "simulate one bouncer request with an invalid OU" {
-    rune curl -s --cert "${tmpdir}/bouncer_bad_ou.pem" --key "${tmpdir}/bouncer_bad_ou-key.pem" --cacert "${tmpdir}/bundle.pem" https://localhost:8080/v1/decisions\?ip=42.42.42.42
+@test "simulate a bouncer request with an invalid OU" {
+    rune -0 curl -s \
+        --cert "$tmpdir/leaf_bad_ou.pem" \
+        --key "$tmpdir/leaf_bad_ou-key.pem" \
+        --cacert "$tmpdir/bundle.pem" \
+        https://localhost:8080/v1/decisions\?ip=42.42.42.42
+    assert_json '{message:"access forbidden"}'
     rune -0 cscli bouncers list -o json
     assert_output "[]"
 }
 
-@test "simulate one bouncer request with a revoked certificate" {
+@test "simulate a bouncer request with a revoked certificate" {
     # we have two certificates revoked by different CRL blocks
-    for cert_name in "revoked_1" "revoked_2"; do
+    # we connect twice to test the cache too
+    for cert in "leaf_rev1" "leaf_rev2" "leaf_rev1" "leaf_rev2"; 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"
+        rune -0 curl -s \
+            --cert "$tmpdir/$cert.pem" \
+            --key "$tmpdir/$cert-key.pem" \
+            --cacert "$tmpdir/bundle.pem" \
+            https://localhost:8080/v1/decisions\?ip=42.42.42.42
+        assert_log --partial "certificate revoked by CRL"
         assert_output --partial "access forbidden"
         rune -0 cscli bouncers list -o json
         assert_output "[]"
     done
 }
+
+# vvv this test must be last, or it can break the ones that follow
+
+@test "allowed_ou can't contain an empty string" {
+    ./instance-crowdsec stop
+    config_set '
+        .common.log_media="stdout" |
+        .api.server.tls.bouncers_allowed_ou=["bouncer-ou", ""]
+    '
+    rune -1 wait-for "$CROWDSEC"
+    assert_stderr --partial "allowed_ou configuration contains invalid empty string"
+}
+
+# ^^^ this test must be last, or it can break the ones that follow

test/bats/30_machines_tls.bats

@@ -3,6 +3,20 @@
 
 set -u
 
+
+# root: root CA
+# inter: intermediate CA
+# inter_rev: intermediate CA revoked by root (CRL3)
+# leaf: valid client cert
+# leaf_rev1: client cert revoked by inter (CRL1)
+# leaf_rev2: client cert revoked by inter (CRL2)
+# leaf_rev3: client cert (indirectly) revoked by root
+#
+# CRL1: inter revokes leaf_rev1
+# CRL2: inter revokes leaf_rev2
+# CRL3: root revokes inter_rev
+# CRL4: root revokes leaf, but is ignored
+
 setup_file() {
     load "../lib/setup_file.sh"
     ./instance-data load
@@ -13,43 +27,96 @@ setup_file() {
     tmpdir="$BATS_FILE_TMPDIR"
     export tmpdir
 
-    CFDIR="${BATS_TEST_DIRNAME}/testdata/cfssl"
+    CFDIR="$BATS_TEST_DIRNAME/testdata/cfssl"
     export CFDIR
 
-    # Generate the CA
-    cfssl gencert --initca "${CFDIR}/ca.json" 2>/dev/null | cfssljson --bare "${tmpdir}/ca"
+    # Root CA
+    cfssl gencert -loglevel 2 \
+        --initca "$CFDIR/ca_root.json" \
+        | cfssljson --bare "$tmpdir/root"
 
-    # 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"
+    # Intermediate CAs (valid or revoked)
+    for cert in "inter" "inter_rev"; do
+        cfssl gencert -loglevel 2 \
+            --initca "$CFDIR/ca_intermediate.json" \
+            | cfssljson --bare "$tmpdir/$cert"
 
-    # 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"
-
-    # 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"
-
-    # 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"
-
-    # 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"
-
-    # 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}"
-        cfssl certinfo -cert "${tmpdir}/${cert_name}.pem" | jq -r '.serial_number' > "${tmpdir}/serials_${cert_name}.txt"
+        cfssl sign -loglevel 2 \
+            -ca "$tmpdir/root.pem" -ca-key "$tmpdir/root-key.pem" \
+            -config "$CFDIR/profiles.json" -profile intermediate_ca "$tmpdir/$cert.csr" \
+            | cfssljson --bare "$tmpdir/$cert"
     done
 
-    # Generate separate CRL blocks and concatenate them
-    for cert_name in "revoked_1" "revoked_2"; do
-        echo '-----BEGIN X509 CRL-----' > "${tmpdir}/crl_${cert_name}.pem"
-        cfssl gencrl "${tmpdir}/serials_${cert_name}.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" >> "${tmpdir}/crl_${cert_name}.pem"
-        echo '-----END X509 CRL-----' >> "${tmpdir}/crl_${cert_name}.pem"
-    done
-    cat "${tmpdir}/crl_revoked_1.pem" "${tmpdir}/crl_revoked_2.pem" >"${tmpdir}/crl.pem"
+    # Server cert for crowdsec with the intermediate
+    cfssl gencert -loglevel 2 \
+        -ca "$tmpdir/inter.pem" -ca-key "$tmpdir/inter-key.pem" \
+        -config "$CFDIR/profiles.json" -profile=server "$CFDIR/server.json" \
+        | cfssljson --bare "$tmpdir/server"
 
-    cat "${tmpdir}/ca.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem"
+    # Client certs (valid or revoked)
+    for cert in "leaf" "leaf_rev1" "leaf_rev2"; do
+        cfssl gencert -loglevel 3 \
+            -ca "$tmpdir/inter.pem" -ca-key "$tmpdir/inter-key.pem" \
+            -config "$CFDIR/profiles.json" -profile=client \
+            "$CFDIR/agent.json" \
+            | cfssljson --bare "$tmpdir/$cert"
+    done
+
+    # Client cert (by revoked inter)
+    cfssl gencert -loglevel 3 \
+        -ca "$tmpdir/inter_rev.pem" -ca-key "$tmpdir/inter_rev-key.pem" \
+        -config "$CFDIR/profiles.json" -profile=client \
+        "$CFDIR/agent.json" \
+        | cfssljson --bare "$tmpdir/leaf_rev3"
+
+    # Bad client cert (invalid OU)
+    cfssl gencert -loglevel 3 \
+        -ca "$tmpdir/inter.pem" -ca-key "$tmpdir/inter-key.pem" \
+        -config "$CFDIR/profiles.json" -profile=client \
+        "$CFDIR/agent_invalid.json" \
+        | cfssljson --bare "$tmpdir/leaf_bad_ou"
+
+    # Bad client cert (directly signed by the CA, it should be refused by crowdsec as it uses the intermediate)
+    cfssl gencert -loglevel 3 \
+        -ca "$tmpdir/root.pem" -ca-key "$tmpdir/root-key.pem" \
+        -config "$CFDIR/profiles.json" -profile=client \
+        "$CFDIR/agent.json" \
+        | cfssljson --bare "$tmpdir/leaf_invalid"
+
+    truncate -s 0 "$tmpdir/crl.pem"
+
+    # Revoke certs
+    {
+        echo '-----BEGIN X509 CRL-----'
+        cfssl gencrl \
+            <(cert_serial_number "$tmpdir/leaf_rev1.pem") \
+            "$tmpdir/inter.pem" \
+            "$tmpdir/inter-key.pem"
+        echo '-----END X509 CRL-----'
+
+        echo '-----BEGIN X509 CRL-----'
+        cfssl gencrl \
+            <(cert_serial_number "$tmpdir/leaf_rev2.pem") \
+            "$tmpdir/inter.pem" \
+            "$tmpdir/inter-key.pem"
+        echo '-----END X509 CRL-----'
+
+        echo '-----BEGIN X509 CRL-----'
+        cfssl gencrl \
+            <(cert_serial_number "$tmpdir/inter_rev.pem") \
+            "$tmpdir/root.pem" \
+            "$tmpdir/root-key.pem"
+        echo '-----END X509 CRL-----'
+
+        echo '-----BEGIN X509 CRL-----'
+        cfssl gencrl \
+            <(cert_serial_number "$tmpdir/leaf.pem") \
+            "$tmpdir/root.pem" \
+            "$tmpdir/root-key.pem"
+        echo '-----END X509 CRL-----'
+    } >> "$tmpdir/crl.pem"
+
+    cat "$tmpdir/root.pem" "$tmpdir/inter.pem" > "$tmpdir/bundle.pem"
 
     config_set '
         .api.server.tls.cert_file=strenv(tmpdir) + "/server.pem" |
@@ -62,7 +129,7 @@ setup_file() {
     # remove all machines
 
     for machine in $(cscli machines list -o json | jq -r '.[].machineId'); do
-        cscli machines delete "${machine}" >/dev/null 2>&1
+        cscli machines delete "$machine" >/dev/null 2>&1
     done
 
     config_disable_agent
@@ -106,30 +173,32 @@ teardown() {
 }
 
 @test "invalid OU for agent" {
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" '
+    config_set "$CONFIG_DIR/local_api_credentials.yaml" '
         .ca_cert_path=strenv(tmpdir) + "/bundle.pem" |
-        .key_path=strenv(tmpdir) + "/agent_bad_ou-key.pem" |
-        .cert_path=strenv(tmpdir) + "/agent_bad_ou.pem" |
+        .key_path=strenv(tmpdir) + "/leaf_bad_ou-key.pem" |
+        .cert_path=strenv(tmpdir) + "/leaf_bad_ou.pem" |
         .url="https://127.0.0.1:8080"
     '
 
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)'
+    config_set "$CONFIG_DIR/local_api_credentials.yaml" 'del(.login,.password)'
     ./instance-crowdsec start
     rune -0 cscli machines list -o json
     assert_output '[]'
 }
 
 @test "we have exactly one machine registered with TLS" {
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" '
+    config_set "$CONFIG_DIR/local_api_credentials.yaml" '
         .ca_cert_path=strenv(tmpdir) + "/bundle.pem" |
-        .key_path=strenv(tmpdir) + "/agent-key.pem" |
-        .cert_path=strenv(tmpdir) + "/agent.pem" |
+        .key_path=strenv(tmpdir) + "/leaf-key.pem" |
+        .cert_path=strenv(tmpdir) + "/leaf.pem" |
         .url="https://127.0.0.1:8080"
     '
 
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)'
+    config_set "$CONFIG_DIR/local_api_credentials.yaml" 'del(.login,.password)'
     ./instance-crowdsec start
     rune -0 cscli lapi status
+    # second connection, test the tls cache
+    rune -0 cscli lapi status
     rune -0 cscli machines list -o json
     rune -0 jq -c '[. | length, .[0].machineId[0:32], .[0].isValidated, .[0].ipAddress, .[0].auth_type]' <(output)
 
@@ -154,24 +223,24 @@ teardown() {
 
     # TLS cannot be used with a unix socket
 
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" '
+    config_set "$CONFIG_DIR/local_api_credentials.yaml" '
         .ca_cert_path=strenv(tmpdir) + "/bundle.pem"
     '
 
     rune -1 cscli lapi status
     assert_stderr --partial "loading api client: cannot use TLS with a unix socket"
 
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" '
+    config_set "$CONFIG_DIR/local_api_credentials.yaml" '
         del(.ca_cert_path) |
-        .key_path=strenv(tmpdir) + "/agent-key.pem"
+        .key_path=strenv(tmpdir) + "/leaf-key.pem"
     '
 
     rune -1 cscli lapi status
     assert_stderr --partial "loading api client: cannot use TLS with a unix socket"
 
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" '
+    config_set "$CONFIG_DIR/local_api_credentials.yaml" '
         del(.key_path) |
-        .cert_path=strenv(tmpdir) + "/agent.pem"
+        .cert_path=strenv(tmpdir) + "/leaf.pem"
     '
 
     rune -1 cscli lapi status
@@ -181,13 +250,13 @@ teardown() {
 }
 
 @test "invalid cert for agent" {
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" '
+    config_set "$CONFIG_DIR/local_api_credentials.yaml" '
         .ca_cert_path=strenv(tmpdir) + "/bundle.pem" |
-        .key_path=strenv(tmpdir) + "/agent_invalid-key.pem" |
-        .cert_path=strenv(tmpdir) + "/agent_invalid.pem" |
+        .key_path=strenv(tmpdir) + "/leaf_invalid-key.pem" |
+        .cert_path=strenv(tmpdir) + "/leaf_invalid.pem" |
         .url="https://127.0.0.1:8080"
     '
-    config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)'
+    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
@@ -196,22 +265,35 @@ teardown() {
 
 @test "revoked cert for agent" {
     # we have two certificates revoked by different CRL blocks
-    for cert_name in "revoked_1" "revoked_2"; do
+    # we connect twice to test the cache too
+    for cert in "leaf_rev1" "leaf_rev2" "leaf_rev1" "leaf_rev2"; do
         truncate_log
-        cert_name="$cert_name" config_set "${CONFIG_DIR}/local_api_credentials.yaml" '
+        cert="$cert" 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" |
+            .key_path=strenv(tmpdir) + "/" + strenv(cert) + "-key.pem" |
+            .cert_path=strenv(tmpdir) + "/" + strenv(cert) + ".pem" |
             .url="https://127.0.0.1:8080"
         '
 
-        config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)'
+        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"
+        assert_log --partial "certificate revoked by CRL"
         rune -0 cscli machines list -o json
         assert_output '[]'
         ./instance-crowdsec stop
     done
 }
+
+# vvv this test must be last, or it can break the ones that follow
+
+@test "allowed_ou can't contain an empty string" {
+    config_set '
+        .common.log_media="stdout" |
+        .api.server.tls.agents_allowed_ou=["agent-ou", ""]
+    '
+    rune -1 wait-for "$CROWDSEC"
+    assert_stderr --partial "allowed_ou configuration contains invalid empty string"
+}
+
+# ^^^ this test must be last, or it can break the ones that follow

test/bats/testdata/cfssl/agent.json

@@ -1,10 +1,10 @@
 {
-    "CN": "localhost",
-    "key": {
-      "algo": "rsa",
-      "size": 2048
-    },
-    "names": [
+  "CN": "localhost",
+  "key": {
+    "algo": "rsa",
+    "size": 2048
+  },
+  "names": [
     {
       "C": "FR",
       "L": "Paris",
@@ -12,5 +12,5 @@
       "OU": "agent-ou",
       "ST": "France"
     }
-    ]
-  }
+  ]
+}

test/bats/testdata/cfssl/agent_invalid.json

@@ -1,10 +1,10 @@
 {
-    "CN": "localhost",
-    "key": {
-      "algo": "rsa",
-      "size": 2048
-    },
-    "names": [
+  "CN": "localhost",
+  "key": {
+    "algo": "rsa",
+    "size": 2048
+  },
+  "names": [
     {
       "C": "FR",
       "L": "Paris",
@@ -12,5 +12,5 @@
       "OU": "this-is-not-the-ou-youre-looking-for",
       "ST": "France"
     }
-    ]
-  }
+  ]
+}

test/bats/testdata/cfssl/bouncer.json

@@ -1,10 +1,10 @@
 {
-    "CN": "localhost",
-    "key": {
-      "algo": "rsa",
-      "size": 2048
-    },
-    "names": [
+  "CN": "localhost",
+  "key": {
+    "algo": "rsa",
+    "size": 2048
+  },
+  "names": [
     {
       "C": "FR",
       "L": "Paris",
@@ -12,5 +12,5 @@
       "OU": "bouncer-ou",
       "ST": "France"
     }
-    ]
-  }
+  ]
+}

test/bats/testdata/cfssl/bouncer_invalid.json

@@ -1,10 +1,10 @@
 {
-    "CN": "localhost",
-    "key": {
-      "algo": "rsa",
-      "size": 2048
-    },
-    "names": [
+  "CN": "localhost",
+  "key": {
+    "algo": "rsa",
+    "size": 2048
+  },
+  "names": [
     {
       "C": "FR",
       "L": "Paris",
@@ -12,5 +12,5 @@
       "OU": "this-is-not-the-ou-youre-looking-for",
       "ST": "France"
     }
-    ]
-  }
+  ]
+}

test/bats/testdata/cfssl/ca.json

@@ -1,16 +0,0 @@
-{
-  "CN": "CrowdSec Test CA",
-  "key": {
-    "algo": "rsa",
-    "size": 2048
-  },
-  "names": [
-  {
-    "C": "FR",
-    "L": "Paris",
-    "O": "Crowdsec",
-    "OU": "Crowdsec",
-    "ST": "France"
-  }
- ]
-}

test/bats/testdata/cfssl/ca_intermediate.json

@@ -1,10 +1,10 @@
 {
-    "CN": "CrowdSec Test CA Intermediate",
-    "key": {
-      "algo": "rsa",
-      "size": 2048
-    },
-    "names": [
+  "CN": "CrowdSec Test CA Intermediate",
+  "key": {
+    "algo": "rsa",
+    "size": 2048
+  },
+  "names": [
     {
       "C": "FR",
       "L": "Paris",
@@ -12,8 +12,8 @@
       "OU": "Crowdsec Intermediate",
       "ST": "France"
     }
-   ],
-   "ca": {
+  ],
+  "ca": {
     "expiry": "42720h"
   }
-  }
+}

test/bats/testdata/cfssl/ca_root.json

@@ -0,0 +1,16 @@
+{
+  "CN": "CrowdSec Test CA",
+  "key": {
+    "algo": "rsa",
+    "size": 2048
+  },
+  "names": [
+    {
+      "C": "FR",
+      "L": "Paris",
+      "O": "Crowdsec",
+      "OU": "Crowdsec",
+      "ST": "France"
+    }
+  ]
+}

test/bats/testdata/cfssl/profiles.json

@@ -1,44 +1,37 @@
 {
-    "signing": {
-      "default": {
+  "signing": {
+    "default": {
+      "expiry": "8760h"
+    },
+    "profiles": {
+      "intermediate_ca": {
+        "usages": [
+          "signing",
+          "key encipherment",
+          "cert sign",
+          "crl sign",
+          "server auth",
+          "client auth"
+        ],
+        "expiry": "8760h",
+        "ca_constraint": {
+          "is_ca": true,
+          "max_path_len": 0,
+          "max_path_len_zero": true
+        }
+      },
+      "server": {
+        "usages": [
+          "server auth"
+        ],
         "expiry": "8760h"
       },
-      "profiles": {
-        "intermediate_ca": {
-          "usages": [
-              "signing",
-              "digital signature",
-              "key encipherment",
-              "cert sign",
-              "crl sign",
-              "server auth",
-              "client auth"
-          ],
-          "expiry": "8760h",
-          "ca_constraint": {
-              "is_ca": true,
-              "max_path_len": 0, 
-              "max_path_len_zero": true
-          }
-        },
-        "server": {
-          "usages": [
-            "signing",
-            "digital signing",
-            "key encipherment",
-            "server auth"
-          ],
-          "expiry": "8760h"
-        },
-        "client": {
-          "usages": [
-            "signing",
-            "digital signature",
-            "key encipherment", 
-            "client auth"
-          ],
-          "expiry": "8760h"
-        }
+      "client": {
+        "usages": [
+          "client auth"
+        ],
+        "expiry": "8760h"
       }
     }
-  }
+  }
+}

test/bats/testdata/cfssl/server.json

@@ -1,10 +1,10 @@
 {
-    "CN": "localhost",
-    "key": {
-      "algo": "rsa",
-      "size": 2048
-    },
-    "names": [
+  "CN": "localhost",
+  "key": {
+    "algo": "rsa",
+    "size": 2048
+  },
+  "names": [
     {
       "C": "FR",
       "L": "Paris",
@@ -12,9 +12,9 @@
       "OU": "Crowdsec Server",
       "ST": "France"
     }
-    ],
-    "hosts": [
-      "127.0.0.1",
-      "localhost"
-    ]
-  }
+  ],
+  "hosts": [
+    "127.0.0.1",
+    "localhost"
+  ]
+}

test/lib/setup_file.sh

@@ -155,6 +155,11 @@ assert_log() {
 }
 export -f assert_log
 
+cert_serial_number() {
+    cfssl certinfo -cert "$1" | jq -r '.serial_number'
+}
+export -f cert_serial_number
+
 # Compare ignoring the key order, and allow "expected" without quoted identifiers.
 # Preserve the output variable in case the following commands require it.
 assert_json() {