From 3351c95f9e5b78fde6aa846fc2eb7b6d61cb367d Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Sun, 29 Dec 2024 19:20:48 +0800 Subject: [PATCH] backport: timestamping cert chain revocation check during signing from `main` to `release-1.3` branch (#1121) Signed-off-by: Patrick Zheng --- cmd/notation/sign.go | 7 +++ cmd/notation/verify.go | 40 +----------- internal/{ => revocation}/crl/crl.go | 0 internal/{ => revocation}/crl/crl_test.go | 0 internal/revocation/revocation.go | 54 ++++++++++++++++ internal/revocation/revocation_test.go | 75 +++++++++++++++++++++++ test/e2e/suite/command/verify.go | 4 +- 7 files changed, 141 insertions(+), 39 deletions(-) rename internal/{ => revocation}/crl/crl.go (100%) rename internal/{ => revocation}/crl/crl_test.go (100%) create mode 100644 internal/revocation/revocation.go create mode 100644 internal/revocation/revocation_test.go diff --git a/cmd/notation/sign.go b/cmd/notation/sign.go index b241424df..60bca1509 100644 --- a/cmd/notation/sign.go +++ b/cmd/notation/sign.go @@ -23,6 +23,7 @@ import ( "strings" "time" + "github.com/notaryproject/notation-core-go/revocation/purpose" corex509 "github.com/notaryproject/notation-core-go/x509" "github.com/notaryproject/notation-go" "github.com/notaryproject/notation-go/log" @@ -30,6 +31,7 @@ import ( "github.com/notaryproject/notation/internal/cmd" "github.com/notaryproject/notation/internal/envelope" "github.com/notaryproject/notation/internal/httputil" + clirev "github.com/notaryproject/notation/internal/revocation" nx509 "github.com/notaryproject/notation/internal/x509" "github.com/notaryproject/tspclient-go" ocispec "github.com/opencontainers/image-spec/specs-go/v1" @@ -251,6 +253,11 @@ func prepareSigningOpts(ctx context.Context, opts *signOpts) (notation.SignOptio rootCAs := x509.NewCertPool() rootCAs.AddCert(tsaRootCert) signOpts.TSARootCAs = rootCAs + tsaRevocationValidator, err := clirev.NewRevocationValidator(ctx, purpose.Timestamping) + if err != nil { + return notation.SignOptions{}, fmt.Errorf("failed to create timestamping revocation validator: %w", err) + } + signOpts.TSARevocationValidator = tsaRevocationValidator } return signOpts, nil } diff --git a/cmd/notation/verify.go b/cmd/notation/verify.go index 933d41661..d320db47b 100644 --- a/cmd/notation/verify.go +++ b/cmd/notation/verify.go @@ -18,28 +18,22 @@ import ( "errors" "fmt" "io/fs" - "net/http" "os" "reflect" - "time" - "github.com/notaryproject/notation-core-go/revocation" "github.com/notaryproject/notation-core-go/revocation/purpose" "github.com/notaryproject/notation-go" "github.com/notaryproject/notation-go/dir" "github.com/notaryproject/notation-go/plugin" "github.com/notaryproject/notation-go/verifier" - "github.com/notaryproject/notation-go/verifier/crl" "github.com/notaryproject/notation-go/verifier/trustpolicy" "github.com/notaryproject/notation-go/verifier/truststore" "github.com/notaryproject/notation/cmd/notation/internal/experimental" "github.com/notaryproject/notation/internal/cmd" - "github.com/notaryproject/notation/internal/httputil" "github.com/notaryproject/notation/internal/ioutil" "github.com/spf13/cobra" - corecrl "github.com/notaryproject/notation-core-go/revocation/crl" - clicrl "github.com/notaryproject/notation/internal/crl" + clirev "github.com/notaryproject/notation/internal/revocation" ) type verifyOpts struct { @@ -234,39 +228,11 @@ func printMetadataIfPresent(outcome *notation.VerificationOutcome) { func getVerifier(ctx context.Context) (notation.Verifier, error) { // revocation check - ocspHttpClient := httputil.NewClient(ctx, &http.Client{Timeout: 2 * time.Second}) - crlFetcher, err := corecrl.NewHTTPFetcher(httputil.NewClient(ctx, &http.Client{Timeout: 5 * time.Second})) + revocationCodeSigningValidator, err := clirev.NewRevocationValidator(ctx, purpose.CodeSigning) if err != nil { return nil, err } - crlFetcher.DiscardCacheError = true // discard crl cache error - cacheRoot, err := dir.CacheFS().SysPath(dir.PathCRLCache) - if err != nil { - return nil, err - } - fileCache, err := crl.NewFileCache(cacheRoot) - if err != nil { - // discard NewFileCache error as cache errors are not critical - fmt.Fprintf(os.Stderr, "Warning: %v\n", err) - } else { - crlFetcher.Cache = &clicrl.CacheWithLog{ - Cache: fileCache, - DiscardCacheError: crlFetcher.DiscardCacheError, - } - } - revocationCodeSigningValidator, err := revocation.NewWithOptions(revocation.Options{ - OCSPHTTPClient: ocspHttpClient, - CRLFetcher: crlFetcher, - CertChainPurpose: purpose.CodeSigning, - }) - if err != nil { - return nil, err - } - revocationTimestampingValidator, err := revocation.NewWithOptions(revocation.Options{ - OCSPHTTPClient: ocspHttpClient, - CRLFetcher: crlFetcher, - CertChainPurpose: purpose.Timestamping, - }) + revocationTimestampingValidator, err := clirev.NewRevocationValidator(ctx, purpose.Timestamping) if err != nil { return nil, err } diff --git a/internal/crl/crl.go b/internal/revocation/crl/crl.go similarity index 100% rename from internal/crl/crl.go rename to internal/revocation/crl/crl.go diff --git a/internal/crl/crl_test.go b/internal/revocation/crl/crl_test.go similarity index 100% rename from internal/crl/crl_test.go rename to internal/revocation/crl/crl_test.go diff --git a/internal/revocation/revocation.go b/internal/revocation/revocation.go new file mode 100644 index 000000000..412d6f0e5 --- /dev/null +++ b/internal/revocation/revocation.go @@ -0,0 +1,54 @@ +// 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 revocation + +import ( + "context" + "fmt" + "net/http" + "os" + "time" + + "github.com/notaryproject/notation-core-go/revocation" + corecrl "github.com/notaryproject/notation-core-go/revocation/crl" + "github.com/notaryproject/notation-core-go/revocation/purpose" + "github.com/notaryproject/notation-go/dir" + "github.com/notaryproject/notation-go/verifier/crl" + "github.com/notaryproject/notation/internal/httputil" + clicrl "github.com/notaryproject/notation/internal/revocation/crl" +) + +// NewRevocationValidator returns a revocation.Validator given the certificate +// purpose +func NewRevocationValidator(ctx context.Context, purpose purpose.Purpose) (revocation.Validator, error) { + // err is always nil + crlFetcher, _ := corecrl.NewHTTPFetcher(httputil.NewClient(ctx, &http.Client{Timeout: 5 * time.Second})) + crlFetcher.DiscardCacheError = true // discard crl cache error + cacheRoot, _ := dir.CacheFS().SysPath(dir.PathCRLCache) // err is always nil + fileCache, err := crl.NewFileCache(cacheRoot) + if err != nil { + // discard NewFileCache error as cache errors are not critical + fmt.Fprintf(os.Stderr, "Warning: %v\n", err) + } else { + crlFetcher.Cache = &clicrl.CacheWithLog{ + Cache: fileCache, + DiscardCacheError: crlFetcher.DiscardCacheError, + } + } + return revocation.NewWithOptions(revocation.Options{ + OCSPHTTPClient: httputil.NewClient(ctx, &http.Client{Timeout: 2 * time.Second}), + CRLFetcher: crlFetcher, + CertChainPurpose: purpose, + }) +} diff --git a/internal/revocation/revocation_test.go b/internal/revocation/revocation_test.go new file mode 100644 index 000000000..eeb7ff54b --- /dev/null +++ b/internal/revocation/revocation_test.go @@ -0,0 +1,75 @@ +// 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 revocation + +import ( + "context" + "net/http" + "os" + "runtime" + "testing" + "time" + + corecrl "github.com/notaryproject/notation-core-go/revocation/crl" + "github.com/notaryproject/notation-core-go/revocation/purpose" + "github.com/notaryproject/notation-go/dir" + "github.com/notaryproject/notation/internal/httputil" +) + +func TestNewRevocationValidator(t *testing.T) { + defer func(oldCacheDir string) { + dir.UserCacheDir = oldCacheDir + }(dir.UserCacheDir) + + t.Run("Success", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on Windows") + } + if _, err := NewRevocationValidator(context.Background(), purpose.Timestamping); err != nil { + t.Fatal(err) + } + }) + + tempRoot := t.TempDir() + t.Run("Success but without permission to create cache directory", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on Windows") + } + dir.UserCacheDir = tempRoot + if err := os.Chmod(tempRoot, 0); err != nil { + t.Fatal(err) + } + defer func() { + // restore permission + if err := os.Chmod(tempRoot, 0755); err != nil { + t.Fatalf("failed to change permission: %v", err) + } + }() + if _, err := NewRevocationValidator(context.Background(), purpose.Timestamping); err != nil { + t.Fatal(err) + } + }) +} + +func TestNilError(t *testing.T) { + _, err := corecrl.NewHTTPFetcher(httputil.NewClient(context.Background(), &http.Client{Timeout: 5 * time.Second})) + if err != nil { + t.Fatal(err) + } + + _, err = dir.CacheFS().SysPath(dir.PathCRLCache) + if err != nil { + t.Fatal(err) + } +} diff --git a/test/e2e/suite/command/verify.go b/test/e2e/suite/command/verify.go index b9fef3ea3..9ab8d29d0 100644 --- a/test/e2e/suite/command/verify.go +++ b/test/e2e/suite/command/verify.go @@ -229,7 +229,7 @@ var _ = Describe("notation verify", func() { notation.Exec("sign", "--timestamp-url", "http://timestamp.digicert.com", "--timestamp-root-cert", filepath.Join(NotationE2EConfigPath, "timestamp", "DigiCertTSARootSHA384.cer"), artifact.ReferenceWithDigest()). MatchKeyWords(SignSuccessfully) - notation.Exec("verify", artifact.ReferenceWithDigest(), "-v"). + notation.Exec("verify", artifact.ReferenceWithDigest(), "-d"). MatchKeyWords(VerifySuccessfully). MatchErrKeyWords("Timestamp verification disabled") }) @@ -251,7 +251,7 @@ var _ = Describe("notation verify", func() { notation.Exec("sign", "--timestamp-url", "http://timestamp.digicert.com", "--timestamp-root-cert", filepath.Join(NotationE2EConfigPath, "timestamp", "DigiCertTSARootSHA384.cer"), artifact.ReferenceWithDigest()). MatchKeyWords(SignSuccessfully) - notation.Exec("verify", artifact.ReferenceWithDigest(), "-v"). + notation.Exec("verify", artifact.ReferenceWithDigest(), "-d"). MatchKeyWords(VerifySuccessfully). MatchErrKeyWords("Timestamp verification disabled: verifyTimestamp is set to \"afterCertExpiry\" and signing cert chain unexpired") })