Merge pull request #916 from souleb/fix-915

Accept a slice of remote.Option for cosign verification

controllers/ocirepository_controller.go

@@ -299,6 +299,8 @@ func (r *OCIRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.O
 // reconcileSource fetches the upstream OCI artifact metadata and content.
 // If this fails, it records v1beta2.FetchFailedCondition=True on the object and returns early.
 func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) {
+	var auth authn.Authenticator
+
 	ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
 	defer cancel()
 
@@ -310,8 +312,6 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour
 		conditions.Delete(obj, sourcev1.SourceVerifiedCondition)
 	}
 
-	options := r.craneOptions(ctxTimeout, obj.Spec.Insecure)
-
 	// Generate the registry credential keychain either from static credentials or using cloud OIDC
 	keychain, err := r.keychain(ctx, obj)
 	if err != nil {
@@ -322,10 +322,10 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		return sreconcile.ResultEmpty, e
 	}
-	options = append(options, crane.WithAuthFromKeychain(keychain))
 
 	if _, ok := keychain.(soci.Anonymous); obj.Spec.Provider != sourcev1.GenericOCIProvider && ok {
-		auth, authErr := oidcAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider)
+		var authErr error
+		auth, authErr = oidcAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider)
 		if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) {
 			e := serror.NewGeneric(
 				fmt.Errorf("failed to get credential from %s: %w", obj.Spec.Provider, authErr),
@@ -334,9 +334,6 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour
 			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 			return sreconcile.ResultEmpty, e
 		}
-		if auth != nil {
-			options = append(options, crane.WithAuth(auth))
-		}
 	}
 
 	// Generate the transport for remote operations
@@ -349,12 +346,11 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		return sreconcile.ResultEmpty, e
 	}
-	if transport != nil {
+
-		options = append(options, crane.WithTransport(transport))
+	opts := makeRemoteOptions(ctx, obj, transport, keychain, auth)
-	}
 
 	// Determine which artifact revision to pull
