Skip to content

Add test for re-signed OCSP revocation reasons#4937

Merged
rolandshoemaker merged 5 commits intomainfrom
ocsp-resign-test
Jul 10, 2020
Merged

Add test for re-signed OCSP revocation reasons#4937
rolandshoemaker merged 5 commits intomainfrom
ocsp-resign-test

Conversation

@aarongable
Copy link
Copy Markdown
Contributor

@aarongable aarongable commented Jul 6, 2020

In https://bugzilla.mozilla.org/show_bug.cgi?id=1648840, it was seen
that Let's Encrypt's OCSP responder was generating OCSP responses with
an empty revocationReason field, but only for responses which had been
re-signed (i.e. it was correctly providing a revocationReason for the
first few days after a revocation).

This test issues and then revokes a cert, then jumps 20 days forward
to force the ocsp-updater to re-sign the ocsp response. It then checks
that the new response still has the correct revocation reason.

Fixes #4907

In https://bugzilla.mozilla.org/show_bug.cgi?id=1648840, it was seen
that Let's Encrypts OCSP responder was generating OCSP responses with
an empty revocationReason field, but only for responses which had been
re-signed (i.e. it was correctly providing a revocationReason for the
first few days after a revocation).

This test issues and then revokes a cert, then jumps 20 days forward
to force the ocsp-updater to re-sign the ocsp response. It then checks
that the new response still has the correct revocation reason.

Fixes #4907
Copy link
Copy Markdown
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.

This is a nice, clean test improvement! In particular, good insight that we could use the existing twenty_days_ago phase. For some reason I had gotten fixated on the idea that we needed another, "future" phase.

This test just checks that the certStatus is "revoked"; it doesn't check the revocation reason. That's a valuable improvement and I'd be willing to merge this on its own with a tweak to the description and the intent to follow up with a change to also test revocation reason. But it's probably more straightforward to incorporate the reason testing into this PR. Note that the numeric arg to client.revoke(..., 0) is the reason code. To read the reason code we probably just want to look for strings in the OpenSSL output as we do elsewhere.

Also I think we should be getting a copy of the OCSP response body in twenty_days_ago_setup and looping in the "present" until we see different bytes. Otherwise, the test could pass on the strength of the CA's initial GenerateOCSP as part of revocation, without ever hitting the re-signing path.

@aarongable aarongable marked this pull request as ready for review July 8, 2020 02:03
@aarongable aarongable requested a review from a team as a code owner July 8, 2020 02:03
@rolandshoemaker rolandshoemaker merged commit e906b9e into main Jul 10, 2020
@rolandshoemaker rolandshoemaker deleted the ocsp-resign-test branch July 10, 2020 18:13
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.

integration: simulate passage of time after revoking a certificate

3 participants