From 0ec49784b53ab576a271a42546b2a8d31b4b6888 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 17 May 2023 15:12:11 +0200 Subject: [PATCH] oci: sort remaining quirks in cosign verify logic This commit properly sets `IgnoreTlog` to `true` when a public key is provided to check the signature against, which matches the (silent) default behavior from cosign v1. However, during this exercise it has become apparant that this assumption isn't necessarily true. As you can theoretically have a custom key and a tlog entry. Given this, we should inventarise the possible configuration options and the potential value they have to users (e.g. defining a custom Rekor URL seems to be valuable as well), and extend our API to facilitate these needs. In addition to the above, the CTLog public keys are now properly retrieved to avoid a `none of the CTFE keys have been found` error. Signed-off-by: Hidde Beydals --- .../controller/helmchart_controller_test.go | 25 +++++----- .../ocirepository_controller_test.go | 4 +- internal/controller/suite_test.go | 4 +- internal/oci/verifier.go | 50 ++++++++++--------- 4 files changed, 44 insertions(+), 39 deletions(-) diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index 3856d82d..b6b3430d 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -23,7 +23,6 @@ import ( "errors" "fmt" "io" - "io/ioutil" "net/http" "os" "path" @@ -1058,7 +1057,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { ) // Load a test chart - chartData, err := ioutil.ReadFile(chartPath) + chartData, err := os.ReadFile(chartPath) + g.Expect(err).NotTo(HaveOccurred()) // Upload the test chart metadata, err := loadTestChartToOCI(chartData, chartPath, testRegistryServer) @@ -2333,16 +2333,16 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()) workspaceDir := t.TempDir() - server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts) + server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts) g.Expect(err).NotTo(HaveOccurred()) // Load a test chart - chartData, err := ioutil.ReadFile(chartPath) + chartData, err := os.ReadFile(chartPath) + g.Expect(err).ToNot(HaveOccurred()) // Upload the test chart metadata, err := loadTestChartToOCI(chartData, chartPath, server) - g.Expect(err).NotTo(HaveOccurred()) g.Expect(err).ToNot(HaveOccurred()) repo := &helmv1.HelmRepository{ @@ -2452,7 +2452,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T ) // Load a test chart - chartData, err := ioutil.ReadFile(chartPath) + chartData, err := os.ReadFile(chartPath) + g.Expect(err).ToNot(HaveOccurred()) // Upload the test chart metadata, err := loadTestChartToOCI(chartData, chartPath, server) @@ -2504,10 +2505,10 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T }, want: sreconcile.ResultEmpty, wantErr: true, - wantErrMsg: "chart verification error: failed to verify : no matching signatures:", + wantErrMsg: "chart verification error: failed to verify : no signatures found for image", assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify : no matching signatures:"), - *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify : no matching signatures:"), + *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify : no signatures found for image"), + *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify : no signatures found for image"), }, }, { @@ -2522,8 +2523,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T want: sreconcile.ResultEmpty, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify : no matching signatures:"), - *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify : no matching signatures:"), + *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify : no signatures found for image"), + *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify : no signatures found for image"), }, }, { @@ -2696,7 +2697,7 @@ func loadTestChartToOCI(chartData []byte, chartPath string, server *registryClie } // Load a test chart - chartData, err = ioutil.ReadFile(chartPath) + chartData, err = os.ReadFile(chartPath) if err != nil { return nil, err } diff --git a/internal/controller/ocirepository_controller_test.go b/internal/controller/ocirepository_controller_test.go index 111b40ce..072e9811 100644 --- a/internal/controller/ocirepository_controller_test.go +++ b/internal/controller/ocirepository_controller_test.go @@ -1095,7 +1095,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), - *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "failed to verify the signature using provider ' keyless': no matching signatures"), + *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "failed to verify the signature using provider ' keyless': no signatures found for image"), }, }, { @@ -1193,6 +1193,8 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + obj := &ociv1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "verify-oci-source-signature-", diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index ad1798e8..d45779c7 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -21,7 +21,6 @@ import ( "context" "fmt" "io" - "io/ioutil" "math/rand" "os" "path/filepath" @@ -164,8 +163,7 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry } htpasswdPath := filepath.Join(workspaceDir, testRegistryHtpasswdFileBasename) - err = ioutil.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testRegistryUsername, string(pwBytes))), 0644) - if err != nil { + if err = os.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testRegistryUsername, string(pwBytes))), 0644); err != nil { return nil, fmt.Errorf("failed to create htpasswd file: %s", err) } diff --git a/internal/oci/verifier.go b/internal/oci/verifier.go index 1cb35280..77306c7d 100644 --- a/internal/oci/verifier.go +++ b/internal/oci/verifier.go @@ -21,15 +21,14 @@ import ( "crypto" "fmt" + "github.com/google/go-containerregistry/pkg/name" "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/sigstore/cosign/v2/cmd/cosign/cli/fulcio" + coptions "github.com/sigstore/cosign/v2/cmd/cosign/cli/options" "github.com/sigstore/cosign/v2/cmd/cosign/cli/rekor" "github.com/sigstore/cosign/v2/pkg/cosign" - ociremote "github.com/sigstore/cosign/v2/pkg/oci/remote" - - "github.com/google/go-containerregistry/pkg/name" - coptions "github.com/sigstore/cosign/v2/cmd/cosign/cli/options" "github.com/sigstore/cosign/v2/pkg/oci" + ociremote "github.com/sigstore/cosign/v2/pkg/oci/remote" "github.com/sigstore/sigstore/pkg/cryptoutils" "github.com/sigstore/sigstore/pkg/signature" ) @@ -93,6 +92,11 @@ func NewCosignVerifier(ctx context.Context, opts ...Options) (*CosignVerifier, e // If there is no public key provided, it will try keyless verification. // https://github.com/sigstore/cosign/blob/main/KEYLESS.md. if len(o.PublicKey) > 0 { + checkOpts.Offline = true + // TODO(hidde): this is an oversight in our implementation. As it is + // theoretically possible to have a custom PK, without disabling tlog. + checkOpts.IgnoreTlog = true + pubKeyRaw, err := cryptoutils.UnmarshalPEMToPublicKey(o.PublicKey) if err != nil { return nil, err @@ -102,32 +106,32 @@ func NewCosignVerifier(ctx context.Context, opts ...Options) (*CosignVerifier, e if err != nil { return nil, err } - - checkOpts.Offline = true - } else { - rcerts, err := fulcio.GetRoots() - if err != nil { - return nil, fmt.Errorf("unable to get Fulcio root certs: %w", err) - } - checkOpts.RootCerts = rcerts - - icerts, err := fulcio.GetIntermediates() - if err != nil { - return nil, fmt.Errorf("unable to get Fulcio intermediate certs: %w", err) - } - checkOpts.IntermediateCerts = icerts - - rc, err := rekor.NewClient(coptions.DefaultRekorURL) + checkOpts.RekorClient, err = rekor.NewClient(coptions.DefaultRekorURL) if err != nil { return nil, fmt.Errorf("unable to create Rekor client: %w", err) } - checkOpts.RekorClient = rc - checkOpts.RekorPubKeys, err = cosign.GetRekorPubs(ctx) - if err != nil { + // This performs an online fetch of the Rekor public keys, but this is needed + // for verifying tlog entries (both online and offline). + // TODO(hidde): above note is important to keep in mind when we implement + // "offline" tlog above. + if checkOpts.RekorPubKeys, err = cosign.GetRekorPubs(ctx); err != nil { return nil, fmt.Errorf("unable to get Rekor public keys: %w", err) } + + checkOpts.CTLogPubKeys, err = cosign.GetCTLogPubs(ctx) + if err != nil { + return nil, fmt.Errorf("unable to get CTLog public keys: %w", err) + } + + if checkOpts.RootCerts, err = fulcio.GetRoots(); err != nil { + return nil, fmt.Errorf("unable to get Fulcio root certs: %w", err) + } + + if checkOpts.IntermediateCerts, err = fulcio.GetIntermediates(); err != nil { + return nil, fmt.Errorf("unable to get Fulcio intermediate certs: %w", err) + } } return &CosignVerifier{