From 9ea484428a3a122a42d432c2633bc94a71cd8b5f Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Fri, 20 Sep 2024 14:12:12 +0900 Subject: [PATCH 1/2] fix: make image digest check more permissive --- cmd/deploy.go | 30 +++++------------------------- cmd/deploy_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 25 deletions(-) diff --git a/cmd/deploy.go b/cmd/deploy.go index e986eff99c..2957d39b64 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -794,42 +794,22 @@ func isDigested(v string) (validDigest bool, err error) { var digest string vv := strings.Split(v, "@") if len(vv) < 2 { - // image does NOT have a digest, validate further - if v == "" { - err = fmt.Errorf("provided image is empty, cannot validate") - return - } - vvv := strings.Split(v, ":") - if len(vvv) < 2 { - // assume user knows what hes doing - return - } else if len(vvv) > 2 { - err = fmt.Errorf("image '%v' contains an invalid tag (extra ':')", v) - return - } - tag := vvv[1] - if tag == "" { - err = fmt.Errorf("image '%v' has an empty tag", v) - return - } - return - } else if len(vv) > 2 { - // image is invalid - err = fmt.Errorf("image '%v' contains an invalid digest (extra '@')", v) - return + return // can not be digested without an @ } - // image has a digest, validate further - digest = vv[1] + // Ensure it has the static string prefix + digest = vv[len(vv)-1] if !strings.HasPrefix(digest, "sha256:") { err = fmt.Errorf("image digest '%s' requires 'sha256:' prefix", digest) return } + // Ensure it has the exact right length if len(digest[7:]) != 64 { err = fmt.Errorf("image digest '%v' has an invalid sha256 hash length of %v when it should be 64", digest, len(digest[7:])) } + // It's likely a valid digest (at least syntactically) validDigest = true return } diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 50d49d28a6..9c11876565 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -2140,3 +2140,39 @@ func TestDeploy_CorrectImageDeployed(t *testing.T) { }) } } + +func Test_isDigested(t *testing.T) { + // Simple references are not digested + var tests = []struct { + Name string + Image string + Digested bool + Error bool + }{ + {Name: "simple", Image: "alpine", Digested: false, Error: false}, + {Name: "tagged", Image: "alpine:latest", Digested: false, Error: false}, + {Name: "domain", Image: "registry.example.com/alice/alpine", Digested: false, Error: false}, + {Name: "domain-tagged", Image: "registry.example.com/alice/alpine:latest", Digested: false, Error: false}, + {Name: "domain-port", Image: "registry.example.com:5001/alice/alpine", Digested: false, Error: false}, + {Name: "domain-port-tagged", Image: "registry.example.com:5001/alice/alpine:latest", Digested: false, Error: false}, + {Name: "domain-port-tagged-digested", Image: "registry.example.com:5001/alice/alpine@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Digested: true, Error: false}, + {Name: "user-domain-port-tagged-digested", Image: "user@registry.example.com:5001/alice/alpine@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Digested: true, Error: false}, + {Name: "error-missing-prefix", Image: "registry.example.com/alice/alpine@e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Digested: false, Error: true}, + {Name: "error-invalid-length", Image: "registry.example.com/alice/alpine@sha256:invalid-hash-value", Digested: false, Error: true}, + } + for _, test := range tests { + t.Run(test.Name, func(t *testing.T) { + digested, err := isDigested(test.Image) + if err != nil { + if test.Error == true { + return // an error was expected + } + t.Fatal(err) + } + if digested != test.Digested { + t.Fatalf("expected image '%v' be digested==%v", test.Image, test.Digested) + } + }) + } + +} From 1be1e142a3219ceec96b9dc656632db337d57df7 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Tue, 24 Sep 2024 15:17:36 +0900 Subject: [PATCH 2/2] use extant implementation for digest check --- cmd/deploy.go | 33 +++++------------- cmd/deploy_test.go | 85 ++++++++++++++++++++++------------------------ 2 files changed, 48 insertions(+), 70 deletions(-) diff --git a/cmd/deploy.go b/cmd/deploy.go index 2957d39b64..a518b4e0c4 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/AlecAivazis/survey/v2" + "github.com/google/go-containerregistry/pkg/name" "github.com/ory/viper" "github.com/spf13/cobra" "k8s.io/apimachinery/pkg/api/resource" @@ -785,31 +786,13 @@ func printDeployMessages(out io.Writer, f fn.Function) { } } -// isDigested returns true if provided image string 'v' has digest and false if not. -// Includes basic validation that a provided digest is correctly formatted. -// Given that image is not digested, image will still be validated and return -// a combination of bool (img has valid digest) and err (img is in valid format) -// Therefore returned combination of [false,nil] means "valid undigested image". +// isDigested checks that the given image reference has a digest. Invalid +// reference return error. func isDigested(v string) (validDigest bool, err error) { - var digest string - vv := strings.Split(v, "@") - if len(vv) < 2 { - return // can not be digested without an @ - } - - // Ensure it has the static string prefix - digest = vv[len(vv)-1] - if !strings.HasPrefix(digest, "sha256:") { - err = fmt.Errorf("image digest '%s' requires 'sha256:' prefix", digest) - return - } - - // Ensure it has the exact right length - if len(digest[7:]) != 64 { - err = fmt.Errorf("image digest '%v' has an invalid sha256 hash length of %v when it should be 64", digest, len(digest[7:])) + ref, err := name.ParseReference(v) + if err != nil { + return false, err } - - // It's likely a valid digest (at least syntactically) - validDigest = true - return + _, ok := ref.(name.Digest) + return ok, nil } diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 9c11876565..0e40175dfc 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -756,34 +756,34 @@ func TestDeploy_ImageWithDigestErrors(t *testing.T) { image string // value to provide as --image build string // If provided, the value of the build flag push bool // if true, explicitly set argument --push=true - errString string // the string value of an expected error + errPrefix string // the string value of an expected error }{ { name: "correctly formatted full image with digest yields no error (degen case)", - image: "example.com/myNamespace/myFunction:latest@sha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4e", + image: "example.com/namespace/myfunction@sha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4e", build: "false", }, { name: "--build forced on yields error", - image: "example.com/myNamespace/myFunction:latest@sha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4e", + image: "example.com/mynamespace/myfunction@sha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4e", build: "true", - errString: "building can not be enabled when using an image with digest", + errPrefix: "building can not be enabled when using an image with digest", }, { name: "push flag explicitly set with digest should error", - image: "example.com/myNamespace/myFunction:latest@sha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4e", + image: "example.com/mynamespace/myfunction@sha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4e", push: true, - errString: "pushing is not valid when specifying an image with digest", + errPrefix: "pushing is not valid when specifying an image with digest", }, { name: "invalid digest prefix 'Xsha256', expect error", - image: "example.com/myNamespace/myFunction:latest@Xsha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4e", - errString: "image digest 'Xsha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4e' requires 'sha256:' prefix", + image: "example.com/mynamespace/myfunction@Xsha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4e", + errPrefix: "could not parse reference", }, { name: "invalid sha hash length(added X at the end), expect error", - image: "example.com/myNamespace/myFunction:latest@sha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4eX", - errString: "image digest 'sha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4eX' has an invalid sha256 hash length of 65 when it should be 64", + image: "example.com/mynamespace/myfunction@sha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4eX", + errPrefix: "could not parse reference", }, } @@ -823,11 +823,11 @@ func TestDeploy_ImageWithDigestErrors(t *testing.T) { cmd.SetArgs(args) err = cmd.Execute() if err != nil { - if tt.errString == "" { + if tt.errPrefix == "" { t.Fatal(err) // no error was expected. fail } - if tt.errString != err.Error() { - t.Fatalf("expected error '%v' not received. got '%v'", tt.errString, err.Error()) + if !strings.HasPrefix(err.Error(), tt.errPrefix) { + t.Fatalf("expected error prefix '%v' not received. got '%v'", tt.errPrefix, err.Error()) } // There was an error, but it was expected } @@ -842,7 +842,7 @@ func TestDeploy_ImageWithDigestErrors(t *testing.T) { func TestDeploy_ImageWithDigestDoesntPopulateBuild(t *testing.T) { root := FromTempDirectory(t) // image with digest (well almost, atleast in length and syntax) - const img = "docker.io/4141gauron3268@sha256:XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" + const img = "docker.io/4141gauron3268@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" // Create a new Function in the temp directory _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { @@ -869,7 +869,7 @@ func TestDeploy_ImageWithDigestDoesntPopulateBuild(t *testing.T) { // TestDeploy_WithoutDigest ensures that images specified with --image and // without digest are correctly processed and propagated to .Deploy.Image func TestDeploy_WithoutDigest(t *testing.T) { - const sha = "sha256:xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + const sha = "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" tests := []struct { name string //name of the test @@ -2141,38 +2141,33 @@ func TestDeploy_CorrectImageDeployed(t *testing.T) { } } +// Test_isDigested ensures that the function is properly delegating to +// by checking both that it will fail on an invalid reference and will +// return true if the image contains a digest and false otherwise. +// See the delegate from the implementation for comprehensive tests. func Test_isDigested(t *testing.T) { - // Simple references are not digested - var tests = []struct { - Name string - Image string - Digested bool - Error bool - }{ - {Name: "simple", Image: "alpine", Digested: false, Error: false}, - {Name: "tagged", Image: "alpine:latest", Digested: false, Error: false}, - {Name: "domain", Image: "registry.example.com/alice/alpine", Digested: false, Error: false}, - {Name: "domain-tagged", Image: "registry.example.com/alice/alpine:latest", Digested: false, Error: false}, - {Name: "domain-port", Image: "registry.example.com:5001/alice/alpine", Digested: false, Error: false}, - {Name: "domain-port-tagged", Image: "registry.example.com:5001/alice/alpine:latest", Digested: false, Error: false}, - {Name: "domain-port-tagged-digested", Image: "registry.example.com:5001/alice/alpine@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Digested: true, Error: false}, - {Name: "user-domain-port-tagged-digested", Image: "user@registry.example.com:5001/alice/alpine@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Digested: true, Error: false}, - {Name: "error-missing-prefix", Image: "registry.example.com/alice/alpine@e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", Digested: false, Error: true}, - {Name: "error-invalid-length", Image: "registry.example.com/alice/alpine@sha256:invalid-hash-value", Digested: false, Error: true}, + var digested bool + var err error + + // Ensure validation + _, err = isDigested("invalid&image@sha256:12345") + if err == nil { + t.Fatal("did not validate image reference") } - for _, test := range tests { - t.Run(test.Name, func(t *testing.T) { - digested, err := isDigested(test.Image) - if err != nil { - if test.Error == true { - return // an error was expected - } - t.Fatal(err) - } - if digested != test.Digested { - t.Fatalf("expected image '%v' be digested==%v", test.Image, test.Digested) - } - }) + + // Ensure positive case + if digested, err = isDigested("alpine"); err != nil { + t.Fatal(err) + } + if digested { + t.Fatal("reported digested on undigested image reference") } + // Ensure negative case + if digested, err = isDigested("alpine@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"); err != nil { + t.Fatal(err) + } + if !digested { + t.Fatal("did not report image reference has digest") + } }