Fix deletion of issuers, CRLs #15254
Conversation
cipherboy
force-pushed
the
pki-pod-rotation
branch
from
3049d93
to
94169e5
Compare
When deleting a specific issuer, we might impact the chains. From a consistency perspective, we need to ensure the remaining chains are correct and don't refer to the since-deleted issuer, so trigger a full rebuild here. We don't need to call this in the delete-the-world (DELETE /root) code path, as there shouldn't be any remaining issuers or chains to build. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
When calling DELETE /root, we should remove the legacy CRL bundle, since we're deleting the legacy CA issuer bundle as well. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Since CRLs are no longer resolvable after deletion (due to missing issuer ID, which will cause resolution to fail regardless of if an ID or a name/default reference was used), we should delete these CRLs from storage to avoid leaking them. In the event that this issuer comes back (with key material), we can simply rebuild the CRL at that time (from the remaining revoked storage entries). Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
cipherboy
force-pushed
the
cipherboy-fix-delete-stuff
branch
from
8967ca9
to
75632ef
Compare
| @@ -360,6 +360,40 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b | |||
| } | |||
| } | |||
|
|
|||
| // Before persisting our updated CRL config, check to see if we have | |||
There was a problem hiding this comment.
At the moment we do not call a CRL rebuild on a deleteIssuer API call, that feels wrong but maybe I'm missing something?
There was a problem hiding this comment.
If you delete the issuer, the lookup (by ref) will fail, so you can't fetch it any more. The deletion of the actual CRL can be lazily done; the remaining pieces will either be the same (no certs were actually revoked, so its equivalent to previous CRLs) or there's presently not-deleted yet unaccessible CRL that nobody really cares about since they can't see it, that'll get removed on next CRL rebuild (if it was the last of its kind).
So I think its fine?
There was a problem hiding this comment.
Yup I figured after I wrote the comment it was something along these lines, no need for additional changes.
One interesting but non-important point is that the secondary clusters now will force the rebuild I believe of the CRL while the primary will have lagged behind :)
|
Thanks @stevendpclark! Manually merging... |
Apparently I had this branch sitting around but haven't proposed it yet? :-D
We:
DELETE /root.