Add cached OCSP client support to Cert Auth #17093
Conversation
Yeah, I expected a lot of nits given the sprint of coding. |
1 similar comment
Yeah, I expected a lot of nits given the sprint of coding. |
…ed values to stay under the storage entry limit
…l testing of multiple servers
Co-authored-by: Alexander Scheel <[email protected]>
| } | ||
| if len(rest) > 0 { | ||
| // extra bytes to the end | ||
| return nil, err |
There was a problem hiding this comment.
This returns nil, nil, wouldn't we want to return a non-nil error here?
|
|
||
| // isInValidityRange checks the validity | ||
| func isInValidityRange(currTime, nextUpdate time.Time) bool { | ||
| return !nextUpdate.IsZero() && !currTime.After(nextUpdate) |
There was a problem hiding this comment.
My reading of https://datatracker.ietf.org/doc/html/rfc6960#section-3.2 seems to state that nextUpdate is an optional field, i.e., if the OCSP responder doesn't know, they can leave this field blank.
This matches the ASN.1:
nextUpdate [0] EXPLICIT GeneralizedTime OPTIONAL,
If that's the case, I'd probably just sub in the default cache expiry period to make things simple, tbh.
| case ocsp.Good: | ||
| return &ocspStatus{ | ||
| code: ocspStatusGood, | ||
| err: nil, |
There was a problem hiding this comment.
Can probably elide err: nil for consistency.
| return returnOCSPStatus(ocspRes), nil | ||
| } | ||
|
|
||
| func returnOCSPStatus(ocspRes *ocsp.Response) *ocspStatus { |
There was a problem hiding this comment.
Seems weird that we're translating from one internal form to another internal form?
There was a problem hiding this comment.
Is this possibly for their file caching?
There was a problem hiding this comment.
Yeah, wondering if perhaps it is that. Then they're stable internal identifiers rather than external identifiers that might potentially change and break their cached entries.
But still, I don't think Go upstream would make that type of constants change, except maybe if they import x/ocsp into the main series.
| if cacheValue == nil { | ||
| return &ocspStatus{ | ||
| code: ocspMissedCache, | ||
| err: fmt.Errorf("miss cache data. subject: %v", subjectName), |
There was a problem hiding this comment.
Subject... is just used to have something to log?
| producedAt: float64(ocspRes.ProducedAt.UTC().Unix()), | ||
| thisUpdate: float64(ocspRes.ThisUpdate.UTC().Unix()), | ||
| nextUpdate: float64(ocspRes.NextUpdate.UTC().Unix()), | ||
| } |
There was a problem hiding this comment.
Don't we need to set status here, out of curiosity?
| if !verifiedChains[i][numberOfNoneRootCerts].IsCA || string(verifiedChains[i][numberOfNoneRootCerts].RawIssuer) != string(verifiedChains[i][numberOfNoneRootCerts].RawSubject) { | ||
| // Check if the last Non Root Cert is also a CA or is self signed. | ||
| // if the last certificate is not, add it to the list | ||
| rca := c.caRoot[string(verifiedChains[i][numberOfNoneRootCerts].RawIssuer)] |
There was a problem hiding this comment.
Root subjects are not necessarily unique, but we can fix this later I suppose :-)
sdk/helper/ocsp/client.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (c *Client) ValidateWithCacheForAllCertificates(verifiedChains []*x509.Certificate) (bool, error) { |
There was a problem hiding this comment.
Just curious why this is exported?
There was a problem hiding this comment.
Earlier version of the code called this. Fixed.
sdk/helper/ocsp/client.go
Outdated
| return true, nil | ||
| } | ||
|
|
||
| func (c *Client) ValidateWithCache(subject, issuer *x509.Certificate) (*ocspStatus, []byte, *certIDKey, error) { |
| } | ||
|
|
||
| // insecureOcspTransport is the transport object that doesn't do certificate revocation check. | ||
| func newInsecureOcspTransport(extraCas []*x509.Certificate) *http.Transport { |
There was a problem hiding this comment.
Should this just go in the test helpers?
There was a problem hiding this comment.
Now that we have them, yes.
There was a problem hiding this comment.
Actually, no, it is used by the OCSP request logic itself.
There was a problem hiding this comment.
Ah, d'oh, sorry.
I don't like the name then, its not any more insecure than the default Go helper, perhaps just newTransportWithoutOcsp?
Co-authored-by: Alexander Scheel <[email protected]>
sdk/helper/ocsp/client.go
Outdated
|
|
||
| // cache key | ||
| type certIDKey struct { | ||
| HashAlgorithm crypto.Hash |
There was a problem hiding this comment.
Per discussion on Slack, we can probably remove this field and clarify that NameHash is the Issuer's Name (for uniqueness) and the SerialNumber is the leaf's serial number; combination of (IssuerName, IssuerKey) should uniquely identify the issuer, and LeafSerialNumber should uniquely identify the leaf within that parent issuer.
Consensus seems to be that the hash algorithm doesn't matter (as it is whatever the OCSP responder uses and doesn't really change our behavior if we want to prefer other hashes) even if it has changed, doesn't really impact our treating previously cached entries as valid or not, given the other three fields are still unique.
| return returnOCSPStatus(ocspRes), nil | ||
| } | ||
|
|
||
| func returnOCSPStatus(ocspRes *ocsp.Response) *ocspStatus { |
There was a problem hiding this comment.
Yeah, wondering if perhaps it is that. Then they're stable internal identifiers rather than external identifiers that might potentially change and break their cached entries.
But still, I don't think Go upstream would make that type of constants change, except maybe if they import x/ocsp into the main series.
|
Can we please get configurable ocsp request timeouts? Imagine ocsp responder being down (firewalled, incorrectly configured, etc), your ocsp client would be banging it 5 times before giving up. And the cert auth would hang for ~1 min. This isn't the way to do things. Thanks. |
Using a modified version of an Apache licensed OCSP client for Go, modified
to use Vault's storage and have better more consistent error handling,
support for custom configured CAs and overridden OCSP servers.
Mostly for early consideration.