From a3bd9baec1ff5faf1ae3e126d45bfa68601bd805 Mon Sep 17 00:00:00 2001 From: mmetc <92726601+mmetc@users.noreply.github.com> Date: Wed, 26 Jun 2024 10:41:30 +0200 Subject: [PATCH] improved tls middleware revocation checks (#3034) --- .golangci.yml | 4 - docker/test/tests/test_tls.py | 2 +- pkg/apiclient/alerts_service.go | 2 - pkg/apiclient/auth_service.go | 2 - pkg/apiserver/middlewares/v1/api_key.go | 12 +- pkg/apiserver/middlewares/v1/cache.go | 99 +++++ pkg/apiserver/middlewares/v1/crl.go | 145 ++++++++ pkg/apiserver/middlewares/v1/jwt.go | 31 +- pkg/apiserver/middlewares/v1/ocsp.go | 100 ++++++ pkg/apiserver/middlewares/v1/tls_auth.go | 339 ++++++------------ test/bats/11_bouncers_tls.bats | 179 ++++++--- test/bats/30_machines_tls.bats | 192 +++++++--- test/bats/testdata/cfssl/agent.json | 16 +- test/bats/testdata/cfssl/agent_invalid.json | 16 +- test/bats/testdata/cfssl/bouncer.json | 16 +- test/bats/testdata/cfssl/bouncer_invalid.json | 16 +- test/bats/testdata/cfssl/ca.json | 16 - ...intermediate.json => ca_intermediate.json} | 18 +- test/bats/testdata/cfssl/ca_root.json | 16 + test/bats/testdata/cfssl/profiles.json | 71 ++-- test/bats/testdata/cfssl/server.json | 24 +- test/lib/setup_file.sh | 5 + 22 files changed, 850 insertions(+), 471 deletions(-) create mode 100644 pkg/apiserver/middlewares/v1/cache.go create mode 100644 pkg/apiserver/middlewares/v1/crl.go create mode 100644 pkg/apiserver/middlewares/v1/ocsp.go delete mode 100644 test/bats/testdata/cfssl/ca.json rename test/bats/testdata/cfssl/{intermediate.json => ca_intermediate.json} (53%) create mode 100644 test/bats/testdata/cfssl/ca_root.json diff --git a/.golangci.yml b/.golangci.yml index ea8712054..d89c8e9ed 100644 --- a/.golangci.yml +++ b/.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 diff --git a/docker/test/tests/test_tls.py b/docker/test/tests/test_tls.py index fe899b000..d2f512fcb 100644 --- a/docker/test/tests/test_tls.py +++ b/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' diff --git a/pkg/apiclient/alerts_service.go b/pkg/apiclient/alerts_service.go index ad75dd393..a3da84d30 100644 --- a/pkg/apiclient/alerts_service.go +++ b/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 { diff --git a/pkg/apiclient/auth_service.go b/pkg/apiclient/auth_service.go index e43503852..e7a423cfd 100644 --- a/pkg/apiclient/auth_service.go +++ b/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 diff --git a/pkg/apiserver/middlewares/v1/api_key.go b/pkg/apiserver/middlewares/v1/api_key.go index 314a4da10..e822666db 100644 --- a/pkg/apiserver/middlewares/v1/api_key.go +++ b/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() diff --git a/pkg/apiserver/middlewares/v1/cache.go b/pkg/apiserver/middlewares/v1/cache.go new file mode 100644 index 000000000..a058ec403 --- /dev/null +++ b/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) +} diff --git a/pkg/apiserver/middlewares/v1/crl.go b/pkg/apiserver/middlewares/v1/crl.go new file mode 100644 index 000000000..f85a41099 --- /dev/null +++ b/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 +} diff --git a/pkg/apiserver/middlewares/v1/jwt.go b/pkg/apiserver/middlewares/v1/jwt.go index 735c5f058..64406deff 100644 --- a/pkg/apiserver/middlewares/v1/jwt.go +++ b/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") } diff --git a/pkg/apiserver/middlewares/v1/ocsp.go b/pkg/apiserver/middlewares/v1/ocsp.go new file mode 100644 index 000000000..24557bfda --- /dev/null +++ b/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 +} diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index c2fcc9c72..673c8d0cd 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/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 } diff --git a/test/bats/11_bouncers_tls.bats b/test/bats/11_bouncers_tls.bats index 6b4986d45..765e93ebe 100644 --- a/test/bats/11_bouncers_tls.bats +++ b/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 diff --git a/test/bats/30_machines_tls.bats b/test/bats/30_machines_tls.bats index 522317045..ef2915e38 100644 --- a/test/bats/30_machines_tls.bats +++ b/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 diff --git a/test/bats/testdata/cfssl/agent.json b/test/bats/testdata/cfssl/agent.json index 693e3aa51..47b342e5a 100644 --- a/test/bats/testdata/cfssl/agent.json +++ b/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" } - ] - } \ No newline at end of file + ] +} diff --git a/test/bats/testdata/cfssl/agent_invalid.json b/test/bats/testdata/cfssl/agent_invalid.json index c61d4dee6..eb7db8d96 100644 --- a/test/bats/testdata/cfssl/agent_invalid.json +++ b/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" } - ] - } \ No newline at end of file + ] +} diff --git a/test/bats/testdata/cfssl/bouncer.json b/test/bats/testdata/cfssl/bouncer.json index 9a07f5766..bf642c48a 100644 --- a/test/bats/testdata/cfssl/bouncer.json +++ b/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" } - ] - } \ No newline at end of file + ] +} diff --git a/test/bats/testdata/cfssl/bouncer_invalid.json b/test/bats/testdata/cfssl/bouncer_invalid.json index c61d4dee6..eb7db8d96 100644 --- a/test/bats/testdata/cfssl/bouncer_invalid.json +++ b/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" } - ] - } \ No newline at end of file + ] +} diff --git a/test/bats/testdata/cfssl/ca.json b/test/bats/testdata/cfssl/ca.json deleted file mode 100644 index ed907e037..000000000 --- a/test/bats/testdata/cfssl/ca.json +++ /dev/null @@ -1,16 +0,0 @@ -{ - "CN": "CrowdSec Test CA", - "key": { - "algo": "rsa", - "size": 2048 - }, - "names": [ - { - "C": "FR", - "L": "Paris", - "O": "Crowdsec", - "OU": "Crowdsec", - "ST": "France" - } - ] -} \ No newline at end of file diff --git a/test/bats/testdata/cfssl/intermediate.json b/test/bats/testdata/cfssl/ca_intermediate.json similarity index 53% rename from test/bats/testdata/cfssl/intermediate.json rename to test/bats/testdata/cfssl/ca_intermediate.json index 3996ce6e1..34f1583da 100644 --- a/test/bats/testdata/cfssl/intermediate.json +++ b/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" } - } \ No newline at end of file +} diff --git a/test/bats/testdata/cfssl/ca_root.json b/test/bats/testdata/cfssl/ca_root.json new file mode 100644 index 000000000..a0d647966 --- /dev/null +++ b/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" + } + ] +} diff --git a/test/bats/testdata/cfssl/profiles.json b/test/bats/testdata/cfssl/profiles.json index d0dfced4a..47611beb6 100644 --- a/test/bats/testdata/cfssl/profiles.json +++ b/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" } } - } \ No newline at end of file + } +} diff --git a/test/bats/testdata/cfssl/server.json b/test/bats/testdata/cfssl/server.json index 37018259e..cce97037c 100644 --- a/test/bats/testdata/cfssl/server.json +++ b/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" - ] - } \ No newline at end of file + ], + "hosts": [ + "127.0.0.1", + "localhost" + ] +} diff --git a/test/lib/setup_file.sh b/test/lib/setup_file.sh index 3e6db0f12..ac651c68c 100755 --- a/test/lib/setup_file.sh +++ b/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() {