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

crl-storer: check number before uploading #7065

Merged
merged 6 commits into from
Sep 27, 2023
Merged

crl-storer: check number before uploading #7065

merged 6 commits into from
Sep 27, 2023

Conversation

aarongable
Copy link
Contributor

@aarongable aarongable commented Sep 1, 2023

Have the crl-storer download the previous CRL from S3, parse it, and compare its number against the about-to-be-uploaded CRL. This is not an atomic operation, so it is not a 100% guarantee, but it is still a useful safety check to prevent accidentally uploading CRL shards whose CRL Numbers are not strictly increasing.

Part of #6456
DO NOT MERGE until IN-9559 is resolved

test/s3-test-srv/main.go Dismissed Show dismissed Hide dismissed
@pgporada
Copy link
Member

pgporada commented Sep 1, 2023

There's s3 object locking which on the surface seems like an interesting way to make this action atomic, but alas

Object Lock works only in versioned buckets, and retention periods and legal holds apply to individual object versions.
Placing a retention period or legal hold on an object protects only the version specified in the request. It doesn't prevent new versions of the object from being created.

@aarongable aarongable marked this pull request as ready for review September 7, 2023 22:16
@aarongable aarongable requested a review from a team as a code owner September 7, 2023 22:16
@aarongable aarongable requested a review from jsha September 7, 2023 22:16
@jsha
Copy link
Contributor

jsha commented Sep 11, 2023

This looks good. Have you filed a ticket to get the appropriate permissions (get) added to the crl-storeer's IAM role in AWS? We should make sure that's finished before we merge, otherwise this will fail on deploy.

@aarongable
Copy link
Contributor Author

Good point, thank you for the reminder! I'd forgotten that we moved from a world-readable to behind-cloudfront model. I've filed IN-9559 for this.

@aarongable
Copy link
Contributor Author

IN-9559 has confirmed that the crl-storer does have Read access to the storage bucket. I'm cleaning up merge conflicts and then sending for re-review.

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.

Looks good with one small nit.

crl/storer/storer.go Outdated Show resolved Hide resolved
@jsha jsha requested a review from a team September 25, 2023 22:53
@aarongable aarongable merged commit 19f03da into main Sep 27, 2023
12 checks passed
@aarongable aarongable deleted the crl-storer-check branch September 27, 2023 16:12
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.

3 participants