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

RA: compute CRL shard upon revocation #7133

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

RA: compute CRL shard upon revocation #7133

wants to merge 3 commits into from

Conversation

aarongable
Copy link
Contributor

@aarongable aarongable commented Nov 2, 2023

Add three new config values to the RA: crlDPBase, crlNumShards, and crlShardWidth. When a revocation request comes in, use these values to compute the CRL Shard that the revoked cert falls into, and include that shard number in the RevokeCertificate request to the SA. This will cause the SA to create a row in the revokedCertificates table, thanks to #7095.

The first new config value is used to extract the appropriate shard number from a CRL Distribution Point URL already embedded in the to-be-revoked certificate. Therefore, this value must exactly match the crlDPBase config item in the CA, which controls the CRL URLs it generates.

However, that method only works once CRLDP URLs are baked into certs, which isn't the case yet. So the second and third new config values are used to directly compute the appropriate shard based on the certificate's notAfter date, in the same way that crl-updater computes that mapping today. Therefore these values must exactly match the crl-updater's numShards and shardWidth config values.

All of the code described above only becomes active once all three new config fields are specified in the RA. This allows this change to be deployed safely, and turned on later by setting those config values.

Also update the RA's audit log for administratively-revoked certificates to include the correct revocation method name.

Part of #7094

Copy link
Contributor

github-actions bot commented Nov 2, 2023

@aarongable, this PR appears to contain configuration changes. Please ensure that a corresponding deployment ticket has been filed with the new configuration values.

@aarongable aarongable force-pushed the ra-crl-shards branch 2 times, most recently from a360f72 to a21e729 Compare November 7, 2023 00:50
@aarongable aarongable marked this pull request as ready for review November 7, 2023 01:03
@aarongable aarongable requested a review from a team as a code owner November 7, 2023 01:03
@aarongable aarongable requested a review from jsha November 7, 2023 01:03
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

crlDPBase, crlNumShards, and crlShardWidth
...
Therefore these values must exactly match the crl-updater's numShards and shardWidth config values.

Seems like making them match might be slightly easier if RA grouped these config fields into a struct "crls" with numShards and shardWidth as fields of that struct (i.e. use the same field names as crl-updater).

cmd/admin-revoker/main_test.go Outdated Show resolved Hide resolved
@@ -2148,9 +2212,15 @@ func (ra *RegistrationAuthorityImpl) RevokeCertByKey(ctx context.Context, req *r
}

issuerID := issuance.GetIssuerNameID(cert)
crlShard, err := ra.crlShardForCert(cert)
Copy link
Contributor

Choose a reason for hiding this comment

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

The certificate here hasn't been validated as signed by our issuer and we haven't checked that we actually have this issuer. While the WFE has checked those properties, given that the importance of getting the crlShard right, maybe we should do it here too? Or perhaps just look up the cert in our database and verify the two are identical?

ra/ra.go Show resolved Hide resolved
ra/ra.go Outdated Show resolved Hide resolved
ra/ra.go Show resolved Hide resolved
@aarongable
Copy link
Contributor Author

I've uploaded a commit which addresses most of jsha's comments. However, I'm putting this PR back into Draft mode because we've decided that this PR will end up more elegant if we first address #7159, and then do the shard index extraction in the CA instead of in the RA.

@aarongable aarongable marked this pull request as draft November 15, 2023 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants