feat: add OCSP and CRL revocation checks #128

kody-kimberl · 2023-03-11T00:06:12Z

This PR adds OCSP and CRL revocation checking to the Verify function of COSE and JWS envelopes.

To do this, it implements a new dependency on https://pkg.go.dev/github.com/cloudflare/cfssl/revoke, as well as a dependency on https://pkg.go.dev/golang.org/x/crypto/ocsp for testing purposes.

I added a few new tests that use a mock OCSP and CRL server to simulate revoked certificates.

This PR intends to resolve the following issues:

Before investing too much more time, I wanted to publish this initial PR to ensure we agree on this approach.

Signed-off-by: Kody Kimberl [email protected]

Signed-off-by: Kody Kimberl <[email protected]>

priteshbandi · 2023-03-14T01:07:52Z

signature/cose/envelope.go

+	rc := signature.NewRevocationChecker()
+	revoked, err := rc.CheckRevocationStatus(cert)
+	if err != nil {
+		return nil, err
+	} else if revoked {
+		return nil, &signature.InvalidSignatureError{Msg: "certificate has been revoked"}
+	}


Check should be done in notation-go, depending upon trust policy configuration the outcome of verification will change.
Ref: https://github.com/notaryproject/notaryproject/blob/main/specs/trust-store-trust-policy.md#certificate-revocation-evaluation

The idea is that functions to check for revocation status should be in notation-core-go but actual check should happen in notation-go

priteshbandi · 2023-03-14T01:09:43Z

signature/revocation.go

