Add an OCSP responder to Vault's PKI plugin #16723
Conversation
|
|
||
| info.ocspStatus = ocsp.Revoked | ||
| info.revocationTimeUTC = &revEntry.RevocationTimeUTC | ||
| info.issuerID = revEntry.CertificateIssuer // This might be empty if the CRL hasn't been rebuilt |
There was a problem hiding this comment.
So I'm kinda of two minds here.
If the CRL is never rebuilt, we're stuck always incurring the iteration over all issuers down in lookupOcspIssuer. This increases request time, and isn't strictly correct (w.r.t. if the issuer hash/... doesn't match as expected).
However, if the CRL is eventually rebuilt, we'd be doing extra work for potentially no reason.
My 2c., but I'd kinda prefer that we refactor the CRL rebuilding logic so that it can be reused here: https://github.com/hashicorp/vault/blob/main/builtin/logical/pki/crl_util.go#L523-L567
That way, if there is no issuer entry, we can help future lookups of the same cert and avoid recomputing the value each time (when CRL isn't used).
Then, down below, in the lookupIsssuerIds, we only need to compare the expected issuer's hashes against the actual issuer ID's backing issuer's hashes (since they're correct up to isomorphism) and if they mismatch, we can give a proper error (malformed request probably).
The reason for this is, IMO, I'd love to see a standalone (CRL disabled), performant OCSP server. If we require the CRL to be rebuilt to pick up new issuer revocations, I think that's less than ideal.
Alternatively (or additionally) we could also move issuer association to revocation time if we wanted?
- pki.mdx doc update - parens around logical.go comment to indicate DER encoded request is related to OCSP and not the snapshots - Use AllIssuers instead of writing them all out - Drop zero initialization of crl config's Disable flag if not present - Upgrade issuer on the fly instead of an initial migration
stevendpclark
force-pushed
the
stevendpclark/pki-ocsp-responder
branch
from
ad2e82e to
2296498
Compare
|
Updated all feedback, I did not change the atomic boolean/setOcspStatus stuff as there is another PR that will update that. Also this has been rebased on top of the latest main branch. @cipherboy @kitography this should be ready for re-review! |
stevendpclark
force-pushed
the
stevendpclark/pki-ocsp-responder
branch
from
18fc058 to
954cafd
Compare
|
|
||
| if matches { | ||
| if !issuer.Usage.HasUsage(OCSPSigningUsage) { | ||
| // We found the correct issuer, but it's not allowed to sign the |
There was a problem hiding this comment.
Is it not possible for there to be more than one matching issuer here? (Same name, key, different expiry/certificate usages?)
There was a problem hiding this comment.
Hmm, this is a good point. I'd probably suggest addressing it in a follow-up though, so as not to block too much.
There was a problem hiding this comment.
Yup noted, I'll address this feedback in a different PR, but great catch @kitography!
- Leverage ocsp response constant - Remove duplicate errors regarding unknown issuers
| @@ -0,0 +1,4 @@ | |||
| ```release-note:feature | |||
There was a problem hiding this comment.
👋🏻 This changelog entry isn't correctly formatted for new features.
**OCSP Responder**: <text>
You can check out the 1.11 changelog for a reference, and there's also a wiki page about this. Could you please update this file on main so that it formats correctly in the final version of the 1.12 changelog?
Add an OCSP responder to Vault's PKI plugin. This is a simplistic responder that at the moment will only accept a single query at a time with no support for any extensions defined within RFC 6960, mainly due to the limitations in Go's OCSP libraries.
An end-user will be able to disable the responder through the existing
config/crlendpoint with the newly added keyocsp_disable. The responder will be enabled by default, if the key is set to true to disable the responder, an OCSP response ofUnauthorizedwill be returned to the caller.