fix: resolve comments

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

revocation/crl/bundle.go

@@ -13,14 +13,16 @@
 
 package crl
 
-import (
-	"crypto/x509"
-)
+import "crypto/x509"
 
-// Bundle is in memory representation of the Bundle tarball, including base CRL.
-//
-// TODO: consider adding DeltaCRL field in the future
+// Bundle is a collection of CRLs, including base and delta CRLs
 type Bundle struct {
 	// BaseCRL is the parsed base CRL
 	BaseCRL *x509.RevocationList
+
+	// DeltaCRL is the parsed delta CRL
+	//
+	// TODO: support delta CRL
+	// It will always be nil until we support delta CRL
+	DeltaCRL *x509.RevocationList
 }

revocation/crl/cache.go

@@ -13,19 +13,17 @@
 
 package crl
 
-import (
-	"context"
-)
+import "context"
 
 // Cache is an interface that specifies methods used for caching
 type Cache interface {
-	// Get retrieves the CRL bundle with the given uri
+	// Get retrieves the CRL bundle with the given url
 	//
-	// uri is the URI of the CRL
+	// url is the URI of the CRL
 	//
 	// if the key does not exist or the content is expired, return ErrCacheMiss.
 	Get(ctx context.Context, url string) (*Bundle, error)
 
-	// Set stores the CRL bundle with the given uri
+	// Set stores the CRL bundle with the given url
 	Set(ctx context.Context, url string, bundle *Bundle) error
 }

revocation/crl/fetcher.go

