diff --git a/revocation/crl/errors.go b/revocation/crl/errors.go deleted file mode 100644 index f23c2fb3..00000000 --- a/revocation/crl/errors.go +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright The Notary Project Authors. -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package crl provides the error message for processing crls. -package crl - -import "github.com/notaryproject/notation-core-go/revocation/internal/crl" - -var ( - // ErrDeltaCRLNotSupported is returned when the CRL contains a delta CRL but - // the delta CRL is not supported - ErrDeltaCRLNotSupported = crl.ErrDeltaCRLNotSupported -) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 134e8117..227c50c6 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -67,7 +67,11 @@ type CertCheckStatusOptions struct { // // Please ensure that the certificate supports CRL by calling Supported before // calling this function. -func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts CertCheckStatusOptions) *result.CertRevocationResult { +func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts CertCheckStatusOptions) (*result.CertRevocationResult, error) { + if !Supported(cert) { + return nil, fmt.Errorf("certificate %v does not support CRL", cert.Subject.CommonName) + } + // The CRLDistributionPoints contains the URIs of all the CRL distribution // points. Since it does not distinguish the reason field, it needs to check // all the URIs to avoid missing any partial CRLs. @@ -103,26 +107,26 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C return &result.CertRevocationResult{ Result: result.ResultRevoked, CRLResults: crlResults, - } + }, nil } } if lastErr != nil { - crlResults = append(crlResults, &result.CRLResult{ - Result: result.ResultUnknown, - URI: crlURL, - Error: lastErr, - }) return &result.CertRevocationResult{ - Result: result.ResultUnknown, - CRLResults: crlResults, - } + Result: result.ResultUnknown, + CRLResults: []*result.CRLResult{ + &result.CRLResult{ + Result: result.ResultUnknown, + URI: crlURL, + Error: lastErr, + }}, + }, nil } return &result.CertRevocationResult{ Result: result.ResultOK, CRLResults: crlResults, - } + }, nil } // Supported checks if the certificate supports CRL. diff --git a/revocation/internal/crl/crl_test.go b/revocation/internal/crl/crl_test.go index 4b903fe8..afb9587d 100644 --- a/revocation/internal/crl/crl_test.go +++ b/revocation/internal/crl/crl_test.go @@ -37,11 +37,14 @@ func TestCertCheckStatus(t *testing.T) { cert := &x509.Certificate{ CRLDistributionPoints: []string{"http://example.com"}, } - r := CertCheckStatus(context.Background(), cert, &x509.Certificate{}, CertCheckStatusOptions{ + r, err := CertCheckStatus(context.Background(), cert, &x509.Certificate{}, CertCheckStatusOptions{ HTTPClient: &http.Client{ Transport: errorRoundTripperMock{}, }, }) + if err != nil { + t.Fatal(err) + } if r.CRLResults[0].Error == nil { t.Fatal("expected error") } @@ -51,11 +54,14 @@ func TestCertCheckStatus(t *testing.T) { cert := &x509.Certificate{ CRLDistributionPoints: []string{"http://example.com"}, } - r := CertCheckStatus(context.Background(), cert, &x509.Certificate{}, CertCheckStatusOptions{ + r, err := CertCheckStatus(context.Background(), cert, &x509.Certificate{}, CertCheckStatusOptions{ HTTPClient: &http.Client{ Transport: expiredCRLRoundTripperMock{}, }, }) + if err != nil { + t.Fatal(err) + } if r.CRLResults[0].Error == nil { t.Fatal("expected error") } @@ -82,11 +88,14 @@ func TestCertCheckStatus(t *testing.T) { t.Fatal(err) } - r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ + r, err := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ HTTPClient: &http.Client{ Transport: expectedRoundTripperMock{Body: crlBytes}, }, }) + if err != nil { + t.Fatal(err) + } if r.Result != result.ResultRevoked { t.Fatalf("expected revoked, got %s", r.Result) } @@ -114,11 +123,14 @@ func TestCertCheckStatus(t *testing.T) { t.Fatal(err) } - r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ + r, err := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ HTTPClient: &http.Client{ Transport: expectedRoundTripperMock{Body: crlBytes}, }, }) + if err != nil { + t.Fatal(err) + } if r.CRLResults[0].Error == nil { t.Fatal("expected error") } @@ -133,11 +145,14 @@ func TestCertCheckStatus(t *testing.T) { t.Fatal(err) } - r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ + r, err := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ HTTPClient: &http.Client{ Transport: expectedRoundTripperMock{Body: crlBytes}, }, }) + if err != nil { + t.Fatal(err) + } if r.Result != result.ResultOK { t.Fatalf("expected OK, got %s", r.Result) } @@ -158,11 +173,14 @@ func TestCertCheckStatus(t *testing.T) { t.Fatal(err) } - r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ + r, err := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ HTTPClient: &http.Client{ Transport: expectedRoundTripperMock{Body: crlBytes}, }, }) + if err != nil { + t.Fatal(err) + } if !errors.Is(r.CRLResults[0].Error, ErrDeltaCRLNotSupported) { t.Fatal("expected ErrDeltaCRLNotChecked") } diff --git a/revocation/internal/ocsp/ocsp.go b/revocation/internal/ocsp/ocsp.go index 04ba0467..da929da2 100644 --- a/revocation/internal/ocsp/ocsp.go +++ b/revocation/internal/ocsp/ocsp.go @@ -52,13 +52,10 @@ const ( ) // CertCheckStatus checks the revocation status of a certificate using OCSP -func CertCheckStatus(cert, issuer *x509.Certificate, opts CertCheckStatusOptions) *result.CertRevocationResult { +func CertCheckStatus(cert, issuer *x509.Certificate, opts CertCheckStatusOptions) (*result.CertRevocationResult, error) { if !Supported(cert) { // OCSP not enabled for this certificate. - return &result.CertRevocationResult{ - Result: result.ResultNonRevokable, - ServerResults: []*result.ServerResult{toServerResult("", NoServerError{})}, - } + return nil, NoServerError{} } ocspURLs := cert.OCSPServer @@ -71,11 +68,11 @@ func CertCheckStatus(cert, issuer *x509.Certificate, opts CertCheckStatusOptions // A valid response has been received from an OCSP server // Result should be based on only this response, not any errors from // other servers - return serverResultsToCertRevocationResult([]*result.ServerResult{serverResult}) + return serverResultsToCertRevocationResult([]*result.ServerResult{serverResult}), nil } serverResults[serverIndex] = serverResult } - return serverResultsToCertRevocationResult(serverResults) + return serverResultsToCertRevocationResult(serverResults), nil } // Supported returns true if the certificate supports OCSP. diff --git a/revocation/internal/ocsp/ocsp_test.go b/revocation/internal/ocsp/ocsp_test.go index 54bc8c9a..d4c161fd 100644 --- a/revocation/internal/ocsp/ocsp_test.go +++ b/revocation/internal/ocsp/ocsp_test.go @@ -15,6 +15,7 @@ package ocsp import ( "crypto/x509" + "errors" "fmt" "net/http" "strings" @@ -83,7 +84,10 @@ func TestCheckStatus(t *testing.T) { HTTPClient: client, } - certResult := CertCheckStatus(revokableChain[0], revokableChain[1], opts) + certResult, err := CertCheckStatus(revokableChain[0], revokableChain[1], opts) + if err != nil { + t.Errorf("Expected no error, but got %v", err) + } expectedCertResults := []*result.CertRevocationResult{getOKCertResult(ocspServer)} validateEquivalentCertResults([]*result.CertRevocationResult{certResult}, expectedCertResults, t) }) @@ -94,7 +98,10 @@ func TestCheckStatus(t *testing.T) { HTTPClient: client, } - certResult := CertCheckStatus(revokableChain[0], revokableChain[1], opts) + certResult, err := CertCheckStatus(revokableChain[0], revokableChain[1], opts) + if err != nil { + t.Errorf("Expected no error, but got %v", err) + } expectedCertResults := []*result.CertRevocationResult{{ Result: result.ResultUnknown, ServerResults: []*result.ServerResult{ @@ -110,7 +117,10 @@ func TestCheckStatus(t *testing.T) { HTTPClient: client, } - certResult := CertCheckStatus(revokableChain[0], revokableChain[1], opts) + certResult, err := CertCheckStatus(revokableChain[0], revokableChain[1], opts) + if err != nil { + t.Errorf("Expected no error, but got %v", err) + } expectedCertResults := []*result.CertRevocationResult{{ Result: result.ResultRevoked, ServerResults: []*result.ServerResult{ @@ -127,19 +137,20 @@ func TestCheckStatus(t *testing.T) { HTTPClient: client, } - certResult := CertCheckStatus(revokableChain[0], revokableChain[1], opts) + certResult, err := CertCheckStatus(revokableChain[0], revokableChain[1], opts) + if err != nil { + t.Errorf("Expected no error, but got %v", err) + } expectedCertResults := []*result.CertRevocationResult{getOKCertResult(ocspServer)} validateEquivalentCertResults([]*result.CertRevocationResult{certResult}, expectedCertResults, t) }) t.Run("certificate doesn't support OCSP", func(t *testing.T) { - ocspResult := CertCheckStatus(&x509.Certificate{}, revokableIssuerTuple.Cert, CertCheckStatusOptions{}) - expectedResult := &result.CertRevocationResult{ - Result: result.ResultNonRevokable, - ServerResults: []*result.ServerResult{toServerResult("", NoServerError{})}, + _, err := CertCheckStatus(&x509.Certificate{}, revokableIssuerTuple.Cert, CertCheckStatusOptions{}) + if !errors.Is(err, NoServerError{}) { + t.Errorf("Expected error to be NoServerError, but got %v", err) } - validateEquivalentCertResults([]*result.CertRevocationResult{ocspResult}, []*result.CertRevocationResult{expectedResult}, t) }) } diff --git a/revocation/ocsp/ocsp.go b/revocation/ocsp/ocsp.go index c2274f75..f70ba2cf 100644 --- a/revocation/ocsp/ocsp.go +++ b/revocation/ocsp/ocsp.go @@ -62,12 +62,23 @@ func CheckStatus(opts Options) ([]*result.CertRevocationResult, error) { // Check status for each cert in cert chain var wg sync.WaitGroup for i, cert := range opts.CertChain[:len(opts.CertChain)-1] { - wg.Add(1) - // Assume cert chain is accurate and next cert in chain is the issuer - go func(i int, cert *x509.Certificate) { - defer wg.Done() - certResults[i] = ocsp.CertCheckStatus(cert, opts.CertChain[i+1], certCheckStatusOptions) - }(i, cert) + if ocsp.Supported(cert) { + wg.Add(1) + // Assume cert chain is accurate and next cert in chain is + // the issuer + go func(i int, cert *x509.Certificate) { + defer wg.Done() + // skip the error as it is not possible to get error + certResults[i], _ = ocsp.CertCheckStatus(cert, opts.CertChain[i+1], certCheckStatusOptions) + }(i, cert) + } else { + certResults[i] = &result.CertRevocationResult{ + Result: result.ResultNonRevokable, + ServerResults: []*result.ServerResult{{ + Result: result.ResultNonRevokable, + }}, + } + } } // Last is root cert, which will never be revoked by OCSP certResults[len(opts.CertChain)-1] = &result.CertRevocationResult{ diff --git a/revocation/result/results.go b/revocation/result/results.go index b5cfeb5e..319bf4de 100644 --- a/revocation/result/results.go +++ b/revocation/result/results.go @@ -159,9 +159,6 @@ type CertRevocationResult struct { // Thus, in the case of more than one ServerResult, this will be ResultUnknown Result Result - // CRLResults is the result of the CRL check for this certificate - CRLResults []*CRLResult - // An array of results for each server associated with the certificate. // The length will be either 1 or the number of OCSPServers for the cert. // @@ -172,4 +169,13 @@ type CertRevocationResult struct { // Otherwise, every server specified had some error that prevented the // status from being retrieved. These are all contained here for evaluation ServerResults []*ServerResult + + // CRLResults is the result of the CRL check for this certificate. Each + // element in the array corresponds to a different CRL distribution point + // for the certificate. + // + // If the length is 0, the CRL is not checked for this certificate. + // The length will be at most the number of CRLDistributionPoints for the + // certificate, which means all CRLs were checked. + CRLResults []*CRLResult } diff --git a/revocation/revocation.go b/revocation/revocation.go index 45fcacfb..2b51bdfa 100644 --- a/revocation/revocation.go +++ b/revocation/revocation.go @@ -197,12 +197,13 @@ func (r *revocation) ValidateContext(ctx context.Context, validateContextOpts Va } }() - ocspResult := ocsp.CertCheckStatus(cert, certChain[i+1], ocspOpts) + // skip the error as it will not happen + ocspResult, _ := ocsp.CertCheckStatus(cert, certChain[i+1], ocspOpts) if ocspResult != nil && ocspResult.Result == result.ResultUnknown && crl.Supported(cert) { // try CRL check if OCSP result is unknown - result := crl.CertCheckStatus(ctx, cert, certChain[i+1], crlOpts) + result, _ := crl.CertCheckStatus(ctx, cert, certChain[i+1], crlOpts) - // insert OCSP result into CRL result + // insert OCSP result into final result result.ServerResults = ocspResult.ServerResults certResults[i] = result } else { @@ -223,15 +224,15 @@ func (r *revocation) ValidateContext(ctx context.Context, validateContextOpts Va } }() - certResults[i] = crl.CertCheckStatus(ctx, cert, certChain[i+1], crlOpts) + certResults[i], _ = crl.CertCheckStatus(ctx, cert, certChain[i+1], crlOpts) }(i, cert) default: certResults[i] = &result.CertRevocationResult{ Result: result.ResultNonRevokable, - CRLResults: []*result.CRLResult{{ + ServerResults: []*result.ServerResult{{ Result: result.ResultNonRevokable, }}, - ServerResults: []*result.ServerResult{{ + CRLResults: []*result.CRLResult{{ Result: result.ResultNonRevokable, }}, } @@ -241,10 +242,10 @@ func (r *revocation) ValidateContext(ctx context.Context, validateContextOpts Va // Last is root cert, which will never be revoked by OCSP or CRL certResults[len(certChain)-1] = &result.CertRevocationResult{ Result: result.ResultNonRevokable, - CRLResults: []*result.CRLResult{{ + ServerResults: []*result.ServerResult{{ Result: result.ResultNonRevokable, }}, - ServerResults: []*result.ServerResult{{ + CRLResults: []*result.CRLResult{{ Result: result.ResultNonRevokable, }}, }