From df159b016705aa78ac64020634d66152efd0e76b Mon Sep 17 00:00:00 2001 From: mmetc <92726601+mmetc@users.noreply.github.com> Date: Fri, 9 Feb 2024 13:55:24 +0100 Subject: [PATCH] update calls to deprecated x509 methods (#2824) --- .github/workflows/docker-tests.yml | 4 +- .golangci.yml | 4 - pkg/apiserver/middlewares/v1/api_key.go | 2 +- pkg/apiserver/middlewares/v1/tls_auth.go | 108 ++++++++++++----------- test/bats/11_bouncers_tls.bats | 3 + test/bats/30_machines_tls.bats | 7 +- 6 files changed, 67 insertions(+), 61 deletions(-) diff --git a/.github/workflows/docker-tests.yml b/.github/workflows/docker-tests.yml index 7bc63de01..d3ae4f90d 100644 --- a/.github/workflows/docker-tests.yml +++ b/.github/workflows/docker-tests.yml @@ -50,7 +50,7 @@ jobs: cache-to: type=gha,mode=min - name: "Setup Python" - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: "3.x" @@ -61,7 +61,7 @@ jobs: - name: "Cache virtualenvs" id: cache-pipenv - uses: actions/cache@v3 + uses: actions/cache@v4 with: path: ~/.local/share/virtualenvs key: ${{ runner.os }}-pipenv-${{ hashFiles('**/Pipfile.lock') }} diff --git a/.golangci.yml b/.golangci.yml index f69bf66ea..3161b2c0a 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -310,10 +310,6 @@ issues: # Will fix, might be trickier # - - linters: - - staticcheck - text: "x509.ParseCRL has been deprecated since Go 1.19: Use ParseRevocationList instead" - # https://github.com/pkg/errors/issues/245 - linters: - depguard diff --git a/pkg/apiserver/middlewares/v1/api_key.go b/pkg/apiserver/middlewares/v1/api_key.go index ae7645e1b..41ee15b44 100644 --- a/pkg/apiserver/middlewares/v1/api_key.go +++ b/pkg/apiserver/middlewares/v1/api_key.go @@ -66,7 +66,7 @@ func (a *APIKey) authTLS(c *gin.Context, logger *log.Entry) *ent.Bouncer { validCert, extractedCN, err := a.TlsAuth.ValidateCert(c) if !validCert { - logger.Errorf("invalid client certificate: %s", err) + logger.Error(err) return nil } diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index 904f6cd44..bd2c4bb30 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -4,6 +4,7 @@ import ( "bytes" "crypto" "crypto/x509" + "encoding/pem" "fmt" "io" "net/http" @@ -19,14 +20,13 @@ import ( type TLSAuth struct { AllowedOUs []string CrlPath string - revokationCache map[string]cacheEntry + revocationCache map[string]cacheEntry cacheExpiration time.Duration logger *log.Entry } type cacheEntry struct { revoked bool - err error timestamp time.Time } @@ -89,10 +89,12 @@ func (ta *TLSAuth) isExpired(cert *x509.Certificate) bool { return false } -func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) { - if cert.OCSPServer == nil || (cert.OCSPServer != nil && len(cert.OCSPServer) == 0) { +// isOCSPRevoked checks if the client certificate is revoked by any of the OCSP servers present in the certificate. +// It returns a boolean indicating if the certificate is revoked and a boolean indicating if the OCSP check was successful and could be cached. +func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, bool) { + if cert.OCSPServer == nil || len(cert.OCSPServer) == 0 { ta.logger.Infof("TLSAuth: no OCSP Server present in client certificate, skipping OCSP verification") - return false, nil + return false, true } for _, server := range cert.OCSPServer { @@ -104,9 +106,10 @@ func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificat switch ocspResponse.Status { case ocsp.Good: - return false, nil + return false, true case ocsp.Revoked: - return true, fmt.Errorf("client certificate is revoked by server %s", server) + ta.logger.Errorf("TLSAuth: client certificate is revoked by server %s", server) + return true, true case ocsp.Unknown: log.Debugf("unknow OCSP status for server %s", server) continue @@ -115,83 +118,82 @@ func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificat log.Infof("Could not get any valid OCSP response, assuming the cert is revoked") - return true, nil + return true, false } -func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, error) { +// isCRLRevoked checks if the client certificate is revoked by the CRL present in the CrlPath. +// It returns a boolean indicating if the certificate is revoked and a boolean indicating if the CRL check was successful and could be cached. +func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, bool) { if ta.CrlPath == "" { - ta.logger.Warn("no crl_path, skipping CRL check") - return false, nil + ta.logger.Info("no crl_path, skipping CRL check") + return false, true } crlContent, err := os.ReadFile(ta.CrlPath) if err != nil { - ta.logger.Warnf("could not read CRL file, skipping check: %s", err) - return false, nil + ta.logger.Errorf("could not read CRL file, skipping check: %s", err) + return false, false } - crl, err := x509.ParseCRL(crlContent) + crlBinary, rest := pem.Decode(crlContent) + if len(rest) > 0 { + ta.logger.Warn("CRL file contains more than one PEM block, ignoring the rest") + } + + crl, err := x509.ParseRevocationList(crlBinary.Bytes) if err != nil { - ta.logger.Warnf("could not parse CRL file, skipping check: %s", err) - return false, nil + ta.logger.Errorf("could not parse CRL file, skipping check: %s", err) + return false, false } - if crl.HasExpired(time.Now().UTC()) { + now := time.Now().UTC() + + if now.After(crl.NextUpdate) { ta.logger.Warn("CRL has expired, will still validate the cert against it.") } - for _, revoked := range crl.TBSCertList.RevokedCertificates { + if now.Before(crl.ThisUpdate) { + ta.logger.Warn("CRL is not yet valid, will still validate the cert against it.") + } + + for _, revoked := range crl.RevokedCertificateEntries { if revoked.SerialNumber.Cmp(cert.SerialNumber) == 0 { - return true, fmt.Errorf("client certificate is revoked by CRL") + ta.logger.Warn("client certificate is revoked by CRL") + return true, true } } - return false, nil + return false, true } func (ta *TLSAuth) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) { sn := cert.SerialNumber.String() - if cacheValue, ok := ta.revokationCache[sn]; ok { + if cacheValue, ok := ta.revocationCache[sn]; ok { if time.Now().UTC().Sub(cacheValue.timestamp) < ta.cacheExpiration { - ta.logger.Debugf("TLSAuth: using cached value for cert %s: %t | %s", sn, cacheValue.revoked, cacheValue.err) - return cacheValue.revoked, cacheValue.err - } else { - ta.logger.Debugf("TLSAuth: cached value expired, removing from cache") - delete(ta.revokationCache, sn) + ta.logger.Debugf("TLSAuth: using cached value for cert %s: %t", sn, cacheValue.revoked) + return cacheValue.revoked, nil } + + ta.logger.Debugf("TLSAuth: cached value expired, removing from cache") + delete(ta.revocationCache, sn) } else { ta.logger.Tracef("TLSAuth: no cached value for cert %s", sn) } - revoked, err := ta.isOCSPRevoked(cert, issuer) - if err != nil { - ta.revokationCache[sn] = cacheEntry{ + revokedByOCSP, cacheOCSP := ta.isOCSPRevoked(cert, issuer) + + revokedByCRL, cacheCRL := ta.isCRLRevoked(cert) + + revoked := revokedByOCSP || revokedByCRL + + if cacheOCSP && cacheCRL { + ta.revocationCache[sn] = cacheEntry{ revoked: revoked, - err: err, timestamp: time.Now().UTC(), } - - return true, err } - if revoked { - ta.revokationCache[sn] = cacheEntry{ - revoked: revoked, - err: err, - timestamp: time.Now().UTC(), - } - - return true, nil - } - - revoked, err = ta.isCRLRevoked(cert) - ta.revokationCache[sn] = cacheEntry{ - revoked: revoked, - err: err, - timestamp: time.Now().UTC(), - } - - return revoked, err + return revoked, nil } func (ta *TLSAuth) isInvalid(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) { @@ -265,11 +267,11 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { revoked, err := ta.isInvalid(clientCert, c.Request.TLS.VerifiedChains[0][1]) if err != nil { ta.logger.Errorf("TLSAuth: error checking if client certificate is revoked: %s", err) - return false, "", fmt.Errorf("could not check for client certification revokation status: %w", err) + return false, "", fmt.Errorf("could not check for client certification revocation status: %w", err) } if revoked { - return false, "", fmt.Errorf("client certificate is revoked") + return false, "", fmt.Errorf("client certificate for CN=%s OU=%s is revoked", clientCert.Subject.CommonName, clientCert.Subject.OrganizationalUnit) } ta.logger.Debugf("client OU %v is allowed vs required OU %v", clientCert.Subject.OrganizationalUnit, ta.AllowedOUs) @@ -282,7 +284,7 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { func NewTLSAuth(allowedOus []string, crlPath string, cacheExpiration time.Duration, logger *log.Entry) (*TLSAuth, error) { ta := &TLSAuth{ - revokationCache: map[string]cacheEntry{}, + revocationCache: map[string]cacheEntry{}, cacheExpiration: cacheExpiration, CrlPath: crlPath, logger: logger, diff --git a/test/bats/11_bouncers_tls.bats b/test/bats/11_bouncers_tls.bats index 8fb457925..2c39aae30 100644 --- a/test/bats/11_bouncers_tls.bats +++ b/test/bats/11_bouncers_tls.bats @@ -90,7 +90,10 @@ teardown() { } @test "simulate one bouncer request with a revoked certificate" { + truncate_log rune -0 curl -i -s --cert "${tmpdir}/bouncer_revoked.pem" --key "${tmpdir}/bouncer_revoked-key.pem" --cacert "${tmpdir}/bundle.pem" https://localhost:8080/v1/decisions\?ip=42.42.42.42 + assert_log --partial "client certificate is revoked by CRL" + assert_log --partial "client certificate for CN=localhost OU=[bouncer-ou] is revoked" assert_output --partial "access forbidden" rune -0 cscli bouncers list -o json assert_output "[]" diff --git a/test/bats/30_machines_tls.bats b/test/bats/30_machines_tls.bats index 535435336..311293ca7 100644 --- a/test/bats/30_machines_tls.bats +++ b/test/bats/30_machines_tls.bats @@ -132,13 +132,15 @@ teardown() { ' config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)' ./instance-crowdsec start + rune -1 cscli lapi status rune -0 cscli machines list -o json assert_output '[]' } @test "revoked cert for agent" { + truncate_log config_set "${CONFIG_DIR}/local_api_credentials.yaml" ' - .ca_cert_path=strenv(tmpdir) + "/bundle.pem" | + .ca_cert_path=strenv(tmpdir) + "/bundle.pem" | .key_path=strenv(tmpdir) + "/agent_revoked-key.pem" | .cert_path=strenv(tmpdir) + "/agent_revoked.pem" | .url="https://127.0.0.1:8080" @@ -146,6 +148,9 @@ teardown() { config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)' ./instance-crowdsec start + rune -1 cscli lapi status + assert_log --partial "client certificate is revoked by CRL" + assert_log --partial "client certificate for CN=localhost OU=[agent-ou] is revoked" rune -0 cscli machines list -o json assert_output '[]' }