@@ -18,6 +18,7 @@ package crl
 import (
 	"context"
 	"crypto/x509"
+	"encoding/asn1"
 	"errors"
 	"fmt"
 	"io"
@@ -26,17 +27,21 @@ import (
 	"time"
 )
 
-// MaxCRLSize is the maximum size of CRL in bytes
+// oidFreshestCRL is the object identifier for the distribution point
+// for the delta CRL. (See RFC 5280, Section 5.2.6)
+var oidFreshestCRL = asn1.ObjectIdentifier{2, 5, 29, 46}
+
+// maxCRLSize is the maximum size of CRL in bytes
 //
 // The 32 MiB limit is based on investigation that even the largest CRLs
 // are less than 16 MiB. The limit is set to 32 MiB to prevent
-const MaxCRLSize = 32 * 1024 * 1024 // 32 MiB
+const maxCRLSize = 32 * 1024 * 1024 // 32 MiB
 
 // Fetcher is an interface that specifies methods used for fetching CRL
 // from the given URL
 type Fetcher interface {
 	// Fetch retrieves the CRL from the given URL.
-	Fetch(ctx context.Context, url string) (base, delta *x509.RevocationList, err error)
+	Fetch(ctx context.Context, url string) (bundle *Bundle, err error)
 }
 
 // HTTPFetcher is a Fetcher implementation that fetches CRL from the given URL
@@ -49,24 +54,24 @@ type HTTPFetcher struct {
 }
 
 // NewHTTPFetcher creates a new HTTPFetcher with the given HTTP client
-func NewHTTPFetcher(httpClient *http.Client) *HTTPFetcher {
+func NewHTTPFetcher(httpClient *http.Client) (*HTTPFetcher, error) {
 	if httpClient == nil {
-		httpClient = http.DefaultClient
+		return nil, errors.New("httpClient is nil")
 	}
 
 	return &HTTPFetcher{
 		httpClient: httpClient,
-	}
+	}, nil
 }
 
 // Fetch retrieves the CRL from the given URL
 //
 // It try to get the CRL from the cache first, if the cache is not nil or have
 // an error (e.g. cache miss), it will download the CRL from the URL, then
 // store it to the cache if the cache is not nil.
-func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (base, delta *x509.RevocationList, err error) {
+func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (bundle *Bundle, err error) {
 	if url == "" {
-		return nil, nil, errors.New("CRL URL is empty")
+		return nil, errors.New("CRL URL is empty")
 	}
 
 	if f.Cache == nil {
@@ -75,7 +80,7 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (base, delta *x509.
 	}
 
 	// try to get from cache
-	bundle, err := f.Cache.Get(ctx, url)
+	bundle, err = f.Cache.Get(ctx, url)
 	if err != nil {
 		return f.download(ctx, url)
 	}
@@ -86,29 +91,37 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (base, delta *x509.
 		return f.download(ctx, url)
 	}
 
-	return bundle.BaseCRL, nil, nil
+	return bundle, nil
 }
 
 // Download downloads the CRL from the given URL and saves it to the
 // cache
-func (f *HTTPFetcher) download(ctx context.Context, url string) (base, delta *x509.RevocationList, err error) {
-	base, err = download(ctx, url, f.httpClient)
+func (f *HTTPFetcher) download(ctx context.Context, url string) (bundle *Bundle, err error) {
+	base, err := download(ctx, url, f.httpClient)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
+	}
+	// check deltaCRL
+	for _, ext := range base.Extensions {
+		if ext.Id.Equal(oidFreshestCRL) {
+			// TODO: support delta CRL
+			return nil, errors.New("delta CRL is not supported")
+		}
+	}
+
+	bundle = &Bundle{
+		BaseCRL: base,
 	}
 
 	if f.Cache == nil {
 		// no cache, return directly
-		return base, delta, nil
+		return bundle, nil
 	}
 
-	bundle := &Bundle{
-		BaseCRL: base,
-	}
 	// ignore the error, as the cache is not critical
 	_ = f.Cache.Set(ctx, url, bundle)
 
-	return base, delta, nil
+	return bundle, nil
 }
 
 func download(ctx context.Context, baseURL string, client *http.Client) (*x509.RevocationList, error) {
@@ -135,12 +148,12 @@ func download(ctx context.Context, baseURL string, client *http.Client) (*x509.R
 		return nil, fmt.Errorf("failed to download with status code: %d", resp.StatusCode)
 	}
 	// read with size limit
-	data, err := io.ReadAll(io.LimitReader(resp.Body, MaxCRLSize))
+	data, err := io.ReadAll(io.LimitReader(resp.Body, maxCRLSize))
 	if err != nil {
 		return nil, fmt.Errorf("failed to read CRL response: %w", err)
 	}
-	if len(data) == MaxCRLSize {
-		return nil, fmt.Errorf("CRL size exceeds the limit: %d", MaxCRLSize)
+	if len(data) == maxCRLSize {
+		return nil, fmt.Errorf("CRL size exceeds the limit: %d", maxCRLSize)
 	}
 
 	// parse CRL

revocation/crl/fetcher_test.go

@@ -30,7 +30,10 @@ import (
 
 func TestNewHTTPFetcher(t *testing.T) {
 	t.Run("httpClient is nil", func(t *testing.T) {
-		_ = NewHTTPFetcher(nil)
+		_, err := NewHTTPFetcher(nil)
+		if err.Error() != "httpClient is nil" {
+			t.Errorf("NewHTTPFetcher() error = %v, want %v", err, "httpClient is nil")
+		}
 	})
 }
 
@@ -61,9 +64,13 @@ func TestFetch(t *testing.T) {
 	}
 
 	t.Run("url is empty", func(t *testing.T) {
-		f := NewHTTPFetcher(nil)
+		httpClient := &http.Client{}
+		f, err := NewHTTPFetcher(httpClient)
+		if err != nil {
+			t.Errorf("NewHTTPFetcher() error = %v, want nil", err)
+		}
 		f.Cache = c
-		_, _, err = f.Fetch(context.Background(), "")
+		_, err = f.Fetch(context.Background(), "")
 		if err == nil {
 			t.Errorf("Fetcher.Fetch() error = nil, want not nil")
 		}
@@ -73,49 +80,62 @@ func TestFetch(t *testing.T) {
 		httpClient := &http.Client{
 			Transport: expectedRoundTripperMock{Body: baseCRL.Raw},
 		}
-		f := NewHTTPFetcher(httpClient)
-		base, _, err := f.Fetch(context.Background(), exampleURL)
+		f, err := NewHTTPFetcher(httpClient)
+		if err != nil {
+			t.Errorf("NewHTTPFetcher() error = %v, want nil", err)
+		}
+		bundle, err := f.Fetch(context.Background(), exampleURL)
 		if err != nil {
 			t.Errorf("Fetcher.Fetch() error = %v, want nil", err)
 		}
-		if !bytes.Equal(base.Raw, baseCRL.Raw) {
-			t.Errorf("Fetcher.Fetch() base.Raw = %v, want %v", base.Raw, baseCRL.Raw)
+		if !bytes.Equal(bundle.BaseCRL.Raw, baseCRL.Raw) {
+			t.Errorf("Fetcher.Fetch() base.Raw = %v, want %v", bundle.BaseCRL.Raw, baseCRL.Raw)
 		}
 	})
 
 	t.Run("cache hit", func(t *testing.T) {
-		f := NewHTTPFetcher(nil)
+		httpClient := &http.Client{}
+		f, err := NewHTTPFetcher(httpClient)
+		if err != nil {
+			t.Errorf("NewHTTPFetcher() error = %v, want nil", err)
+		}
 		f.Cache = c
-		base, _, err := f.Fetch(context.Background(), exampleURL)
+		bundle, err := f.Fetch(context.Background(), exampleURL)
 		if err != nil {
 			t.Errorf("Fetcher.Fetch() error = %v, want nil", err)
 		}
-		if !bytes.Equal(base.Raw, baseCRL.Raw) {
-			t.Errorf("Fetcher.Fetch() base.Raw = %v, want %v", base.Raw, baseCRL.Raw)
+		if !bytes.Equal(bundle.BaseCRL.Raw, baseCRL.Raw) {
+			t.Errorf("Fetcher.Fetch() base.Raw = %v, want %v", bundle.BaseCRL.Raw, baseCRL.Raw)
 		}
 	})
 
 	t.Run("cache miss", func(t *testing.T) {
 		httpClient := &http.Client{
 			Transport: expectedRoundTripperMock{Body: baseCRL.Raw},
 		}
-		f := NewHTTPFetcher(httpClient)
+		f, err := NewHTTPFetcher(httpClient)
+		if err != nil {
+			t.Errorf("NewHTTPFetcher() error = %v, want nil", err)
+		}
 		f.Cache = c
-		base, _, err := f.Fetch(context.Background(), uncachedURL)
+		bundle, err := f.Fetch(context.Background(), uncachedURL)
 		if err != nil {
 			t.Errorf("Fetcher.Fetch() error = %v, want nil", err)
 		}
-		if !bytes.Equal(base.Raw, baseCRL.Raw) {
-			t.Errorf("Fetcher.Fetch() base.Raw = %v, want %v", base.Raw, baseCRL.Raw)
+		if !bytes.Equal(bundle.BaseCRL.Raw, baseCRL.Raw) {
+			t.Errorf("Fetcher.Fetch() base.Raw = %v, want %v", bundle.BaseCRL.Raw, baseCRL.Raw)
 		}
 	})
 
 	t.Run("cache miss and download failed error", func(t *testing.T) {
 		httpClient := &http.Client{
 			Transport: errorRoundTripperMock{},
 		}
-		f := NewHTTPFetcher(httpClient)
-		_, _, err = f.Fetch(context.Background(), uncachedURL)
+		f, err := NewHTTPFetcher(httpClient)
+		if err != nil {
+			t.Errorf("NewHTTPFetcher() error = %v, want nil", err)
+		}
+		_, err = f.Fetch(context.Background(), uncachedURL)
 		if err == nil {
 			t.Errorf("Fetcher.Fetch() error = nil, want not nil")
 		}
@@ -174,7 +194,7 @@ func TestDownload(t *testing.T) {
 
 	t.Run("exceed the size limit", func(t *testing.T) {
 		_, err := download(context.Background(), "http://example.com", &http.Client{
-			Transport: expectedRoundTripperMock{Body: make([]byte, MaxCRLSize+1)},
+			Transport: expectedRoundTripperMock{Body: make([]byte, maxCRLSize+1)},
 		})
 		if err == nil {
 			t.Fatal("expected error")

revocation/internal/crl/crl.go

@@ -100,18 +100,18 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C
 	// not a performance issue.
 	for _, crlURL = range cert.CRLDistributionPoints {
 		// ignore delta CRL as it is not implemented
-		base, _, err := opts.Fetcher.Fetch(ctx, crlURL)
+		bundle, err := opts.Fetcher.Fetch(ctx, crlURL)
 		if err != nil {
 			lastErr = fmt.Errorf("failed to download CRL from %s: %w", crlURL, err)
 			break
 		}
 
-		if err = validate(base, issuer); err != nil {
+		if err = validate(bundle.BaseCRL, issuer); err != nil {
 			lastErr = fmt.Errorf("failed to validate CRL from %s: %w", crlURL, err)
 			break
 		}
 
-		crlResult, err := checkRevocation(cert, base, opts.SigningTime, crlURL)
+		crlResult, err := checkRevocation(cert, bundle.BaseCRL, opts.SigningTime, crlURL)
 		if err != nil {
 			lastErr = fmt.Errorf("failed to check revocation status from %s: %w", crlURL, err)
 			break
@@ -168,7 +168,7 @@ func validate(crl *x509.RevocationList, issuer *x509.Certificate) error {
 	for _, ext := range crl.Extensions {
 		switch {
 		case ext.Id.Equal(oidFreshestCRL):
-			return ErrDeltaCRLNotSupported
+			return errors.New("delta CRL is not supported")
 		case ext.Id.Equal(oidIssuingDistributionPoint):
 			// IssuingDistributionPoint is a critical extension that identifies
 			// the scope of the CRL. Since we will check all the CRL