Skip to content

Commit

Permalink
Merge pull request #916 from souleb/fix-915
Browse files Browse the repository at this point in the history 
Accept a slice of remote.Option for cosign verification
  • Loading branch information
@stefanprodan
stefanprodan authored Sep 29, 2022
2 parents 95cbf40 + f51c98e commit 2a2b525
Show file tree
Hide file tree
Showing 4 changed files with 211 additions and 41 deletions.
104 changes: 73 additions & 31 deletions controllers/ocirepository_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -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 {
Expand All @@ -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),
Expand All @@ -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
Expand All @@ -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(
Expand All @@ -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),
Expand Down Expand Up @@ -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 {
Expand All @@ -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),
Expand Down Expand Up @@ -589,15 +596,15 @@ 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()

provider := obj.Spec.Verify.Provider
switch provider {
case "cosign":
defaultCosignOciOpts := []soci.Options{
soci.WithAuthnKeychain(keychain),
soci.WithRemoteOptions(opt...),
}

ref, err := name.ParseReference(url)
Expand Down Expand Up @@ -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.
//
Expand Down Expand Up @@ -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
}
27 changes: 24 additions & 3 deletions controllers/ocirepository_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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"}
}
Expand All @@ -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())
Expand Down Expand Up @@ -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 {
Expand Down
16 changes: 9 additions & 7 deletions internal/oci/verifier.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand All @@ -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
}
}

Expand All @@ -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
Expand Down
Loading

0 comments on commit 2a2b525

Please sign in to comment.