-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Support OCSP responses without NextUpdate field set #25912
Support OCSP responses without NextUpdate field set #25912
Conversation
CI Results: |
Build Results: |
571e53e
to
33e60aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple quick questions
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed within 929d092
return true | ||
} | ||
|
||
if currTime.After(nextUpdate) || thisUpdate.After(nextUpdate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when would thisUpdate > nextUpdate? Or is this just defensive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ddc3903
to
929d092
Compare
* 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
* 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
* 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
* 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 <steven.clark@hashicorp.com>
* 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 <steven.clark@hashicorp.com>
* 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 <steven.clark@hashicorp.com>
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.