doc: outline design for OCSP revocation

[kody-kimberl]

This issue outlines how the core revocation functionality will be built into notation-core-go, and how it will be integrated with notation-go. This additional functionality will provide a mechanism to check the revocation status of certificates in a way that can be easily integrated into other projects, and to conduct revocation checks as part of the Verify process.

These changes will address the following issue:

• Implement OCSP Support ([revocation] Implement OCSP support #124)

CRL Support will not be implemented until later.

The outlined mechanisms are based upon previously defined specifications (https://github.com/notaryproject/notaryproject/blob/main/specs/trust-store-trust-policy.md#certificate-revocation-evaluation) as well as some ideas from existing solutions (https://pkg.go.dev/github.com/cloudflare/cfssl/revoke and https://github.com/grpc/grpc-go/blob/52ca9571068d/security/advancedtls/crl.go) that don’t fully match our use case.

Implementation is planned to be completed in two PRs. The first will introduce the revocation package in notation-core-go. The second PR will follow, which will incorporate the revocation package into notation-go. These sections are separated by a horizontal line.

---

Notation-core-go

The following will be added as a new package within the x509 package via a new file, revocation.go:

// Internal struct
type revocation struct {
    httpClient            *http.Client
    ocspTimeoutThreshold  time.Duration
}

// Options struct
// Users can specify any options they want for constructing a revocation object
// If an option is unspecified or nil, a default value will be assigned in the NewRevocation function
type RevocationOptions struct {
    ocspTimeoutThreshold  time.Duration
}

// Constructor to return a revocation object that substitutes default values for any that are passed as nil
func NewRevocation(httpClient *http.Client, options RevocationOptions) *revocation


// Errors:
// RevokedInOCSPError is returned when the certificate's status for OCSP is ocsp.Revoked (or ocsp.Unknown?)
type RevokedInOCSPError struct{}
func (e RevokedInOCSPError) Error() string {
    return "certificate is revoked via OCSP"
}

// CheckOCSPError is returned when there is an error during the OCSP revocation check, not necessarily a revocation
type CheckOCSPError struct {
    Err error
}
func (e CheckOCSPError) Error() string {
    if e.Err != nil {
        return fmt.Sprintf("error checking revocation via OCSP: %s", e.Err.Error())
    } else {
        return "error checking revocation via OCSP"
    }
}

// NoOCSPServerError is returned when the OCSPServer is not specified or is not an HTTP URL.
type NoOCSPServerError struct{}
func (e NoOCSPServerError) Error() string {
    return "no valid OCSPServer found"
}

// OCSPTimeoutError is returned when the connection attempt to a OCSP URL exceeds the specified threshold
type OCSPTimeoutError struct{}
func (e OCSPTimeoutError) Error() string {
    return "exceeded timeout threshold for OCSP check"
}

// It may be necessary to add other errors to the ones specified as new errors/results are discovered during development

// Methods:
// Checks OCSP, then CRL status, returns nil if all certs in the chain are not revoked
// If any are revoked, it will return one of the defined errors in this file
// If there is some other error, it will return that error
// NOTE: Will only perform OCSP until CRL is implemented
func (r *revocation) Validate(certChain []*x509.Certificate) (error)

// Only checks OCSP, same returns as above
func (r *revocation) OCSPStatus(certChain []*x509.Certificate) (error)

Since testing this will require validating against OCSP server responses, a mock client needs to be created to mock these responses. This will be added to the testhelper package in a new file named httptest.go:

// Creates a mock HTTP Client (more accurately, a spy) that intercepts requests to /issuer and /ocsp endpoints
// The client's responses are dependent on the cert and desiredOCSPStatus
func MockClient(cert *x509.Certificate, key *rsa.PrivateKey, desiredOCSPStatus ocsp.ResponseStatus) *http.Client

In addition to the mocked client, we will also need a new test certificate that specifies an OCSPServer in the certificate template. This value will be necessary to confirm revocation status with the mocked client. The following will be added to the testhelper package in the existing certificatetest.go file:

var revokableRSALeaf RSACertTuple

// GetRevokableRSALeafCertificate returns leaf certificate that specifies a local OCSPServer signed using RSA algorithm
func GetRevokableRSALeafCertificate() RSACertTuple

---

Notation-go

The following will be added/modified in the verifier package within the existing verifier.go file:

// Since a section has already been added to Verify as a placeholder for revocation, it will be modified to incorporate the verifyRevocation function
func (v *verifier) Verify(ctx context.Context, desc ocispec.Descriptor, signature []byte, opts notation.VerifyOptions) (*notation.VerificationOutcome, error)

// The verifyRevocation function will use the revocation.Validate() function defined in notation-core-go to verify the revocation status
func verifyRevocation(outcome *notation.VerificationOutcome) *notation.ValidationResult

TypeRevocation has already been added to the ValidationTypes in the trustpolicy, so no change is needed there.

[patrickzheng200]

Thanks @kody-kimberl for the design. I have some open discussions below:

1. I'm just trying to go through the workflow, so basically, func OCSPStatus() will take in the cert chain and do OCSP check one by one. Inside this func, I suppose we will be creating OCSP requests using https://pkg.go.dev/golang.org/x/crypto@v0.7.0/ocsp#CreateRequest? Such request requires an issuer from the cert via IssuingCertificateURL. IMO, we could have another internal function func ocspRequest() in this design for generating the OCSP requests. (it'll be called from func OCSPStatus())
2. Do we need the ocspTimeoutThreshold field in type revocation struct? Because a http.Client struct has its built in Timeout field (https://pkg.go.dev/net/http#Client), can we use that direclty?
3. There are a few Error types defined in revocation.go, I think we could move them to a new file with name errors.go.

[shizhMSFT]

Thank @kody-kimberl for sharing the design. Will the revocation implementation in notation-core-go be purely based on the golang built-in crypto library (such as RevocationList.CheckSignatureFrom())?

[kody-kimberl]

Thank you for all the great comments!

---

@patrickzheng200

1. I'm just trying to go through the workflow, so basically, func OCSPStatus() will take in the cert chain and do OCSP check one by one. Inside this func, I suppose we will be creating OCSP requests using https://pkg.go.dev/golang.org/x/crypto@v0.7.0/ocsp#CreateRequest? Such request requires an issuer from the cert via IssuingCertificateURL. IMO, we could have another internal function func ocspRequest() in this design for generating the OCSP requests. (it'll be called from func OCSPStatus())

I 100% agree. I was planning on having a helper function for generating this request as you mentioned, and I like your name for it. Having this logic separated will allow the requests to be done in parallel for each certificate in the chain.

2. Do we need the ocspTimeoutThreshold field in type revocation struct? Because a http.Client struct has its built in Timeout field (https://pkg.go.dev/net/http#Client), can we use that direclty?

The reason I included this is to be forward thinking. The specification states that there should be a different timeout for OCSP and CRL. Once we incorporate CRL revocation, we will need an additional timeout option. Thus, I figured it is best to decouple the OCSP timeout from the client initially in order to minimize the changes that will be necessary to add CRL revocation in the future.

3. There are a few Error types defined in revocation.go, I think we could move them to a new file with name errors.go.

I agree, I'll add this when implementing it.

---

@shizhMSFT

Will the revocation implementation in notation-core-go be purely based on the golang built-in crypto library (such as RevocationList.CheckSignatureFrom())?

Primarily, yes. As Patrick mentioned, there will be an additional need for the https://pkg.go.dev/golang.org/x/crypto (which we already use elsewhere) in order to create the OCSP requests (and the mock responses for testing). No other libraries should be necessary based on how I've planned to implement it.

[priteshbandi]

Do we need the ocspTimeoutThreshold field in type revocation struct? Because a http.Client struct has its built in Timeout field (https://pkg.go.dev/net/http#Client), can we use that directly?

The reason I included this is to be forward thinking. The specification states that there should be a different timeout for OCSP and CRL. Once we incorporate CRL revocation, we will need an additional timeout option. Thus, I figured it is best to decouple the OCSP timeout from the client initially in order to minimize the changes that will be necessary to add CRL revocation in the future.

Was thinking more about it and from end user perspective it wouldn't matter if its OSCP or CRL or any other revocation mechanism, they would want revocation to complete in x seconds. Also, we can add ocspTimeoutThreshold in future if some user needs it.

[shizhMSFT]

No other libraries should be necessary based on how I've planned to implement it.

That’s great!

[patrickzheng200]

Primarily, yes. As Patrick mentioned, there will be an additional need for the https://pkg.go.dev/golang.org/x/crypto (which we already use elsewhere) in order to create the OCSP requests (and the mock responses for testing). No other libraries should be necessary based on how I've planned to implement it.

Awesome, all my questions so far have been answered. This design looks good to me. I guess I'll have further implementation questions once the PR is created. Thanks. /cc: @kody-kimberl, @priteshbandi, @shizhMSFT

[github-actions]

This issue is stale because it has been opened for 60 days with no activity. Remove stale label or comment. Otherwise, it will be closed in 30 days.

[github-actions]

Issue closed due to no activity in the past 30 days.