From e34370f479a6b5f654d9c64718c47b501e9a7c45 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 26 Nov 2024 08:05:51 +0000 Subject: [PATCH 01/25] feat: delta CRL Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 124 +++++++++++++++++++++++--- revocation/crl/fetcher_test.go | 34 -------- revocation/internal/crl/const.go | 19 ++++ revocation/internal/crl/crl.go | 131 +++++++++++++++++++++------- revocation/internal/crl/crl_test.go | 77 ++-------------- 5 files changed, 240 insertions(+), 145 deletions(-) create mode 100644 revocation/internal/crl/const.go diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index f0baacc4..372cd6d4 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -18,6 +18,7 @@ package crl import ( "context" "crypto/x509" + "crypto/x509/pkix" "encoding/asn1" "errors" "fmt" @@ -25,6 +26,9 @@ import ( "net/http" "net/url" "time" + + "golang.org/x/crypto/cryptobyte" + cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1" ) // oidFreshestCRL is the object identifier for the distribution point @@ -44,6 +48,15 @@ type Fetcher interface { Fetch(ctx context.Context, url string) (*Bundle, error) } +// FetcherWithCertificateExtensions is an interface that specifies methods used +// for fetching CRL from the given URL with certificate extensions to identify +// the Freshest CRL extension (Delta CRL) from certificate. +type FetcherWithCertificateExtensions interface { + // FetchWithCertificateExtensions retrieves the CRL from the given URL with + // certificate extensions. + FetchWithCertificateExtensions(ctx context.Context, url string, certExtension *[]pkix.Extension) (*Bundle, error) +} + // HTTPFetcher is a Fetcher implementation that fetches CRL from the given URL type HTTPFetcher struct { // Cache stores fetched CRLs and reuses them until the CRL reaches the @@ -77,6 +90,10 @@ func NewHTTPFetcher(httpClient *http.Client) (*HTTPFetcher, error) { // (e.g. cache miss), it will download the CRL from the URL and store it to the // cache. func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*Bundle, error) { + return f.FetchWithCertificateExtensions(ctx, url, nil) +} + +func (f *HTTPFetcher) FetchWithCertificateExtensions(ctx context.Context, url string, certExtension *[]pkix.Extension) (*Bundle, error) { if url == "" { return nil, errors.New("CRL URL cannot be empty") } @@ -84,9 +101,9 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*Bundle, error) { if f.Cache != nil { bundle, err := f.Cache.Get(ctx, url) if err == nil { - // check expiry - nextUpdate := bundle.BaseCRL.NextUpdate - if !nextUpdate.IsZero() && !time.Now().After(nextUpdate) { + // check expiry of base CRL and delta CRL + if isEffective(bundle.BaseCRL.NextUpdate) && + (bundle.DeltaCRL == nil || isEffective(bundle.DeltaCRL.NextUpdate)) { return bundle, nil } } else if !errors.Is(err, ErrCacheMiss) && !f.DiscardCacheError { @@ -94,7 +111,7 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*Bundle, error) { } } - bundle, err := f.fetch(ctx, url) + bundle, err := f.fetch(ctx, url, certExtension) if err != nil { return nil, fmt.Errorf("failed to retrieve CRL: %w", err) } @@ -109,27 +126,112 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*Bundle, error) { return bundle, nil } +func isEffective(nextUpdate time.Time) bool { + return !nextUpdate.IsZero() && !time.Now().After(nextUpdate) +} + // fetch downloads the CRL from the given URL. -func (f *HTTPFetcher) fetch(ctx context.Context, url string) (*Bundle, error) { +func (f *HTTPFetcher) fetch(ctx context.Context, url string, certificateExtension *[]pkix.Extension) (*Bundle, error) { // fetch base CRL base, err := fetchCRL(ctx, url, f.httpClient) if err != nil { return nil, err } - // check delta CRL - // TODO: support delta CRL https://github.com/notaryproject/notation-core-go/issues/228 - for _, ext := range base.Extensions { - if ext.Id.Equal(oidFreshestCRL) { - return nil, errors.New("delta CRL is not supported") + // fetch delta CRL from CRL extension + deltaCRL, err := f.processDeltaCRL(&base.Extensions) + if err != nil { + return nil, err + } + if certificateExtension != nil && deltaCRL == nil { + // fallback to fetch delta CRL from certificate extension + deltaCRL, err = f.processDeltaCRL(certificateExtension) + if err != nil { + return nil, err } } return &Bundle{ - BaseCRL: base, + BaseCRL: base, + DeltaCRL: deltaCRL, }, nil } +func (f *HTTPFetcher) processDeltaCRL(extensions *[]pkix.Extension) (*x509.RevocationList, error) { + for _, ext := range *extensions { + if ext.Id.Equal(oidFreshestCRL) { + cdp, err := parseFreshestCRL(ext) + if err != nil { + return nil, err + } + if len(cdp) == 0 { + return nil, nil + } + if len(cdp) > 1 { + // to simplify the implementation, we only support one + // Freshest CRL URL for now which is the common case + return nil, errors.New("multiple Freshest CRL distribution points are not supported") + } + return fetchCRL(context.Background(), cdp[0], f.httpClient) + } + } + return nil, nil +} + +// parseFreshestCRL parses the Freshest CRL extension and returns the CRL URLs +func parseFreshestCRL(ext pkix.Extension) ([]string, error) { + var cdp []string + // RFC 5280, 4.2.1.15 + // id-ce-freshestCRL OBJECT IDENTIFIER ::= { id-ce 46 } + // + // FreshestCRL ::= CRLDistributionPoints + // + // RFC 5280, 4.2.1.13 + // + // CRLDistributionPoints ::= SEQUENCE SIZE (1..MAX) OF DistributionPoint + // + // DistributionPoint ::= SEQUENCE { + // distributionPoint [0] DistributionPointName OPTIONAL, + // reasons [1] ReasonFlags OPTIONAL, + // cRLIssuer [2] GeneralNames OPTIONAL } + // + // DistributionPointName ::= CHOICE { + // fullName [0] GeneralNames, + // nameRelativeToCRLIssuer [1] RelativeDistinguishedName } + val := cryptobyte.String(ext.Value) + if !val.ReadASN1(&val, cryptobyte_asn1.SEQUENCE) { + return nil, errors.New("x509: invalid CRL distribution points") + } + for !val.Empty() { + var dpDER cryptobyte.String + if !val.ReadASN1(&dpDER, cryptobyte_asn1.SEQUENCE) { + return nil, errors.New("x509: invalid CRL distribution point") + } + var dpNameDER cryptobyte.String + var dpNamePresent bool + if !dpDER.ReadOptionalASN1(&dpNameDER, &dpNamePresent, cryptobyte_asn1.Tag(0).Constructed().ContextSpecific()) { + return nil, errors.New("x509: invalid CRL distribution point") + } + if !dpNamePresent { + continue + } + if !dpNameDER.ReadASN1(&dpNameDER, cryptobyte_asn1.Tag(0).Constructed().ContextSpecific()) { + return nil, errors.New("x509: invalid CRL distribution point") + } + for !dpNameDER.Empty() { + if !dpNameDER.PeekASN1Tag(cryptobyte_asn1.Tag(6).ContextSpecific()) { + break + } + var uri cryptobyte.String + if !dpNameDER.ReadASN1(&uri, cryptobyte_asn1.Tag(6).ContextSpecific()) { + return nil, errors.New("x509: invalid CRL distribution point") + } + cdp = append(cdp, string(uri)) + } + } + return cdp, nil +} + func fetchCRL(ctx context.Context, crlURL string, client *http.Client) (*x509.RevocationList, error) { // validate URL parsedURL, err := url.Parse(crlURL) diff --git a/revocation/crl/fetcher_test.go b/revocation/crl/fetcher_test.go index 634fbc3a..b289cc7a 100644 --- a/revocation/crl/fetcher_test.go +++ b/revocation/crl/fetcher_test.go @@ -18,7 +18,6 @@ import ( "context" "crypto/rand" "crypto/x509" - "crypto/x509/pkix" "errors" "fmt" "io" @@ -195,39 +194,6 @@ func TestFetch(t *testing.T) { } }) - t.Run("delta CRL is not supported", func(t *testing.T) { - c := &memoryCache{} - // prepare a CRL with refresh CRL extension - certChain := testhelper.GetRevokableRSAChainWithRevocations(2, false, true) - expiredCRLBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ - Number: big.NewInt(1), - NextUpdate: time.Now().Add(-1 * time.Hour), - ExtraExtensions: []pkix.Extension{ - { - Id: oidFreshestCRL, - Value: []byte{0x01, 0x02, 0x03}, - }, - }, - }, certChain[1].Cert, certChain[1].PrivateKey) - if err != nil { - t.Fatalf("failed to create base CRL: %v", err) - } - - httpClient := &http.Client{ - Transport: expectedRoundTripperMock{Body: expiredCRLBytes}, - } - f, err := NewHTTPFetcher(httpClient) - if err != nil { - t.Errorf("NewHTTPFetcher() error = %v, want nil", err) - } - f.Cache = c - f.DiscardCacheError = true - _, err = f.Fetch(context.Background(), uncachedURL) - if !strings.Contains(err.Error(), "delta CRL is not supported") { - t.Errorf("Fetcher.Fetch() error = %v, want delta CRL is not supported", err) - } - }) - t.Run("Set cache error", func(t *testing.T) { c := &errorCache{ GetError: ErrCacheMiss, diff --git a/revocation/internal/crl/const.go b/revocation/internal/crl/const.go new file mode 100644 index 00000000..4df28629 --- /dev/null +++ b/revocation/internal/crl/const.go @@ -0,0 +1,19 @@ +package crl + +const ( + // RFC 5280, 5.3.1 + // CRLReason ::= ENUMERATED { + // unspecified (0), + // keyCompromise (1), + // cACompromise (2), + // affiliationChanged (3), + // superseded (4), + // cessationOfOperation (5), + // certificateHold (6), + // -- value 7 is not used + // removeFromCRL (8), + // privilegeWithdrawn (9), + // aACompromise (10) } + reasonCodeCertificateHold = 6 + reasonCodeRemoveFromCRL = 8 +) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 50f2c085..56eb391e 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -18,6 +18,7 @@ package crl import ( "context" "crypto/x509" + "crypto/x509/pkix" "encoding/asn1" "errors" "fmt" @@ -86,9 +87,10 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C } var ( - serverResults = make([]*result.ServerResult, 0, len(cert.CRLDistributionPoints)) - lastErr error - crlURL string + serverResults = make([]*result.ServerResult, 0, len(cert.CRLDistributionPoints)) + lastErr error + crlURL string + hasFreshestCRL = hasFreshestCRL(&cert.Extensions) ) // The CRLDistributionPoints contains the URIs of all the CRL distribution @@ -99,18 +101,41 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C // point with one CRL URI, which will be cached, so checking all the URIs is // not a performance issue. for _, crlURL = range cert.CRLDistributionPoints { - bundle, err := opts.Fetcher.Fetch(ctx, crlURL) - if err != nil { - lastErr = fmt.Errorf("failed to download CRL from %s: %w", crlURL, err) - break + var ( + bundle *crl.Bundle + err error + ) + if !hasFreshestCRL { + bundle, err = opts.Fetcher.Fetch(ctx, crlURL) + if err != nil { + lastErr = fmt.Errorf("failed to download CRL from %s: %w", crlURL, err) + break + } + } else { + fetcher, ok := opts.Fetcher.(crl.FetcherWithCertificateExtensions) + if !ok { + lastErr = fmt.Errorf("fetcher does not support fetching delta CRL from certificate extension") + break + } + bundle, err = fetcher.FetchWithCertificateExtensions(ctx, crlURL, &cert.Extensions) + if err != nil { + lastErr = fmt.Errorf("failed to download CRL from %s: %w", crlURL, err) + break + } } if err = validate(bundle.BaseCRL, issuer); err != nil { lastErr = fmt.Errorf("failed to validate CRL from %s: %w", crlURL, err) break } + if bundle.DeltaCRL != nil { + if err = validate(bundle.DeltaCRL, issuer); err != nil { + lastErr = fmt.Errorf("failed to validate delta CRL from %s: %w", crlURL, err) + break + } + } - crlResult, err := checkRevocation(cert, bundle.BaseCRL, opts.SigningTime, crlURL) + crlResult, err := checkRevocation(cert, bundle, opts.SigningTime, crlURL) if err != nil { lastErr = fmt.Errorf("failed to check revocation status from %s: %w", crlURL, err) break @@ -153,6 +178,15 @@ func Supported(cert *x509.Certificate) bool { return cert != nil && len(cert.CRLDistributionPoints) > 0 } +func hasFreshestCRL(extensions *[]pkix.Extension) bool { + for _, ext := range *extensions { + if ext.Id.Equal(oidFreshestCRL) { + return true + } + } + return false +} + func validate(crl *x509.RevocationList, issuer *x509.Certificate) error { // check signature if err := crl.CheckSignatureFrom(issuer); err != nil { @@ -170,8 +204,6 @@ func validate(crl *x509.RevocationList, issuer *x509.Certificate) error { for _, ext := range crl.Extensions { switch { - case ext.Id.Equal(oidFreshestCRL): - 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 @@ -188,38 +220,75 @@ func validate(crl *x509.RevocationList, issuer *x509.Certificate) error { } // checkRevocation checks if the certificate is revoked or not -func checkRevocation(cert *x509.Certificate, baseCRL *x509.RevocationList, signingTime time.Time, crlURL string) (*result.ServerResult, error) { +func checkRevocation(cert *x509.Certificate, bundle *crl.Bundle, signingTime time.Time, crlURL string) (*result.ServerResult, error) { if cert == nil { return nil, errors.New("certificate cannot be nil") } - + if bundle == nil { + return nil, errors.New("CRL bundle cannot be nil") + } + baseCRL := bundle.BaseCRL if baseCRL == nil { return nil, errors.New("baseCRL cannot be nil") } - for _, revocationEntry := range baseCRL.RevokedCertificateEntries { - if revocationEntry.SerialNumber.Cmp(cert.SerialNumber) == 0 { - extensions, err := parseEntryExtensions(revocationEntry) - if err != nil { - return nil, err - } + deltaCRL := bundle.DeltaCRL + entriesArray := []*[]x509.RevocationListEntry{&baseCRL.RevokedCertificateEntries} + if deltaCRL != nil { + entriesArray = append(entriesArray, &deltaCRL.RevokedCertificateEntries) + } - // validate signingTime and invalidityDate - if !signingTime.IsZero() && !extensions.invalidityDate.IsZero() && - signingTime.Before(extensions.invalidityDate) { - // signing time is before the invalidity date which means the - // certificate is not revoked at the time of signing. - break + // latestTempRevokedEntry contains the most recent revocation entry with + // reasons such as CertificateHold or RemoveFromCRL. + // + // If the certificate is revoked with CertificateHold, it is temporarily + // revoked. If the certificate is shown in the CRL with RemoveFromCRL, + // it is unrevoked. + var latestTempRevokedEntry *x509.RevocationListEntry + + // iterate over all the entries in the base and delta CRLs + for _, entries := range entriesArray { + for i, revocationEntry := range *entries { + if revocationEntry.SerialNumber.Cmp(cert.SerialNumber) == 0 { + extensions, err := parseEntryExtensions(revocationEntry) + if err != nil { + return nil, err + } + + // validate signingTime and invalidityDate + if !signingTime.IsZero() && !extensions.invalidityDate.IsZero() && + signingTime.Before(extensions.invalidityDate) { + // signing time is before the invalidity date which means the + // certificate is not revoked at the time of signing. + break + } + + switch revocationEntry.ReasonCode { + case int(reasonCodeCertificateHold), int(reasonCodeRemoveFromCRL): + // temporarily revoked or unrevoked + if latestTempRevokedEntry == nil || latestTempRevokedEntry.RevocationTime.Before(revocationEntry.RevocationTime) { + // the revocation status depends on the most recent reason + latestTempRevokedEntry = &baseCRL.RevokedCertificateEntries[i] + } + default: + // permanently revoked + return &result.ServerResult{ + Result: result.ResultRevoked, + Server: crlURL, + RevocationMethod: result.RevocationMethodCRL, + }, nil + } } - - // revoked - return &result.ServerResult{ - Result: result.ResultRevoked, - Server: crlURL, - RevocationMethod: result.RevocationMethodCRL, - }, nil } } + if latestTempRevokedEntry != nil && latestTempRevokedEntry.ReasonCode == reasonCodeCertificateHold { + // revoked with CertificateHold + return &result.ServerResult{ + Result: result.ResultRevoked, + Server: crlURL, + RevocationMethod: result.RevocationMethodCRL, + }, nil + } return &result.ServerResult{ Result: result.ResultOK, diff --git a/revocation/internal/crl/crl_test.go b/revocation/internal/crl/crl_test.go index 27fa2475..e1c5fdaf 100644 --- a/revocation/internal/crl/crl_test.go +++ b/revocation/internal/crl/crl_test.go @@ -203,38 +203,6 @@ func TestCertCheckStatus(t *testing.T) { } }) - t.Run("CRL with delta CRL is not checked", func(t *testing.T) { - memoryCache := &memoryCache{} - - crlBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ - NextUpdate: time.Now().Add(time.Hour), - Number: big.NewInt(20240720), - ExtraExtensions: []pkix.Extension{ - { - Id: oidFreshestCRL, - Critical: false, - }, - }, - }, issuerCert, issuerKey) - if err != nil { - t.Fatal(err) - } - fetcher, err := crlutils.NewHTTPFetcher( - &http.Client{Transport: expectedRoundTripperMock{Body: crlBytes}}, - ) - if err != nil { - t.Fatal(err) - } - fetcher.Cache = memoryCache - fetcher.DiscardCacheError = true - r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ - Fetcher: fetcher, - }) - 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") - } - }) - memoryCache := &memoryCache{} // create a stale CRL @@ -423,35 +391,6 @@ func TestValidate(t *testing.T) { t.Fatal(err) } }) - - t.Run("delta CRL is not supported", func(t *testing.T) { - chain := testhelper.GetRevokableRSAChainWithRevocations(1, false, true) - issuerCert := chain[0].Cert - issuerKey := chain[0].PrivateKey - - crlBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ - NextUpdate: time.Now().Add(time.Hour), - Number: big.NewInt(20240720), - ExtraExtensions: []pkix.Extension{ - { - Id: oidFreshestCRL, - Critical: false, - }, - }, - }, issuerCert, issuerKey) - if err != nil { - t.Fatal(err) - } - - crl, err := x509.ParseRevocationList(crlBytes) - if err != nil { - t.Fatal(err) - } - - if err := validate(crl, issuerCert); err.Error() != "delta CRL is not supported" { - t.Fatalf("got %v, expected delta CRL is not supported", err) - } - }) } func TestCheckRevocation(t *testing.T) { @@ -461,14 +400,14 @@ func TestCheckRevocation(t *testing.T) { signingTime := time.Now() t.Run("certificate is nil", func(t *testing.T) { - _, err := checkRevocation(nil, &x509.RevocationList{}, signingTime, "") + _, err := checkRevocation(nil, &crlutils.Bundle{BaseCRL: &x509.RevocationList{}}, signingTime, "") if err == nil { t.Fatal("expected error") } }) t.Run("CRL is nil", func(t *testing.T) { - _, err := checkRevocation(cert, nil, signingTime, "") + _, err := checkRevocation(cert, &crlutils.Bundle{}, signingTime, "") if err == nil { t.Fatal("expected error") } @@ -482,7 +421,7 @@ func TestCheckRevocation(t *testing.T) { }, }, } - r, err := checkRevocation(cert, baseCRL, signingTime, "") + r, err := checkRevocation(cert, &crlutils.Bundle{BaseCRL: baseCRL}, signingTime, "") if err != nil { t.Fatal(err) } @@ -500,7 +439,7 @@ func TestCheckRevocation(t *testing.T) { }, }, } - r, err := checkRevocation(cert, baseCRL, signingTime, "") + r, err := checkRevocation(cert, &crlutils.Bundle{BaseCRL: baseCRL}, signingTime, "") if err != nil { t.Fatal(err) } @@ -533,7 +472,7 @@ func TestCheckRevocation(t *testing.T) { }, }, } - r, err := checkRevocation(cert, baseCRL, signingTime, "") + r, err := checkRevocation(cert, &crlutils.Bundle{BaseCRL: baseCRL}, signingTime, "") if err != nil { t.Fatal(err) } @@ -566,7 +505,7 @@ func TestCheckRevocation(t *testing.T) { }, }, } - r, err := checkRevocation(cert, baseCRL, signingTime, "") + r, err := checkRevocation(cert, &crlutils.Bundle{BaseCRL: baseCRL}, signingTime, "") if err != nil { t.Fatal(err) } @@ -584,7 +523,7 @@ func TestCheckRevocation(t *testing.T) { }, }, } - r, err := checkRevocation(cert, baseCRL, time.Time{}, "") + r, err := checkRevocation(cert, &crlutils.Bundle{BaseCRL: baseCRL}, time.Time{}, "") if err != nil { t.Fatal(err) } @@ -607,7 +546,7 @@ func TestCheckRevocation(t *testing.T) { }, }, } - _, err := checkRevocation(cert, baseCRL, signingTime, "") + _, err := checkRevocation(cert, &crlutils.Bundle{BaseCRL: baseCRL}, signingTime, "") if err == nil { t.Fatal("expected error") } From 8182be91a27aa2657acd507396f68dc53211b781 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 26 Nov 2024 08:11:08 +0000 Subject: [PATCH 02/25] fix: license and variable names Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 12 ++++++------ revocation/internal/crl/const.go | 13 +++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index 372cd6d4..4fc2c67d 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -54,7 +54,7 @@ type Fetcher interface { type FetcherWithCertificateExtensions interface { // FetchWithCertificateExtensions retrieves the CRL from the given URL with // certificate extensions. - FetchWithCertificateExtensions(ctx context.Context, url string, certExtension *[]pkix.Extension) (*Bundle, error) + FetchWithCertificateExtensions(ctx context.Context, url string, certificateExtensions *[]pkix.Extension) (*Bundle, error) } // HTTPFetcher is a Fetcher implementation that fetches CRL from the given URL @@ -93,7 +93,7 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*Bundle, error) { return f.FetchWithCertificateExtensions(ctx, url, nil) } -func (f *HTTPFetcher) FetchWithCertificateExtensions(ctx context.Context, url string, certExtension *[]pkix.Extension) (*Bundle, error) { +func (f *HTTPFetcher) FetchWithCertificateExtensions(ctx context.Context, url string, certificateExtensions *[]pkix.Extension) (*Bundle, error) { if url == "" { return nil, errors.New("CRL URL cannot be empty") } @@ -111,7 +111,7 @@ func (f *HTTPFetcher) FetchWithCertificateExtensions(ctx context.Context, url st } } - bundle, err := f.fetch(ctx, url, certExtension) + bundle, err := f.fetch(ctx, url, certificateExtensions) if err != nil { return nil, fmt.Errorf("failed to retrieve CRL: %w", err) } @@ -131,7 +131,7 @@ func isEffective(nextUpdate time.Time) bool { } // fetch downloads the CRL from the given URL. -func (f *HTTPFetcher) fetch(ctx context.Context, url string, certificateExtension *[]pkix.Extension) (*Bundle, error) { +func (f *HTTPFetcher) fetch(ctx context.Context, url string, certificateExtensions *[]pkix.Extension) (*Bundle, error) { // fetch base CRL base, err := fetchCRL(ctx, url, f.httpClient) if err != nil { @@ -143,9 +143,9 @@ func (f *HTTPFetcher) fetch(ctx context.Context, url string, certificateExtensio if err != nil { return nil, err } - if certificateExtension != nil && deltaCRL == nil { + if certificateExtensions != nil && deltaCRL == nil { // fallback to fetch delta CRL from certificate extension - deltaCRL, err = f.processDeltaCRL(certificateExtension) + deltaCRL, err = f.processDeltaCRL(certificateExtensions) if err != nil { return nil, err } diff --git a/revocation/internal/crl/const.go b/revocation/internal/crl/const.go index 4df28629..a58ce967 100644 --- a/revocation/internal/crl/const.go +++ b/revocation/internal/crl/const.go @@ -1,3 +1,16 @@ +// 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 const ( From 20491933b6b3524c91176068bd026efeafcc8a05 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 26 Nov 2024 08:58:59 +0000 Subject: [PATCH 03/25] fix: optimize logic Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 9 +++++---- revocation/internal/crl/crl.go | 15 +++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index 4fc2c67d..c04c5d6e 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -102,8 +102,8 @@ func (f *HTTPFetcher) FetchWithCertificateExtensions(ctx context.Context, url st bundle, err := f.Cache.Get(ctx, url) if err == nil { // check expiry of base CRL and delta CRL - if isEffective(bundle.BaseCRL.NextUpdate) && - (bundle.DeltaCRL == nil || isEffective(bundle.DeltaCRL.NextUpdate)) { + if (bundle.BaseCRL != nil && isEffective(bundle.BaseCRL)) && + (bundle.DeltaCRL == nil || isEffective(bundle.DeltaCRL)) { return bundle, nil } } else if !errors.Is(err, ErrCacheMiss) && !f.DiscardCacheError { @@ -126,8 +126,9 @@ func (f *HTTPFetcher) FetchWithCertificateExtensions(ctx context.Context, url st return bundle, nil } -func isEffective(nextUpdate time.Time) bool { - return !nextUpdate.IsZero() && !time.Now().After(nextUpdate) +// isEffective checks if the CRL is effective by checking the NextUpdate time. +func isEffective(crl *x509.RevocationList) bool { + return !crl.NextUpdate.IsZero() && !time.Now().After(crl.NextUpdate) } // fetch downloads the CRL from the given URL. diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 56eb391e..08492374 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -18,7 +18,6 @@ package crl import ( "context" "crypto/x509" - "crypto/x509/pkix" "encoding/asn1" "errors" "fmt" @@ -87,10 +86,10 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C } var ( - serverResults = make([]*result.ServerResult, 0, len(cert.CRLDistributionPoints)) - lastErr error - crlURL string - hasFreshestCRL = hasFreshestCRL(&cert.Extensions) + serverResults = make([]*result.ServerResult, 0, len(cert.CRLDistributionPoints)) + lastErr error + crlURL string + hasDeltaCRL = hasDeltaCRL(cert) ) // The CRLDistributionPoints contains the URIs of all the CRL distribution @@ -105,7 +104,7 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C bundle *crl.Bundle err error ) - if !hasFreshestCRL { + if !hasDeltaCRL { bundle, err = opts.Fetcher.Fetch(ctx, crlURL) if err != nil { lastErr = fmt.Errorf("failed to download CRL from %s: %w", crlURL, err) @@ -178,8 +177,8 @@ func Supported(cert *x509.Certificate) bool { return cert != nil && len(cert.CRLDistributionPoints) > 0 } -func hasFreshestCRL(extensions *[]pkix.Extension) bool { - for _, ext := range *extensions { +func hasDeltaCRL(cert *x509.Certificate) bool { + for _, ext := range cert.Extensions { if ext.Id.Equal(oidFreshestCRL) { return true } From 33d5590ebad0f6f547cb744dd90b00563ab82454 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Wed, 27 Nov 2024 07:33:25 +0000 Subject: [PATCH 04/25] fix: working on delta CRL Signed-off-by: Junjie Gao --- revocation/crl/fetcher_test.go | 48 ++++++++++++++++ .../crl/testdata/certificateWithDeltaCRL.cer | 28 ++++++++++ revocation/internal/crl/crl.go | 55 ++++++++++++++++--- revocation/internal/crl/crl_test.go | 8 +-- 4 files changed, 127 insertions(+), 12 deletions(-) create mode 100644 revocation/crl/testdata/certificateWithDeltaCRL.cer diff --git a/revocation/crl/fetcher_test.go b/revocation/crl/fetcher_test.go index b289cc7a..d96c58e2 100644 --- a/revocation/crl/fetcher_test.go +++ b/revocation/crl/fetcher_test.go @@ -18,11 +18,14 @@ import ( "context" "crypto/rand" "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" "errors" "fmt" "io" "math/big" "net/http" + "os" "strings" "sync" "testing" @@ -257,6 +260,51 @@ func TestFetch(t *testing.T) { }) } +func TestParseFreshestCRL(t *testing.T) { + certPath := "testdata/certificateWithDeltaCRL.cer" + certData, err := os.ReadFile(certPath) + if err != nil { + t.Fatalf("failed to read certificate: %v", err) + } + + block, _ := pem.Decode(certData) + if block == nil { + t.Fatalf("failed to decode PEM block") + } + + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + t.Fatalf("failed to parse certificate: %v", err) + } + + var freshestCRLExtension pkix.Extension + found := false + for _, ext := range cert.Extensions { + if ext.Id.Equal([]int{2, 5, 29, 46}) { // id-ce-freshestCRL + freshestCRLExtension = ext + found = true + break + } + } + + if !found { + t.Fatalf("freshest CRL extension not found in certificate") + } + + urls, err := parseFreshestCRL(freshestCRLExtension) + if err != nil { + t.Fatalf("failed to parse freshest CRL: %v", err) + } + + if len(urls) != 1 { + t.Fatalf("expected 1 URL, got %d", len(urls)) + } + + if !strings.HasPrefix(urls[0], "http://localhost:80") { + t.Fatalf("unexpected URL: %s", urls[0]) + } +} + func TestDownload(t *testing.T) { t.Run("parse url error", func(t *testing.T) { _, err := fetchCRL(context.Background(), ":", http.DefaultClient) diff --git a/revocation/crl/testdata/certificateWithDeltaCRL.cer b/revocation/crl/testdata/certificateWithDeltaCRL.cer new file mode 100644 index 00000000..8b06be8d --- /dev/null +++ b/revocation/crl/testdata/certificateWithDeltaCRL.cer @@ -0,0 +1,28 @@ +-----BEGIN CERTIFICATE----- +MIIE1TCCAz2gAwIBAgIUaHnHWrIVx0qzAZIwvELkQUEs+3cwDQYJKoZIhvcNAQEL +BQAwYTEjMCEGCgmSJomT8ixkAQEME2MtMG53d296cXZhd3ZhbzNlY2gxFTATBgNV +BAMMDE1hbmFnZW1lbnRDQTEjMCEGA1UECgwaRUpCQ0EgQ29udGFpbmVyIFF1aWNr +c3RhcnQwHhcNMjQxMTI1MDc1NzI3WhcNMjYxMTI1MDc1NzI2WjAYMRYwFAYDVQQD +DA1Ob3RhdGlvblRlc3QzMIGbMBAGByqGSM49AgEGBSuBBAAjA4GGAAQAuJlXlrTm +VhEiLz75HgJlZGm0TE7W/7mYn0m03vV+9tBEA/ZV50ACkMDY0ewxxh4Ko6UsGq71 +E466hSVggiaYaE4AdbL5kAolsnEm9/EDfYQNfgiQw0BI7axri9tJ19yZhj4Es31+ +j4RJHAydB4i/qJi6T8cITdT6ViyzWM6AWGl7yRajggHUMIIB0DAMBgNVHRMBAf8E +AjAAMB8GA1UdIwQYMBaAFFPceeG3G4d5oezFSybFwehynbPqMIGuBgNVHS4EgaYw +gaMwgaCggZ2ggZqGgZdodHRwOi8vbG9jYWxob3N0OjgwL2VqYmNhL3B1YmxpY3dl +Yi93ZWJkaXN0L2NlcnRkaXN0P2NtZD1kZWx0YWNybCZpc3N1ZXI9VUlEJTNEYy0w +bnd3b3pxdmF3dmFvM2VjaCUyQ0NOJTNETWFuYWdlbWVudENBJTJDTyUzREVKQkNB +K0NvbnRhaW5lcitRdWlja3N0YXJ0MBMGA1UdJQQMMAoGCCsGAQUFBwMBMIGpBgNV +HR8EgaEwgZ4wgZuggZiggZWGgZJodHRwOi8vbG9jYWxob3N0OjgwL2VqYmNhL3B1 +YmxpY3dlYi93ZWJkaXN0L2NlcnRkaXN0P2NtZD1jcmwmaXNzdWVyPVVJRCUzRGMt +MG53d296cXZhd3ZhbzNlY2glMkNDTiUzRE1hbmFnZW1lbnRDQSUyQ08lM0RFSkJD +QStDb250YWluZXIrUXVpY2tzdGFydDAdBgNVHQ4EFgQUMcTzb/TrE6hxhu8wjIa3 +aCh+TzwwDgYDVR0PAQH/BAQDAgWgMA0GCSqGSIb3DQEBCwUAA4IBgQBXsbtYw+fe +1A2oHxHFgBNXd5WEHdYLkHGoFtDOnaWg3JwNGZf3l9rsRXnFw8hS/jYH5PMu79qe +f0Us4ySoSED4XnOwSiaKRQD4vhy4hYtdSHAhbATYj+l+AcnrUm0wzG/etZAfKjpi +nOkgAbbejEZfOuU3j+g19bNYwhiVyXjGIfELa84qc07Ah3cKTeOwkTXS0iZ0P+1B +heHuHm54RLI9Rm2oz+DZA9qn60QcCumB8BB2kS9GPW9lYCi1wQiv5rmmikTnpRoT +hrb0OylVMDWtVyGEpiXaCkPGJ5JCBCvrV9YdfNGgpx0Vn42Com7B30LGtduSXoQo +Ol5+K/DP0bhvdvpHyUyX7CZ5OA+9JRg7fhzwawXXjcdX9Uo2gG+jPjhDItR/7X5e +Ie5SN8dpcK1eYM6hDrgzd32AavPbcbkokBQG5txtx27sCHcPnOq+J4aChi3yrJ7W +ewhVgLUqtg/YqnmNYJp0u8jC7YHSPt4D4vA3YwFZ2WF1FNI5WQgKNZQ= +-----END CERTIFICATE----- diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 08492374..165c4f92 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -21,10 +21,12 @@ import ( "encoding/asn1" "errors" "fmt" + "math/big" "time" "github.com/notaryproject/notation-core-go/revocation/crl" "github.com/notaryproject/notation-core-go/revocation/result" + "golang.org/x/crypto/cryptobyte" ) var ( @@ -36,6 +38,10 @@ var ( // distribution point CRL extension. (See RFC 5280, Section 5.2.5) oidIssuingDistributionPoint = asn1.ObjectIdentifier{2, 5, 29, 28} + // oidDeltaCRLIndicator is the object identifier for the delta CRL indicator + // (See RFC 5280, Section 5.2.4) + oidDeltaCRLIndicator = asn1.ObjectIdentifier{2, 5, 29, 27} + // oidInvalidityDate is the object identifier for the invalidity date // CRL entry extension. (See RFC 5280, Section 5.3.2) oidInvalidityDate = asn1.ObjectIdentifier{2, 5, 29, 24} @@ -123,16 +129,10 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C } } - if err = validate(bundle.BaseCRL, issuer); err != nil { + if err = validate(bundle, issuer); err != nil { lastErr = fmt.Errorf("failed to validate CRL from %s: %w", crlURL, err) break } - if bundle.DeltaCRL != nil { - if err = validate(bundle.DeltaCRL, issuer); err != nil { - lastErr = fmt.Errorf("failed to validate delta CRL from %s: %w", crlURL, err) - break - } - } crlResult, err := checkRevocation(cert, bundle, opts.SigningTime, crlURL) if err != nil { @@ -186,7 +186,46 @@ func hasDeltaCRL(cert *x509.Certificate) bool { return false } -func validate(crl *x509.RevocationList, issuer *x509.Certificate) error { +func validate(bundle *crl.Bundle, issuer *x509.Certificate) error { + // validate base CRL + baseCRL := bundle.BaseCRL + if err := validateCRL(baseCRL, issuer); err != nil { + return err + } + + if bundle.DeltaCRL != nil { + // validate delta CRL + // RFC 5280, Section 5.2.4 + deltaCRL := bundle.DeltaCRL + if err := validateCRL(bundle.DeltaCRL, issuer); err != nil { + return err + } + + if deltaCRL.Number.Cmp(baseCRL.Number) <= 0 { + return fmt.Errorf("delta CRL number %d is not greater than the base CRL number %d", deltaCRL.Number, baseCRL.Number) + } + // check delta CRL indicator extension + var minimumBaseCRLNumber *big.Int + for _, ext := range deltaCRL.Extensions { + if ext.Id.Equal(oidDeltaCRLIndicator) { + minimumBaseCRLNumber = new(big.Int) + value := cryptobyte.String(ext.Value) + if !value.ReadASN1Integer(minimumBaseCRLNumber) { + return errors.New("failed to parse delta CRL indicator extension") + } + } + } + if minimumBaseCRLNumber == nil { + return errors.New("delta CRL indicator extension is not found") + } + if minimumBaseCRLNumber.Cmp(baseCRL.Number) > 0 { + return fmt.Errorf("delta CRL indicator %d is not less than or equal to the base CRL number %d", minimumBaseCRLNumber, baseCRL.Number) + } + } + return nil +} + +func validateCRL(crl *x509.RevocationList, issuer *x509.Certificate) error { // check signature if err := crl.CheckSignatureFrom(issuer); err != nil { return fmt.Errorf("CRL is not signed by CA %s: %w,", issuer.Subject, err) diff --git a/revocation/internal/crl/crl_test.go b/revocation/internal/crl/crl_test.go index e1c5fdaf..8d9b3b1e 100644 --- a/revocation/internal/crl/crl_test.go +++ b/revocation/internal/crl/crl_test.go @@ -317,7 +317,7 @@ func TestValidate(t *testing.T) { t.Fatal(err) } - if err := validate(crl, issuerCert); err == nil { + if err := validateCRL(crl, issuerCert); err == nil { t.Fatal("expected error") } }) @@ -327,7 +327,7 @@ func TestValidate(t *testing.T) { NextUpdate: time.Now().Add(time.Hour), } - if err := validate(crl, &x509.Certificate{}); err == nil { + if err := validateCRL(crl, &x509.Certificate{}); err == nil { t.Fatal("expected error") } }) @@ -358,7 +358,7 @@ func TestValidate(t *testing.T) { }, } - if err := validate(crl, issuerCert); err == nil { + if err := validateCRL(crl, issuerCert); err == nil { t.Fatal("expected error") } }) @@ -387,7 +387,7 @@ func TestValidate(t *testing.T) { t.Fatal(err) } - if err := validate(crl, issuerCert); err != nil { + if err := validateCRL(crl, issuerCert); err != nil { t.Fatal(err) } }) From 7984f57173223f3190d232a6289aae1d1ff74b3e Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Wed, 27 Nov 2024 08:09:03 +0000 Subject: [PATCH 05/25] fix: bug Signed-off-by: Junjie Gao --- revocation/internal/crl/crl.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 165c4f92..70b7fed9 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -258,22 +258,20 @@ func validateCRL(crl *x509.RevocationList, issuer *x509.Certificate) error { } // checkRevocation checks if the certificate is revoked or not -func checkRevocation(cert *x509.Certificate, bundle *crl.Bundle, signingTime time.Time, crlURL string) (*result.ServerResult, error) { +func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Time, crlURL string) (*result.ServerResult, error) { if cert == nil { return nil, errors.New("certificate cannot be nil") } - if bundle == nil { + if b == nil { return nil, errors.New("CRL bundle cannot be nil") } - baseCRL := bundle.BaseCRL - if baseCRL == nil { + if b.BaseCRL == nil { return nil, errors.New("baseCRL cannot be nil") } - deltaCRL := bundle.DeltaCRL - entriesArray := []*[]x509.RevocationListEntry{&baseCRL.RevokedCertificateEntries} - if deltaCRL != nil { - entriesArray = append(entriesArray, &deltaCRL.RevokedCertificateEntries) + entriesArray := []*[]x509.RevocationListEntry{&b.BaseCRL.RevokedCertificateEntries} + if b.DeltaCRL != nil { + entriesArray = append(entriesArray, &b.DeltaCRL.RevokedCertificateEntries) } // latestTempRevokedEntry contains the most recent revocation entry with @@ -306,7 +304,7 @@ func checkRevocation(cert *x509.Certificate, bundle *crl.Bundle, signingTime tim // temporarily revoked or unrevoked if latestTempRevokedEntry == nil || latestTempRevokedEntry.RevocationTime.Before(revocationEntry.RevocationTime) { // the revocation status depends on the most recent reason - latestTempRevokedEntry = &baseCRL.RevokedCertificateEntries[i] + latestTempRevokedEntry = &(*entries)[i] } default: // permanently revoked From 77b59726396eee9487e821c6f3a8a02aab3489a4 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Thu, 28 Nov 2024 07:46:40 +0000 Subject: [PATCH 06/25] test: add test for fetcher Signed-off-by: Junjie Gao --- revocation/crl/fetcher_test.go | 217 +++++++++++++++--- .../crl/testdata/certificateWith2DeltaCRL.cer | 32 +++ .../certificateWithIncompleteFreshestCRL.cer | 22 ++ .../certificateWithIncompleteFreshestCRL2.cer | 22 ++ .../certificateWithNonURIDeltaCRL.cer | 22 ++ .../certificateWithZeroDeltaCRLURL.cer | 25 ++ .../testdata/crlWithMultipleFreshestCRLs.crl | Bin 0 -> 949 bytes revocation/crl/testdata/delta.crl | Bin 0 -> 622 bytes 8 files changed, 312 insertions(+), 28 deletions(-) create mode 100644 revocation/crl/testdata/certificateWith2DeltaCRL.cer create mode 100644 revocation/crl/testdata/certificateWithIncompleteFreshestCRL.cer create mode 100644 revocation/crl/testdata/certificateWithIncompleteFreshestCRL2.cer create mode 100644 revocation/crl/testdata/certificateWithNonURIDeltaCRL.cer create mode 100644 revocation/crl/testdata/certificateWithZeroDeltaCRLURL.cer create mode 100644 revocation/crl/testdata/crlWithMultipleFreshestCRLs.crl create mode 100644 revocation/crl/testdata/delta.crl diff --git a/revocation/crl/fetcher_test.go b/revocation/crl/fetcher_test.go index d96c58e2..48732060 100644 --- a/revocation/crl/fetcher_test.go +++ b/revocation/crl/fetcher_test.go @@ -261,48 +261,209 @@ func TestFetch(t *testing.T) { } func TestParseFreshestCRL(t *testing.T) { - certPath := "testdata/certificateWithDeltaCRL.cer" - certData, err := os.ReadFile(certPath) - if err != nil { - t.Fatalf("failed to read certificate: %v", err) - } + loadExtentsion := func(certPath string) pkix.Extension { + certData, err := os.ReadFile(certPath) + if err != nil { + t.Fatalf("failed to read certificate: %v", err) + } - block, _ := pem.Decode(certData) - if block == nil { - t.Fatalf("failed to decode PEM block") - } + block, _ := pem.Decode(certData) + if block == nil { + t.Fatalf("failed to decode PEM block") + } - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - t.Fatalf("failed to parse certificate: %v", err) - } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + t.Fatalf("failed to parse certificate: %v", err) + } - var freshestCRLExtension pkix.Extension - found := false - for _, ext := range cert.Extensions { - if ext.Id.Equal([]int{2, 5, 29, 46}) { // id-ce-freshestCRL - freshestCRLExtension = ext - found = true - break + for _, ext := range cert.Extensions { + if ext.Id.Equal([]int{2, 5, 29, 46}) { // id-ce-freshestCRL + return ext + } } + + t.Fatalf("freshestCRL extension not found") + return pkix.Extension{} } - if !found { - t.Fatalf("freshest CRL extension not found in certificate") + t.Run("valid 1 delta CRL URL", func(t *testing.T) { + certPath := "testdata/certificateWithDeltaCRL.cer" + freshestCRLExtension := loadExtentsion(certPath) + urls, err := parseFreshestCRL(freshestCRLExtension) + if err != nil { + t.Fatalf("failed to parse freshest CRL: %v", err) + } + + if len(urls) != 1 { + t.Fatalf("expected 1 URL, got %d", len(urls)) + } + + if !strings.HasPrefix(urls[0], "http://localhost:80") { + t.Fatalf("unexpected URL: %s", urls[0]) + } + }) + + t.Run("empty extension", func(t *testing.T) { + _, err := parseFreshestCRL(pkix.Extension{}) + if err == nil { + t.Fatalf("expected error") + } + }) + + t.Run("URL doesn't exist", func(t *testing.T) { + certPath := "testdata/certificateWithZeroDeltaCRLURL.cer" + freshestCRLExtension := loadExtentsion(certPath) + url, err := parseFreshestCRL(freshestCRLExtension) + if err != nil { + t.Fatalf("failed to parse freshest CRL: %v", err) + } + if len(url) != 0 { + t.Fatalf("expected 0 URL, got %d", len(url)) + } + }) + + t.Run("non URI freshest CRL extension", func(t *testing.T) { + certPath := "testdata/certificateWithNonURIDeltaCRL.cer" + freshestCRLExtension := loadExtentsion(certPath) + url, err := parseFreshestCRL(freshestCRLExtension) + if err != nil { + t.Fatalf("failed to parse freshest CRL: %v", err) + } + if len(url) != 0 { + t.Fatalf("expected 0 URL, got %d", len(url)) + } + }) + + t.Run("certificate with incomplete freshest CRL extension", func(t *testing.T) { + certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" + freshestCRLExtension := loadExtentsion(certPath) + _, err := parseFreshestCRL(freshestCRLExtension) + expectErrorMsg := "x509: invalid CRL distribution point" + if err == nil || err.Error() != expectErrorMsg { + t.Fatalf("expected error %q, got %v", expectErrorMsg, err) + } + }) + + t.Run("certificate with incomplete freshest CRL extension2", func(t *testing.T) { + certPath := "testdata/certificateWithIncompleteFreshestCRL2.cer" + freshestCRLExtension := loadExtentsion(certPath) + url, err := parseFreshestCRL(freshestCRLExtension) + if err != nil { + t.Fatalf("failed to parse freshest CRL: %v", err) + } + if len(url) != 0 { + t.Fatalf("expected 0 URL, got %d", len(url)) + } + }) +} + +func TestProcessDeltaCRL(t *testing.T) { + loadExtentsion := func(certPath string) *[]pkix.Extension { + certData, err := os.ReadFile(certPath) + if err != nil { + t.Fatalf("failed to read certificate: %v", err) + } + + block, _ := pem.Decode(certData) + if block == nil { + t.Fatalf("failed to decode PEM block") + } + + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + t.Fatalf("failed to parse certificate: %v", err) + } + + return &cert.Extensions } - urls, err := parseFreshestCRL(freshestCRLExtension) + deltaCRL, err := os.ReadFile("testdata/delta.crl") if err != nil { - t.Fatalf("failed to parse freshest CRL: %v", err) + t.Fatalf("failed to read delta CRL: %v", err) } - if len(urls) != 1 { - t.Fatalf("expected 1 URL, got %d", len(urls)) + fetcher, err := NewHTTPFetcher(&http.Client{ + Transport: expectedRoundTripperMock{Body: deltaCRL}, + }) + if err != nil { + t.Fatalf("failed to create fetcher: %v", err) } - if !strings.HasPrefix(urls[0], "http://localhost:80") { - t.Fatalf("unexpected URL: %s", urls[0]) + t.Run("parse freshest CRL failed", func(t *testing.T) { + certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" + extensions := loadExtentsion(certPath) + _, err := fetcher.processDeltaCRL(extensions) + expectedErrorMsg := "x509: invalid CRL distribution point" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) + } + }) + + t.Run("zero freshest CRL URL", func(t *testing.T) { + certPath := "testdata/certificateWithZeroDeltaCRLURL.cer" + extensions := loadExtentsion(certPath) + deltaCRL, err := fetcher.processDeltaCRL(extensions) + if err != nil { + t.Fatalf("failed to process delta CRL: %v", err) + } + if deltaCRL != nil { + t.Fatalf("expected nil delta CRL, got %v", deltaCRL) + } + }) + + t.Run("one freshest CRL URL", func(t *testing.T) { + certPath := "testdata/certificateWithDeltaCRL.cer" + extensions := loadExtentsion(certPath) + deltaCRL, err := fetcher.processDeltaCRL(extensions) + if err != nil { + t.Fatalf("failed to process delta CRL: %v", err) + } + if deltaCRL == nil { + t.Fatalf("expected non-nil delta CRL") + } + }) + + t.Run("multiple freshest CRL URLs", func(t *testing.T) { + certPath := "testdata/certificateWith2DeltaCRL.cer" + extensions := loadExtentsion(certPath) + _, err := fetcher.processDeltaCRL(extensions) + expectedErrorMsg := "multiple Freshest CRL distribution points are not supported" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) + } + }) + + t.Run("process delta crl from certificate extension failed", func(t *testing.T) { + certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" + extensions := loadExtentsion(certPath) + _, err := fetcher.fetch(context.Background(), "http://localhost.test", extensions) + expectedErrorMsg := "x509: invalid CRL distribution point" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) + } + }) + + crlFile, err := os.ReadFile("testdata/crlWithMultipleFreshestCRLs.crl") + if err != nil { + t.Fatalf("failed to read CRL file: %v", err) } + fetcher, err = NewHTTPFetcher(&http.Client{ + Transport: expectedRoundTripperMock{Body: crlFile}, + }) + if err != nil { + t.Fatalf("failed to create fetcher: %v", err) + } + + t.Run("fetch delta CRL from CRL failed", func(t *testing.T) { + certPath := "testdata/certificateWithDeltaCRL.cer" + extensions := loadExtentsion(certPath) + _, err := fetcher.fetch(context.Background(), "http://localhost.test", extensions) + expectErrorMsg := "multiple Freshest CRL distribution points are not supported" + if err == nil || err.Error() != expectErrorMsg { + t.Fatalf("expected error %q, got %v", expectErrorMsg, err) + } + }) } func TestDownload(t *testing.T) { diff --git a/revocation/crl/testdata/certificateWith2DeltaCRL.cer b/revocation/crl/testdata/certificateWith2DeltaCRL.cer new file mode 100644 index 00000000..3d029b2a --- /dev/null +++ b/revocation/crl/testdata/certificateWith2DeltaCRL.cer @@ -0,0 +1,32 @@ +-----BEGIN CERTIFICATE----- +MIIFdTCCA92gAwIBAgIUaHnHWrIVx0qzAZIwvELkQUEs+3cwDQYJKoZIhvcNAQEL +BQAwYTEjMCEGCgmSJomT8ixkAQEME2MtMG53d296cXZhd3ZhbzNlY2gxFTATBgNV +BAMMDE1hbmFnZW1lbnRDQTEjMCEGA1UECgwaRUpCQ0EgQ29udGFpbmVyIFF1aWNr +c3RhcnQwHhcNMjQxMTI1MDc1NzI3WhcNMjYxMTI1MDc1NzI2WjAYMRYwFAYDVQQD +DA1Ob3RhdGlvblRlc3QzMIGbMBAGByqGSM49AgEGBSuBBAAjA4GGAAQAuJlXlrTm +VhEiLz75HgJlZGm0TE7W/7mYn0m03vV+9tBEA/ZV50ACkMDY0ewxxh4Ko6UsGq71 +E466hSVggiaYaE4AdbL5kAolsnEm9/EDfYQNfgiQw0BI7axri9tJ19yZhj4Es31+ +j4RJHAydB4i/qJi6T8cITdT6ViyzWM6AWGl7yRajggJ0MIICcDAMBgNVHRMBAf8E +AjAAMB8GA1UdIwQYMBaAFFPceeG3G4d5oezFSybFwehynbPqMIIBTQYDVR0uBIIB +RDCCAUAwggE8oIIBOKCCATSGgZdodHRwOi8vbG9jYWxob3N0OjgwL2VqYmNhL3B1 +YmxpY3dlYi93ZWJkaXN0L2NlcnRkaXN0P2NtZD1kZWx0YWNybCZpc3N1ZXI9VUlE +JTNEYy0wbnd3b3pxdmF3dmFvM2VjaCUyQ0NOJTNETWFuYWdlbWVudENBJTJDTyUz +REVKQkNBK0NvbnRhaW5lcitRdWlja3N0YXJ0hoGXaHR0cDovL2xvY2FsaG9zdDo4 +MC9lamJjYS9wdWJsaWN3ZWIvd2ViZGlzdC9jZXJ0ZGlzdD9jbWQ9ZGVsdGFjcmwm +aXNzdWVyPVVJRCUzRGMtMG53d296cXZhd3ZhbzNlY2glMkNDTiUzRE1hbmFnZW1l +bnRDQSUyQ08lM0RFSkJDQStDb250YWluZXIrUXVpY2tzdGFydDATBgNVHSUEDDAK +BggrBgEFBQcDATCBqQYDVR0fBIGhMIGeMIGboIGYoIGVhoGSaHR0cDovL2xvY2Fs +aG9zdDo4MC9lamJjYS9wdWJsaWN3ZWIvd2ViZGlzdC9jZXJ0ZGlzdD9jbWQ9Y3Js +Jmlzc3Vlcj1VSUQlM0RjLTBud3dvenF2YXd2YW8zZWNoJTJDQ04lM0RNYW5hZ2Vt +ZW50Q0ElMkNPJTNERUpCQ0ErQ29udGFpbmVyK1F1aWNrc3RhcnQwHQYDVR0OBBYE +FDHE82/06xOocYbvMIyGt2gofk88MA4GA1UdDwEB/wQEAwIFoDANBgkqhkiG9w0B +AQsFAAOCAYEAV7G7WMPn3tQNqB8RxYATV3eVhB3WC5BxqBbQzp2loNycDRmX95fa +7EV5xcPIUv42B+TzLu/ann9FLOMkqEhA+F5zsEomikUA+L4cuIWLXUhwIWwE2I/p +fgHJ61JtMMxv3rWQHyo6YpzpIAG23oxGXzrlN4/oNfWzWMIYlcl4xiHxC2vOKnNO +wId3Ck3jsJE10tImdD/tQYXh7h5ueESyPUZtqM/g2QPap+tEHArpgfAQdpEvRj1v +ZWAotcEIr+a5popE56UaE4a29DspVTA1rVchhKYl2gpDxieSQgQr61fWHXzRoKcd +FZ+NgqJuwd9CxrXbkl6EKDpefivwz9G4b3b6R8lMl+wmeTgPvSUYO34c8GsF143H +V/VKNoBvoz44QyLUf+1+XiHuUjfHaXCtXmDOoQ64M3d9gGrz23G5KJAUBubcbcdu +7Ah3D5zqvieGgoYt8qye1nsIVYC1KrYP2Kp5jWCadLvIwu2B0j7eA+LwN2MBWdlh +dRTSOVkICjWU +-----END CERTIFICATE----- diff --git a/revocation/crl/testdata/certificateWithIncompleteFreshestCRL.cer b/revocation/crl/testdata/certificateWithIncompleteFreshestCRL.cer new file mode 100644 index 00000000..35776859 --- /dev/null +++ b/revocation/crl/testdata/certificateWithIncompleteFreshestCRL.cer @@ -0,0 +1,22 @@ +-----BEGIN CERTIFICATE----- +MIIDmTCCAgGgAwIBAgIUaHnHWrIVx0qzAZIwvELkQUEs+3cwDQYJKoZIhvcNAQEL +BQAwYTEjMCEGCgmSJomT8ixkAQEME2MtMG53d296cXZhd3ZhbzNlY2gxFTATBgNV +BAMMDE1hbmFnZW1lbnRDQTEjMCEGA1UECgwaRUpCQ0EgQ29udGFpbmVyIFF1aWNr +c3RhcnQwHhcNMjQxMTI1MDc1NzI3WhcNMjYxMTI1MDc1NzI2WjAYMRYwFAYDVQQD +DA1Ob3RhdGlvblRlc3QzMIGbMBAGByqGSM49AgEGBSuBBAAjA4GGAAQAuJlXlrTm +VhEiLz75HgJlZGm0TE7W/7mYn0m03vV+9tBEA/ZV50ACkMDY0ewxxh4Ko6UsGq71 +E466hSVggiaYaE4AdbL5kAolsnEm9/EDfYQNfgiQw0BI7axri9tJ19yZhj4Es31+ +j4RJHAydB4i/qJi6T8cITdT6ViyzWM6AWGl7yRajgZkwgZYwDAYDVR0TAQH/BAIw +ADAfBgNVHSMEGDAWgBRT3HnhtxuHeaHsxUsmxcHocp2z6jANBgNVHS4EBjAEMAKg +ADATBgNVHSUEDDAKBggrBgEFBQcDATASBgNVHR8ECzAJMAegBaADhQH/MB0GA1Ud +DgQWBBQxxPNv9OsTqHGG7zCMhrdoKH5PPDAOBgNVHQ8BAf8EBAMCBaAwDQYJKoZI +hvcNAQELBQADggGBAFexu1jD597UDagfEcWAE1d3lYQd1guQcagW0M6dpaDcnA0Z +l/eX2uxFecXDyFL+Ngfk8y7v2p5/RSzjJKhIQPhec7BKJopFAPi+HLiFi11IcCFs +BNiP6X4ByetSbTDMb961kB8qOmKc6SABtt6MRl865TeP6DX1s1jCGJXJeMYh8Qtr +zipzTsCHdwpN47CRNdLSJnQ/7UGF4e4ebnhEsj1GbajP4NkD2qfrRBwK6YHwEHaR +L0Y9b2VgKLXBCK/muaaKROelGhOGtvQ7KVUwNa1XIYSmJdoKQ8YnkkIEK+tX1h18 +0aCnHRWfjYKibsHfQsa125JehCg6Xn4r8M/RuG92+kfJTJfsJnk4D70lGDt+HPBr +BdeNx1f1SjaAb6M+OEMi1H/tfl4h7lI3x2lwrV5gzqEOuDN3fYBq89txuSiQFAbm +3G3HbuwIdw+c6r4nhoKGLfKsntZ7CFWAtSq2D9iqeY1gmnS7yMLtgdI+3gPi8Ddj +AVnZYXUU0jlZCAo1lA== +-----END CERTIFICATE----- diff --git a/revocation/crl/testdata/certificateWithIncompleteFreshestCRL2.cer b/revocation/crl/testdata/certificateWithIncompleteFreshestCRL2.cer new file mode 100644 index 00000000..8ef955fc --- /dev/null +++ b/revocation/crl/testdata/certificateWithIncompleteFreshestCRL2.cer @@ -0,0 +1,22 @@ +-----BEGIN CERTIFICATE----- +MIIDlzCCAf+gAwIBAgIUaHnHWrIVx0qzAZIwvELkQUEs+3cwDQYJKoZIhvcNAQEL +BQAwYTEjMCEGCgmSJomT8ixkAQEME2MtMG53d296cXZhd3ZhbzNlY2gxFTATBgNV +BAMMDE1hbmFnZW1lbnRDQTEjMCEGA1UECgwaRUpCQ0EgQ29udGFpbmVyIFF1aWNr +c3RhcnQwHhcNMjQxMTI1MDc1NzI3WhcNMjYxMTI1MDc1NzI2WjAYMRYwFAYDVQQD +DA1Ob3RhdGlvblRlc3QzMIGbMBAGByqGSM49AgEGBSuBBAAjA4GGAAQAuJlXlrTm +VhEiLz75HgJlZGm0TE7W/7mYn0m03vV+9tBEA/ZV50ACkMDY0ewxxh4Ko6UsGq71 +E466hSVggiaYaE4AdbL5kAolsnEm9/EDfYQNfgiQw0BI7axri9tJ19yZhj4Es31+ +j4RJHAydB4i/qJi6T8cITdT6ViyzWM6AWGl7yRajgZcwgZQwDAYDVR0TAQH/BAIw +ADAfBgNVHSMEGDAWgBRT3HnhtxuHeaHsxUsmxcHocp2z6jALBgNVHS4EBDACMAAw +EwYDVR0lBAwwCgYIKwYBBQUHAwEwEgYDVR0fBAswCTAHoAWgA4UB/zAdBgNVHQ4E +FgQUMcTzb/TrE6hxhu8wjIa3aCh+TzwwDgYDVR0PAQH/BAQDAgWgMA0GCSqGSIb3 +DQEBCwUAA4IBgQBXsbtYw+fe1A2oHxHFgBNXd5WEHdYLkHGoFtDOnaWg3JwNGZf3 +l9rsRXnFw8hS/jYH5PMu79qef0Us4ySoSED4XnOwSiaKRQD4vhy4hYtdSHAhbATY +j+l+AcnrUm0wzG/etZAfKjpinOkgAbbejEZfOuU3j+g19bNYwhiVyXjGIfELa84q +c07Ah3cKTeOwkTXS0iZ0P+1BheHuHm54RLI9Rm2oz+DZA9qn60QcCumB8BB2kS9G +PW9lYCi1wQiv5rmmikTnpRoThrb0OylVMDWtVyGEpiXaCkPGJ5JCBCvrV9YdfNGg +px0Vn42Com7B30LGtduSXoQoOl5+K/DP0bhvdvpHyUyX7CZ5OA+9JRg7fhzwawXX +jcdX9Uo2gG+jPjhDItR/7X5eIe5SN8dpcK1eYM6hDrgzd32AavPbcbkokBQG5txt +x27sCHcPnOq+J4aChi3yrJ7WewhVgLUqtg/YqnmNYJp0u8jC7YHSPt4D4vA3YwFZ +2WF1FNI5WQgKNZQ= +-----END CERTIFICATE----- diff --git a/revocation/crl/testdata/certificateWithNonURIDeltaCRL.cer b/revocation/crl/testdata/certificateWithNonURIDeltaCRL.cer new file mode 100644 index 00000000..128bbbe9 --- /dev/null +++ b/revocation/crl/testdata/certificateWithNonURIDeltaCRL.cer @@ -0,0 +1,22 @@ +-----BEGIN CERTIFICATE----- +MIIDnjCCAgagAwIBAgIUaHnHWrIVx0qzAZIwvELkQUEs+3cwDQYJKoZIhvcNAQEL +BQAwYTEjMCEGCgmSJomT8ixkAQEME2MtMG53d296cXZhd3ZhbzNlY2gxFTATBgNV +BAMMDE1hbmFnZW1lbnRDQTEjMCEGA1UECgwaRUpCQ0EgQ29udGFpbmVyIFF1aWNr +c3RhcnQwHhcNMjQxMTI1MDc1NzI3WhcNMjYxMTI1MDc1NzI2WjAYMRYwFAYDVQQD +DA1Ob3RhdGlvblRlc3QzMIGbMBAGByqGSM49AgEGBSuBBAAjA4GGAAQAuJlXlrTm +VhEiLz75HgJlZGm0TE7W/7mYn0m03vV+9tBEA/ZV50ACkMDY0ewxxh4Ko6UsGq71 +E466hSVggiaYaE4AdbL5kAolsnEm9/EDfYQNfgiQw0BI7axri9tJ19yZhj4Es31+ +j4RJHAydB4i/qJi6T8cITdT6ViyzWM6AWGl7yRajgZ4wgZswDAYDVR0TAQH/BAIw +ADAfBgNVHSMEGDAWgBRT3HnhtxuHeaHsxUsmxcHocp2z6jASBgNVHS4ECzAJMAeg +BaADhQH/MBMGA1UdJQQMMAoGCCsGAQUFBwMBMBIGA1UdHwQLMAkwB6AFoAOFAf8w +HQYDVR0OBBYEFDHE82/06xOocYbvMIyGt2gofk88MA4GA1UdDwEB/wQEAwIFoDAN +BgkqhkiG9w0BAQsFAAOCAYEAV7G7WMPn3tQNqB8RxYATV3eVhB3WC5BxqBbQzp2l +oNycDRmX95fa7EV5xcPIUv42B+TzLu/ann9FLOMkqEhA+F5zsEomikUA+L4cuIWL +XUhwIWwE2I/pfgHJ61JtMMxv3rWQHyo6YpzpIAG23oxGXzrlN4/oNfWzWMIYlcl4 +xiHxC2vOKnNOwId3Ck3jsJE10tImdD/tQYXh7h5ueESyPUZtqM/g2QPap+tEHArp +gfAQdpEvRj1vZWAotcEIr+a5popE56UaE4a29DspVTA1rVchhKYl2gpDxieSQgQr +61fWHXzRoKcdFZ+NgqJuwd9CxrXbkl6EKDpefivwz9G4b3b6R8lMl+wmeTgPvSUY +O34c8GsF143HV/VKNoBvoz44QyLUf+1+XiHuUjfHaXCtXmDOoQ64M3d9gGrz23G5 +KJAUBubcbcdu7Ah3D5zqvieGgoYt8qye1nsIVYC1KrYP2Kp5jWCadLvIwu2B0j7e +A+LwN2MBWdlhdRTSOVkICjWU +-----END CERTIFICATE----- diff --git a/revocation/crl/testdata/certificateWithZeroDeltaCRLURL.cer b/revocation/crl/testdata/certificateWithZeroDeltaCRLURL.cer new file mode 100644 index 00000000..6f4de65e --- /dev/null +++ b/revocation/crl/testdata/certificateWithZeroDeltaCRLURL.cer @@ -0,0 +1,25 @@ +-----BEGIN CERTIFICATE----- +MIIENTCCAp2gAwIBAgIUaHnHWrIVx0qzAZIwvELkQUEs+3cwDQYJKoZIhvcNAQEL +BQAwYTEjMCEGCgmSJomT8ixkAQEME2MtMG53d296cXZhd3ZhbzNlY2gxFTATBgNV +BAMMDE1hbmFnZW1lbnRDQTEjMCEGA1UECgwaRUpCQ0EgQ29udGFpbmVyIFF1aWNr +c3RhcnQwHhcNMjQxMTI1MDc1NzI3WhcNMjYxMTI1MDc1NzI2WjAYMRYwFAYDVQQD +DA1Ob3RhdGlvblRlc3QzMIGbMBAGByqGSM49AgEGBSuBBAAjA4GGAAQAuJlXlrTm +VhEiLz75HgJlZGm0TE7W/7mYn0m03vV+9tBEA/ZV50ACkMDY0ewxxh4Ko6UsGq71 +E466hSVggiaYaE4AdbL5kAolsnEm9/EDfYQNfgiQw0BI7axri9tJ19yZhj4Es31+ +j4RJHAydB4i/qJi6T8cITdT6ViyzWM6AWGl7yRajggE0MIIBMDAMBgNVHRMBAf8E +AjAAMB8GA1UdIwQYMBaAFFPceeG3G4d5oezFSybFwehynbPqMA8GA1UdLgQIMAYw +BKACoAAwEwYDVR0lBAwwCgYIKwYBBQUHAwEwgakGA1UdHwSBoTCBnjCBm6CBmKCB +lYaBkmh0dHA6Ly9sb2NhbGhvc3Q6ODAvZWpiY2EvcHVibGljd2ViL3dlYmRpc3Qv +Y2VydGRpc3Q/Y21kPWNybCZpc3N1ZXI9VUlEJTNEYy0wbnd3b3pxdmF3dmFvM2Vj +aCUyQ0NOJTNETWFuYWdlbWVudENBJTJDTyUzREVKQkNBK0NvbnRhaW5lcitRdWlj +a3N0YXJ0MB0GA1UdDgQWBBQxxPNv9OsTqHGG7zCMhrdoKH5PPDAOBgNVHQ8BAf8E +BAMCBaAwDQYJKoZIhvcNAQELBQADggGBAFexu1jD597UDagfEcWAE1d3lYQd1guQ +cagW0M6dpaDcnA0Zl/eX2uxFecXDyFL+Ngfk8y7v2p5/RSzjJKhIQPhec7BKJopF +APi+HLiFi11IcCFsBNiP6X4ByetSbTDMb961kB8qOmKc6SABtt6MRl865TeP6DX1 +s1jCGJXJeMYh8QtrzipzTsCHdwpN47CRNdLSJnQ/7UGF4e4ebnhEsj1GbajP4NkD +2qfrRBwK6YHwEHaRL0Y9b2VgKLXBCK/muaaKROelGhOGtvQ7KVUwNa1XIYSmJdoK +Q8YnkkIEK+tX1h180aCnHRWfjYKibsHfQsa125JehCg6Xn4r8M/RuG92+kfJTJfs +Jnk4D70lGDt+HPBrBdeNx1f1SjaAb6M+OEMi1H/tfl4h7lI3x2lwrV5gzqEOuDN3 +fYBq89txuSiQFAbm3G3HbuwIdw+c6r4nhoKGLfKsntZ7CFWAtSq2D9iqeY1gmnS7 +yMLtgdI+3gPi8DdjAVnZYXUU0jlZCAo1lA== +-----END CERTIFICATE----- diff --git a/revocation/crl/testdata/crlWithMultipleFreshestCRLs.crl b/revocation/crl/testdata/crlWithMultipleFreshestCRLs.crl new file mode 100644 index 0000000000000000000000000000000000000000..49e5f5e08189598b50faa6f6f863b469b6b7a79f GIT binary patch literal 949 zcmXqLV%})b#3aeY$Y{XJ#;Mij(e|B}k&&B~!64C4*+7wvi*u4%=j2a1DU6If!pXV@ zdFAE#RfT1VJZu9=BJ)Pg3)CW9u%IsTQWaJl@SXmh8r)DK3C+ZiJCgo%%m!~G_193`b zafyC%YEcP@VV|6vVw;khQ<9iml%tkeTwI!3WE<+~qH64dB|ub-oSprEQplkJ6!QlP zfkQ$Y9unG!kQlr!LrWFRO^l5UP29^LzOp^OrG4=SY2z>FU;XyG@@4U*sci3yUY}&# z(>2NI`DF8t#+64bn;74U>GXRl3GTmJ`e@d#pUx5w7yCtRJ+_do=}29nfPrM!VYW#D zZ{oh3$km;4B6*+1ij1Rrk<7MBr@f!I!0p1s<*ZA3f6om0_+4>Qz)97l_@-#1HD0fK zO}VW0n$?=`I4@{@_UzUgv)QakmoBa;(Jd(K<95qW-Ocwwj`uqg_roU9o+0+o$meJZL?(4=k!O#)7mU1u%vTRzD)Oof4W5^%WU@)%>)#`rPc-aeo+HoU=w9?k!OkU7 zC+QYjcx-oSRs)08i8o4C~@f+N5Bd|9*M(S`@o!n-P~4)eeBn60?-s!LmQRg=LB5Wr_L5smU3Jq6Wfj%%LpIJUqUMd5P(%xv6<2&W>QcKw&N(DOWEiXGaC+ z{JfIH%)Hbhg}~Cx-#G(>$ULzAjLnBiIb7Mmj!zd)KnTbKv0y_g619_m?vdSzH z24W2&!FMVjZkKMaT=?dwx7yKzFN)@Fer3P~QXs;@%*4nF^Z<}8&B*v4#AQMA4|5Y^ zBSRDS@`tZ%k8f#T{6X6I%lTKo{jPjjd}%7%`=Zw;8TWKea(X`5{G)N@5z8jVw_-Z| zo=SrI@0LEA_3Njz#KXmYQCp8KWNSK7S14d0*>#w0Qox(IFDG(!r<_RMXR#vVs9q$q z?b2!Q=Phu%FmXBSlHT7lLq2|2oD^_UH7UL++Gvf}>t0hXtG#Bm<~z;{TAw|;wZ?2V zYtp5QYf5wr3j4U-@>6&7eURh*&cuDO>Uz^+`C0zYFI-Z1A*8iILt+Ov`&XxXe|Os~ z+4MR6QSr1kiyIDIY|J@H6DyowPd*anJ>#RcYNgBq0llwhyx9_$^34xAp|L&duL|?k z!uHJuDt5f*!61W!qd-Y-O27;*zv1HY-Q`UKj-f7 w_uo6PzS%R}+;Y Date: Thu, 28 Nov 2024 08:28:17 +0000 Subject: [PATCH 07/25] test: complete test for validate Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 2 +- revocation/crl/fetcher_test.go | 4 +- revocation/internal/crl/crl.go | 15 +- revocation/internal/crl/crl_test.go | 207 ++++++++++++++++++++++++++++ 4 files changed, 219 insertions(+), 9 deletions(-) diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index c04c5d6e..44ba4dd1 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -163,7 +163,7 @@ func (f *HTTPFetcher) processDeltaCRL(extensions *[]pkix.Extension) (*x509.Revoc if ext.Id.Equal(oidFreshestCRL) { cdp, err := parseFreshestCRL(ext) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse Freshest CRL extension: %w", err) } if len(cdp) == 0 { return nil, nil diff --git a/revocation/crl/fetcher_test.go b/revocation/crl/fetcher_test.go index 48732060..87fc0dd6 100644 --- a/revocation/crl/fetcher_test.go +++ b/revocation/crl/fetcher_test.go @@ -394,7 +394,7 @@ func TestProcessDeltaCRL(t *testing.T) { certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" extensions := loadExtentsion(certPath) _, err := fetcher.processDeltaCRL(extensions) - expectedErrorMsg := "x509: invalid CRL distribution point" + expectedErrorMsg := "failed to parse Freshest CRL extension: x509: invalid CRL distribution point" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) } @@ -438,7 +438,7 @@ func TestProcessDeltaCRL(t *testing.T) { certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" extensions := loadExtentsion(certPath) _, err := fetcher.fetch(context.Background(), "http://localhost.test", extensions) - expectedErrorMsg := "x509: invalid CRL distribution point" + expectedErrorMsg := "failed to parse Freshest CRL extension: x509: invalid CRL distribution point" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) } diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 70b7fed9..c5684ad8 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -190,15 +190,15 @@ func validate(bundle *crl.Bundle, issuer *x509.Certificate) error { // validate base CRL baseCRL := bundle.BaseCRL if err := validateCRL(baseCRL, issuer); err != nil { - return err + return fmt.Errorf("failed to validate base CRL: %w", err) } if bundle.DeltaCRL != nil { // validate delta CRL // RFC 5280, Section 5.2.4 deltaCRL := bundle.DeltaCRL - if err := validateCRL(bundle.DeltaCRL, issuer); err != nil { - return err + if err := validateCRL(deltaCRL, issuer); err != nil { + return fmt.Errorf("failed to validate delta CRL: %w", err) } if deltaCRL.Number.Cmp(baseCRL.Number) <= 0 { @@ -213,6 +213,7 @@ func validate(bundle *crl.Bundle, issuer *x509.Certificate) error { if !value.ReadASN1Integer(minimumBaseCRLNumber) { return errors.New("failed to parse delta CRL indicator extension") } + break } } if minimumBaseCRLNumber == nil { @@ -246,6 +247,8 @@ func validateCRL(crl *x509.RevocationList, issuer *x509.Certificate) error { // IssuingDistributionPoint is a critical extension that identifies // the scope of the CRL. Since we will check all the CRL // distribution points, it is not necessary to check this extension. + case ext.Id.Equal(oidDeltaCRLIndicator): + // will be checked in validate() default: if ext.Critical { // unsupported critical extensions is not allowed. (See RFC 5280, Section 5.2) @@ -269,9 +272,9 @@ func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Tim return nil, errors.New("baseCRL cannot be nil") } - entriesArray := []*[]x509.RevocationListEntry{&b.BaseCRL.RevokedCertificateEntries} + entriesBundle := []*[]x509.RevocationListEntry{&b.BaseCRL.RevokedCertificateEntries} if b.DeltaCRL != nil { - entriesArray = append(entriesArray, &b.DeltaCRL.RevokedCertificateEntries) + entriesBundle = append(entriesBundle, &b.DeltaCRL.RevokedCertificateEntries) } // latestTempRevokedEntry contains the most recent revocation entry with @@ -283,7 +286,7 @@ func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Tim var latestTempRevokedEntry *x509.RevocationListEntry // iterate over all the entries in the base and delta CRLs - for _, entries := range entriesArray { + for _, entries := range entriesBundle { for i, revocationEntry := range *entries { if revocationEntry.SerialNumber.Cmp(cert.SerialNumber) == 0 { extensions, err := parseEntryExtensions(revocationEntry) diff --git a/revocation/internal/crl/crl_test.go b/revocation/internal/crl/crl_test.go index 8d9b3b1e..2b9105d5 100644 --- a/revocation/internal/crl/crl_test.go +++ b/revocation/internal/crl/crl_test.go @@ -391,6 +391,213 @@ func TestValidate(t *testing.T) { t.Fatal(err) } }) + + chain := testhelper.GetRevokableRSAChainWithRevocations(1, false, true) + issuerCert := chain[0].Cert + issuerKey := chain[0].PrivateKey + + crlBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ + NextUpdate: time.Now().Add(time.Hour), + Number: big.NewInt(20240720), + }, issuerCert, issuerKey) + if err != nil { + t.Fatal(err) + } + + crl, err := x509.ParseRevocationList(crlBytes) + if err != nil { + t.Fatal(err) + } + + t.Run("valid crl and delta crl", func(t *testing.T) { + deltaCRLIndicator := big.NewInt(20240720) + deltaCRLIndicatorBytes, err := asn1.Marshal(deltaCRLIndicator) + if err != nil { + t.Fatal(err) + } + deltaCRLBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ + NextUpdate: time.Now().Add(time.Hour), + Number: big.NewInt(20240721), + ExtraExtensions: []pkix.Extension{ + { + Id: oidDeltaCRLIndicator, + Critical: true, + Value: deltaCRLIndicatorBytes, + }, + }, + }, issuerCert, issuerKey) + if err != nil { + t.Fatal(err) + } + deltaCRL, err := x509.ParseRevocationList(deltaCRLBytes) + if err != nil { + t.Fatal(err) + } + bundle := &crlutils.Bundle{ + BaseCRL: crl, + DeltaCRL: deltaCRL, + } + if err := validate(bundle, issuerCert); err != nil { + t.Fatal(err) + } + }) + + t.Run("invalid delta crl", func(t *testing.T) { + deltaCRLIndicator := big.NewInt(20240720) + deltaCRLIndicatorBytes, err := asn1.Marshal(deltaCRLIndicator) + if err != nil { + t.Fatal(err) + } + deltaCRLBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ + Number: big.NewInt(20240721), + ExtraExtensions: []pkix.Extension{ + { + Id: oidDeltaCRLIndicator, + Critical: true, + Value: deltaCRLIndicatorBytes, + }, + }, + }, issuerCert, issuerKey) + if err != nil { + t.Fatal(err) + } + deltaCRL, err := x509.ParseRevocationList(deltaCRLBytes) + if err != nil { + t.Fatal(err) + } + bundle := &crlutils.Bundle{ + BaseCRL: crl, + DeltaCRL: deltaCRL, + } + err = validate(bundle, issuerCert) + expectedErrorMsg := "failed to validate delta CRL: CRL NextUpdate is not set" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) + } + }) + + t.Run("invalid delta crl number", func(t *testing.T) { + deltaCRLIndicator := big.NewInt(20240720) + deltaCRLIndicatorBytes, err := asn1.Marshal(deltaCRLIndicator) + if err != nil { + t.Fatal(err) + } + deltaCRLBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ + NextUpdate: time.Now().Add(time.Hour), + Number: big.NewInt(20240719), + ExtraExtensions: []pkix.Extension{ + { + Id: oidDeltaCRLIndicator, + Critical: true, + Value: deltaCRLIndicatorBytes, + }, + }, + }, issuerCert, issuerKey) + if err != nil { + t.Fatal(err) + } + deltaCRL, err := x509.ParseRevocationList(deltaCRLBytes) + if err != nil { + t.Fatal(err) + } + bundle := &crlutils.Bundle{ + BaseCRL: crl, + DeltaCRL: deltaCRL, + } + err = validate(bundle, issuerCert) + expectedErrorMsg := "delta CRL number 20240719 is not greater than the base CRL number 20240720" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) + } + }) + + t.Run("delta crl without delta crl indicator", func(t *testing.T) { + deltaCRLBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ + NextUpdate: time.Now().Add(time.Hour), + Number: big.NewInt(20240721), + }, issuerCert, issuerKey) + if err != nil { + t.Fatal(err) + } + deltaCRL, err := x509.ParseRevocationList(deltaCRLBytes) + if err != nil { + t.Fatal(err) + } + bundle := &crlutils.Bundle{ + BaseCRL: crl, + DeltaCRL: deltaCRL, + } + err = validate(bundle, issuerCert) + expectedErrorMsg := "delta CRL indicator extension is not found" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) + } + }) + + t.Run("delta crl minimum base crl number is greater than base crl", func(t *testing.T) { + deltaCRLIndicator := big.NewInt(20240721) + deltaCRLIndicatorBytes, err := asn1.Marshal(deltaCRLIndicator) + if err != nil { + t.Fatal(err) + } + deltaCRLBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ + NextUpdate: time.Now().Add(time.Hour), + Number: big.NewInt(20240722), + ExtraExtensions: []pkix.Extension{ + { + Id: oidDeltaCRLIndicator, + Critical: true, + Value: deltaCRLIndicatorBytes, + }, + }, + }, issuerCert, issuerKey) + if err != nil { + t.Fatal(err) + } + deltaCRL, err := x509.ParseRevocationList(deltaCRLBytes) + if err != nil { + t.Fatal(err) + } + bundle := &crlutils.Bundle{ + BaseCRL: crl, + DeltaCRL: deltaCRL, + } + err = validate(bundle, issuerCert) + expectedErrorMsg := "delta CRL indicator 20240721 is not less than or equal to the base CRL number 20240720" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) + } + }) + + t.Run("delta crl with invalid delta indicator extension", func(t *testing.T) { + deltaCRLBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ + NextUpdate: time.Now().Add(time.Hour), + Number: big.NewInt(20240722), + ExtraExtensions: []pkix.Extension{ + { + Id: oidDeltaCRLIndicator, + Critical: true, + Value: []byte("invalid"), + }, + }, + }, issuerCert, issuerKey) + if err != nil { + t.Fatal(err) + } + deltaCRL, err := x509.ParseRevocationList(deltaCRLBytes) + if err != nil { + t.Fatal(err) + } + bundle := &crlutils.Bundle{ + BaseCRL: crl, + DeltaCRL: deltaCRL, + } + err = validate(bundle, issuerCert) + expectedErrorMsg := "failed to parse delta CRL indicator extension" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) + } + }) } func TestCheckRevocation(t *testing.T) { From c9f936f1b410bf4ebda191757752beb73bf9b3de Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Thu, 28 Nov 2024 09:02:50 +0000 Subject: [PATCH 08/25] test: add test cases Signed-off-by: Junjie Gao --- revocation/internal/crl/crl_test.go | 169 ++++++++++++++++++++++++++++ 1 file changed, 169 insertions(+) diff --git a/revocation/internal/crl/crl_test.go b/revocation/internal/crl/crl_test.go index 2b9105d5..53af4fc4 100644 --- a/revocation/internal/crl/crl_test.go +++ b/revocation/internal/crl/crl_test.go @@ -296,6 +296,74 @@ func TestCertCheckStatus(t *testing.T) { t.Fatalf("expected OK, got %s", r.Result) } }) + + t.Run("certificate with invalid freshest crl extension", func(t *testing.T) { + chain[0].Cert.Extensions = []pkix.Extension{ + { + Id: oidFreshestCRL, + Value: []byte("invalid"), + }, + } + + crlBytes, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ + NextUpdate: time.Now().Add(time.Hour), + Number: big.NewInt(20240720), + }, issuerCert, issuerKey) + if err != nil { + t.Fatal(err) + } + + fetcher, err := crlutils.NewHTTPFetcher( + &http.Client{Transport: expectedRoundTripperMock{Body: crlBytes}}, + ) + if err != nil { + t.Fatal(err) + } + fetcher.DiscardCacheError = true + r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ + Fetcher: fetcher, + }) + if r.Result != result.ResultUnknown { + t.Fatalf("expected Unknown, got %s", r.Result) + } + expectedErrorMsg := "failed to download CRL from http://localhost.test: failed to retrieve CRL: failed to parse Freshest CRL extension: x509: invalid CRL distribution points" + if r.ServerResults[0].Error == nil || r.ServerResults[0].Error.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, r.ServerResults[0].Error) + } + }) + + t.Run("fetcher doesn't support delta CRL", func(t *testing.T) { + chain[0].Cert.CRLDistributionPoints = []string{"http://localhost.test"} + chain[0].Cert.Extensions = []pkix.Extension{ + { + Id: oidFreshestCRL, + Value: []byte{0x00}, + }, + } + + fetcher := &fetcherMock{} + if err != nil { + t.Fatal(err) + } + r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ + Fetcher: fetcher, + }) + if r.Result != result.ResultUnknown { + t.Fatalf("expected Unknown, got %s", r.Result) + } + + expectedErrorMsg := "fetcher does not support fetching delta CRL from certificate extension" + if r.ServerResults[0].Error == nil || r.ServerResults[0].Error.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, r.ServerResults[0].Error) + } + }) + +} + +type fetcherMock struct{} + +func (f *fetcherMock) Fetch(ctx context.Context, url string) (*crlutils.Bundle, error) { + return nil, fmt.Errorf("fetch error") } func TestValidate(t *testing.T) { @@ -613,6 +681,14 @@ func TestCheckRevocation(t *testing.T) { } }) + t.Run("bundle is nil", func(t *testing.T) { + _, err := checkRevocation(cert, nil, signingTime, "") + expectedErrorMsg := "CRL bundle cannot be nil" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) + } + }) + t.Run("CRL is nil", func(t *testing.T) { _, err := checkRevocation(cert, &crlutils.Bundle{}, signingTime, "") if err == nil { @@ -758,6 +834,86 @@ func TestCheckRevocation(t *testing.T) { t.Fatal("expected error") } }) + + t.Run("delta crl with certificate hold", func(t *testing.T) { + baseCRL := &x509.RevocationList{} + deltaCRL := &x509.RevocationList{ + RevokedCertificateEntries: []x509.RevocationListEntry{ + { + SerialNumber: big.NewInt(1), + ReasonCode: reasonCodeCertificateHold, + }, + }, + } + r, err := checkRevocation(cert, &crlutils.Bundle{BaseCRL: baseCRL, DeltaCRL: deltaCRL}, signingTime, "") + if err != nil { + t.Fatal(err) + } + if r.Result != result.ResultRevoked { + t.Fatalf("expected revoked, got %s", r.Result) + } + }) + + t.Run("certificate hold and remove hold", func(t *testing.T) { + baseCRL := &x509.RevocationList{ + RevokedCertificateEntries: []x509.RevocationListEntry{ + { + SerialNumber: big.NewInt(1), + ReasonCode: reasonCodeCertificateHold, + RevocationTime: time.Now().Add(-time.Hour), + }, + }, + } + deltaCRL := &x509.RevocationList{ + RevokedCertificateEntries: []x509.RevocationListEntry{ + { + SerialNumber: big.NewInt(1), + ReasonCode: reasonCodeRemoveFromCRL, + RevocationTime: time.Now(), + }, + }, + } + r, err := checkRevocation(cert, &crlutils.Bundle{BaseCRL: baseCRL, DeltaCRL: deltaCRL}, signingTime, "") + if err != nil { + t.Fatal(err) + } + if r.Result != result.ResultOK { + t.Fatalf("expected OK, got %s", r.Result) + } + }) + + t.Run("certificate hold, remove hold and hold again", func(t *testing.T) { + baseCRL := &x509.RevocationList{ + RevokedCertificateEntries: []x509.RevocationListEntry{ + { + SerialNumber: big.NewInt(1), + ReasonCode: reasonCodeCertificateHold, + RevocationTime: time.Now().Add(-2 * time.Hour), + }, + }, + } + deltaCRL := &x509.RevocationList{ + RevokedCertificateEntries: []x509.RevocationListEntry{ + { + SerialNumber: big.NewInt(1), + ReasonCode: reasonCodeRemoveFromCRL, + RevocationTime: time.Now().Add(-time.Hour), + }, + { + SerialNumber: big.NewInt(1), + ReasonCode: reasonCodeCertificateHold, + RevocationTime: time.Now(), + }, + }, + } + r, err := checkRevocation(cert, &crlutils.Bundle{BaseCRL: baseCRL, DeltaCRL: deltaCRL}, signingTime, "") + if err != nil { + t.Fatal(err) + } + if r.Result != result.ResultRevoked { + t.Fatalf("expected revoked, got %s", r.Result) + } + }) } func TestParseEntryExtension(t *testing.T) { @@ -883,6 +1039,19 @@ func TestSupported(t *testing.T) { }) } +func TestHasDeltaCRL(t *testing.T) { + cert := &x509.Certificate{ + Extensions: []pkix.Extension{ + { + Id: oidFreshestCRL, + }, + }, + } + if !hasDeltaCRL(cert) { + t.Fatal("expected has delta CRL") + } +} + type errorRoundTripperMock struct{} func (rt errorRoundTripperMock) RoundTrip(req *http.Request) (*http.Response, error) { From 483d298c276957784a09a21d2886b0393486b454 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Thu, 5 Dec 2024 08:05:42 +0000 Subject: [PATCH 09/25] fix: only support delta CRL url from base CRL Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 66 ++++++++--------- revocation/crl/fetcher_test.go | 105 ++++++++++++++++------------ revocation/internal/crl/crl.go | 50 +++++++------ revocation/internal/crl/crl_test.go | 36 ++-------- 4 files changed, 121 insertions(+), 136 deletions(-) diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index 44ba4dd1..4f31620e 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -48,15 +48,6 @@ type Fetcher interface { Fetch(ctx context.Context, url string) (*Bundle, error) } -// FetcherWithCertificateExtensions is an interface that specifies methods used -// for fetching CRL from the given URL with certificate extensions to identify -// the Freshest CRL extension (Delta CRL) from certificate. -type FetcherWithCertificateExtensions interface { - // FetchWithCertificateExtensions retrieves the CRL from the given URL with - // certificate extensions. - FetchWithCertificateExtensions(ctx context.Context, url string, certificateExtensions *[]pkix.Extension) (*Bundle, error) -} - // HTTPFetcher is a Fetcher implementation that fetches CRL from the given URL type HTTPFetcher struct { // Cache stores fetched CRLs and reuses them until the CRL reaches the @@ -90,10 +81,6 @@ func NewHTTPFetcher(httpClient *http.Client) (*HTTPFetcher, error) { // (e.g. cache miss), it will download the CRL from the URL and store it to the // cache. func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*Bundle, error) { - return f.FetchWithCertificateExtensions(ctx, url, nil) -} - -func (f *HTTPFetcher) FetchWithCertificateExtensions(ctx context.Context, url string, certificateExtensions *[]pkix.Extension) (*Bundle, error) { if url == "" { return nil, errors.New("CRL URL cannot be empty") } @@ -111,7 +98,7 @@ func (f *HTTPFetcher) FetchWithCertificateExtensions(ctx context.Context, url st } } - bundle, err := f.fetch(ctx, url, certificateExtensions) + bundle, err := f.fetch(ctx, url) if err != nil { return nil, fmt.Errorf("failed to retrieve CRL: %w", err) } @@ -132,25 +119,18 @@ func isEffective(crl *x509.RevocationList) bool { } // fetch downloads the CRL from the given URL. -func (f *HTTPFetcher) fetch(ctx context.Context, url string, certificateExtensions *[]pkix.Extension) (*Bundle, error) { +func (f *HTTPFetcher) fetch(ctx context.Context, url string) (*Bundle, error) { // fetch base CRL base, err := fetchCRL(ctx, url, f.httpClient) if err != nil { return nil, err } - // fetch delta CRL from CRL extension + // fetch delta CRL from base CRL extension deltaCRL, err := f.processDeltaCRL(&base.Extensions) if err != nil { return nil, err } - if certificateExtensions != nil && deltaCRL == nil { - // fallback to fetch delta CRL from certificate extension - deltaCRL, err = f.processDeltaCRL(certificateExtensions) - if err != nil { - return nil, err - } - } return &Bundle{ BaseCRL: base, @@ -158,34 +138,46 @@ func (f *HTTPFetcher) fetch(ctx context.Context, url string, certificateExtensio }, nil } +// processDeltaCRL processes the delta CRL from the given extensions of base CRL. func (f *HTTPFetcher) processDeltaCRL(extensions *[]pkix.Extension) (*x509.RevocationList, error) { for _, ext := range *extensions { if ext.Id.Equal(oidFreshestCRL) { - cdp, err := parseFreshestCRL(ext) + // RFC 5280, 4.2.1.15 + // id-ce-freshestCRL OBJECT IDENTIFIER ::= { id-ce 46 } + // + // FreshestCRL ::= CRLDistributionPoints + urls, err := parseCRLDistributionPoint(ext.Value) if err != nil { return nil, fmt.Errorf("failed to parse Freshest CRL extension: %w", err) } - if len(cdp) == 0 { + if len(urls) == 0 { return nil, nil } - if len(cdp) > 1 { - // to simplify the implementation, we only support one - // Freshest CRL URL for now which is the common case - return nil, errors.New("multiple Freshest CRL distribution points are not supported") + + var lastError error + var deltaCRL *x509.RevocationList + for _, cdpURL := range urls { + // Delta CRLs from the base CRL have the same scope as the base + // CRL, so the URLs are for redundancy and should be tried in + // order until one succeeds. + deltaCRL, lastError = fetchCRL(context.Background(), cdpURL, f.httpClient) + if lastError != nil { + continue + } + return deltaCRL, nil } - return fetchCRL(context.Background(), cdp[0], f.httpClient) + return nil, lastError } } return nil, nil } -// parseFreshestCRL parses the Freshest CRL extension and returns the CRL URLs -func parseFreshestCRL(ext pkix.Extension) ([]string, error) { +// parseCRLDistributionPoint parses the CRL extension and returns the CRL URLs +// +// value is the raw value of the CRL distribution point extension +func parseCRLDistributionPoint(value []byte) ([]string, error) { var cdp []string - // RFC 5280, 4.2.1.15 - // id-ce-freshestCRL OBJECT IDENTIFIER ::= { id-ce 46 } - // - // FreshestCRL ::= CRLDistributionPoints + // borrowed from crypto/x509: https://cs.opensource.google/go/go/+/refs/tags/go1.23.4:src/crypto/x509/parser.go;l=700-743 // // RFC 5280, 4.2.1.13 // @@ -199,7 +191,7 @@ func parseFreshestCRL(ext pkix.Extension) ([]string, error) { // DistributionPointName ::= CHOICE { // fullName [0] GeneralNames, // nameRelativeToCRLIssuer [1] RelativeDistinguishedName } - val := cryptobyte.String(ext.Value) + val := cryptobyte.String(value) if !val.ReadASN1(&val, cryptobyte_asn1.SEQUENCE) { return nil, errors.New("x509: invalid CRL distribution points") } diff --git a/revocation/crl/fetcher_test.go b/revocation/crl/fetcher_test.go index 87fc0dd6..cf58ff27 100644 --- a/revocation/crl/fetcher_test.go +++ b/revocation/crl/fetcher_test.go @@ -80,7 +80,7 @@ func TestFetch(t *testing.T) { t.Run("fetch without cache", func(t *testing.T) { httpClient := &http.Client{ - Transport: expectedRoundTripperMock{Body: baseCRL.Raw}, + Transport: &expectedRoundTripperMock{Body: baseCRL.Raw}, } f, err := NewHTTPFetcher(httpClient) if err != nil { @@ -136,7 +136,7 @@ func TestFetch(t *testing.T) { t.Run("cache miss", func(t *testing.T) { c := &memoryCache{} httpClient := &http.Client{ - Transport: expectedRoundTripperMock{Body: baseCRL.Raw}, + Transport: &expectedRoundTripperMock{Body: baseCRL.Raw}, } f, err := NewHTTPFetcher(httpClient) if err != nil { @@ -179,7 +179,7 @@ func TestFetch(t *testing.T) { // fetch the expired CRL httpClient := &http.Client{ - Transport: expectedRoundTripperMock{Body: baseCRL.Raw}, + Transport: &expectedRoundTripperMock{Body: baseCRL.Raw}, } f, err := NewHTTPFetcher(httpClient) if err != nil { @@ -203,7 +203,7 @@ func TestFetch(t *testing.T) { SetError: errors.New("cache error"), } httpClient := &http.Client{ - Transport: expectedRoundTripperMock{Body: baseCRL.Raw}, + Transport: &expectedRoundTripperMock{Body: baseCRL.Raw}, } f, err := NewHTTPFetcher(httpClient) if err != nil { @@ -225,7 +225,7 @@ func TestFetch(t *testing.T) { GetError: errors.New("cache error"), } httpClient := &http.Client{ - Transport: expectedRoundTripperMock{Body: baseCRL.Raw}, + Transport: &expectedRoundTripperMock{Body: baseCRL.Raw}, } f, err := NewHTTPFetcher(httpClient) if err != nil { @@ -245,7 +245,7 @@ func TestFetch(t *testing.T) { SetError: errors.New("cache error"), } httpClient := &http.Client{ - Transport: expectedRoundTripperMock{Body: baseCRL.Raw}, + Transport: &expectedRoundTripperMock{Body: baseCRL.Raw}, } f, err := NewHTTPFetcher(httpClient) if err != nil { @@ -258,6 +258,28 @@ func TestFetch(t *testing.T) { t.Errorf("Fetcher.Fetch() error = %v, want failed to store CRL to cache:", err) } }) + + t.Run("test fetch delta CRL from base CRL extension failed", func(t *testing.T) { + crlWithDeltaCRL, err := os.ReadFile("testdata/crlWithMultipleFreshestCRLs.crl") + if err != nil { + t.Fatalf("failed to read CRL: %v", err) + } + httpClient := &http.Client{ + Transport: &expectedRoundTripperMock{ + Body: crlWithDeltaCRL, + SecondRoundBody: []byte("invalid crl"), + }, + } + f, err := NewHTTPFetcher(httpClient) + if err != nil { + t.Errorf("NewHTTPFetcher() error = %v, want nil", err) + } + _, err = f.Fetch(context.Background(), exampleURL) + expectedErrorMsg := "failed to retrieve CRL: x509: malformed crl" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) + } + }) } func TestParseFreshestCRL(t *testing.T) { @@ -290,7 +312,7 @@ func TestParseFreshestCRL(t *testing.T) { t.Run("valid 1 delta CRL URL", func(t *testing.T) { certPath := "testdata/certificateWithDeltaCRL.cer" freshestCRLExtension := loadExtentsion(certPath) - urls, err := parseFreshestCRL(freshestCRLExtension) + urls, err := parseCRLDistributionPoint(freshestCRLExtension.Value) if err != nil { t.Fatalf("failed to parse freshest CRL: %v", err) } @@ -305,7 +327,7 @@ func TestParseFreshestCRL(t *testing.T) { }) t.Run("empty extension", func(t *testing.T) { - _, err := parseFreshestCRL(pkix.Extension{}) + _, err := parseCRLDistributionPoint(nil) if err == nil { t.Fatalf("expected error") } @@ -314,7 +336,7 @@ func TestParseFreshestCRL(t *testing.T) { t.Run("URL doesn't exist", func(t *testing.T) { certPath := "testdata/certificateWithZeroDeltaCRLURL.cer" freshestCRLExtension := loadExtentsion(certPath) - url, err := parseFreshestCRL(freshestCRLExtension) + url, err := parseCRLDistributionPoint(freshestCRLExtension.Value) if err != nil { t.Fatalf("failed to parse freshest CRL: %v", err) } @@ -326,7 +348,7 @@ func TestParseFreshestCRL(t *testing.T) { t.Run("non URI freshest CRL extension", func(t *testing.T) { certPath := "testdata/certificateWithNonURIDeltaCRL.cer" freshestCRLExtension := loadExtentsion(certPath) - url, err := parseFreshestCRL(freshestCRLExtension) + url, err := parseCRLDistributionPoint(freshestCRLExtension.Value) if err != nil { t.Fatalf("failed to parse freshest CRL: %v", err) } @@ -338,7 +360,7 @@ func TestParseFreshestCRL(t *testing.T) { t.Run("certificate with incomplete freshest CRL extension", func(t *testing.T) { certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" freshestCRLExtension := loadExtentsion(certPath) - _, err := parseFreshestCRL(freshestCRLExtension) + _, err := parseCRLDistributionPoint(freshestCRLExtension.Value) expectErrorMsg := "x509: invalid CRL distribution point" if err == nil || err.Error() != expectErrorMsg { t.Fatalf("expected error %q, got %v", expectErrorMsg, err) @@ -348,7 +370,7 @@ func TestParseFreshestCRL(t *testing.T) { t.Run("certificate with incomplete freshest CRL extension2", func(t *testing.T) { certPath := "testdata/certificateWithIncompleteFreshestCRL2.cer" freshestCRLExtension := loadExtentsion(certPath) - url, err := parseFreshestCRL(freshestCRLExtension) + url, err := parseCRLDistributionPoint(freshestCRLExtension.Value) if err != nil { t.Fatalf("failed to parse freshest CRL: %v", err) } @@ -384,7 +406,7 @@ func TestProcessDeltaCRL(t *testing.T) { } fetcher, err := NewHTTPFetcher(&http.Client{ - Transport: expectedRoundTripperMock{Body: deltaCRL}, + Transport: &expectedRoundTripperMock{Body: deltaCRL}, }) if err != nil { t.Fatalf("failed to create fetcher: %v", err) @@ -424,12 +446,18 @@ func TestProcessDeltaCRL(t *testing.T) { } }) - t.Run("multiple freshest CRL URLs", func(t *testing.T) { + t.Run("multiple freshest CRL URLs failed", func(t *testing.T) { + fetcherWithError, err := NewHTTPFetcher(&http.Client{ + Transport: errorRoundTripperMock{}, + }) + if err != nil { + t.Fatalf("failed to create fetcher: %v", err) + } certPath := "testdata/certificateWith2DeltaCRL.cer" extensions := loadExtentsion(certPath) - _, err := fetcher.processDeltaCRL(extensions) - expectedErrorMsg := "multiple Freshest CRL distribution points are not supported" - if err == nil || err.Error() != expectedErrorMsg { + _, err = fetcherWithError.processDeltaCRL(extensions) + expectedErrorMsg := "request failed" + if err == nil || !strings.Contains(err.Error(), expectedErrorMsg) { t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) } }) @@ -437,33 +465,12 @@ func TestProcessDeltaCRL(t *testing.T) { t.Run("process delta crl from certificate extension failed", func(t *testing.T) { certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" extensions := loadExtentsion(certPath) - _, err := fetcher.fetch(context.Background(), "http://localhost.test", extensions) + _, err := fetcher.processDeltaCRL(extensions) expectedErrorMsg := "failed to parse Freshest CRL extension: x509: invalid CRL distribution point" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) } }) - - crlFile, err := os.ReadFile("testdata/crlWithMultipleFreshestCRLs.crl") - if err != nil { - t.Fatalf("failed to read CRL file: %v", err) - } - fetcher, err = NewHTTPFetcher(&http.Client{ - Transport: expectedRoundTripperMock{Body: crlFile}, - }) - if err != nil { - t.Fatalf("failed to create fetcher: %v", err) - } - - t.Run("fetch delta CRL from CRL failed", func(t *testing.T) { - certPath := "testdata/certificateWithDeltaCRL.cer" - extensions := loadExtentsion(certPath) - _, err := fetcher.fetch(context.Background(), "http://localhost.test", extensions) - expectErrorMsg := "multiple Freshest CRL distribution points are not supported" - if err == nil || err.Error() != expectErrorMsg { - t.Fatalf("expected error %q, got %v", expectErrorMsg, err) - } - }) } func TestDownload(t *testing.T) { @@ -518,7 +525,7 @@ func TestDownload(t *testing.T) { t.Run("exceed the size limit", func(t *testing.T) { _, err := fetchCRL(context.Background(), "http://localhost.test", &http.Client{ - Transport: expectedRoundTripperMock{Body: make([]byte, maxCRLSize+1)}, + Transport: &expectedRoundTripperMock{Body: make([]byte, maxCRLSize+1)}, }) if err == nil { t.Fatal("expected error") @@ -527,7 +534,7 @@ func TestDownload(t *testing.T) { t.Run("invalid crl", func(t *testing.T) { _, err := fetchCRL(context.Background(), "http://localhost.test", &http.Client{ - Transport: expectedRoundTripperMock{Body: []byte("invalid crl")}, + Transport: &expectedRoundTripperMock{Body: []byte("invalid crl")}, }) if err == nil { t.Fatal("expected error") @@ -570,14 +577,24 @@ func (r errorReaderMock) Close() error { } type expectedRoundTripperMock struct { - Body []byte + Body []byte + SecondRoundBody []byte + count int } -func (rt expectedRoundTripperMock) RoundTrip(req *http.Request) (*http.Response, error) { +func (rt *expectedRoundTripperMock) RoundTrip(req *http.Request) (*http.Response, error) { + if rt.count == 0 { + rt.count += 1 + return &http.Response{ + Request: req, + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBuffer(rt.Body)), + }, nil + } return &http.Response{ Request: req, StatusCode: http.StatusOK, - Body: io.NopCloser(bytes.NewBuffer(rt.Body)), + Body: io.NopCloser(bytes.NewBuffer(rt.SecondRoundBody)), }, nil } diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index c5684ad8..899f13df 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -18,6 +18,7 @@ package crl import ( "context" "crypto/x509" + "crypto/x509/pkix" "encoding/asn1" "errors" "fmt" @@ -92,10 +93,10 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C } var ( - serverResults = make([]*result.ServerResult, 0, len(cert.CRLDistributionPoints)) - lastErr error - crlURL string - hasDeltaCRL = hasDeltaCRL(cert) + serverResults = make([]*result.ServerResult, 0, len(cert.CRLDistributionPoints)) + lastErr error + crlURL string + hasFreshestCRLInCertificate = hasFreshestCRL(&cert.Extensions) ) // The CRLDistributionPoints contains the URIs of all the CRL distribution @@ -110,23 +111,26 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C bundle *crl.Bundle err error ) - if !hasDeltaCRL { - bundle, err = opts.Fetcher.Fetch(ctx, crlURL) - if err != nil { - lastErr = fmt.Errorf("failed to download CRL from %s: %w", crlURL, err) - break - } - } else { - fetcher, ok := opts.Fetcher.(crl.FetcherWithCertificateExtensions) - if !ok { - lastErr = fmt.Errorf("fetcher does not support fetching delta CRL from certificate extension") - break - } - bundle, err = fetcher.FetchWithCertificateExtensions(ctx, crlURL, &cert.Extensions) - if err != nil { - lastErr = fmt.Errorf("failed to download CRL from %s: %w", crlURL, err) - break - } + bundle, err = opts.Fetcher.Fetch(ctx, crlURL) + if err != nil { + lastErr = fmt.Errorf("failed to download CRL from %s: %w", crlURL, err) + break + } + + if hasFreshestCRLInCertificate && bundle.DeltaCRL == nil { + // deltaCRL URL in cert deltaCRL URL in baseCRL support it? + // --------------------------- ----------------------- --------- + // True True Yes + // True False No + // False True Yes + // False False Yes + // + // if only the certificate has the freshest CRL, the bundle.DeltaCRL + // should be nil. We don't support this case now because the delta + // CRLs may have different scopes, but the Go built-in function + // skips the scope of the base CRL when parsing the certificate. + lastErr = errors.New("freshest CRL from certificate extension is not supported") + break } if err = validate(bundle, issuer); err != nil { @@ -177,8 +181,8 @@ func Supported(cert *x509.Certificate) bool { return cert != nil && len(cert.CRLDistributionPoints) > 0 } -func hasDeltaCRL(cert *x509.Certificate) bool { - for _, ext := range cert.Extensions { +func hasFreshestCRL(extensions *[]pkix.Extension) bool { + for _, ext := range *extensions { if ext.Id.Equal(oidFreshestCRL) { return true } diff --git a/revocation/internal/crl/crl_test.go b/revocation/internal/crl/crl_test.go index 53af4fc4..f14be30a 100644 --- a/revocation/internal/crl/crl_test.go +++ b/revocation/internal/crl/crl_test.go @@ -297,11 +297,10 @@ func TestCertCheckStatus(t *testing.T) { } }) - t.Run("certificate with invalid freshest crl extension", func(t *testing.T) { + t.Run("freshest CRL from certificate extension is not supported", func(t *testing.T) { chain[0].Cert.Extensions = []pkix.Extension{ { - Id: oidFreshestCRL, - Value: []byte("invalid"), + Id: oidFreshestCRL, }, } @@ -326,38 +325,11 @@ func TestCertCheckStatus(t *testing.T) { if r.Result != result.ResultUnknown { t.Fatalf("expected Unknown, got %s", r.Result) } - expectedErrorMsg := "failed to download CRL from http://localhost.test: failed to retrieve CRL: failed to parse Freshest CRL extension: x509: invalid CRL distribution points" - if r.ServerResults[0].Error == nil || r.ServerResults[0].Error.Error() != expectedErrorMsg { - t.Fatalf("expected error %q, got %v", expectedErrorMsg, r.ServerResults[0].Error) - } - }) - - t.Run("fetcher doesn't support delta CRL", func(t *testing.T) { - chain[0].Cert.CRLDistributionPoints = []string{"http://localhost.test"} - chain[0].Cert.Extensions = []pkix.Extension{ - { - Id: oidFreshestCRL, - Value: []byte{0x00}, - }, - } - - fetcher := &fetcherMock{} - if err != nil { - t.Fatal(err) - } - r := CertCheckStatus(context.Background(), chain[0].Cert, issuerCert, CertCheckStatusOptions{ - Fetcher: fetcher, - }) - if r.Result != result.ResultUnknown { - t.Fatalf("expected Unknown, got %s", r.Result) - } - - expectedErrorMsg := "fetcher does not support fetching delta CRL from certificate extension" + expectedErrorMsg := "freshest CRL from certificate extension is not supported" if r.ServerResults[0].Error == nil || r.ServerResults[0].Error.Error() != expectedErrorMsg { t.Fatalf("expected error %q, got %v", expectedErrorMsg, r.ServerResults[0].Error) } }) - } type fetcherMock struct{} @@ -1047,7 +1019,7 @@ func TestHasDeltaCRL(t *testing.T) { }, }, } - if !hasDeltaCRL(cert) { + if !hasFreshestCRL(&cert.Extensions) { t.Fatal("expected has delta CRL") } } From 2910e31dc0e98a747e29b36e8c5a9356b6e71104 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Thu, 5 Dec 2024 08:16:01 +0000 Subject: [PATCH 10/25] fix: update Signed-off-by: Junjie Gao --- revocation/internal/crl/crl.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 899f13df..1db42723 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -120,10 +120,10 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C if hasFreshestCRLInCertificate && bundle.DeltaCRL == nil { // deltaCRL URL in cert deltaCRL URL in baseCRL support it? // --------------------------- ----------------------- --------- - // True True Yes - // True False No - // False True Yes - // False False Yes + // True True Yes + // True False No + // False True Yes + // False False Yes // // if only the certificate has the freshest CRL, the bundle.DeltaCRL // should be nil. We don't support this case now because the delta From 934698a9d117182a482f7115dad5dcf2e4fec5cc Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 9 Dec 2024 03:06:25 +0000 Subject: [PATCH 11/25] fix: improve code style Signed-off-by: Junjie Gao --- revocation/crl/errors.go | 3 +++ revocation/crl/fetcher.go | 27 ++++++++++++++++----------- revocation/crl/fetcher_test.go | 20 +++++++++----------- revocation/internal/crl/crl.go | 18 +++++++----------- revocation/internal/crl/crl_test.go | 6 ------ 5 files changed, 35 insertions(+), 39 deletions(-) diff --git a/revocation/crl/errors.go b/revocation/crl/errors.go index a1978910..24217d39 100644 --- a/revocation/crl/errors.go +++ b/revocation/crl/errors.go @@ -17,3 +17,6 @@ import "errors" // ErrCacheMiss is returned when a cache miss occurs. var ErrCacheMiss = errors.New("cache miss") + +// errDeltaCRLNotFound is returned when a delta CRL is not found. +var errDeltaCRLNotFound = errors.New("delta CRL not found") diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index 4f31620e..70d81a16 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -127,8 +127,8 @@ func (f *HTTPFetcher) fetch(ctx context.Context, url string) (*Bundle, error) { } // fetch delta CRL from base CRL extension - deltaCRL, err := f.processDeltaCRL(&base.Extensions) - if err != nil { + deltaCRL, err := f.fetchDeltaCRL(&base.Extensions) + if err != nil && !errors.Is(err, errDeltaCRLNotFound) { return nil, err } @@ -138,8 +138,10 @@ func (f *HTTPFetcher) fetch(ctx context.Context, url string) (*Bundle, error) { }, nil } -// processDeltaCRL processes the delta CRL from the given extensions of base CRL. -func (f *HTTPFetcher) processDeltaCRL(extensions *[]pkix.Extension) (*x509.RevocationList, error) { +// fetchDeltaCRL fetches the delta CRL from the given extensions of base CRL. +// +// It returns errDeltaCRLNotFound if the delta CRL is not found. +func (f *HTTPFetcher) fetchDeltaCRL(extensions *[]pkix.Extension) (*x509.RevocationList, error) { for _, ext := range *extensions { if ext.Id.Equal(oidFreshestCRL) { // RFC 5280, 4.2.1.15 @@ -151,12 +153,15 @@ func (f *HTTPFetcher) processDeltaCRL(extensions *[]pkix.Extension) (*x509.Revoc return nil, fmt.Errorf("failed to parse Freshest CRL extension: %w", err) } if len(urls) == 0 { - return nil, nil + return nil, errDeltaCRLNotFound } - var lastError error - var deltaCRL *x509.RevocationList + var ( + lastError error + deltaCRL *x509.RevocationList + ) for _, cdpURL := range urls { + // RFC 5280, 5.2.6 // Delta CRLs from the base CRL have the same scope as the base // CRL, so the URLs are for redundancy and should be tried in // order until one succeeds. @@ -169,14 +174,14 @@ func (f *HTTPFetcher) processDeltaCRL(extensions *[]pkix.Extension) (*x509.Revoc return nil, lastError } } - return nil, nil + return nil, errDeltaCRLNotFound } // parseCRLDistributionPoint parses the CRL extension and returns the CRL URLs // // value is the raw value of the CRL distribution point extension func parseCRLDistributionPoint(value []byte) ([]string, error) { - var cdp []string + var urls []string // borrowed from crypto/x509: https://cs.opensource.google/go/go/+/refs/tags/go1.23.4:src/crypto/x509/parser.go;l=700-743 // // RFC 5280, 4.2.1.13 @@ -219,10 +224,10 @@ func parseCRLDistributionPoint(value []byte) ([]string, error) { if !dpNameDER.ReadASN1(&uri, cryptobyte_asn1.Tag(6).ContextSpecific()) { return nil, errors.New("x509: invalid CRL distribution point") } - cdp = append(cdp, string(uri)) + urls = append(urls, string(uri)) } } - return cdp, nil + return urls, nil } func fetchCRL(ctx context.Context, crlURL string, client *http.Client) (*x509.RevocationList, error) { diff --git a/revocation/crl/fetcher_test.go b/revocation/crl/fetcher_test.go index cf58ff27..460cbd14 100644 --- a/revocation/crl/fetcher_test.go +++ b/revocation/crl/fetcher_test.go @@ -380,7 +380,7 @@ func TestParseFreshestCRL(t *testing.T) { }) } -func TestProcessDeltaCRL(t *testing.T) { +func TestFetchDeltaCRL(t *testing.T) { loadExtentsion := func(certPath string) *[]pkix.Extension { certData, err := os.ReadFile(certPath) if err != nil { @@ -415,7 +415,7 @@ func TestProcessDeltaCRL(t *testing.T) { t.Run("parse freshest CRL failed", func(t *testing.T) { certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" extensions := loadExtentsion(certPath) - _, err := fetcher.processDeltaCRL(extensions) + _, err := fetcher.fetchDeltaCRL(extensions) expectedErrorMsg := "failed to parse Freshest CRL extension: x509: invalid CRL distribution point" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) @@ -425,19 +425,17 @@ func TestProcessDeltaCRL(t *testing.T) { t.Run("zero freshest CRL URL", func(t *testing.T) { certPath := "testdata/certificateWithZeroDeltaCRLURL.cer" extensions := loadExtentsion(certPath) - deltaCRL, err := fetcher.processDeltaCRL(extensions) - if err != nil { - t.Fatalf("failed to process delta CRL: %v", err) - } - if deltaCRL != nil { - t.Fatalf("expected nil delta CRL, got %v", deltaCRL) + _, err := fetcher.fetchDeltaCRL(extensions) + expectedErr := errDeltaCRLNotFound + if err == nil || !errors.Is(err, expectedErr) { + t.Fatalf("expected error %v, got %v", expectedErr, err) } }) t.Run("one freshest CRL URL", func(t *testing.T) { certPath := "testdata/certificateWithDeltaCRL.cer" extensions := loadExtentsion(certPath) - deltaCRL, err := fetcher.processDeltaCRL(extensions) + deltaCRL, err := fetcher.fetchDeltaCRL(extensions) if err != nil { t.Fatalf("failed to process delta CRL: %v", err) } @@ -455,7 +453,7 @@ func TestProcessDeltaCRL(t *testing.T) { } certPath := "testdata/certificateWith2DeltaCRL.cer" extensions := loadExtentsion(certPath) - _, err = fetcherWithError.processDeltaCRL(extensions) + _, err = fetcherWithError.fetchDeltaCRL(extensions) expectedErrorMsg := "request failed" if err == nil || !strings.Contains(err.Error(), expectedErrorMsg) { t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) @@ -465,7 +463,7 @@ func TestProcessDeltaCRL(t *testing.T) { t.Run("process delta crl from certificate extension failed", func(t *testing.T) { certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" extensions := loadExtentsion(certPath) - _, err := fetcher.processDeltaCRL(extensions) + _, err := fetcher.fetchDeltaCRL(extensions) expectedErrorMsg := "failed to parse Freshest CRL extension: x509: invalid CRL distribution point" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 1db42723..a842b4bc 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -107,23 +107,19 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C // point with one CRL URI, which will be cached, so checking all the URIs is // not a performance issue. for _, crlURL = range cert.CRLDistributionPoints { - var ( - bundle *crl.Bundle - err error - ) - bundle, 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 hasFreshestCRLInCertificate && bundle.DeltaCRL == nil { - // deltaCRL URL in cert deltaCRL URL in baseCRL support it? - // --------------------------- ----------------------- --------- - // True True Yes - // True False No - // False True Yes - // False False Yes + // deltaCRL URL in cert deltaCRL URL in baseCRL support it? + // --------------------------- ----------------------- ------------- + // True True Yes + // True False No + // False True Yes + // False False Yes // // if only the certificate has the freshest CRL, the bundle.DeltaCRL // should be nil. We don't support this case now because the delta diff --git a/revocation/internal/crl/crl_test.go b/revocation/internal/crl/crl_test.go index f14be30a..a25a5585 100644 --- a/revocation/internal/crl/crl_test.go +++ b/revocation/internal/crl/crl_test.go @@ -332,12 +332,6 @@ func TestCertCheckStatus(t *testing.T) { }) } -type fetcherMock struct{} - -func (f *fetcherMock) Fetch(ctx context.Context, url string) (*crlutils.Bundle, error) { - return nil, fmt.Errorf("fetch error") -} - func TestValidate(t *testing.T) { t.Run("expired CRL", func(t *testing.T) { chain := testhelper.GetRevokableRSAChainWithRevocations(1, false, true) From cc34d0d9ecc5fa83543f783484113c6ee0c26677 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 9 Dec 2024 03:32:07 +0000 Subject: [PATCH 12/25] fix: update Signed-off-by: Junjie Gao --- revocation/internal/crl/crl.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index a842b4bc..0a5a6f9b 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -272,9 +272,9 @@ func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Tim return nil, errors.New("baseCRL cannot be nil") } - entriesBundle := []*[]x509.RevocationListEntry{&b.BaseCRL.RevokedCertificateEntries} + revocationListBundle := []*[]x509.RevocationListEntry{&b.BaseCRL.RevokedCertificateEntries} if b.DeltaCRL != nil { - entriesBundle = append(entriesBundle, &b.DeltaCRL.RevokedCertificateEntries) + revocationListBundle = append(revocationListBundle, &b.DeltaCRL.RevokedCertificateEntries) } // latestTempRevokedEntry contains the most recent revocation entry with @@ -286,8 +286,8 @@ func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Tim var latestTempRevokedEntry *x509.RevocationListEntry // iterate over all the entries in the base and delta CRLs - for _, entries := range entriesBundle { - for i, revocationEntry := range *entries { + for _, revocationList := range revocationListBundle { + for i, revocationEntry := range *revocationList { if revocationEntry.SerialNumber.Cmp(cert.SerialNumber) == 0 { extensions, err := parseEntryExtensions(revocationEntry) if err != nil { @@ -307,7 +307,7 @@ func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Tim // temporarily revoked or unrevoked if latestTempRevokedEntry == nil || latestTempRevokedEntry.RevocationTime.Before(revocationEntry.RevocationTime) { // the revocation status depends on the most recent reason - latestTempRevokedEntry = &(*entries)[i] + latestTempRevokedEntry = &(*revocationList)[i] } default: // permanently revoked From 630c7a56b5c6e3d8a941694cc1102aaa3438ceaf Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 9 Dec 2024 05:33:00 +0000 Subject: [PATCH 13/25] fix: simplify code Signed-off-by: Junjie Gao --- revocation/internal/crl/const.go | 32 -------------------------------- revocation/internal/crl/crl.go | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 32 deletions(-) delete mode 100644 revocation/internal/crl/const.go diff --git a/revocation/internal/crl/const.go b/revocation/internal/crl/const.go deleted file mode 100644 index a58ce967..00000000 --- a/revocation/internal/crl/const.go +++ /dev/null @@ -1,32 +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 - -const ( - // RFC 5280, 5.3.1 - // CRLReason ::= ENUMERATED { - // unspecified (0), - // keyCompromise (1), - // cACompromise (2), - // affiliationChanged (3), - // superseded (4), - // cessationOfOperation (5), - // certificateHold (6), - // -- value 7 is not used - // removeFromCRL (8), - // privilegeWithdrawn (9), - // aACompromise (10) } - reasonCodeCertificateHold = 6 - reasonCodeRemoveFromCRL = 8 -) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 0a5a6f9b..f497c888 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -30,6 +30,24 @@ import ( "golang.org/x/crypto/cryptobyte" ) +const ( + // RFC 5280, 5.3.1 + // CRLReason ::= ENUMERATED { + // unspecified (0), + // keyCompromise (1), + // cACompromise (2), + // affiliationChanged (3), + // superseded (4), + // cessationOfOperation (5), + // certificateHold (6), + // -- value 7 is not used + // removeFromCRL (8), + // privilegeWithdrawn (9), + // aACompromise (10) } + reasonCodeCertificateHold = 6 + reasonCodeRemoveFromCRL = 8 +) + var ( // oidFreshestCRL is the object identifier for the distribution point // for the delta CRL. (See RFC 5280, Section 5.2.6) From ad51cf8b130c9e478b2e70a7d9dce55093663f89 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 9 Dec 2024 05:43:19 +0000 Subject: [PATCH 14/25] fix: simplify code Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index 70d81a16..31f61eac 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -142,6 +142,10 @@ func (f *HTTPFetcher) fetch(ctx context.Context, url string) (*Bundle, error) { // // It returns errDeltaCRLNotFound if the delta CRL is not found. func (f *HTTPFetcher) fetchDeltaCRL(extensions *[]pkix.Extension) (*x509.RevocationList, error) { + var ( + lastError error + deltaCRL *x509.RevocationList + ) for _, ext := range *extensions { if ext.Id.Equal(oidFreshestCRL) { // RFC 5280, 4.2.1.15 @@ -152,28 +156,23 @@ func (f *HTTPFetcher) fetchDeltaCRL(extensions *[]pkix.Extension) (*x509.Revocat if err != nil { return nil, fmt.Errorf("failed to parse Freshest CRL extension: %w", err) } - if len(urls) == 0 { - return nil, errDeltaCRLNotFound - } - var ( - lastError error - deltaCRL *x509.RevocationList - ) for _, cdpURL := range urls { // RFC 5280, 5.2.6 // Delta CRLs from the base CRL have the same scope as the base // CRL, so the URLs are for redundancy and should be tried in // order until one succeeds. deltaCRL, lastError = fetchCRL(context.Background(), cdpURL, f.httpClient) - if lastError != nil { - continue + if lastError == nil { + return deltaCRL, nil } - return deltaCRL, nil } - return nil, lastError + break } } + if lastError != nil { + return nil, lastError + } return nil, errDeltaCRLNotFound } From a9330e580a1829c8ac91f31cae11ffcf8515a885 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Fri, 13 Dec 2024 08:24:34 +0000 Subject: [PATCH 15/25] fix: update Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 59 +++++++++++---------- revocation/crl/fetcher_test.go | 14 ++--- revocation/internal/crl/crl.go | 79 ++++++++++++++--------------- revocation/internal/crl/crl_test.go | 2 +- 4 files changed, 77 insertions(+), 77 deletions(-) diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index 31f61eac..e0c7bc71 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -25,6 +25,7 @@ import ( "io" "net/http" "net/url" + "slices" "time" "golang.org/x/crypto/cryptobyte" @@ -127,7 +128,7 @@ func (f *HTTPFetcher) fetch(ctx context.Context, url string) (*Bundle, error) { } // fetch delta CRL from base CRL extension - deltaCRL, err := f.fetchDeltaCRL(&base.Extensions) + deltaCRL, err := f.fetchDeltaCRL(ctx, base.Extensions) if err != nil && !errors.Is(err, errDeltaCRLNotFound) { return nil, err } @@ -141,39 +142,41 @@ func (f *HTTPFetcher) fetch(ctx context.Context, url string) (*Bundle, error) { // fetchDeltaCRL fetches the delta CRL from the given extensions of base CRL. // // It returns errDeltaCRLNotFound if the delta CRL is not found. -func (f *HTTPFetcher) fetchDeltaCRL(extensions *[]pkix.Extension) (*x509.RevocationList, error) { +func (f *HTTPFetcher) fetchDeltaCRL(ctx context.Context, extensions []pkix.Extension) (*x509.RevocationList, error) { + + idx := slices.IndexFunc(extensions, func(ext pkix.Extension) bool { + return ext.Id.Equal(oidFreshestCRL) + }) + if idx == -1 { + return nil, errDeltaCRLNotFound + } + // RFC 5280, 4.2.1.15 + // id-ce-freshestCRL OBJECT IDENTIFIER ::= { id-ce 46 } + // + // FreshestCRL ::= CRLDistributionPoints + urls, err := parseCRLDistributionPoint(extensions[idx].Value) + if err != nil { + return nil, fmt.Errorf("failed to parse Freshest CRL extension: %w", err) + } + if len(urls) == 0 { + return nil, errDeltaCRLNotFound + } + var ( lastError error deltaCRL *x509.RevocationList ) - for _, ext := range *extensions { - if ext.Id.Equal(oidFreshestCRL) { - // RFC 5280, 4.2.1.15 - // id-ce-freshestCRL OBJECT IDENTIFIER ::= { id-ce 46 } - // - // FreshestCRL ::= CRLDistributionPoints - urls, err := parseCRLDistributionPoint(ext.Value) - if err != nil { - return nil, fmt.Errorf("failed to parse Freshest CRL extension: %w", err) - } - - for _, cdpURL := range urls { - // RFC 5280, 5.2.6 - // Delta CRLs from the base CRL have the same scope as the base - // CRL, so the URLs are for redundancy and should be tried in - // order until one succeeds. - deltaCRL, lastError = fetchCRL(context.Background(), cdpURL, f.httpClient) - if lastError == nil { - return deltaCRL, nil - } - } - break + for _, cdpURL := range urls { + // RFC 5280, 5.2.6 + // Delta CRLs from the base CRL have the same scope as the base + // CRL, so the URLs are for redundancy and should be tried in + // order until one succeeds. + deltaCRL, lastError = fetchCRL(ctx, cdpURL, f.httpClient) + if lastError == nil { + return deltaCRL, nil } } - if lastError != nil { - return nil, lastError - } - return nil, errDeltaCRLNotFound + return nil, lastError } // parseCRLDistributionPoint parses the CRL extension and returns the CRL URLs diff --git a/revocation/crl/fetcher_test.go b/revocation/crl/fetcher_test.go index 460cbd14..2b7c0caf 100644 --- a/revocation/crl/fetcher_test.go +++ b/revocation/crl/fetcher_test.go @@ -381,7 +381,7 @@ func TestParseFreshestCRL(t *testing.T) { } func TestFetchDeltaCRL(t *testing.T) { - loadExtentsion := func(certPath string) *[]pkix.Extension { + loadExtentsion := func(certPath string) []pkix.Extension { certData, err := os.ReadFile(certPath) if err != nil { t.Fatalf("failed to read certificate: %v", err) @@ -397,7 +397,7 @@ func TestFetchDeltaCRL(t *testing.T) { t.Fatalf("failed to parse certificate: %v", err) } - return &cert.Extensions + return cert.Extensions } deltaCRL, err := os.ReadFile("testdata/delta.crl") @@ -415,7 +415,7 @@ func TestFetchDeltaCRL(t *testing.T) { t.Run("parse freshest CRL failed", func(t *testing.T) { certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" extensions := loadExtentsion(certPath) - _, err := fetcher.fetchDeltaCRL(extensions) + _, err := fetcher.fetchDeltaCRL(context.Background(), extensions) expectedErrorMsg := "failed to parse Freshest CRL extension: x509: invalid CRL distribution point" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) @@ -425,7 +425,7 @@ func TestFetchDeltaCRL(t *testing.T) { t.Run("zero freshest CRL URL", func(t *testing.T) { certPath := "testdata/certificateWithZeroDeltaCRLURL.cer" extensions := loadExtentsion(certPath) - _, err := fetcher.fetchDeltaCRL(extensions) + _, err := fetcher.fetchDeltaCRL(context.Background(), extensions) expectedErr := errDeltaCRLNotFound if err == nil || !errors.Is(err, expectedErr) { t.Fatalf("expected error %v, got %v", expectedErr, err) @@ -435,7 +435,7 @@ func TestFetchDeltaCRL(t *testing.T) { t.Run("one freshest CRL URL", func(t *testing.T) { certPath := "testdata/certificateWithDeltaCRL.cer" extensions := loadExtentsion(certPath) - deltaCRL, err := fetcher.fetchDeltaCRL(extensions) + deltaCRL, err := fetcher.fetchDeltaCRL(context.Background(), extensions) if err != nil { t.Fatalf("failed to process delta CRL: %v", err) } @@ -453,7 +453,7 @@ func TestFetchDeltaCRL(t *testing.T) { } certPath := "testdata/certificateWith2DeltaCRL.cer" extensions := loadExtentsion(certPath) - _, err = fetcherWithError.fetchDeltaCRL(extensions) + _, err = fetcherWithError.fetchDeltaCRL(context.Background(), extensions) expectedErrorMsg := "request failed" if err == nil || !strings.Contains(err.Error(), expectedErrorMsg) { t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) @@ -463,7 +463,7 @@ func TestFetchDeltaCRL(t *testing.T) { t.Run("process delta crl from certificate extension failed", func(t *testing.T) { certPath := "testdata/certificateWithIncompleteFreshestCRL.cer" extensions := loadExtentsion(certPath) - _, err := fetcher.fetchDeltaCRL(extensions) + _, err := fetcher.fetchDeltaCRL(context.Background(), extensions) expectedErrorMsg := "failed to parse Freshest CRL extension: x509: invalid CRL distribution point" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected error %q, got %v", expectedErrorMsg, err) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index f497c888..955db5df 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "math/big" + "slices" "time" "github.com/notaryproject/notation-core-go/revocation/crl" @@ -114,7 +115,7 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C serverResults = make([]*result.ServerResult, 0, len(cert.CRLDistributionPoints)) lastErr error crlURL string - hasFreshestCRLInCertificate = hasFreshestCRL(&cert.Extensions) + hasFreshestCRLInCertificate = hasFreshestCRL(cert.Extensions) ) // The CRLDistributionPoints contains the URIs of all the CRL distribution @@ -195,13 +196,10 @@ func Supported(cert *x509.Certificate) bool { return cert != nil && len(cert.CRLDistributionPoints) > 0 } -func hasFreshestCRL(extensions *[]pkix.Extension) bool { - for _, ext := range *extensions { - if ext.Id.Equal(oidFreshestCRL) { - return true - } - } - return false +func hasFreshestCRL(extensions []pkix.Extension) bool { + return slices.ContainsFunc(extensions, func(ext pkix.Extension) bool { + return ext.Id.Equal(oidFreshestCRL) + }) } func validate(bundle *crl.Bundle, issuer *x509.Certificate) error { @@ -211,35 +209,34 @@ func validate(bundle *crl.Bundle, issuer *x509.Certificate) error { return fmt.Errorf("failed to validate base CRL: %w", err) } - if bundle.DeltaCRL != nil { - // validate delta CRL - // RFC 5280, Section 5.2.4 - deltaCRL := bundle.DeltaCRL - if err := validateCRL(deltaCRL, issuer); err != nil { - return fmt.Errorf("failed to validate delta CRL: %w", err) - } + if bundle.DeltaCRL == nil { + return nil + } - if deltaCRL.Number.Cmp(baseCRL.Number) <= 0 { - return fmt.Errorf("delta CRL number %d is not greater than the base CRL number %d", deltaCRL.Number, baseCRL.Number) - } - // check delta CRL indicator extension - var minimumBaseCRLNumber *big.Int - for _, ext := range deltaCRL.Extensions { - if ext.Id.Equal(oidDeltaCRLIndicator) { - minimumBaseCRLNumber = new(big.Int) - value := cryptobyte.String(ext.Value) - if !value.ReadASN1Integer(minimumBaseCRLNumber) { - return errors.New("failed to parse delta CRL indicator extension") - } - break - } - } - if minimumBaseCRLNumber == nil { - return errors.New("delta CRL indicator extension is not found") - } - if minimumBaseCRLNumber.Cmp(baseCRL.Number) > 0 { - return fmt.Errorf("delta CRL indicator %d is not less than or equal to the base CRL number %d", minimumBaseCRLNumber, baseCRL.Number) - } + // validate delta CRL + // RFC 5280, Section 5.2.4 + deltaCRL := bundle.DeltaCRL + if err := validateCRL(deltaCRL, issuer); err != nil { + return fmt.Errorf("failed to validate delta CRL: %w", err) + } + if deltaCRL.Number.Cmp(baseCRL.Number) <= 0 { + return fmt.Errorf("delta CRL number %d is not greater than the base CRL number %d", deltaCRL.Number, baseCRL.Number) + } + + // check delta CRL indicator extension + idx := slices.IndexFunc(deltaCRL.Extensions, func(ext pkix.Extension) bool { + return ext.Id.Equal(oidDeltaCRLIndicator) + }) + if idx == -1 { + return errors.New("delta CRL indicator extension is not found") + } + minimumBaseCRLNumber := new(big.Int) + value := cryptobyte.String(deltaCRL.Extensions[idx].Value) + if !value.ReadASN1Integer(minimumBaseCRLNumber) { + return errors.New("failed to parse delta CRL indicator extension") + } + if minimumBaseCRLNumber.Cmp(baseCRL.Number) > 0 { + return fmt.Errorf("delta CRL indicator %d is not less than or equal to the base CRL number %d", minimumBaseCRLNumber, baseCRL.Number) } return nil } @@ -290,9 +287,9 @@ func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Tim return nil, errors.New("baseCRL cannot be nil") } - revocationListBundle := []*[]x509.RevocationListEntry{&b.BaseCRL.RevokedCertificateEntries} + revocationListBundle := [][]x509.RevocationListEntry{b.BaseCRL.RevokedCertificateEntries} if b.DeltaCRL != nil { - revocationListBundle = append(revocationListBundle, &b.DeltaCRL.RevokedCertificateEntries) + revocationListBundle = append(revocationListBundle, b.DeltaCRL.RevokedCertificateEntries) } // latestTempRevokedEntry contains the most recent revocation entry with @@ -305,7 +302,7 @@ func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Tim // iterate over all the entries in the base and delta CRLs for _, revocationList := range revocationListBundle { - for i, revocationEntry := range *revocationList { + for i, revocationEntry := range revocationList { if revocationEntry.SerialNumber.Cmp(cert.SerialNumber) == 0 { extensions, err := parseEntryExtensions(revocationEntry) if err != nil { @@ -321,11 +318,11 @@ func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Tim } switch revocationEntry.ReasonCode { - case int(reasonCodeCertificateHold), int(reasonCodeRemoveFromCRL): + case reasonCodeCertificateHold, reasonCodeRemoveFromCRL: // temporarily revoked or unrevoked if latestTempRevokedEntry == nil || latestTempRevokedEntry.RevocationTime.Before(revocationEntry.RevocationTime) { // the revocation status depends on the most recent reason - latestTempRevokedEntry = &(*revocationList)[i] + latestTempRevokedEntry = &revocationList[i] } default: // permanently revoked diff --git a/revocation/internal/crl/crl_test.go b/revocation/internal/crl/crl_test.go index a25a5585..c6e965c1 100644 --- a/revocation/internal/crl/crl_test.go +++ b/revocation/internal/crl/crl_test.go @@ -1013,7 +1013,7 @@ func TestHasDeltaCRL(t *testing.T) { }, }, } - if !hasFreshestCRL(&cert.Extensions) { + if !hasFreshestCRL(cert.Extensions) { t.Fatal("expected has delta CRL") } } From b2f2852fa217690072ec53e8142ef45366ee5351 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Fri, 13 Dec 2024 09:41:47 +0000 Subject: [PATCH 16/25] fix: optimize Fetch Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index e0c7bc71..7ea5b0ff 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -90,8 +90,7 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*Bundle, error) { bundle, err := f.Cache.Get(ctx, url) if err == nil { // check expiry of base CRL and delta CRL - if (bundle.BaseCRL != nil && isEffective(bundle.BaseCRL)) && - (bundle.DeltaCRL == nil || isEffective(bundle.DeltaCRL)) { + if isEffective(bundle.BaseCRL) && (bundle.DeltaCRL == nil || isEffective(bundle.DeltaCRL)) { return bundle, nil } } else if !errors.Is(err, ErrCacheMiss) && !f.DiscardCacheError { @@ -143,7 +142,6 @@ func (f *HTTPFetcher) fetch(ctx context.Context, url string) (*Bundle, error) { // // It returns errDeltaCRLNotFound if the delta CRL is not found. func (f *HTTPFetcher) fetchDeltaCRL(ctx context.Context, extensions []pkix.Extension) (*x509.RevocationList, error) { - idx := slices.IndexFunc(extensions, func(ext pkix.Extension) bool { return ext.Id.Equal(oidFreshestCRL) }) From 1239c5c0843f7771ad1c61e46c49ba4428e54808 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Fri, 13 Dec 2024 09:46:52 +0000 Subject: [PATCH 17/25] fix: update Signed-off-by: Junjie Gao --- revocation/internal/crl/crl.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 955db5df..4bc3b337 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -133,12 +133,12 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C } if hasFreshestCRLInCertificate && bundle.DeltaCRL == nil { - // deltaCRL URL in cert deltaCRL URL in baseCRL support it? - // --------------------------- ----------------------- ------------- - // True True Yes - // True False No - // False True Yes - // False False Yes + // | deltaCRL URL in cert | deltaCRL URL in baseCRL | support it? | + // |----------------------|-------------------------|-------------| + // | True | True | Yes | + // | True | False | No | + // | False | True | Yes | + // | False | False | Yes | // // if only the certificate has the freshest CRL, the bundle.DeltaCRL // should be nil. We don't support this case now because the delta From 28069eed0b04edfaa78272614d9881f71be50d37 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 16 Dec 2024 05:37:40 +0000 Subject: [PATCH 18/25] fix: resolve comment for Shiwei Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 2 +- revocation/internal/crl/crl.go | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index 7ea5b0ff..d1480893 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -145,7 +145,7 @@ func (f *HTTPFetcher) fetchDeltaCRL(ctx context.Context, extensions []pkix.Exten idx := slices.IndexFunc(extensions, func(ext pkix.Extension) bool { return ext.Id.Equal(oidFreshestCRL) }) - if idx == -1 { + if idx < 0 { return nil, errDeltaCRLNotFound } // RFC 5280, 4.2.1.15 diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 4bc3b337..14829217 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -227,7 +227,7 @@ func validate(bundle *crl.Bundle, issuer *x509.Certificate) error { idx := slices.IndexFunc(deltaCRL.Extensions, func(ext pkix.Extension) bool { return ext.Id.Equal(oidDeltaCRLIndicator) }) - if idx == -1 { + if idx < 0 { return errors.New("delta CRL indicator extension is not found") } minimumBaseCRLNumber := new(big.Int) @@ -315,6 +315,11 @@ func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Tim // signing time is before the invalidity date which means the // certificate is not revoked at the time of signing. break + return &result.ServerResult{ + Result: result.ResultOK, + Server: crlURL, + RevocationMethod: result.RevocationMethodCRL, + }, nil } switch revocationEntry.ReasonCode { From 6becbe1f24e34da12ed3a9e74531ac8efb1bf7e1 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 16 Dec 2024 05:48:25 +0000 Subject: [PATCH 19/25] fix: update Signed-off-by: Junjie Gao --- revocation/internal/crl/crl.go | 1 - 1 file changed, 1 deletion(-) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 14829217..ba1b65c2 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -314,7 +314,6 @@ func checkRevocation(cert *x509.Certificate, b *crl.Bundle, signingTime time.Tim signingTime.Before(extensions.invalidityDate) { // signing time is before the invalidity date which means the // certificate is not revoked at the time of signing. - break return &result.ServerResult{ Result: result.ResultOK, Server: crlURL, From 43e6407831a7fbe8a5c81ead33c513c6c1cbea4b Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 23 Dec 2024 07:08:17 +0000 Subject: [PATCH 20/25] fix: resolve comments for Two-Hearts Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 25 +++++----- revocation/internal/crl/crl.go | 46 +++++++++---------- revocation/internal/x509util/extension.go | 31 +++++++++++++ .../internal/x509util/extension_test.go | 45 ++++++++++++++++++ revocation/internal/x509util/validate.go | 3 +- 5 files changed, 113 insertions(+), 37 deletions(-) create mode 100644 revocation/internal/x509util/extension.go create mode 100644 revocation/internal/x509util/extension_test.go diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index d1480893..13056e2d 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -25,11 +25,11 @@ import ( "io" "net/http" "net/url" - "slices" "time" + "github.com/notaryproject/notation-core-go/revocation/internal/x509util" "golang.org/x/crypto/cryptobyte" - cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1" + cbasn1 "golang.org/x/crypto/cryptobyte/asn1" ) // oidFreshestCRL is the object identifier for the distribution point @@ -142,17 +142,16 @@ func (f *HTTPFetcher) fetch(ctx context.Context, url string) (*Bundle, error) { // // It returns errDeltaCRLNotFound if the delta CRL is not found. func (f *HTTPFetcher) fetchDeltaCRL(ctx context.Context, extensions []pkix.Extension) (*x509.RevocationList, error) { - idx := slices.IndexFunc(extensions, func(ext pkix.Extension) bool { - return ext.Id.Equal(oidFreshestCRL) - }) - if idx < 0 { + extension := x509util.FindExtensionByOID(oidFreshestCRL, extensions) + if extension == nil { return nil, errDeltaCRLNotFound } + // RFC 5280, 4.2.1.15 // id-ce-freshestCRL OBJECT IDENTIFIER ::= { id-ce 46 } // // FreshestCRL ::= CRLDistributionPoints - urls, err := parseCRLDistributionPoint(extensions[idx].Value) + urls, err := parseCRLDistributionPoint(extension.Value) if err != nil { return nil, fmt.Errorf("failed to parse Freshest CRL extension: %w", err) } @@ -197,31 +196,31 @@ func parseCRLDistributionPoint(value []byte) ([]string, error) { // fullName [0] GeneralNames, // nameRelativeToCRLIssuer [1] RelativeDistinguishedName } val := cryptobyte.String(value) - if !val.ReadASN1(&val, cryptobyte_asn1.SEQUENCE) { + if !val.ReadASN1(&val, cbasn1.SEQUENCE) { return nil, errors.New("x509: invalid CRL distribution points") } for !val.Empty() { var dpDER cryptobyte.String - if !val.ReadASN1(&dpDER, cryptobyte_asn1.SEQUENCE) { + if !val.ReadASN1(&dpDER, cbasn1.SEQUENCE) { return nil, errors.New("x509: invalid CRL distribution point") } var dpNameDER cryptobyte.String var dpNamePresent bool - if !dpDER.ReadOptionalASN1(&dpNameDER, &dpNamePresent, cryptobyte_asn1.Tag(0).Constructed().ContextSpecific()) { + if !dpDER.ReadOptionalASN1(&dpNameDER, &dpNamePresent, cbasn1.Tag(0).Constructed().ContextSpecific()) { return nil, errors.New("x509: invalid CRL distribution point") } if !dpNamePresent { continue } - if !dpNameDER.ReadASN1(&dpNameDER, cryptobyte_asn1.Tag(0).Constructed().ContextSpecific()) { + if !dpNameDER.ReadASN1(&dpNameDER, cbasn1.Tag(0).Constructed().ContextSpecific()) { return nil, errors.New("x509: invalid CRL distribution point") } for !dpNameDER.Empty() { - if !dpNameDER.PeekASN1Tag(cryptobyte_asn1.Tag(6).ContextSpecific()) { + if !dpNameDER.PeekASN1Tag(cbasn1.Tag(6).ContextSpecific()) { break } var uri cryptobyte.String - if !dpNameDER.ReadASN1(&uri, cryptobyte_asn1.Tag(6).ContextSpecific()) { + if !dpNameDER.ReadASN1(&uri, cbasn1.Tag(6).ContextSpecific()) { return nil, errors.New("x509: invalid CRL distribution point") } urls = append(urls, string(uri)) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index ba1b65c2..cd6301a9 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -23,30 +23,34 @@ import ( "errors" "fmt" "math/big" - "slices" "time" "github.com/notaryproject/notation-core-go/revocation/crl" + "github.com/notaryproject/notation-core-go/revocation/internal/x509util" "github.com/notaryproject/notation-core-go/revocation/result" "golang.org/x/crypto/cryptobyte" ) +// RFC 5280, 5.3.1 +// +// CRLReason ::= ENUMERATED { +// unspecified (0), +// keyCompromise (1), +// cACompromise (2), +// affiliationChanged (3), +// superseded (4), +// cessationOfOperation (5), +// certificateHold (6), +// -- value 7 is not used +// removeFromCRL (8), +// privilegeWithdrawn (9), +// aACompromise (10) } const ( - // RFC 5280, 5.3.1 - // CRLReason ::= ENUMERATED { - // unspecified (0), - // keyCompromise (1), - // cACompromise (2), - // affiliationChanged (3), - // superseded (4), - // cessationOfOperation (5), - // certificateHold (6), - // -- value 7 is not used - // removeFromCRL (8), - // privilegeWithdrawn (9), - // aACompromise (10) } + // certificateHold reasonCodeCertificateHold = 6 - reasonCodeRemoveFromCRL = 8 + + // removeFromCRL + reasonCodeRemoveFromCRL = 8 ) var ( @@ -197,9 +201,7 @@ func Supported(cert *x509.Certificate) bool { } func hasFreshestCRL(extensions []pkix.Extension) bool { - return slices.ContainsFunc(extensions, func(ext pkix.Extension) bool { - return ext.Id.Equal(oidFreshestCRL) - }) + return x509util.FindExtensionByOID(oidFreshestCRL, extensions) != nil } func validate(bundle *crl.Bundle, issuer *x509.Certificate) error { @@ -224,14 +226,12 @@ func validate(bundle *crl.Bundle, issuer *x509.Certificate) error { } // check delta CRL indicator extension - idx := slices.IndexFunc(deltaCRL.Extensions, func(ext pkix.Extension) bool { - return ext.Id.Equal(oidDeltaCRLIndicator) - }) - if idx < 0 { + extension := x509util.FindExtensionByOID(oidDeltaCRLIndicator, deltaCRL.Extensions) + if extension == nil { return errors.New("delta CRL indicator extension is not found") } minimumBaseCRLNumber := new(big.Int) - value := cryptobyte.String(deltaCRL.Extensions[idx].Value) + value := cryptobyte.String(extension.Value) if !value.ReadASN1Integer(minimumBaseCRLNumber) { return errors.New("failed to parse delta CRL indicator extension") } diff --git a/revocation/internal/x509util/extension.go b/revocation/internal/x509util/extension.go new file mode 100644 index 00000000..f1536203 --- /dev/null +++ b/revocation/internal/x509util/extension.go @@ -0,0 +1,31 @@ +// 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 x509util + +import ( + "crypto/x509/pkix" + "encoding/asn1" + "slices" +) + +// FindExtensionByOID finds the extension by the given OID. +func FindExtensionByOID(oid asn1.ObjectIdentifier, extensions []pkix.Extension) *pkix.Extension { + idx := slices.IndexFunc(extensions, func(ext pkix.Extension) bool { + return ext.Id.Equal(oid) + }) + if idx < 0 { + return nil + } + return &extensions[idx] +} diff --git a/revocation/internal/x509util/extension_test.go b/revocation/internal/x509util/extension_test.go new file mode 100644 index 00000000..4dc91d39 --- /dev/null +++ b/revocation/internal/x509util/extension_test.go @@ -0,0 +1,45 @@ +package x509util + +import ( + "crypto/x509/pkix" + "encoding/asn1" + "testing" +) + +func TestFindExtensionByOID(t *testing.T) { + oid1 := asn1.ObjectIdentifier{1, 2, 3, 4} + oid2 := asn1.ObjectIdentifier{1, 2, 3, 5} + extensions := []pkix.Extension{ + {Id: oid1, Value: []byte("value1")}, + {Id: oid2, Value: []byte("value2")}, + } + + tests := []struct { + name string + oid asn1.ObjectIdentifier + extensions []pkix.Extension + expected *pkix.Extension + }{ + { + name: "Extension found", + oid: oid1, + extensions: extensions, + expected: &extensions[0], + }, + { + name: "Extension not found", + oid: asn1.ObjectIdentifier{1, 2, 3, 6}, + extensions: extensions, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := FindExtensionByOID(tt.oid, tt.extensions) + if result != tt.expected { + t.Errorf("expected %v, got %v", tt.expected, result) + } + }) + } +} diff --git a/revocation/internal/x509util/validate.go b/revocation/internal/x509util/validate.go index 134ef22b..86c4c3c2 100644 --- a/revocation/internal/x509util/validate.go +++ b/revocation/internal/x509util/validate.go @@ -12,7 +12,8 @@ // limitations under the License. // Package x509util provides the method to validate the certificate chain for a -// specific purpose, including code signing and timestamping. +// specific purpose, including code signing and timestamping. It also provides +// the method to find the extension by the given OID. package x509util import ( From 52c48a81a5bb147882eac7a363375de0c6c7092e Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 23 Dec 2024 07:12:13 +0000 Subject: [PATCH 21/25] fix: add license Signed-off-by: Junjie Gao --- revocation/internal/x509util/extension_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/revocation/internal/x509util/extension_test.go b/revocation/internal/x509util/extension_test.go index 4dc91d39..744bd419 100644 --- a/revocation/internal/x509util/extension_test.go +++ b/revocation/internal/x509util/extension_test.go @@ -1,3 +1,16 @@ +// 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 x509util import ( From 0e5b4d0977adafffeacfedf8eb061b5737a4f533 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 24 Dec 2024 02:13:04 +0000 Subject: [PATCH 22/25] fix: simplify code Signed-off-by: Junjie Gao --- revocation/internal/crl/crl.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index cd6301a9..ae5df7ec 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -18,7 +18,6 @@ package crl import ( "context" "crypto/x509" - "crypto/x509/pkix" "encoding/asn1" "errors" "fmt" @@ -119,7 +118,7 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C serverResults = make([]*result.ServerResult, 0, len(cert.CRLDistributionPoints)) lastErr error crlURL string - hasFreshestCRLInCertificate = hasFreshestCRL(cert.Extensions) + hasFreshestCRLInCertificate = x509util.FindExtensionByOID(oidFreshestCRL, cert.Extensions) != nil ) // The CRLDistributionPoints contains the URIs of all the CRL distribution @@ -200,10 +199,6 @@ func Supported(cert *x509.Certificate) bool { return cert != nil && len(cert.CRLDistributionPoints) > 0 } -func hasFreshestCRL(extensions []pkix.Extension) bool { - return x509util.FindExtensionByOID(oidFreshestCRL, extensions) != nil -} - func validate(bundle *crl.Bundle, issuer *x509.Certificate) error { // validate base CRL baseCRL := bundle.BaseCRL From dc3a62e999a03c3951710ae545890b5f92e4a6db Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 24 Dec 2024 02:22:26 +0000 Subject: [PATCH 23/25] fix: test Signed-off-by: Junjie Gao --- revocation/internal/crl/crl_test.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/revocation/internal/crl/crl_test.go b/revocation/internal/crl/crl_test.go index c6e965c1..64559e66 100644 --- a/revocation/internal/crl/crl_test.go +++ b/revocation/internal/crl/crl_test.go @@ -1005,19 +1005,6 @@ func TestSupported(t *testing.T) { }) } -func TestHasDeltaCRL(t *testing.T) { - cert := &x509.Certificate{ - Extensions: []pkix.Extension{ - { - Id: oidFreshestCRL, - }, - }, - } - if !hasFreshestCRL(cert.Extensions) { - t.Fatal("expected has delta CRL") - } -} - type errorRoundTripperMock struct{} func (rt errorRoundTripperMock) RoundTrip(req *http.Request) (*http.Response, error) { From 9c5f91922f22dcc7a614e047aa12a85c82a4e19a Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Thu, 26 Dec 2024 08:39:01 +0000 Subject: [PATCH 24/25] fix: resolve comment for Shiwei Signed-off-by: Junjie Gao --- revocation/crl/fetcher.go | 2 +- revocation/internal/crl/crl.go | 32 +++++++++---------- revocation/internal/x509util/extension.go | 2 +- .../internal/x509util/extension_test.go | 2 +- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/revocation/crl/fetcher.go b/revocation/crl/fetcher.go index 13056e2d..02daa452 100644 --- a/revocation/crl/fetcher.go +++ b/revocation/crl/fetcher.go @@ -142,7 +142,7 @@ func (f *HTTPFetcher) fetch(ctx context.Context, url string) (*Bundle, error) { // // It returns errDeltaCRLNotFound if the delta CRL is not found. func (f *HTTPFetcher) fetchDeltaCRL(ctx context.Context, extensions []pkix.Extension) (*x509.RevocationList, error) { - extension := x509util.FindExtensionByOID(oidFreshestCRL, extensions) + extension := x509util.FindExtensionByOID(extensions, oidFreshestCRL) if extension == nil { return nil, errDeltaCRLNotFound } diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index ae5df7ec..5e341f5d 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -30,21 +30,21 @@ import ( "golang.org/x/crypto/cryptobyte" ) -// RFC 5280, 5.3.1 -// -// CRLReason ::= ENUMERATED { -// unspecified (0), -// keyCompromise (1), -// cACompromise (2), -// affiliationChanged (3), -// superseded (4), -// cessationOfOperation (5), -// certificateHold (6), -// -- value 7 is not used -// removeFromCRL (8), -// privilegeWithdrawn (9), -// aACompromise (10) } const ( + // RFC 5280, 5.3.1 + // + // CRLReason ::= ENUMERATED { + // unspecified (0), + // keyCompromise (1), + // cACompromise (2), + // affiliationChanged (3), + // superseded (4), + // cessationOfOperation (5), + // certificateHold (6), + // -- value 7 is not used + // removeFromCRL (8), + // privilegeWithdrawn (9), + // aACompromise (10) } // certificateHold reasonCodeCertificateHold = 6 @@ -118,7 +118,7 @@ func CertCheckStatus(ctx context.Context, cert, issuer *x509.Certificate, opts C serverResults = make([]*result.ServerResult, 0, len(cert.CRLDistributionPoints)) lastErr error crlURL string - hasFreshestCRLInCertificate = x509util.FindExtensionByOID(oidFreshestCRL, cert.Extensions) != nil + hasFreshestCRLInCertificate = x509util.FindExtensionByOID(cert.Extensions, oidFreshestCRL) != nil ) // The CRLDistributionPoints contains the URIs of all the CRL distribution @@ -221,7 +221,7 @@ func validate(bundle *crl.Bundle, issuer *x509.Certificate) error { } // check delta CRL indicator extension - extension := x509util.FindExtensionByOID(oidDeltaCRLIndicator, deltaCRL.Extensions) + extension := x509util.FindExtensionByOID(deltaCRL.Extensions, oidDeltaCRLIndicator) if extension == nil { return errors.New("delta CRL indicator extension is not found") } diff --git a/revocation/internal/x509util/extension.go b/revocation/internal/x509util/extension.go index f1536203..a4f46ada 100644 --- a/revocation/internal/x509util/extension.go +++ b/revocation/internal/x509util/extension.go @@ -20,7 +20,7 @@ import ( ) // FindExtensionByOID finds the extension by the given OID. -func FindExtensionByOID(oid asn1.ObjectIdentifier, extensions []pkix.Extension) *pkix.Extension { +func FindExtensionByOID(extensions []pkix.Extension, oid asn1.ObjectIdentifier) *pkix.Extension { idx := slices.IndexFunc(extensions, func(ext pkix.Extension) bool { return ext.Id.Equal(oid) }) diff --git a/revocation/internal/x509util/extension_test.go b/revocation/internal/x509util/extension_test.go index 744bd419..22f72f76 100644 --- a/revocation/internal/x509util/extension_test.go +++ b/revocation/internal/x509util/extension_test.go @@ -49,7 +49,7 @@ func TestFindExtensionByOID(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := FindExtensionByOID(tt.oid, tt.extensions) + result := FindExtensionByOID(tt.extensions, tt.oid) if result != tt.expected { t.Errorf("expected %v, got %v", tt.expected, result) } From a24fe9d1eb5eee957b6e657ed66128acd9d2ff5c Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Thu, 26 Dec 2024 08:50:18 +0000 Subject: [PATCH 25/25] fix: update comment Signed-off-by: Junjie Gao --- revocation/internal/crl/crl.go | 35 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/revocation/internal/crl/crl.go b/revocation/internal/crl/crl.go index 5e341f5d..a6b54b93 100644 --- a/revocation/internal/crl/crl.go +++ b/revocation/internal/crl/crl.go @@ -30,26 +30,23 @@ import ( "golang.org/x/crypto/cryptobyte" ) +// RFC 5280, 5.3.1 +// +// CRLReason ::= ENUMERATED { +// unspecified (0), +// keyCompromise (1), +// cACompromise (2), +// affiliationChanged (3), +// superseded (4), +// cessationOfOperation (5), +// certificateHold (6), +// -- value 7 is not used +// removeFromCRL (8), +// privilegeWithdrawn (9), +// aACompromise (10) } const ( - // RFC 5280, 5.3.1 - // - // CRLReason ::= ENUMERATED { - // unspecified (0), - // keyCompromise (1), - // cACompromise (2), - // affiliationChanged (3), - // superseded (4), - // cessationOfOperation (5), - // certificateHold (6), - // -- value 7 is not used - // removeFromCRL (8), - // privilegeWithdrawn (9), - // aACompromise (10) } - // certificateHold - reasonCodeCertificateHold = 6 - - // removeFromCRL - reasonCodeRemoveFromCRL = 8 + reasonCodeCertificateHold = 6 // certificateHold + reasonCodeRemoveFromCRL = 8 // removeFromCRL ) var (