From bc57ad938ab0a613e5e23fa0fa9468fafec7c2be Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Thu, 23 Sep 2021 17:33:09 -0700 Subject: [PATCH] Ensure we resolve tags once. As I've been refactoring things, I noticed a handful of places where a single command potentially used a `name.Tag` multiple times for remote access. There is an exceedingly small window here for a race to happen where the first tag access points to one image, and the subsequent access points to another. I've already fixed a handful of these, and this change fixes one more (in `sget` IIRC), but also tries to add some defensive logic in a few places where we were already doing the right thing. The defensive logic I added is to clobber `ref` with `digest` after resolving the reference: ```go digest, err := ociremote.ResolveDigest(ref, regOpts.ClientOpts(ctx)...) if err != nil { return err } // Overwrite "ref" with a digest to avoid a race where we use a tag // multiple times, and it potentially points to different things at // each access. ref = digest ``` This ensures that regardless of which reference is used below resolution, we always get what we resolved at this point. There are some other superficial cleanups lumped in, which I noticed as I skimmed the code. Signed-off-by: Matt Moore --- cmd/cosign/cli/attach/sbom.go | 7 ++---- cmd/cosign/cli/attach/sig.go | 5 ++++- cmd/cosign/cli/attest.go | 4 ++++ cmd/cosign/cli/generate/generate.go | 5 ++++- pkg/cosign/kubernetes/webhook/validation.go | 1 + pkg/sget/sget.go | 24 ++++++++++++--------- 6 files changed, 29 insertions(+), 17 deletions(-) diff --git a/cmd/cosign/cli/attach/sbom.go b/cmd/cosign/cli/attach/sbom.go index 1541d2d02004..3875a7604d7b 100644 --- a/cmd/cosign/cli/attach/sbom.go +++ b/cmd/cosign/cli/attach/sbom.go @@ -86,11 +86,8 @@ func SBOMCmd(ctx context.Context, regOpts options.RegistryOpts, sbomRef, sbomTyp } fmt.Fprintf(os.Stderr, "Uploading SBOM file for [%s] to [%s] with mediaType [%s].\n", ref.Name(), dstRef.Name(), sbomType) - if _, err := cremote.UploadFile(b, dstRef, types.MediaType(sbomType), types.OCIConfigJSON, remoteOpts...); err != nil { - return err - } - - return nil + _, err = cremote.UploadFile(b, dstRef, types.MediaType(sbomType), types.OCIConfigJSON, remoteOpts...) + return err } func sbomBytes(sbomRef string) ([]byte, error) { diff --git a/cmd/cosign/cli/attach/sig.go b/cmd/cosign/cli/attach/sig.go index a803dceb1281..6781a4b3ec82 100644 --- a/cmd/cosign/cli/attach/sig.go +++ b/cmd/cosign/cli/attach/sig.go @@ -69,11 +69,14 @@ func SignatureCmd(ctx context.Context, regOpts options.RegistryOpts, sigRef, pay if err != nil { return err } - digest, err := ociremote.ResolveDigest(ref, regOpts.ClientOpts(ctx)...) if err != nil { return err } + // Overwrite "ref" with a digest to avoid a race where we use a tag + // multiple times, and it potentially points to different things at + // each access. + ref = digest var payload []byte if payloadRef == "" { diff --git a/cmd/cosign/cli/attest.go b/cmd/cosign/cli/attest.go index 5f511642a0ee..b002876b7cf1 100644 --- a/cmd/cosign/cli/attest.go +++ b/cmd/cosign/cli/attest.go @@ -154,6 +154,10 @@ func AttestCmd(ctx context.Context, ko sign.KeyOpts, regOpts options.RegistryOpt return err } h, _ := v1.NewHash(digest.Identifier()) + // Overwrite "ref" with a digest to avoid a race where we use a tag + // multiple times, and it potentially points to different things at + // each access. + ref = digest sv, err := sign.SignerFromKeyOpts(ctx, certPath, ko) if err != nil { diff --git a/cmd/cosign/cli/generate/generate.go b/cmd/cosign/cli/generate/generate.go index 80ba43660a17..199aa3177a02 100644 --- a/cmd/cosign/cli/generate/generate.go +++ b/cmd/cosign/cli/generate/generate.go @@ -73,11 +73,14 @@ func GenerateCmd(ctx context.Context, regOpts options.RegistryOpts, imageRef str if err != nil { return err } - digest, err := ociremote.ResolveDigest(ref, regOpts.ClientOpts(ctx)...) if err != nil { return err } + // Overwrite "ref" with a digest to avoid a race where we use a tag + // multiple times, and it potentially points to different things at + // each access. + ref = digest json, err := (&payload.Cosign{Image: digest, Annotations: annotations}).MarshalJSON() if err != nil { diff --git a/pkg/cosign/kubernetes/webhook/validation.go b/pkg/cosign/kubernetes/webhook/validation.go index 20449090232f..d508b7a68aeb 100644 --- a/pkg/cosign/kubernetes/webhook/validation.go +++ b/pkg/cosign/kubernetes/webhook/validation.go @@ -49,6 +49,7 @@ func valid(ctx context.Context, img string, keys []*ecdsa.PublicKey) bool { } func validSignatures(ctx context.Context, img string, key *ecdsa.PublicKey) ([]oci.Signature, error) { + // TODO(mattmoor): take the name.Reference as the param? ref, err := name.ParseReference(img) if err != nil { return nil, err diff --git a/pkg/sget/sget.go b/pkg/sget/sget.go index 1a41d5de9f92..7facb7a6ab09 100644 --- a/pkg/sget/sget.go +++ b/pkg/sget/sget.go @@ -52,12 +52,6 @@ func (sg *SecureGet) Do(ctx context.Context) error { return err } - if _, ok := ref.(name.Tag); ok { - if sg.KeyRef == "" && !options.EnableExperimental() { - return errors.New("public key must be specified when fetching by tag, you must fetch by digest or supply a public key") - } - } - opts := []remote.Option{ remote.WithAuthFromKeychain(authn.DefaultKeychain), remote.WithContext(ctx), @@ -67,6 +61,19 @@ func (sg *SecureGet) Do(ctx context.Context) error { ClaimVerifier: cosign.SimpleClaimVerifier, RegistryClientOpts: []ociremote.Option{ociremote.WithRemoteOptions(opts...)}, } + if _, ok := ref.(name.Tag); ok { + if sg.KeyRef == "" && !options.EnableExperimental() { + return errors.New("public key must be specified when fetching by tag, you must fetch by digest or supply a public key") + } + } + // Overwrite "ref" with a digest to avoid a race where we verify the tag, + // and then access the file through the tag. This has a race where we + // might download content that isn't what we verified. + ref, err = ociremote.ResolveDigest(ref, co.RegistryClientOpts...) + if err != nil { + return err + } + if sg.KeyRef != "" { pub, err := sigs.LoadPublicKey(ctx, sg.KeyRef) if err != nil { @@ -104,8 +111,5 @@ func (sg *SecureGet) Do(ctx context.Context) error { } _, err = io.Copy(sg.Out, rc) - if err != nil { - return err - } - return nil + return err }