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

Identify issuer on revocation #16763

Merged
merged 2 commits into from Aug 24, 2022
Merged

Conversation

cipherboy
Copy link
Contributor

@cipherboy cipherboy commented Aug 17, 2022

When we attempt to revoke a leaf certificate, we already parse all of
the issuers within the mount (to x509.Certificate) to ensure we don't
accidentally revoke an issuer via the leaf revocation endpoint. We can
reuse this information to associate the issuer (via issuer/subject
comparison and signature checking) to the revoked cert in its revocation
info. This will help OCSP, avoiding the case where the OCSP handler
needs to associate a certificate to its issuer.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>


@cipherboy cipherboy added this to the 1.12.0-rc1 milestone Aug 17, 2022
@cipherboy cipherboy force-pushed the cipherboy-auto-rebuild-crls branch 4 times, most recently from 2d2d398 to e7fe9ea Compare August 22, 2022 18:12
@cipherboy cipherboy force-pushed the cipherboy-identify-issuer-on-revocation branch from 59dadd6 to c99f0c2 Compare August 22, 2022 18:47
When we attempt to revoke a leaf certificate, we already parse all of
the issuers within the mount (to x509.Certificate) to ensure we don't
accidentally revoke an issuer via the leaf revocation endpoint. We can
reuse this information to associate the issuer (via issuer/subject
comparison and signature checking) to the revoked cert in its revocation
info. This will help OCSP, avoiding the case where the OCSP handler
needs to associate a certificate to its issuer.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy cipherboy marked this pull request as ready for review August 23, 2022 20:35
@cipherboy cipherboy changed the base branch from cipherboy-auto-rebuild-crls to main August 23, 2022 20:36
@cipherboy cipherboy force-pushed the cipherboy-identify-issuer-on-revocation branch from c99f0c2 to 93da855 Compare August 23, 2022 20:36
@cipherboy cipherboy removed the request for review from taoism4504 August 23, 2022 20:36
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy cipherboy requested a review from a team August 24, 2022 16:11
@cipherboy cipherboy enabled auto-merge (squash) August 24, 2022 16:15
Copy link
Contributor

@kitography kitography left a comment

Choose a reason for hiding this comment

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

This looks awesome... and I have nits.

builtin/logical/pki/crl_test.go Show resolved Hide resolved
builtin/logical/pki/crl_test.go Show resolved Hide resolved
builtin/logical/pki/crl_test.go Show resolved Hide resolved
if bytes.Equal(revokedCert.RawIssuer, issuerCert.RawSubject) {
if err := revokedCert.CheckSignatureFrom(issuerCert); err == nil {
// Valid mapping. Add it to the specified entry.
revInfo.CertificateIssuer = issuerId
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would be great to add a comment here that this isn't stable - that if multiple issuers could have possibly issued this certificate, it's picking the first one in the map.

@cipherboy cipherboy merged commit 319dc82 into main Aug 24, 2022
@cipherboy cipherboy deleted the cipherboy-identify-issuer-on-revocation branch December 1, 2022 14:58
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.

None yet

2 participants