Support OCSP responses without NextUpdate field set (#25912) (#26023)

* 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]>

Author: hc-github-team-secure-vault-core

builtin/credential/cert/path_certs.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/go-secure-stdlib/parseutil"
 	"github.com/hashicorp/go-sockaddr"
 
 	"github.com/hashicorp/vault/sdk/framework"
@@ -91,6 +92,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_age": {
+				Type:        framework.TypeDurationSecond,
+				Default:     0,
+				Description: "If greater than 0, specifies the maximum age of an OCSP thisUpdate field to avoid accepting old responses without a nextUpdate field.",
+			},
 			"allowed_names": {
 				Type: framework.TypeCommaStringSlice,
 				Description: `A comma-separated list of names.
@@ -309,6 +315,7 @@ func (b *backend) pathCertRead(ctx context.Context, req *logical.Request, d *fra
 		"ocsp_servers_override":        cert.OcspServersOverride,
 		"ocsp_fail_open":               cert.OcspFailOpen,
 		"ocsp_query_all_servers":       cert.OcspQueryAllServers,
+		"ocsp_this_update_max_age":     int64(cert.OcspThisUpdateMaxAge.Seconds()),
 	}
 	cert.PopulateTokenData(data)
 
@@ -367,6 +374,13 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr
 	if ocspQueryAll, ok := d.GetOk("ocsp_query_all_servers"); ok {
 		cert.OcspQueryAllServers = ocspQueryAll.(bool)
 	}
+	if ocspThisUpdateMaxAge, ok := d.GetOk("ocsp_this_update_max_age"); ok {
+		maxAgeDuration, err := parseutil.ParseDurationSecond(ocspThisUpdateMaxAge)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse ocsp_this_update_max_age: %w", err)
+		}
+		cert.OcspThisUpdateMaxAge = maxAgeDuration
+	}
 	if displayNameRaw, ok := d.GetOk("display_name"); ok {
 		cert.DisplayName = displayNameRaw.(string)
 	}
@@ -512,11 +526,12 @@ type CertEntry struct {
 	AllowedMetadataExtensions  []string
 	BoundCIDRs                 []*sockaddr.SockAddrMarshaler
 
-	OcspCaCertificates  string
-	OcspEnabled         bool
-	OcspServersOverride []string
-	OcspFailOpen        bool
-	OcspQueryAllServers bool
+	OcspCaCertificates   string
+	OcspEnabled          bool
+	OcspServersOverride  []string
+	OcspFailOpen         bool
+	OcspQueryAllServers  bool
+	OcspThisUpdateMaxAge time.Duration
 }
 
 const pathCertHelpSyn = `

builtin/credential/cert/path_login.go

@@ -25,7 +25,7 @@ import (
 
 	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/go-multierror"
-	glob "github.com/ryanuber/go-glob"
+	"github.com/ryanuber/go-glob"
 )
 
 // ParsedCert is a certificate that has been configured as trusted
@@ -663,6 +663,7 @@ func (b *backend) loadTrustedCerts(ctx context.Context, storage logical.Storage,
 				conf.OcspFailureMode = ocsp.FailOpenFalse
 			}
 			conf.QueryAllServers = conf.QueryAllServers || entry.OcspQueryAllServers
+			conf.OcspThisUpdateMaxAge = entry.OcspThisUpdateMaxAge
 		}
 	}
 

