Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CRL handling for multiple issuers #15100

Merged
merged 5 commits into from
Apr 22, 2022

Conversation

cipherboy
Copy link
Contributor

@cipherboy cipherboy commented Apr 20, 2022

Signed-off-by: Alexander Scheel <[email protected]>


This updates the CRL handling code to support generating a per-issuer CRL. Notably, when two issuers use the same public/private key and have the same Subject, their CRLs are functionally equivalent and can be combined.

We update revoked cert entries to have an issuer field, allowing us to do the (expensive) issuer check once.

We also update to the CRLv2 format, using code from @kitography.

@cipherboy cipherboy changed the title Update CRLs Update CRL handling for multiple issuers Apr 20, 2022
Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing completely wrong, assuming we are overlooking the fact that we are overriding the final CRL and not actually writing out CRL's based on different issuers...

A few nits to address.

builtin/logical/pki/crl_util.go Outdated Show resolved Hide resolved
builtin/logical/pki/crl_util.go Show resolved Hide resolved
@stevendpclark stevendpclark self-requested a review April 21, 2022 14:21
@cipherboy cipherboy force-pushed the cipherboy-update-crls branch 3 times, most recently from dad7f77 to f3bf782 Compare April 21, 2022 17:30
builtin/logical/pki/cert_util.go Outdated Show resolved Hide resolved
builtin/logical/pki/crl_util.go Show resolved Hide resolved
builtin/logical/pki/crl_util.go Outdated Show resolved Hide resolved
builtin/logical/pki/crl_util.go Outdated Show resolved Hide resolved
cipherboy and others added 5 commits April 21, 2022 14:29
When building CRLs, we've gotta make sure certs issued by that issuer
land up on that issuer's CRL and not some other CRL. If no CRL is
found (matching a cert), we'll place it on the default CRL.
However, in the event of equivalent issuers (those with the same subject
AND the same key  material) -- perhaps due to reissuance -- we'll only
create a single (unified) CRL for them.

Signed-off-by: Alexander Scheel <[email protected]>
This updates fetchCertBySerial to support querying the default issuer's
CRL.

Signed-off-by: Alexander Scheel <[email protected]>
When using the older Certificate.CreateCRL(...) call, Go's x509 library
copies the parsed pkix.Name version of the CRL Issuer's Subject field.
For certain constructed CAs, this fails since pkix.Name is not suitable
for round-tripping. This also builds a CRLv1 (per RFC 5280) CRL.

In updating to the newer x509.CreateRevocationList(...) call, we can
construct the CRL in the CRLv2 format and correctly copy the issuer's
name. However, this requires holding an additional field per-CRL, the
CRLNumber field, which is required in Go's implementation of CRLv2
(though OPTIONAL in the spec). We store this on the new
LocalCRLConfigEntry object, per-CRL.

Co-authored-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
In previous versions of Vault, it was possible to sign an empty CRL
(when the CRL was disabled and a force-rebuild was requested). Add a
comment about this case.

Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy cipherboy merged commit 8cfe955 into pki-pod-rotation Apr 22, 2022
@cipherboy
Copy link
Contributor Author

Manually merged, thanks all!

@cipherboy cipherboy deleted the cipherboy-update-crls branch May 17, 2022 14:33
@cipherboy
Copy link
Contributor Author

This PR was merged in #15277. See that PR and the relevant docs PR #15238 for more information about this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants