Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cosigned] Glob matching improvement #1842

Merged
merged 1 commit into from May 6, 2022

Conversation

DennyHoang
Copy link
Contributor

@DennyHoang DennyHoang commented May 4, 2022

Summary

Instead of doing a prefix check only for glob matching, use the filepath implementation of Match to match against glob patterns.
This will also remove the restrictions for only supporting trailing wildcard *

Alongside this change:

  • I also added defaulting to use index.docker.io/ host if there is no host in the glob pattern and there was not a match prior.
    This is important since when a user deploys using myproject/nginx for example, the image reference gets resolved to index.docker.io/myproject/nginx
  • I also added defaulting to the library repository when a glob is specified without multiple path elements.
    This is important since when a user deploys busybox for example, the image reference gets resolved to index.docker.io/library/busybox

Now a user should be able to able to create a cluster image policy with glob patterns:

- busybox
- myproject/image

when previously they needed to prefix those as such:

- index.docker.io/library/busybox
- index.docker.io/myproject/image

Ticket Link

Fixes #1840
Related #1834

Release Note

* Updated cosigned glob matching logic to allow for multiple wildcards
* Updated cosigned glob matching to handle dockerhub image references better

cc: @coyote240 @elfotografo007 @hectorj2f @vaikas

@codecov-commenter
Copy link

codecov-commenter commented May 4, 2022

Codecov Report

Merging #1842 (1487f6a) into main (d46a3da) will decrease coverage by 0.02%.
The diff coverage is 77.77%.

❗ Current head 1487f6a differs from pull request most recent head ce39f88. Consider uploading reports for the commit ce39f88 to get more accurate results

@@            Coverage Diff             @@
##             main    #1842      +/-   ##
==========================================
- Coverage   33.43%   33.41%   -0.03%     
==========================================
  Files         146      146              
  Lines        9328     9340      +12     
==========================================
+ Hits         3119     3121       +2     
- Misses       5839     5845       +6     
- Partials      370      374       +4     
Impacted Files Coverage Δ
pkg/apis/config/image_policies.go 64.70% <0.00%> (-4.69%) ⬇️
pkg/apis/config/glob.go 81.81% <85.71%> (-18.19%) ⬇️
...cosigned/v1alpha1/clusterimagepolicy_validation.go 94.07% <100.00%> (-0.23%) ⬇️
pkg/cosign/tuf/client.go 61.68% <0.00%> (-0.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d46a3da...ce39f88. Read the comment docs.

Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, i'd like to see some examples using gcr.io

Signed-off-by: Denny Hoang <dhoang@vmware.com>
@DennyHoang
Copy link
Contributor Author

Hmm, so Windows has \\ as path separators instead of /.

For the glob_test.go, I have a different expected match result for one of the test cases because Windows is different.

I'm thinking this is not a big concern because:

  1. Images do not use \\ as separators
  2. cosigned isn't even installed on a windows platform afaik
  3. If cosigned is installed on a windows environment, if someone specifies gcr.io/*, it would be a more open inclusion anyways...

If this does not get merged today, I will have to hand off this PR to @elfotografo007 as I will not be available for a bit.

Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dlorenc dlorenc merged commit 1258512 into sigstore:main May 6, 2022
@github-actions github-actions bot added this to the v1.9.0 milestone May 6, 2022
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
Signed-off-by: Denny Hoang <dhoang@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cosigned] glob matching is basic right now
4 participants