From e2f4e5e7c551d14a45693b59356c0e71b3340d22 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Mon, 26 Sep 2022 18:12:28 +0200 Subject: [PATCH 1/2] Accept a slice of remote.Option for cosign verification If implemented this enable passing a keychain, an authenticator and a custom transport as remote.Option to the verifier. It enables contextual login, self-signed certificates and insecure registries. Signed-off-by: Soule BA refactor makeOptions Reduce complexity by replacing the functional options with a flat out conditional logic in makeOptions. Signed-off-by: Soule BA --- controllers/ocirepository_controller.go | 93 ++++++++++------ controllers/ocirepository_controller_test.go | 6 +- internal/oci/verifier.go | 16 +-- internal/oci/verifier_test.go | 105 +++++++++++++++++++ 4 files changed, 179 insertions(+), 41 deletions(-) create mode 100644 internal/oci/verifier_test.go diff --git a/controllers/ocirepository_controller.go b/controllers/ocirepository_controller.go index 8d1b4128b..d62bb60ff 100644 --- a/controllers/ocirepository_controller.go +++ b/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,7 @@ 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) + err := r.verifySignature(ctx, obj, url, opts.verifyOpts...) if err != nil { provider := obj.Spec.Verify.Provider if obj.Spec.Verify.SecretRef == nil { @@ -429,7 +425,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 +585,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 +593,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 +853,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 +1147,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 +} diff --git a/controllers/ocirepository_controller_test.go b/controllers/ocirepository_controller_test.go index 206ca2fed..41c2e4c37 100644 --- a/controllers/ocirepository_controller_test.go +++ b/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()) @@ -1194,7 +1194,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 +1677,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 { diff --git a/internal/oci/verifier.go b/internal/oci/verifier.go index 17a5345db..b8d9c5d49 100644 --- a/internal/oci/verifier.go +++ b/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 { - return func(opts *options) { - opts.Keychain = keychain +// WithRemoteOptions is a functional option for overriding the default +// remote options used by the verifier. +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 { - co = append(co, ociremote.WithRemoteOptions(remote.WithAuthFromKeychain(o.Keychain))) + if o.ROpt != nil { + co = append(co, ociremote.WithRemoteOptions(o.ROpt...)) } checkOpts.RegistryClientOpts = co diff --git a/internal/oci/verifier_test.go b/internal/oci/verifier_test.go new file mode 100644 index 000000000..8b3ae3865 --- /dev/null +++ b/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) + } + } + }) + } +} From f51c98ecad4c3f497982760c89dc70a96d0289dd Mon Sep 17 00:00:00 2001 From: Soule BA Date: Thu, 29 Sep 2022 11:36:48 +0200 Subject: [PATCH 2/2] Fail when verifying with insecure If implemented we fails when trying to verify with insecure set. This will likely change once cosign add support for insecure registries. Signed-off-by: Soule BA --- controllers/ocirepository_controller.go | 11 ++++++++++ controllers/ocirepository_controller_test.go | 21 ++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/controllers/ocirepository_controller.go b/controllers/ocirepository_controller.go index d62bb60ff..2a6d44429 100644 --- a/controllers/ocirepository_controller.go +++ b/controllers/ocirepository_controller.go @@ -399,6 +399,17 @@ 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) { + + // 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 diff --git a/controllers/ocirepository_controller_test.go b/controllers/ocirepository_controller_test.go index 41c2e4c37..bdd861120 100644 --- a/controllers/ocirepository_controller_test.go +++ b/controllers/ocirepository_controller_test.go @@ -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 '' for ''"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *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"} }