mTLS auth only uses first CRL

[plinss]

What happened?

In my crowdsec setup, I'm using mTLS authentication with machine-specific certificates generated by a private CA (step-ca). My CA uses an intermediate certificate.

This CA configuration requires two CRLs, one for the root certificate (listing revoked intermediate certs), and one for the intermediate certificate. The clients present both their client certificate and the intermediate certificate. For other servers that perform mTLS, (such as Nginx) the server requires both CRLs in PEM format concatenated together, root first.

When crowdsec does certificate validation, it only reads the first CRL (the root) from the file and generates a warning about the second CRL being present. It then checks if the client's certificate's serial number is in the CRL. The client certificate will never be in the root CRL and revoked certificates will never be detected..

What did you expect to happen?

Crowdsec needs to read all the CRLs present in the file and check both the client certificate and any intermediate certificates against the appropriate CRLs (presume the last CRL would contain the client certificate's serial number, then work backwards as you go up the certificate chain, this appears to be the standard order for concatenated CRLs).

How can we reproduce it (as minimally and precisely as possible)?

Generate a client certificate from a CA that uses intermediate certificates. Get the CRLs from the CA and concatenate them (root first). Configure crowdsec to use the combined CRL file.

Anything else we need to know?

As a stopgap, it would be better to use the last CRL in the file rather than the first, as that will generally be the CRL that actually contains revoked client certificates when intermediate certificates are in play.

Crowdsec version

$ cscli version
2024/05/07 21:48:45 version: v1.6.1-0746e0c0
2024/05/07 21:48:45 Codename: alphaga
2024/05/07 21:48:45 BuildDate: 2024-04-16_09:45:33
2024/05/07 21:48:45 GoVersion: 1.21.9
2024/05/07 21:48:45 Platform: linux
2024/05/07 21:48:45 libre2: C++
2024/05/07 21:48:45 Constraint_parser: >= 1.0, <= 3.0
2024/05/07 21:48:45 Constraint_scenario: >= 1.0, <= 3.0
2024/05/07 21:48:45 Constraint_api: v1
2024/05/07 21:48:45 Constraint_acquis: >= 1.0, < 2.0

OS version

# On Linux:
$ cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux 12 (bookworm)"
NAME="Debian GNU/Linux"
VERSION_ID="12"
VERSION="12 (bookworm)"
VERSION_CODENAME=bookworm
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

$ uname -a
Linux core 6.2.9-x86_64-linode160 #1 SMP PREEMPT_DYNAMIC Wed Apr  5 15:30:32 EDT 2023 x86_64 GNU/Linux

Enabled collections and parsers

$ cscli hub list -o raw
# paste output here

Acquisition config

```console # On Linux: $ cat /etc/crowdsec/acquis.yaml /etc/crowdsec/acquis.d/* # paste output here

On Windows:

C:> Get-Content C:\ProgramData\CrowdSec\config\acquis.yaml

paste output here

Config show

$ cscli config show
# paste output here

Prometheus metrics

$ cscli metrics
# paste output here

Related custom configs versions (if applicable) : notification plugins, custom scenarios, parsers etc.

[github-actions]

@plinss: Thanks for opening an issue, it is currently awaiting triage.

In the meantime, you can:

1. Check Crowdsec Documentation to see if your issue can be self resolved.
2. You can also join our Discord.
3. Check Releases to make sure your agent is on the latest version.

Details

I am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository.

[mmetc]

should be fixed in 1.6.2. Thanks again!

[plinss]

@mmetc thanks for taking a look at this so quickly. But looking at the PR I'm not sure the implementation is correct.

You are parsing all the CRL blocks, but it appears that you're evaluating the client's end certificate against all of them and only checking the serial number. When there are multiple CRLs, generally one will be signed by the certificate that signed the end certificate (and contain the serial numbers of revoked end certificates), and the other(s) will be for intermediate certificate(s) that signed the signing certificate (and contain the serial numbers of the revoked intermediate(s)).

It's possible that:

1. an intermediate certificate has been revoked without revoking all the end certificates (in which case all the end certificates should not be accepted).
2. there's a serial number collision between the end certificates and the intermediates (in which case a certificate that hasn't been revoked may get considered as revoked by the current code).

A full implementation needs to walk the entire client certificate's chain, match each certificate to the proper CRL, and check each certificate against the corresponding CRL only. Ideally you should also be calling CheckSignatureFrom to test the signature of the CRL against the root or intermediate certificate(s) that signed it before trusting it.

The RevocationList struct has an Issuer property that has the DN of the certificate that issued/signed the CRL (and should be the certificate that signed the certificate that's being checked against the CRL).

It also appears that the rest of the client certificate validation code is presuming a single issuer. It really should be passing the entire chain into isInvalid and checking the intermediate and root certificates for expiration as well (unless VerifiedChains is already doing that). Note that there will usually be zero or one intermediate certificates, but theoretically there can be any number of them.

[mmetc]

Hi,

a quick note to tell you I'm rewriting the auth middleware, taking your observations into account. it should also perform better as a result, but it's too late for inclusion in 1.6.2. I'll link the PR here when it's ready

Thanks again

[plinss]

@mmetc I appreciate the effort, fwiw I'm happy to review a PR and/or test locally