+
+func (r RevocationChecker) CheckRevocationStatus(cert *x509.Certificate) (bool, error) {
+	revoke.HTTPClient = r.HTTPClient
+	revoked, _, err := revoke.VerifyCertificateError(cert)


Does it do OSCP check first then CRL?

Ref: https://github.com/notaryproject/notaryproject/blob/main/specs/trust-store-trust-policy.md#ocsp

priteshbandi · 2023-03-14T01:22:26Z

signature/revocation.go

+
+func (r RevocationChecker) CheckRevocationStatus(cert *x509.Certificate) (bool, error) {
+	revoke.HTTPClient = r.HTTPClient
+	revoked, _, err := revoke.VerifyCertificateError(cert)


This method does more than revocation check which we dont want
https://github.com/cloudflare/cfssl/blob/master/revoke/revoke.go#L191-L202

priteshbandi · 2023-03-14T02:01:00Z

go.mod

+	cloud.google.com/go v0.81.0 // indirect
+	github.com/beorn7/perks v1.0.1 // indirect
+	github.com/bgentry/speakeasy v0.1.0 // indirect
+	github.com/census-instrumentation/opencensus-proto v0.3.0 // indirect
+	github.com/cespare/xxhash/v2 v2.1.1 // indirect
+	github.com/cncf/udpa/go v0.0.0-20210322005330-6414d713912e // indirect
+	github.com/coreos/go-semver v0.3.0 // indirect
+	github.com/coreos/go-systemd/v22 v22.3.2 // indirect
+	github.com/cpuguy83/go-md2man/v2 v2.0.0 // indirect
+	github.com/dustin/go-humanize v1.0.0 // indirect
+	github.com/envoyproxy/go-control-plane v0.9.9-0.20210217033140-668b12f5399d // indirect
+	github.com/envoyproxy/protoc-gen-validate v0.6.1 // indirect
+	github.com/form3tech-oss/jwt-go v3.2.3+incompatible // indirect
+	github.com/fullstorydev/grpcurl v1.8.1 // indirect
+	github.com/gogo/protobuf v1.3.2 // indirect
+	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
+	github.com/golang/mock v1.5.0 // indirect
+	github.com/golang/protobuf v1.5.2 // indirect
+	github.com/google/btree v1.0.1 // indirect
+	github.com/google/certificate-transparency-go v1.1.2-0.20210511102531-373a877eec92 // indirect
+	github.com/google/go-cmp v0.5.5 // indirect
+	github.com/google/uuid v1.2.0 // indirect
+	github.com/gorilla/websocket v1.4.2 // indirect
+	github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 // indirect
+	github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect
+	github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect
+	github.com/inconshreveable/mousetrap v1.0.0 // indirect
+	github.com/jhump/protoreflect v1.8.2 // indirect
+	github.com/jmoiron/sqlx v1.3.3 // indirect
+	github.com/jonboulle/clockwork v0.2.2 // indirect
+	github.com/json-iterator/go v1.1.11 // indirect
+	github.com/mattn/go-runewidth v0.0.12 // indirect
+	github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
+	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
+	github.com/modern-go/reflect2 v1.0.1 // indirect
+	github.com/olekukonko/tablewriter v0.0.5 // indirect
+	github.com/prometheus/client_golang v1.10.0 // indirect
+	github.com/prometheus/client_model v0.2.0 // indirect
+	github.com/prometheus/common v0.24.0 // indirect
+	github.com/prometheus/procfs v0.6.0 // indirect
+	github.com/rivo/uniseg v0.2.0 // indirect
+	github.com/russross/blackfriday/v2 v2.1.0 // indirect
+	github.com/sirupsen/logrus v1.8.1 // indirect
+	github.com/soheilhy/cmux v0.1.5 // indirect
+	github.com/spf13/cobra v1.1.3 // indirect
+	github.com/spf13/pflag v1.0.5 // indirect
+	github.com/tmc/grpc-websocket-proxy v0.0.0-20201229170055-e5319fda7802 // indirect
+	github.com/urfave/cli v1.22.5 // indirect
+	github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2 // indirect
+	go.etcd.io/bbolt v1.3.5 // indirect
+	go.etcd.io/etcd/api/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/client/v2 v2.305.0-alpha.0 // indirect
+	go.etcd.io/etcd/client/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/etcdctl/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/pkg/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/raft/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/server/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/tests/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/v3 v3.5.0-alpha.0 // indirect
+	go.uber.org/atomic v1.7.0 // indirect
+	go.uber.org/multierr v1.7.0 // indirect
+	go.uber.org/zap v1.16.0 // indirect
+	golang.org/x/mod v0.4.2 // indirect
+	golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2 // indirect
+	golang.org/x/oauth2 v0.0.0-20210427180440-81ed05c6b58c // indirect
+	golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1 // indirect
+	golang.org/x/text v0.3.6 // indirect
+	golang.org/x/time v0.0.0-20210220033141-f8bda1e9f3ba // indirect
+	golang.org/x/tools v0.1.0 // indirect
+	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
+	google.golang.org/appengine v1.6.7 // indirect
+	google.golang.org/genproto v0.0.0-20210510173355-fb37daa5cd7a // indirect
+	google.golang.org/grpc v1.37.0 // indirect
+	google.golang.org/protobuf v1.26.0 // indirect
+	gopkg.in/cheggaaa/pb.v1 v1.0.28 // indirect
+	gopkg.in/yaml.v2 v2.4.0 // indirect
+	sigs.k8s.io/yaml v1.2.0 // indirect


These seems to be an awful lot of dependency for crl and ocsp check :O

Yes, the notation-core-go's dependency should be as much as concise.

notation-core-go should not import so many non-relevant dependencies.

shizhMSFT

It would be better if a design doc for the golang implementation is provided.

shizhMSFT · 2023-03-14T14:13:52Z

go.mod

@@ -6,6 +6,88 @@ require (
 	github.com/fxamacker/cbor/v2 v2.4.0
 	github.com/golang-jwt/jwt/v4 v4.5.0
 	github.com/veraison/go-cose v1.0.0
+	golang.org/x/crypto v0.0.0-20220824171710-5757bc0c5503


A tagged version of golang.org/x/crypto should be used. The current latest one is v0.7.0

shizhMSFT · 2023-03-14T14:14:27Z

go.mod

+	cloud.google.com/go v0.81.0 // indirect
+	github.com/beorn7/perks v1.0.1 // indirect
+	github.com/bgentry/speakeasy v0.1.0 // indirect
+	github.com/census-instrumentation/opencensus-proto v0.3.0 // indirect
+	github.com/cespare/xxhash/v2 v2.1.1 // indirect
+	github.com/cncf/udpa/go v0.0.0-20210322005330-6414d713912e // indirect
+	github.com/coreos/go-semver v0.3.0 // indirect
+	github.com/coreos/go-systemd/v22 v22.3.2 // indirect
+	github.com/cpuguy83/go-md2man/v2 v2.0.0 // indirect
+	github.com/dustin/go-humanize v1.0.0 // indirect
+	github.com/envoyproxy/go-control-plane v0.9.9-0.20210217033140-668b12f5399d // indirect
+	github.com/envoyproxy/protoc-gen-validate v0.6.1 // indirect
+	github.com/form3tech-oss/jwt-go v3.2.3+incompatible // indirect
+	github.com/fullstorydev/grpcurl v1.8.1 // indirect
+	github.com/gogo/protobuf v1.3.2 // indirect
+	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
+	github.com/golang/mock v1.5.0 // indirect
+	github.com/golang/protobuf v1.5.2 // indirect
+	github.com/google/btree v1.0.1 // indirect
+	github.com/google/certificate-transparency-go v1.1.2-0.20210511102531-373a877eec92 // indirect
+	github.com/google/go-cmp v0.5.5 // indirect
+	github.com/google/uuid v1.2.0 // indirect
+	github.com/gorilla/websocket v1.4.2 // indirect
+	github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 // indirect
+	github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect
+	github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect
+	github.com/inconshreveable/mousetrap v1.0.0 // indirect
+	github.com/jhump/protoreflect v1.8.2 // indirect
+	github.com/jmoiron/sqlx v1.3.3 // indirect
+	github.com/jonboulle/clockwork v0.2.2 // indirect
+	github.com/json-iterator/go v1.1.11 // indirect
+	github.com/mattn/go-runewidth v0.0.12 // indirect
+	github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
+	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
+	github.com/modern-go/reflect2 v1.0.1 // indirect
+	github.com/olekukonko/tablewriter v0.0.5 // indirect
+	github.com/prometheus/client_golang v1.10.0 // indirect
+	github.com/prometheus/client_model v0.2.0 // indirect
+	github.com/prometheus/common v0.24.0 // indirect
+	github.com/prometheus/procfs v0.6.0 // indirect
+	github.com/rivo/uniseg v0.2.0 // indirect
+	github.com/russross/blackfriday/v2 v2.1.0 // indirect
+	github.com/sirupsen/logrus v1.8.1 // indirect
+	github.com/soheilhy/cmux v0.1.5 // indirect
+	github.com/spf13/cobra v1.1.3 // indirect
+	github.com/spf13/pflag v1.0.5 // indirect
+	github.com/tmc/grpc-websocket-proxy v0.0.0-20201229170055-e5319fda7802 // indirect
+	github.com/urfave/cli v1.22.5 // indirect
+	github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2 // indirect
+	go.etcd.io/bbolt v1.3.5 // indirect
+	go.etcd.io/etcd/api/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/client/v2 v2.305.0-alpha.0 // indirect
+	go.etcd.io/etcd/client/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/etcdctl/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/pkg/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/raft/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/server/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/tests/v3 v3.5.0-alpha.0 // indirect
+	go.etcd.io/etcd/v3 v3.5.0-alpha.0 // indirect
+	go.uber.org/atomic v1.7.0 // indirect
+	go.uber.org/multierr v1.7.0 // indirect
+	go.uber.org/zap v1.16.0 // indirect
+	golang.org/x/mod v0.4.2 // indirect
+	golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2 // indirect
+	golang.org/x/oauth2 v0.0.0-20210427180440-81ed05c6b58c // indirect
+	golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1 // indirect
+	golang.org/x/text v0.3.6 // indirect
+	golang.org/x/time v0.0.0-20210220033141-f8bda1e9f3ba // indirect
+	golang.org/x/tools v0.1.0 // indirect
+	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
+	google.golang.org/appengine v1.6.7 // indirect
+	google.golang.org/genproto v0.0.0-20210510173355-fb37daa5cd7a // indirect
+	google.golang.org/grpc v1.37.0 // indirect
+	google.golang.org/protobuf v1.26.0 // indirect
+	gopkg.in/cheggaaa/pb.v1 v1.0.28 // indirect
+	gopkg.in/yaml.v2 v2.4.0 // indirect
+	sigs.k8s.io/yaml v1.2.0 // indirect


notation-core-go should not import so many non-relevant dependencies.

shizhMSFT · 2023-03-14T14:16:30Z

go.mod

@@ -6,6 +6,88 @@ require (
 	github.com/fxamacker/cbor/v2 v2.4.0
 	github.com/golang-jwt/jwt/v4 v4.5.0
 	github.com/veraison/go-cose v1.0.0
+	golang.org/x/crypto v0.0.0-20220824171710-5757bc0c5503
+	github.com/cloudflare/cfssl v1.6.3


Is the use of github.com/cloudflare/cfssl approved by crypto board of any organization?

shizhMSFT · 2023-03-14T14:41:47Z

testhelper/httptest.go

Please document all exported types and methods.

shizhMSFT · 2023-03-14T14:44:34Z

testhelper/httptest.go

+	return len(b), nil
+}
+
+func MockServer(cert *x509.Certificate, key *rsa.PrivateKey, desiredOCSPStatus ocsp.ResponseStatus, revokedInCRL bool, wg *sync.WaitGroup) *http.Server {


package httptest should be used as a mock server instead of a real server.

shizhMSFT · 2023-03-14T14:46:50Z

signature/revocation.go

Please document all exported types and methods.

shizhMSFT · 2023-03-14T14:49:29Z

signature/revocation.go

+	HTTPClient *http.Client
+}
+
+func NewRevocationChecker(httpClients ...http.Client) *RevocationChecker {


What's the scenario to take multiple http client?

shizhMSFT · 2023-03-14T14:51:23Z

signature/revocation.go

+}
+
+func (r RevocationChecker) CheckRevocationStatus(cert *x509.Certificate) (bool, error) {
+	revoke.HTTPClient = r.HTTPClient


Setting to a global variable revoke.HTTPClient is dangerous due to race conditions.

shizhMSFT · 2023-03-14T14:52:14Z

signature/cose/envelope.go

+	rc := signature.NewRevocationChecker()
+	revoked, err := rc.CheckRevocationStatus(cert)
+	if err != nil {
+		return nil, err
+	} else if revoked {
+		return nil, &signature.InvalidSignatureError{Msg: "certificate has been revoked"}
+	}


shizhMSFT · 2023-03-14T14:53:18Z

signature/jws/envelope.go

+	revoked, err := rc.CheckRevocationStatus(cert)
+	if err != nil {
+		return nil, err
+	} else if revoked {
+		return nil, &signature.InvalidSignatureError{Msg: "certificate has been revoked"}
+	}


This can be refactored as

Suggested change

revoked, err := rc.CheckRevocationStatus(cert)

if err != nil {

return nil, err

} else if revoked {

return nil, &signature.InvalidSignatureError{Msg: "certificate has been revoked"}

}

if err := rc.CheckRevocationStatus(cert); err != nil {

return nil, err

}

Caller just needs the error. The revoked value can be one type of error.

kody-kimberl · 2023-03-21T21:09:14Z

Proposed design for OCSP revocation: #132

feat: add OCSP and CRL revocation checks

649e291

Signed-off-by: Kody Kimberl <[email protected]>

kody-kimberl requested review from gokarnm, JeyJeyGao, justincormack, NiazFK, patrickzheng200, priteshbandi, rgnote, shizhMSFT and SteveLasker as code owners March 11, 2023 00:06

priteshbandi reviewed Mar 14, 2023

View reviewed changes

shizhMSFT requested changes Mar 14, 2023

View reviewed changes

priteshbandi marked this pull request as draft March 17, 2023 17:17

kody-kimberl closed this Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add OCSP and CRL revocation checks #128

feat: add OCSP and CRL revocation checks #128

kody-kimberl commented Mar 11, 2023

priteshbandi Mar 14, 2023 •

edited

Loading

shizhMSFT Mar 14, 2023

priteshbandi Mar 14, 2023 •

edited

Loading

priteshbandi Mar 14, 2023

priteshbandi Mar 14, 2023

JeyJeyGao Mar 14, 2023

shizhMSFT Mar 14, 2023

shizhMSFT left a comment

shizhMSFT Mar 14, 2023

shizhMSFT Mar 14, 2023

shizhMSFT Mar 14, 2023

shizhMSFT Mar 14, 2023

shizhMSFT Mar 14, 2023

shizhMSFT Mar 14, 2023

shizhMSFT Mar 14, 2023

shizhMSFT Mar 14, 2023

shizhMSFT Mar 14, 2023

shizhMSFT Mar 14, 2023

kody-kimberl commented Mar 21, 2023

feat: add OCSP and CRL revocation checks #128

feat: add OCSP and CRL revocation checks #128

Conversation

kody-kimberl commented Mar 11, 2023

priteshbandi Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priteshbandi Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kody-kimberl commented Mar 21, 2023

priteshbandi Mar 14, 2023 •

edited

Loading

priteshbandi Mar 14, 2023 •

edited

Loading