diff --git a/revocation/crl/bundle.go b/revocation/crl/bundle.go index 75eba8c..0c4e439 100644 --- a/revocation/crl/bundle.go +++ b/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 } diff --git a/revocation/crl/cache.go b/revocation/crl/cache.go index 8899ea2..45e6bd1 100644 --- a/revocation/crl/cache.go +++ b/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 } diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index 8efc5f8..8cbf11e 100644 --- a/revocation/crl/fetcher.go +++ b/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,14 +54,14 @@ 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 @@ -64,9 +69,9 @@ func NewHTTPFetcher(httpClient *http.Client) *HTTPFetcher { // 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 diff --git a/revocation/crl/fetcher_test.go b/revocation/crl/fetcher_test.go index e098802..6cecf62 100644 --- a/revocation/crl/fetcher_test.go +++ b/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,25 +80,32 @@ 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) } }) @@ -99,14 +113,17 @@ func TestFetch(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) } }) @@ -114,8 +131,11 @@ func TestFetch(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") diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index de5362a..2a945b5 100644 --- a/revocation/internal/crl/crl.go +++ b/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 diff --git a/revocation/internal/crl/crl_test.go b/revocation/internal/crl/crl_test.go index 65aa641..5c13760 100644 --- a/revocation/internal/crl/crl_test.go +++ b/revocation/internal/crl/crl_test.go @@ -20,7 +20,6 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/asn1" - "errors" "fmt" "io" "math/big" @@ -60,9 +59,12 @@ func TestCertCheckStatus(t *testing.T) { cert := &x509.Certificate{ CRLDistributionPoints: []string{"http://example.com"}, } - fetcher := crlutils.NewHTTPFetcher( + fetcher, err := crlutils.NewHTTPFetcher( &http.Client{Transport: errorRoundTripperMock{}}, ) + if err != nil { + t.Fatal(err) + } fetcher.Cache = memoryCache r := CertCheckStatus(context.Background(), cert, &x509.Certificate{}, CertCheckStatusOptions{ @@ -80,9 +82,12 @@ func TestCertCheckStatus(t *testing.T) { cert := &x509.Certificate{ CRLDistributionPoints: []string{"http://example.com"}, } - fetcher := crlutils.NewHTTPFetcher( + fetcher, err := crlutils.NewHTTPFetcher( &http.Client{Transport: expiredCRLRoundTripperMock{}}, ) + if err != nil { + t.Fatal(err) + } fetcher.Cache = memoryCache r := CertCheckStatus(context.Background(), cert, &x509.Certificate{}, CertCheckStatusOptions{ @@ -115,9 +120,12 @@ func TestCertCheckStatus(t *testing.T) { t.Fatal(err) } - fetcher := crlutils.NewHTTPFetcher( + fetcher, err := crlutils.NewHTTPFetcher( &http.Client{Transport: expectedRoundTripperMock{Body: crlBytes}}, ) + if err != nil { + t.Fatal(err) + } fetcher.Cache = memoryCache r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ Fetcher: fetcher, @@ -150,9 +158,13 @@ func TestCertCheckStatus(t *testing.T) { t.Fatal(err) } - fetcher := crlutils.NewHTTPFetcher( + fetcher, err := crlutils.NewHTTPFetcher( &http.Client{Transport: expectedRoundTripperMock{Body: crlBytes}}, ) + if err != nil { + t.Fatal(err) + } + fetcher.Cache = memoryCache r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ Fetcher: fetcher, @@ -173,9 +185,12 @@ func TestCertCheckStatus(t *testing.T) { t.Fatal(err) } - fetcher := crlutils.NewHTTPFetcher( + fetcher, err := crlutils.NewHTTPFetcher( &http.Client{Transport: expectedRoundTripperMock{Body: crlBytes}}, ) + if err != nil { + t.Fatal(err) + } fetcher.Cache = memoryCache r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ Fetcher: fetcher, @@ -201,15 +216,18 @@ func TestCertCheckStatus(t *testing.T) { if err != nil { t.Fatal(err) } - fetcher := crlutils.NewHTTPFetcher( + fetcher, err := crlutils.NewHTTPFetcher( &http.Client{Transport: expectedRoundTripperMock{Body: crlBytes}}, ) + if err != nil { + t.Fatal(err) + } fetcher.Cache = memoryCache r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ Fetcher: fetcher, }) - if !errors.Is(r.ServerResults[0].Error, ErrDeltaCRLNotSupported) { - t.Fatal("expected ErrDeltaCRLNotChecked") + if !strings.Contains(r.ServerResults[0].Error.Error(), "delta CRL is not supported") { + t.Fatalf("unexpected error, got %v, expected %v", r.ServerResults[0].Error, "delta CRL is not supported") } }) @@ -239,9 +257,12 @@ func TestCertCheckStatus(t *testing.T) { t.Fatal(err) } - fetcher := crlutils.NewHTTPFetcher( + fetcher, err := crlutils.NewHTTPFetcher( &http.Client{Transport: errorRoundTripperMock{}}, ) + if err != nil { + t.Fatal(err) + } fetcher.Cache = memoryCache r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ Fetcher: fetcher, @@ -257,9 +278,12 @@ func TestCertCheckStatus(t *testing.T) { t.Fatal(err) } - fetcher := crlutils.NewHTTPFetcher( + fetcher, err := crlutils.NewHTTPFetcher( &http.Client{Transport: expectedRoundTripperMock{Body: crlBytes}}, ) + if err != nil { + t.Fatal(err) + } fetcher.Cache = memoryCache r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ Fetcher: fetcher, @@ -283,9 +307,12 @@ func TestCertCheckStatus(t *testing.T) { t.Fatal(err) } - fetcher := crlutils.NewHTTPFetcher( + fetcher, err := crlutils.NewHTTPFetcher( &http.Client{Transport: expectedRoundTripperMock{Body: crlBytes}}, ) + if err != nil { + t.Fatal(err) + } fetcher.Cache = memoryCache r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ Fetcher: fetcher, diff --git a/revocation/internal/crl/errors.go b/revocation/internal/crl/errors.go deleted file mode 100644 index 3786655..0000000 --- a/revocation/internal/crl/errors.go +++ /dev/null @@ -1,22 +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 - -import "errors" - -var ( - // ErrDeltaCRLNotSupported is returned when the CRL contains a delta CRL but - // the delta CRL is not supported. - ErrDeltaCRLNotSupported = errors.New("delta CRL is not supported") -) diff --git a/revocation/revocation.go b/revocation/revocation.go index bd50b0b..9c78699 100644 --- a/revocation/revocation.go +++ b/revocation/revocation.go @@ -24,7 +24,7 @@ import ( "sync" "time" - crlutils "github.com/notaryproject/notation-core-go/revocation/crl" + crlutil "github.com/notaryproject/notation-core-go/revocation/crl" "github.com/notaryproject/notation-core-go/revocation/internal/crl" "github.com/notaryproject/notation-core-go/revocation/internal/ocsp" "github.com/notaryproject/notation-core-go/revocation/internal/x509util" @@ -70,9 +70,8 @@ type Validator interface { // revocation is an internal struct used for revocation checking type revocation struct { ocspHTTPClient *http.Client - crlHTTPClient *http.Client certChainPurpose purpose.Purpose - crlCache crlutils.Cache + crlFetcher crlutil.Fetcher } // New constructs a revocation object for code signing certificate chain. @@ -83,13 +82,15 @@ func New(httpClient *http.Client) (Revocation, error) { if httpClient == nil { return nil, errors.New("invalid input: a non-nil httpClient must be specified") } + fetcher, err := crlutil.NewHTTPFetcher(httpClient) + if err != nil { + return nil, err + } return &revocation{ ocspHTTPClient: httpClient, - crlHTTPClient: httpClient, certChainPurpose: purpose.CodeSigning, - // no cache by default - crlCache: nil, + crlFetcher: fetcher, }, nil } @@ -105,14 +106,14 @@ type Options struct { // OPTIONAL. CRLHTTPClient *http.Client + // CRLCache is the cache client used to store the CRL. if not provided, + // no cache will be used. + CRLCache crlutil.Cache + // CertChainPurpose is the purpose of the certificate chain. Supported // values are CodeSigning and Timestamping. Default value is CodeSigning. // OPTIONAL. CertChainPurpose purpose.Purpose - - // CRLCache is the cache client used to store the CRL. if not provided, - // no cache will be used. - CRLCache crlutils.Cache } // NewWithOptions constructs a Validator with the specified options @@ -131,11 +132,16 @@ func NewWithOptions(opts Options) (Validator, error) { return nil, fmt.Errorf("unsupported certificate chain purpose %v", opts.CertChainPurpose) } + fetcher, err := crlutil.NewHTTPFetcher(opts.CRLHTTPClient) + if err != nil { + return nil, err + } + fetcher.Cache = opts.CRLCache + return &revocation{ ocspHTTPClient: opts.OCSPHTTPClient, - crlHTTPClient: opts.CRLHTTPClient, certChainPurpose: opts.CertChainPurpose, - crlCache: opts.CRLCache, + crlFetcher: fetcher, }, nil } @@ -181,10 +187,8 @@ func (r *revocation) ValidateContext(ctx context.Context, validateContextOpts Va SigningTime: validateContextOpts.AuthenticSigningTime, } - fetcher := crlutils.NewHTTPFetcher(r.crlHTTPClient) - fetcher.Cache = r.crlCache crlOpts := crl.CertCheckStatusOptions{ - Fetcher: fetcher, + Fetcher: r.crlFetcher, SigningTime: validateContextOpts.AuthenticSigningTime, }