From 3561fa85924de2abe23a34b1317b95b583c92dad Mon Sep 17 00:00:00 2001 From: Jason Hall Date: Tue, 24 May 2022 09:20:02 -0400 Subject: [PATCH] Loosen glob requirements, add tests, remove warnings for now Signed-off-by: Jason Hall --- pkg/apis/config/glob.go | 34 ++++++++++------ pkg/apis/config/glob_test.go | 36 ++++++++++------- pkg/apis/config/image_policies.go | 2 +- pkg/apis/config/image_policies_test.go | 55 ++++++++++++++++---------- 4 files changed, 79 insertions(+), 48 deletions(-) diff --git a/pkg/apis/config/glob.go b/pkg/apis/config/glob.go index f4eb1d0e899..ab61b90a4c9 100644 --- a/pkg/apis/config/glob.go +++ b/pkg/apis/config/glob.go @@ -46,34 +46,46 @@ var validGlob = regexp.MustCompile(`^[a-zA-Z0-9-_:\/\*\.]+$`) // // Note that the tag delimiter (":") does not act as a breaking separator for the purposes of a "*" glob. // To match any tag, the glob should end with ":**". -func GlobMatch(glob, image string) (match bool, warnings []string, err error) { +func GlobMatch(glob, image string) (bool, error) { if glob == "*/*" { - warnings = []string{`The glob match "*/*" should be "index.docker.io/*/*"`} + // TODO: Warn that the glob match "*/*" should be "index.docker.io/*/*". glob = "index.docker.io/*/*" } if glob == "*" { - warnings = []string{`The glob match "*" should be "index.docker.io/library/*"`} + // TODO: Warn that the glob match "*" should be "index.docker.io/library/*". glob = "index.docker.io/library/*" } ref, err := name.ParseReference(image, name.WeakValidation) if err != nil { - return false, warnings, err + return false, err } // Reject that glob doesn't look like a regexp if !validGlob.MatchString(glob) { - return false, warnings, fmt.Errorf("invalid glob %q", glob) + return false, fmt.Errorf("invalid glob %q", glob) } // Translate glob to regexp. - glob = strings.ReplaceAll(glob, ".", `\.`) // . in glob means \. in regexp - glob = strings.ReplaceAll(glob, "**", ".+") // ** in glob means .* in regexp - glob = strings.ReplaceAll(glob, "*", "[^/]+") // * in glob means any non-/ in regexp - glob = fmt.Sprintf("^%s$", glob) // glob must match the whole string + glob = strings.ReplaceAll(glob, ".", `\.`) // . in glob means \. in regexp + glob = strings.ReplaceAll(glob, "**", "#") // ** in glob means 0+ of any character in regexp + // We replace ** with a placeholder here, rather than `.*` directly, because the next line + // would replace that `*` again, breaking the regexp. So we stash the change with a placeholder, + // then replace the placeholder later to preserve the original intent. + glob = strings.ReplaceAll(glob, "*", "[^/]*") // * in glob means 0+ of any non-`/` character in regexp + glob = strings.ReplaceAll(glob, "#", ".*") + glob = fmt.Sprintf("^%s$", glob) // glob must match the whole string // TODO: do we want ":" to count as a separator like "/" is? - match, err = regexp.MatchString(glob, ref.Name()) - return match, warnings, err + match, err := regexp.MatchString(glob, ref.Name()) + if err != nil { + return false, err + } + if !match && ref.Name() != image { + // If the image was not fully qualified, try matching the glob against the original non-fully-qualified. + // This should be a warning and this behavior should eventually be removed. + match, err = regexp.MatchString(glob, image) + } + return match, err } diff --git a/pkg/apis/config/glob_test.go b/pkg/apis/config/glob_test.go index e845ce25e89..fa1ea3d25a5 100644 --- a/pkg/apis/config/glob_test.go +++ b/pkg/apis/config/glob_test.go @@ -15,16 +15,14 @@ package config import ( - "reflect" "testing" ) func TestGlobMatch(t *testing.T) { for _, c := range []struct { - image, glob string - wantMatch bool - wantWarnings []string - wantErr bool + image, glob string + wantMatch bool + wantErr bool }{ {image: "foo", glob: "index.docker.io/library/foo:latest", wantMatch: true}, {image: "foo", glob: "index.docker.io/library/foo:*", wantMatch: true}, @@ -35,7 +33,10 @@ func TestGlobMatch(t *testing.T) { {image: "foo", glob: "index.docker.**", wantMatch: true}, {image: "foo", glob: "inde**", wantMatch: true}, {image: "foo", glob: "**", wantMatch: true}, - {image: "foo", glob: "foo", wantMatch: false}, // must have index.docker.io/library prefix. + {image: "foo", glob: "foo", wantMatch: true}, // matches because of deprecated fallback logic. + {image: "foo", glob: "foo*", wantMatch: true}, // * matches 0+ characters + {image: "foo", glob: "foo**", wantMatch: true}, // ** matches 0+ characters + {image: "myuser/myapp", glob: "index.docker.io/myuser/myapp:latest", wantMatch: true}, {image: "myuser/myapp", glob: "index.docker.io/myuser/myapp:*", wantMatch: true}, {image: "myuser/myapp", glob: "index.docker.io/myuser/*", wantMatch: true}, @@ -45,7 +46,11 @@ func TestGlobMatch(t *testing.T) { {image: "myuser/myapp", glob: "index.docker.**", wantMatch: true}, {image: "myuser/myapp", glob: "inde**", wantMatch: true}, {image: "myuser/myapp", glob: "**", wantMatch: true}, - {image: "myuser/myapp", glob: "myuser/myapp", wantMatch: false}, // must have index.docker.io prefix. + {image: "myuser/myapp", glob: "myuser/myapp", wantMatch: true}, // matches because of deprecated fallback logic. + {image: "myuser/myapp", glob: "myuser/myapp*", wantMatch: true}, // * matches 0+ characters + {image: "myuser/myapp", glob: "myuser/myapp**", wantMatch: true}, // ** matches 0+ characters + + // Fully qualified refs and globs. {image: "ghcr.io/foo/bar", glob: "ghcr.io/*/*", wantMatch: true}, {image: "ghcr.io/foo/bar", glob: "ghcr.io/**", wantMatch: true}, {image: "ghcr.io/foo", glob: "ghcr.io/*/*", wantMatch: false}, // doesn't match second * @@ -53,25 +58,26 @@ func TestGlobMatch(t *testing.T) { {image: "ghcr.io/foo", glob: "ghc**", wantMatch: true}, {image: "ghcr.io/foo", glob: "**", wantMatch: true}, {image: "ghcr.io/foo", glob: "*/**", wantMatch: true}, + {image: "ghcr.io/foo", glob: "ghcr.io/foo*", wantMatch: true}, // * matches 0+ characters + {image: "ghcr.io/foo", glob: "ghcr.io/foo**", wantMatch: true}, // ** matches 0+ characters + + // Various error cases. {image: "prefix-ghcr.io/foo", glob: "ghcr.io/foo", wantMatch: false}, // glob starts at beginning. {image: "ghcr.io/foo-suffix", glob: "ghcr.io/foo", wantMatch: false}, // glob ends at the end. {image: "ghcrxio/foo", glob: "ghcr.io/**", wantMatch: false}, // dots in glob are replaced with \., not treated as regexp . {image: "invalid&name", glob: "**", wantMatch: false, wantErr: true}, // invalid refs are not matched. {image: "invalid-glob", glob: ".+", wantMatch: false, wantErr: true}, // invalid globs are rejected. {image: "invalid-glob", glob: "[a-z]*", wantMatch: false, wantErr: true}, // invalid globs are rejected. - {image: "foo", glob: "*", wantMatch: true, - wantWarnings: []string{`The glob match "*" should be "index.docker.io/library/*"`}}, - {image: "myuser/myapp", glob: "*/*", wantMatch: true, - wantWarnings: []string{`The glob match "*/*" should be "index.docker.io/*/*"`}}, + + // Upgrading unqualified globs to assume index.docker.io prefix. + {image: "foo", glob: "*", wantMatch: true}, + {image: "myuser/myapp", glob: "*/*", wantMatch: true}, } { t.Run(c.image+"|"+c.glob, func(t *testing.T) { - match, warnings, err := GlobMatch(c.glob, c.image) + match, err := GlobMatch(c.glob, c.image) if match != c.wantMatch { t.Errorf("match: got %t, want %t", match, c.wantMatch) } - if !reflect.DeepEqual(warnings, c.wantWarnings) { - t.Errorf("warnings: got %v, want %v", warnings, c.wantWarnings) - } if gotErr := err != nil; gotErr != c.wantErr { t.Errorf("err: got %v, want %t", err, c.wantErr) } diff --git a/pkg/apis/config/image_policies.go b/pkg/apis/config/image_policies.go index 73aeba17630..9f7a6948194 100644 --- a/pkg/apis/config/image_policies.go +++ b/pkg/apis/config/image_policies.go @@ -92,7 +92,7 @@ func (p *ImagePolicyConfig) GetMatchingPolicies(image string) (map[string]webhoo for k, v := range p.Policies { for _, pattern := range v.Images { if pattern.Glob != "" { - if matched, _, err := GlobMatch(pattern.Glob, image); err != nil { + if matched, err := GlobMatch(pattern.Glob, image); err != nil { lastError = err } else if matched { ret[k] = v diff --git a/pkg/apis/config/image_policies_test.go b/pkg/apis/config/image_policies_test.go index 1c9c91a95da..6e14b59a64a 100644 --- a/pkg/apis/config/image_policies_test.go +++ b/pkg/apis/config/image_policies_test.go @@ -42,6 +42,19 @@ func TestDefaultsConfigurationFromFile(t *testing.T) { } func TestGetAuthorities(t *testing.T) { + // TODO: Clean up this test to be table-driven with sub-tests, instead of one big test. + getAuthority := func(t *testing.T, m map[string]webhookcip.ClusterImagePolicy, mp string) webhookcip.Authority { + t.Helper() + cip, found := m[mp] + if !found { + t.Fatalf("failed to find matching policy %q", mp) + } + if len(cip.Authorities) == 0 { + t.Fatalf("no authorities for matching policy %q", mp) + } + return cip.Authorities[0] + } + _, example := ConfigMapsFromTestFile(t, ImagePoliciesConfigName) defaults, err := NewImagePoliciesConfigFromConfigMap(example) if err != nil { @@ -51,7 +64,7 @@ func TestGetAuthorities(t *testing.T) { checkGetMatches(t, c, err) matchedPolicy := "cluster-image-policy-0" want := "inlinedata here" - if got := c[matchedPolicy].Authorities[0].Key.Data; got != want { + if got := getAuthority(t, c, matchedPolicy).Key.Data; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } // Make sure glob matches 'randomstuff*' @@ -59,22 +72,22 @@ func TestGetAuthorities(t *testing.T) { checkGetMatches(t, c, err) matchedPolicy = "cluster-image-policy-1" want = "otherinline here" - if got := c[matchedPolicy].Authorities[0].Key.Data; got != want { + if got := getAuthority(t, c, matchedPolicy).Key.Data; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } c, err = defaults.GetMatchingPolicies("rando3") checkGetMatches(t, c, err) matchedPolicy = "cluster-image-policy-2" want = "cacert chilling here" - if got := c[matchedPolicy].Authorities[0].Keyless.CACert.Data; got != want { + if got := getAuthority(t, c, matchedPolicy).Keyless.CACert.Data; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } want = "issuer" - if got := c[matchedPolicy].Authorities[0].Keyless.Identities[0].Issuer; got != want { + if got := getAuthority(t, c, matchedPolicy).Keyless.Identities[0].Issuer; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } want = "subject" - if got := c[matchedPolicy].Authorities[0].Keyless.Identities[0].Subject; got != want { + if got := getAuthority(t, c, matchedPolicy).Keyless.Identities[0].Subject; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } // Make sure regex matches "regexstring*" @@ -82,30 +95,30 @@ func TestGetAuthorities(t *testing.T) { checkGetMatches(t, c, err) matchedPolicy = "cluster-image-policy-4" want = inlineKeyData - if got := c[matchedPolicy].Authorities[0].Key.Data; got != want { + if got := getAuthority(t, c, matchedPolicy).Key.Data; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } - checkPublicKey(t, c[matchedPolicy].Authorities[0].Key.PublicKeys[0]) + checkPublicKey(t, getAuthority(t, c, matchedPolicy).Key.PublicKeys[0]) // Test multiline yaml cert c, err = defaults.GetMatchingPolicies("inlinecert") checkGetMatches(t, c, err) matchedPolicy = "cluster-image-policy-3" want = inlineKeyData - if got := c[matchedPolicy].Authorities[0].Key.Data; got != want { + if got := getAuthority(t, c, matchedPolicy).Key.Data; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } - checkPublicKey(t, c[matchedPolicy].Authorities[0].Key.PublicKeys[0]) + checkPublicKey(t, getAuthority(t, c, matchedPolicy).Key.PublicKeys[0]) // Test multiline cert but json encoded - c, err = defaults.GetMatchingPolicies("ghcr.io/example/*") + c, err = defaults.GetMatchingPolicies("ghcr.io/example/foo") checkGetMatches(t, c, err) matchedPolicy = "cluster-image-policy-json" want = inlineKeyData - if got := c[matchedPolicy].Authorities[0].Key.Data; got != want { + if got := getAuthority(t, c, matchedPolicy).Key.Data; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } - checkPublicKey(t, c[matchedPolicy].Authorities[0].Key.PublicKeys[0]) + checkPublicKey(t, getAuthority(t, c, matchedPolicy).Key.PublicKeys[0]) // Test multiple matches c, err = defaults.GetMatchingPolicies("regexstringtoo") @@ -115,14 +128,14 @@ func TestGetAuthorities(t *testing.T) { } matchedPolicy = "cluster-image-policy-4" want = inlineKeyData - if got := c[matchedPolicy].Authorities[0].Key.Data; got != want { + if got := getAuthority(t, c, matchedPolicy).Key.Data; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } - checkPublicKey(t, c[matchedPolicy].Authorities[0].Key.PublicKeys[0]) + checkPublicKey(t, getAuthority(t, c, matchedPolicy).Key.PublicKeys[0]) matchedPolicy = "cluster-image-policy-5" want = "inlinedata here" - if got := c[matchedPolicy].Authorities[0].Key.Data; got != want { + if got := getAuthority(t, c, matchedPolicy).Key.Data; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } @@ -134,7 +147,7 @@ func TestGetAuthorities(t *testing.T) { } matchedPolicy = "cluster-image-policy-with-policy-attestations" want = "attestation-0" - if got := c[matchedPolicy].Authorities[0].Name; got != want { + if got := getAuthority(t, c, matchedPolicy).Name; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } // Both top & authority policy is using cue @@ -147,11 +160,11 @@ func TestGetAuthorities(t *testing.T) { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } want = "cue" - if got := c[matchedPolicy].Authorities[0].Attestations[0].Type; got != want { + if got := getAuthority(t, c, matchedPolicy).Attestations[0].Type; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } want = "test-cue-here" - if got := c[matchedPolicy].Authorities[0].Attestations[0].Data; got != want { + if got := getAuthority(t, c, matchedPolicy).Attestations[0].Data; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } @@ -165,7 +178,7 @@ func TestGetAuthorities(t *testing.T) { checkSourceOCI(t, c[matchedPolicy].Authorities) want = "example.registry.com/alternative/signature" - if got := c[matchedPolicy].Authorities[0].Sources[0].OCI; got != want { + if got := getAuthority(t, c, matchedPolicy).Sources[0].OCI; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } @@ -178,11 +191,11 @@ func TestGetAuthorities(t *testing.T) { } checkSourceOCI(t, c[matchedPolicy].Authorities) - if got := len(c[matchedPolicy].Authorities[0].Sources[0].SignaturePullSecrets); got != 1 { + if got := len(getAuthority(t, c, matchedPolicy).Sources[0].SignaturePullSecrets); got != 1 { t.Errorf("Did not get what I wanted %d, got %d", 1, got) } want = "examplePullSecret" - if got := c[matchedPolicy].Authorities[0].Sources[0].SignaturePullSecrets[0].Name; got != want { + if got := getAuthority(t, c, matchedPolicy).Sources[0].SignaturePullSecrets[0].Name; got != want { t.Errorf("Did not get what I wanted %q, got %+v", want, got) } }