changelog/25912.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+auth/cert: Allow validation with OCSP responses with no NextUpdate time
+```

sdk/helper/ocsp/client.go

@@ -162,9 +162,27 @@ func (c *Client) getHashAlgorithmFromOID(target pkix.AlgorithmIdentifier) crypto
 	return crypto.SHA1
 }
 
-// isInValidityRange checks the validity
-func isInValidityRange(currTime, nextUpdate time.Time) bool {
-	return !nextUpdate.IsZero() && !currTime.After(nextUpdate)
+// isInValidityRange checks the validity times of the OCSP response making sure
+// that thisUpdate and nextUpdate values are bounded within currTime
+func isInValidityRange(currTime time.Time, ocspRes *ocsp.Response) bool {
+	thisUpdate := ocspRes.ThisUpdate
+
+	// If the thisUpdate value in the OCSP response wasn't set or greater than current time fail this check
+	if thisUpdate.IsZero() || thisUpdate.After(currTime) {
+		return false
+	}
+
+	nextUpdate := ocspRes.NextUpdate
+	if nextUpdate.IsZero() {
+		// We don't have a nextUpdate field set, assume we are okay.
+		return true
+	}
+
+	if currTime.After(nextUpdate) || thisUpdate.After(nextUpdate) {
+		return false
+	}
+
+	return true
 }
 
 func extractCertIDKeyFromRequest(ocspReq []byte) (*certIDKey, *ocspStatus) {
@@ -209,7 +227,7 @@ func (c *Client) encodeCertIDKey(certIDKeyBase64 string) (*certIDKey, error) {
 	}, nil
 }
 
-func (c *Client) checkOCSPResponseCache(encodedCertID *certIDKey, subject, issuer *x509.Certificate) (*ocspStatus, error) {
+func (c *Client) checkOCSPResponseCache(encodedCertID *certIDKey, subject, issuer *x509.Certificate, config *VerifyConfig) (*ocspStatus, error) {
 	c.ocspResponseCacheLock.RLock()
 	var cacheValue *ocspCachedResponse
 	v, ok := c.ocspResponseCache.Get(*encodedCertID)
@@ -218,7 +236,7 @@ func (c *Client) checkOCSPResponseCache(encodedCertID *certIDKey, subject, issue
 	}
 	c.ocspResponseCacheLock.RUnlock()
 
-	status, err := c.extractOCSPCacheResponseValue(cacheValue, subject, issuer)
+	status, err := c.extractOCSPCacheResponseValue(cacheValue, subject, issuer, config)
 	if err != nil {
 		return nil, err
 	}
@@ -235,18 +253,25 @@ func (c *Client) deleteOCSPCache(encodedCertID *certIDKey) {
 	c.ocspResponseCacheLock.Unlock()
 }
 
-func validateOCSP(ocspRes *ocsp.Response) (*ocspStatus, error) {
+func validateOCSP(conf *VerifyConfig, ocspRes *ocsp.Response) (*ocspStatus, error) {
 	curTime := time.Now()
 
 	if ocspRes == nil {
 		return nil, errors.New("OCSP Response is nil")
 	}
-	if !isInValidityRange(curTime, ocspRes.NextUpdate) {
+	if !isInValidityRange(curTime, ocspRes) {
 		return &ocspStatus{
 			code: ocspInvalidValidity,
 			err:  fmt.Errorf("invalid validity: producedAt: %v, thisUpdate: %v, nextUpdate: %v", ocspRes.ProducedAt, ocspRes.ThisUpdate, ocspRes.NextUpdate),
 		}, nil
 	}
+
+	if conf.OcspThisUpdateMaxAge > 0 && curTime.Sub(ocspRes.ThisUpdate) > conf.OcspThisUpdateMaxAge {
+		return &ocspStatus{
+			code: ocspInvalidValidity,
+			err:  fmt.Errorf("invalid validity: thisUpdate: %v is greater than max age: %s", ocspRes.ThisUpdate, conf.OcspThisUpdateMaxAge),
+		}, nil
+	}
 	return returnOCSPStatus(ocspRes), nil
 }
 
@@ -450,7 +475,7 @@ func (c *Client) retryOCSP(
 
 // GetRevocationStatus checks the certificate revocation status for subject using issuer certificate.
 func (c *Client) GetRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate, conf *VerifyConfig) (*ocspStatus, error) {
-	status, ocspReq, encodedCertID, err := c.validateWithCache(subject, issuer)
+	status, ocspReq, encodedCertID, err := c.validateWithCache(subject, issuer, conf)
 	if err != nil {
 		return nil, err
 	}
@@ -510,7 +535,7 @@ func (c *Client) GetRevocationStatus(ctx context.Context, subject, issuer *x509.
 				return nil
 			}
 
-			ret, err := validateOCSP(ocspRes)
+			ret, err := validateOCSP(conf, ocspRes)
 			if err != nil {
 				errors[i] = err
 				return err
@@ -582,6 +607,12 @@ func (c *Client) GetRevocationStatus(ctx context.Context, subject, issuer *x509.
 	if !isValidOCSPStatus(ret.code) {
 		return ret, nil
 	}
+
+	if ocspRes.NextUpdate.IsZero() {
+		// We should not cache values with no NextUpdate values
+		return ret, nil
+	}
+
 	v := ocspCachedResponse{
 		status:     ret.code,
 		time:       float64(time.Now().UTC().Unix()),
@@ -602,11 +633,12 @@ func isValidOCSPStatus(status ocspStatusCode) bool {
 }
 
 type VerifyConfig struct {
-	OcspEnabled         bool
-	ExtraCas            []*x509.Certificate
-	OcspServersOverride []string
-	OcspFailureMode     FailOpenMode
-	QueryAllServers     bool
+	OcspEnabled          bool
+	ExtraCas             []*x509.Certificate
+	OcspServersOverride  []string
+	OcspFailureMode      FailOpenMode
+	QueryAllServers      bool
+	OcspThisUpdateMaxAge time.Duration
 }
 
 // VerifyLeafCertificate verifies just the subject against it's direct issuer
@@ -692,12 +724,12 @@ func (c *Client) canEarlyExitForOCSP(results []*ocspStatus, chainSize int, conf
 	return nil
 }
 
-func (c *Client) validateWithCacheForAllCertificates(verifiedChains []*x509.Certificate) (bool, error) {
+func (c *Client) validateWithCacheForAllCertificates(verifiedChains []*x509.Certificate, config *VerifyConfig) (bool, error) {
 	n := len(verifiedChains) - 1
 	for j := 0; j < n; j++ {
 		subject := verifiedChains[j]
 		issuer := verifiedChains[j+1]
-		status, _, _, err := c.validateWithCache(subject, issuer)
+		status, _, _, err := c.validateWithCache(subject, issuer, config)
 		if err != nil {
 			return false, err
 		}
@@ -708,7 +740,7 @@ func (c *Client) validateWithCacheForAllCertificates(verifiedChains []*x509.Cert
 	return true, nil
 }
 
-func (c *Client) validateWithCache(subject, issuer *x509.Certificate) (*ocspStatus, []byte, *certIDKey, error) {
+func (c *Client) validateWithCache(subject, issuer *x509.Certificate, config *VerifyConfig) (*ocspStatus, []byte, *certIDKey, error) {
 	ocspReq, err := ocsp.CreateRequest(subject, issuer, &ocsp.RequestOptions{})
 	if err != nil {
 		return nil, nil, nil, fmt.Errorf("failed to create OCSP request from the certificates: %v", err)
@@ -717,15 +749,15 @@ func (c *Client) validateWithCache(subject, issuer *x509.Certificate) (*ocspStat
 	if ocspS.code != ocspSuccess {
 		return nil, nil, nil, fmt.Errorf("failed to extract CertID from OCSP Request: %v", err)
 	}
-	status, err := c.checkOCSPResponseCache(encodedCertID, subject, issuer)
+	status, err := c.checkOCSPResponseCache(encodedCertID, subject, issuer, config)
 	if err != nil {
 		return nil, nil, nil, err
 	}
 	return status, ocspReq, encodedCertID, nil
 }
 
 func (c *Client) GetAllRevocationStatus(ctx context.Context, verifiedChains []*x509.Certificate, conf *VerifyConfig) ([]*ocspStatus, error) {
-	_, err := c.validateWithCacheForAllCertificates(verifiedChains)
+	_, err := c.validateWithCacheForAllCertificates(verifiedChains, conf)
 	if err != nil {
 		return nil, err
 	}
@@ -750,11 +782,11 @@ func (c *Client) verifyPeerCertificateSerial(conf *VerifyConfig) func(_ [][]byte
 	}
 }
 
-func (c *Client) extractOCSPCacheResponseValueWithoutSubject(cacheValue ocspCachedResponse) (*ocspStatus, error) {
-	return c.extractOCSPCacheResponseValue(&cacheValue, nil, nil)
+func (c *Client) extractOCSPCacheResponseValueWithoutSubject(cacheValue ocspCachedResponse, conf *VerifyConfig) (*ocspStatus, error) {
+	return c.extractOCSPCacheResponseValue(&cacheValue, nil, nil, conf)
 }
 
-func (c *Client) extractOCSPCacheResponseValue(cacheValue *ocspCachedResponse, subject, issuer *x509.Certificate) (*ocspStatus, error) {
+func (c *Client) extractOCSPCacheResponseValue(cacheValue *ocspCachedResponse, subject, issuer *x509.Certificate, conf *VerifyConfig) (*ocspStatus, error) {
 	subjectName := "Unknown"
 	if subject != nil {
 		subjectName = subject.Subject.CommonName
@@ -778,7 +810,7 @@ func (c *Client) extractOCSPCacheResponseValue(cacheValue *ocspCachedResponse, s
 
 	sdkOcspStatus := internalStatusCodeToSDK(cacheValue.status)
 
-	return validateOCSP(&ocsp.Response{
+	return validateOCSP(conf, &ocsp.Response{
 		ProducedAt: time.Unix(int64(cacheValue.producedAt), 0).UTC(),
 		ThisUpdate: time.Unix(int64(cacheValue.thisUpdate), 0).UTC(),
 		NextUpdate: time.Unix(int64(cacheValue.nextUpdate), 0).UTC(),