-	url, err := r.getArtifactURL(obj, options)
+	url, err := r.getArtifactURL(obj, opts.craneOpts)
 	if err != nil {
 		if _, ok := err.(invalidOCIURLError); ok {
 			e := serror.NewStalling(
@@ -372,7 +368,7 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour
 	}
 
 	// Get the upstream revision from the artifact digest
-	revision, err := r.getRevision(url, options)
+	revision, err := r.getRevision(url, opts.craneOpts)
 	if err != nil {
 		e := serror.NewGeneric(
 			fmt.Errorf("failed to determine artifact digest: %w", err),
@@ -403,7 +399,18 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour
 	} else if !obj.GetArtifact().HasRevision(revision) ||
 		conditions.GetObservedGeneration(obj, sourcev1.SourceVerifiedCondition) != obj.Generation ||
 		conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) {
-		err := r.verifySignature(ctx, obj, url, keychain)
+
+		// Insecure is not supported for verification
+		if obj.Spec.Insecure {
+			e := serror.NewGeneric(
+				fmt.Errorf("cosign does not support insecure registries"),
+				sourcev1.VerificationError,
+			)
+			conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())
+			return sreconcile.ResultEmpty, e
+		}
+
+		err := r.verifySignature(ctx, obj, url, opts.verifyOpts...)
 		if err != nil {
 			provider := obj.Spec.Verify.Provider
 			if obj.Spec.Verify.SecretRef == nil {
@@ -429,7 +436,7 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour
 	}
 
 	// Pull artifact from the remote container registry
-	img, err := crane.Pull(url, options...)
+	img, err := crane.Pull(url, opts.craneOpts...)
 	if err != nil {
 		e := serror.NewGeneric(
 			fmt.Errorf("failed to pull artifact from '%s': %w", obj.Spec.URL, err),
@@ -589,7 +596,7 @@ func (r *OCIRepositoryReconciler) digestFromRevision(revision string) string {
 
 // verifySignature verifies the authenticity of the given image reference url. First, it tries using a key
 // if a secret with a valid public key is provided. If not, it falls back to a keyless approach for verification.
-func (r *OCIRepositoryReconciler) verifySignature(ctx context.Context, obj *sourcev1.OCIRepository, url string, keychain authn.Keychain) error {
+func (r *OCIRepositoryReconciler) verifySignature(ctx context.Context, obj *sourcev1.OCIRepository, url string, opt ...remote.Option) error {
 	ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
 	defer cancel()
 
@@ -597,7 +604,7 @@ func (r *OCIRepositoryReconciler) verifySignature(ctx context.Context, obj *sour
 	switch provider {
 	case "cosign":
 		defaultCosignOciOpts := []soci.Options{
-			soci.WithAuthnKeychain(keychain),
+			soci.WithRemoteOptions(opt...),
 		}
 
 		ref, err := name.ParseReference(url)
@@ -857,21 +864,6 @@ func oidcAuth(ctx context.Context, url, provider string) (authn.Authenticator, e
 	return login.NewManager().Login(ctx, u, ref, opts)
 }
 
-// craneOptions sets the auth headers, timeout and user agent
-// for all operations against remote container registries.
-func (r *OCIRepositoryReconciler) craneOptions(ctx context.Context, insecure bool) []crane.Option {
-	options := []crane.Option{
-		crane.WithContext(ctx),
-		crane.WithUserAgent(oci.UserAgent),
-	}
-
-	if insecure {
-		options = append(options, crane.Insecure)
-	}
-
-	return options
-}
-
 // reconcileStorage ensures the current state of the storage matches the
 // desired and previously observed state.
 //
@@ -1166,3 +1158,53 @@ func (r *OCIRepositoryReconciler) calculateContentConfigChecksum(obj *sourcev1.O
 
 	return fmt.Sprintf("sha256:%x", sha256.Sum256(c))
 }
+
+// craneOptions sets the auth headers, timeout and user agent
+// for all operations against remote container registries.
+func craneOptions(ctx context.Context, insecure bool) []crane.Option {
+	options := []crane.Option{
+		crane.WithContext(ctx),
+		crane.WithUserAgent(oci.UserAgent),
+	}
+
+	if insecure {
+		options = append(options, crane.Insecure)
+	}
+
+	return options
+}
+
+// makeRemoteOptions returns a remoteOptions struct with the authentication and transport options set.
+// The returned struct can be used to interact with a remote registry using go-containerregistry based libraries.
+func makeRemoteOptions(ctxTimeout context.Context, obj *sourcev1.OCIRepository, transport http.RoundTripper,
+	keychain authn.Keychain, auth authn.Authenticator) remoteOptions {
+	o := remoteOptions{
+		craneOpts:  craneOptions(ctxTimeout, obj.Spec.Insecure),
+		verifyOpts: []remote.Option{},
+	}
+
+	if transport != nil {
+		o.craneOpts = append(o.craneOpts, crane.WithTransport(transport))
+		o.verifyOpts = append(o.verifyOpts, remote.WithTransport(transport))
+	}
+
+	if auth != nil {
+		// auth take precedence over keychain here as we expect the caller to set
+		// the auth only if it is required.
+		o.verifyOpts = append(o.verifyOpts, remote.WithAuth(auth))
+		o.craneOpts = append(o.craneOpts, crane.WithAuth(auth))
+		return o
+	}
+
+	o.verifyOpts = append(o.verifyOpts, remote.WithAuthFromKeychain(keychain))
+	o.craneOpts = append(o.craneOpts, crane.WithAuthFromKeychain(keychain))
+
+	return o
+}
+
+// remoteOptions contains the options to interact with a remote registry.
+// It can be used to pass options to go-containerregistry based libraries.
+type remoteOptions struct {
+	craneOpts  []crane.Option
+	verifyOpts []remote.Option
+}

controllers/ocirepository_controller_test.go

@@ -681,7 +681,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) {
 				Storage:       testStorage,
 			}
 
-			opts := r.craneOptions(ctx, true)
+			opts := craneOptions(ctx, true)
 			opts = append(opts, crane.WithAuthFromKeychain(authn.DefaultKeychain))
 			repoURL, err := r.getArtifactURL(obj, opts)
 			g.Expect(err).To(BeNil())
@@ -1036,6 +1036,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) {
 	tests := []struct {
 		name             string
 		reference        *sourcev1.OCIRepositoryRef
+		insecure         bool
 		digest           string
 		want             sreconcile.Result
 		wantErr          bool
@@ -1132,6 +1133,22 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) {
 				*conditions.TrueCondition(sourcev1.SourceVerifiedCondition, "Verified", "verified"),
 			},
 		},
+		{
+			name: "insecure registries are not supported",
+			reference: &sourcev1.OCIRepositoryRef{
+				Tag: "6.1.4",
+			},
+			digest:     img4.digest.Hex,
+			shouldSign: true,
+			insecure:   true,
+			wantErr:    true,
+			want:       sreconcile.ResultEmpty,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '<digest>' for '<url>'"),
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '<digest>' for '<url>'"),
+				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "cosign does not support insecure registries"),
+			},
+		},
 	}
 
 	builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme())
@@ -1181,6 +1198,10 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) {
 				},
 			}
 
+			if tt.insecure {
+				obj.Spec.Insecure = true
+			}
+
 			if !tt.keyless {
 				obj.Spec.Verify.SecretRef = &meta.LocalObjectReference{Name: "cosign-key"}
 			}
@@ -1194,7 +1215,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) {
 				g.Expect(err).ToNot(HaveOccurred())
 			}
 
-			opts := r.craneOptions(ctx, true)
+			opts := craneOptions(ctx, true)
 			opts = append(opts, crane.WithAuthFromKeychain(keychain))
 			artifactURL, err := r.getArtifactURL(obj, opts)
 			g.Expect(err).ToNot(HaveOccurred())
@@ -1677,7 +1698,7 @@ func TestOCIRepository_getArtifactURL(t *testing.T) {
 				obj.Spec.Reference = tt.reference
 			}
 
-			opts := r.craneOptions(ctx, true)
+			opts := craneOptions(ctx, true)
 			opts = append(opts, crane.WithAuthFromKeychain(authn.DefaultKeychain))
 			got, err := r.getArtifactURL(obj, opts)
 			if tt.wantErr {

internal/oci/verifier.go

@@ -20,7 +20,7 @@ import (
 	"context"
 	"crypto"
 	"fmt"
-	"github.com/google/go-containerregistry/pkg/authn"
+
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	"github.com/sigstore/cosign/cmd/cosign/cli/fulcio"
 	"github.com/sigstore/cosign/cmd/cosign/cli/rekor"
@@ -37,7 +37,7 @@ import (
 // options is a struct that holds options for verifier.
 type options struct {
 	PublicKey []byte
-	Keychain  authn.Keychain
+	ROpt      []remote.Option
 }
 
 // Options is a function that configures the options applied to a Verifier.
@@ -50,9 +50,11 @@ func WithPublicKey(publicKey []byte) Options {
 	}
 }
 
-func WithAuthnKeychain(keychain authn.Keychain) Options {
+// WithRemoteOptions is a functional option for overriding the default
-	return func(opts *options) {
+// remote options used by the verifier.
-		opts.Keychain = keychain
+func WithRemoteOptions(opts ...remote.Option) Options {
+	return func(o *options) {
+		o.ROpt = opts
 	}
 }
 
@@ -76,8 +78,8 @@ func NewVerifier(ctx context.Context, opts ...Options) (*Verifier, error) {
 		return nil, err
 	}
 
-	if o.Keychain != nil {
+	if o.ROpt != nil {
-		co = append(co, ociremote.WithRemoteOptions(remote.WithAuthFromKeychain(o.Keychain)))
+		co = append(co, ociremote.WithRemoteOptions(o.ROpt...))
 	}
 
 	checkOpts.RegistryClientOpts = co

internal/oci/verifier_test.go

@@ -0,0 +1,105 @@
+/*
+Copyright 2022 The Flux 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 oci
+
+import (
+	"net/http"
+	"reflect"
+	"testing"
+
+	"github.com/google/go-containerregistry/pkg/authn"
+	"github.com/google/go-containerregistry/pkg/v1/remote"
+)
+
+func TestOptions(t *testing.T) {
+	tests := []struct {
+		name string
+		opts []Options
+		want *options
+	}{{
+		name: "no options",
+		want: &options{},
+	}, {
+		name: "signature option",
+		opts: []Options{WithPublicKey([]byte("foo"))},
+		want: &options{
+			PublicKey: []byte("foo"),
+			ROpt:      nil,
+		},
+	}, {
+		name: "keychain option",
+		opts: []Options{WithRemoteOptions(remote.WithAuthFromKeychain(authn.DefaultKeychain))},
+		want: &options{
+			PublicKey: nil,
+			ROpt:      []remote.Option{remote.WithAuthFromKeychain(authn.DefaultKeychain)},
+		},
+	}, {
+		name: "keychain and authenticator option",
+		opts: []Options{WithRemoteOptions(
+			remote.WithAuth(&authn.Basic{Username: "foo", Password: "bar"}),
+			remote.WithAuthFromKeychain(authn.DefaultKeychain),
+		)},
+		want: &options{
+			PublicKey: nil,
+			ROpt: []remote.Option{
+				remote.WithAuth(&authn.Basic{Username: "foo", Password: "bar"}),
+				remote.WithAuthFromKeychain(authn.DefaultKeychain),
+			},
+		},
+	}, {
+		name: "keychain, authenticator and transport option",
+		opts: []Options{WithRemoteOptions(
+			remote.WithAuth(&authn.Basic{Username: "foo", Password: "bar"}),
+			remote.WithAuthFromKeychain(authn.DefaultKeychain),
+			remote.WithTransport(http.DefaultTransport),
+		)},
+		want: &options{
+			PublicKey: nil,
+			ROpt: []remote.Option{
+				remote.WithAuth(&authn.Basic{Username: "foo", Password: "bar"}),
+				remote.WithAuthFromKeychain(authn.DefaultKeychain),
+				remote.WithTransport(http.DefaultTransport),
+			},
+		},
+	},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			o := options{}
+			for _, opt := range test.opts {
+				opt(&o)
+			}
+			if !reflect.DeepEqual(o.PublicKey, test.want.PublicKey) {
+				t.Errorf("got %#v, want %#v", &o.PublicKey, test.want.PublicKey)
+			}
+
+			if test.want.ROpt != nil {
+				if len(o.ROpt) != len(test.want.ROpt) {
+					t.Errorf("got %d remote options, want %d", len(o.ROpt), len(test.want.ROpt))
+				}
+				return
+			}
+
+			if test.want.ROpt == nil {
+				if len(o.ROpt) != 0 {
+					t.Errorf("got %d remote options, want %d", len(o.ROpt), 0)
+				}
+			}
+		})
+	}
+}