Want to use GlobMatch on root level domain (e.g. "ghcr.io/*")

[jdolitsky]

The GlobMatch method does not accept a pattern such as ghcr.io/*

Seems there is a bug on Go opened in 2015: golang/go#11862

cc @tcnghia

[jdolitsky]

Related question: is * meant to match anything, or just docker hub images?

[imjasonh]

Related question: is * meant to match anything, or just docker hub images?

Using the glob match rules, I believe it would match a dockerhub official image like ubuntu, but not a user owned image like myuser/myapp, which would need */*.

I'd like to suggest that image refs like ubuntu get expanded into their canonical form (index.docker.io/library/ubuntu) and matched that way. User owned images are index.docker.io/myuser/myapp. go-containerregistry can parse the ref with WeakValidation (the default) and return the full version with ref.Name().

The webhook could return a warning if the match is specified like *, until it becomes an error in the future. That's probably better anyway since * looks like "all images" and isn't using glob rules.

There's also docker.io/library/ubuntu and docker.io/myuser/myapp which are confusingly equivalent to the index versions. We should probably warn on those too and eventually reject that form, to avoid confusion.

[imjasonh]

Another issue: The glob pattern ghcr.io/*/* won't match a (valid) GHCR image like ghcr.io/foo/bar/more/path/parts. DockerHub doesn't support arbitrarily nested paths, but GHCR and GCR at least both do, and probably more.

Unless we want users to maintain a large list of valid globs like:

images:
- glob: 'ghcr.io/*/*'
- glob: 'ghcr.io/*/*/*'
- glob: 'ghcr.io/*/*/*/*'
- glob: 'ghcr.io/*/*/*/*/*'

I think it would be useful to support regexps, and maybe just warn or error on potentially confusing regexps like ghcr.io matching ghcrxio.

[imjasonh]

Re #1903 (comment) -- based on some tests with @tcnghia it sounds like we're already canonicalizing ubuntu into index.docker.io/library/ubuntu and matching the policy using that expanded form. So that's good. 🎉

All that's left then is to warn when a policy includes - glob: '*/*' or `- glob: 'docker.io//', since these will never match any image, regardless of how it was specified. Eventually that warning can become an error.

[hectorj2f]

@imjasonh @jdolitsky I was looking to the Globing function, and generally I started to consider re-using this function from the Kubernetes core https://github.com/kubernetes/kubernetes/blob/9720d130e466f401d00e93a3c537848cbf76ca26/pkg/credentialprovider/keyring.go#L184 instead of the current function that we have. Do you have any thoughts about it ?

[imjasonh]

@imjasonh @jdolitsky I was looking to the Globing function, and generally I started to consider re-using this function from the Kubernetes core https://github.com/kubernetes/kubernetes/blob/9720d130e466f401d00e93a3c537848cbf76ca26/pkg/credentialprovider/keyring.go#L184 instead of the current function that we have. Do you have any thoughts about it ?

That method doesn't seem to support ** either (or it doesn't seem to have tests for it at least), and seems to just be a wrapper around filepath.Match.

There's also this comment:

// Note that we don't support wildcards in ports and paths yet.

I think that's something we'd need, for example, to support ghcr.io/my-org/* or ghcr.io/my-org/*/*.

But in any case, without ** I'm not sure it really helps us much.

I think we should change GlobMatch to:

• ref, err := name.ParseReference(image, name.WeakValidation) // the default, but let's be explicit
• use ref.Name() which handles the index.docker.io/library convention
• validate that glob doesn't look like a regexp, reject any characters that aren't [a-zA-Z0-9-_:/*]
• replace any **s in glob with .+
• replace any *s in glob with [^/]+
• regexp.MatchString(newGlob, ref.Name())

Then absolutely go nuts with tests for every possible conceivable case. Tests are effectively our documentation.

If this plan sounds okay to folks I can send a PR.

rough first pass: https://gist.github.com/imjasonh/1e2bd9cbe67ca9522bbdd0c7a2842d52

[hectorj2f]

Yeah, let's get that in https://gist.github.com/imjasonh/1e2bd9cbe67ca9522bbdd0c7a2842d52.