Open plinss opened 1 month ago
@plinss: Thanks for opening an issue, it is currently awaiting triage.
In the meantime, you can:
should be fixed in 1.6.2. Thanks again!
@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.
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
@mmetc I appreciate the effort, fwiw I'm happy to review a PR and/or test locally
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
OS version
Enabled collections and parsers
Acquisition config
Config show
Prometheus metrics
Related custom configs versions (if applicable) : notification plugins, custom scenarios, parsers etc.