fix: update

Signed-off-by: Junjie Gao <[email protected]>

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.

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")
 		}

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.

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)
 	})
 }
 

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{

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
 }

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,
 		}},
 	}