Support OCSP responses without NextUpdate field set #25912
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
hashicorp-contributed-pr
|
CI Results: |
|
Build Results: |
stevendpclark
force-pushed
the
stevendpclark/vault-24614-ocsp-no-nextupdate
branch
3 times, most recently
from
571e53e
to
33e60aa
Compare
sgmiller
reviewed
Mar 18, 2024
| @@ -90,6 +91,11 @@ from the AuthorityInformationAccess extension on the certificate being inspected | |||
| Default: false, | |||
| Description: "If set to true, rather than accepting the first successful OCSP response, query all servers and consider the certificate valid only if all servers agree.", | |||
| }, | |||
| "ocsp_this_update_max_ttl": { | |||
There was a problem hiding this comment.
I'd call it max_age actually. TTL doesn't feel quite like what now() - this_update means.
There was a problem hiding this comment.
Renamed within 929d092
| return true | ||
| } | ||
|
|
||
| if currTime.After(nextUpdate) || thisUpdate.After(nextUpdate) { |
There was a problem hiding this comment.
when would thisUpdate > nextUpdate? Or is this just defensive?
There was a problem hiding this comment.
Yup completely defensive, it should never be greater.
- Validate that the ThisUpdate value is properly prior to our current time and if NextUpdate is set that, ThisUpdate is before NextUpdate. - If we don't have a value for NextUpdate just compare against ThisUpdate.
- Allow configuring a maximum TTL of the OCSP response based on the ThisUpdate time like OpenSSL does - Add test to validate that we don't cache OCSP responses with no NextUpdate
stevendpclark
force-pushed
the
stevendpclark/vault-24614-ocsp-no-nextupdate
branch
from
ddc3903
to
929d092
Compare
sgmiller
approved these changes
Mar 18, 2024
This was referenced Mar 18, 2024
stevendpclark
added a commit
that referenced
this pull request
Mar 19, 2024
* Support OCSP responses without a NextUpdate value set - Validate that the ThisUpdate value is properly prior to our current time and if NextUpdate is set that, ThisUpdate is before NextUpdate. - If we don't have a value for NextUpdate just compare against ThisUpdate. * Add ocsp_this_update_max_ttl support to cert auth - Allow configuring a maximum TTL of the OCSP response based on the ThisUpdate time like OpenSSL does - Add test to validate that we don't cache OCSP responses with no NextUpdate * Add cl * Add missing ` in docs * Rename ocsp_this_update_max_ttl to ocsp_this_update_max_age * Missed a few TTL references * Fix error message
stevendpclark
added a commit
that referenced
this pull request
Mar 19, 2024
* Support OCSP responses without a NextUpdate value set - Validate that the ThisUpdate value is properly prior to our current time and if NextUpdate is set that, ThisUpdate is before NextUpdate. - If we don't have a value for NextUpdate just compare against ThisUpdate. * Add ocsp_this_update_max_ttl support to cert auth - Allow configuring a maximum TTL of the OCSP response based on the ThisUpdate time like OpenSSL does - Add test to validate that we don't cache OCSP responses with no NextUpdate * Add cl * Add missing ` in docs * Rename ocsp_this_update_max_ttl to ocsp_this_update_max_age * Missed a few TTL references * Fix error message
stevendpclark
added a commit
that referenced
this pull request
Mar 19, 2024
* Support OCSP responses without a NextUpdate value set - Validate that the ThisUpdate value is properly prior to our current time and if NextUpdate is set that, ThisUpdate is before NextUpdate. - If we don't have a value for NextUpdate just compare against ThisUpdate. * Add ocsp_this_update_max_ttl support to cert auth - Allow configuring a maximum TTL of the OCSP response based on the ThisUpdate time like OpenSSL does - Add test to validate that we don't cache OCSP responses with no NextUpdate * Add cl * Add missing ` in docs * Rename ocsp_this_update_max_ttl to ocsp_this_update_max_age * Missed a few TTL references * Fix error message
stevendpclark
added a commit
that referenced
this pull request
Mar 19, 2024
* Support OCSP responses without a NextUpdate value set - Validate that the ThisUpdate value is properly prior to our current time and if NextUpdate is set that, ThisUpdate is before NextUpdate. - If we don't have a value for NextUpdate just compare against ThisUpdate. * Add ocsp_this_update_max_ttl support to cert auth - Allow configuring a maximum TTL of the OCSP response based on the ThisUpdate time like OpenSSL does - Add test to validate that we don't cache OCSP responses with no NextUpdate * Add cl * Add missing ` in docs * Rename ocsp_this_update_max_ttl to ocsp_this_update_max_age * Missed a few TTL references * Fix error message Co-authored-by: Steven Clark <[email protected]>
stevendpclark
added a commit
that referenced
this pull request
Mar 19, 2024
* Support OCSP responses without a NextUpdate value set - Validate that the ThisUpdate value is properly prior to our current time and if NextUpdate is set that, ThisUpdate is before NextUpdate. - If we don't have a value for NextUpdate just compare against ThisUpdate. * Add ocsp_this_update_max_ttl support to cert auth - Allow configuring a maximum TTL of the OCSP response based on the ThisUpdate time like OpenSSL does - Add test to validate that we don't cache OCSP responses with no NextUpdate * Add cl * Add missing ` in docs * Rename ocsp_this_update_max_ttl to ocsp_this_update_max_age * Missed a few TTL references * Fix error message Co-authored-by: Steven Clark <[email protected]>
stevendpclark
added a commit
that referenced
this pull request
Mar 19, 2024
* Support OCSP responses without a NextUpdate value set - Validate that the ThisUpdate value is properly prior to our current time and if NextUpdate is set that, ThisUpdate is before NextUpdate. - If we don't have a value for NextUpdate just compare against ThisUpdate. * Add ocsp_this_update_max_ttl support to cert auth - Allow configuring a maximum TTL of the OCSP response based on the ThisUpdate time like OpenSSL does - Add test to validate that we don't cache OCSP responses with no NextUpdate * Add cl * Add missing ` in docs * Rename ocsp_this_update_max_ttl to ocsp_this_update_max_age * Missed a few TTL references * Fix error message Co-authored-by: Steven Clark <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support OCSP responses without a NextUpdate value set within cert-auth OCSP verification. Responses without a NextUpdate value are allowed based on RFC6960 Section 4.2.2.1
Previous to this fix, if OCSP checks were enabled and an OCSP response didn't contain a NextUpdate value, cert auth logins would fail with a message similar to
Now OCSP responses without a NextUpdate field set will be allowed, the response will not be cached, as mentioned in the RFC, a lack of a NextUpdate field means there is newer revocation information available all the time.
Additional checks that the ThisUpdate field does contain a proper timestamp and is before then the current time have also been added within the list of verifications performed for us to trust an OCSP response.
A new cert auth configuration has been introduced called
ocsp_this_update_max_ttl, with a default of 0. If this field is greater than 0, cert-auth will enforce an OCSP response's ThisUpdate field is less than the configured value. This can be used to avoid accepting older OCSP response values based on policies. We have avoided enforcing this by default for backwards compatibility reasons.