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

feat: fixes for release 1.0 #61

Merged
merged 6 commits into from
Nov 1, 2023
Merged

feat: fixes for release 1.0 #61

merged 6 commits into from
Nov 1, 2023

Conversation

vishal-chdhry
Copy link
Contributor

@vishal-chdhry vishal-chdhry commented Oct 26, 2023

Explanation

Closes #54
Closes #56
Closes #57

This PR adds an option called imageReferences that can be used to specify which images to verify

Signed-off-by: Vishal Choudhary <[email protected]>
Signed-off-by: Vishal Choudhary <[email protected]>
Signed-off-by: Vishal Choudhary <[email protected]>
auth.go Outdated
@@ -36,13 +37,27 @@ func getRegion(registry string) (string, error) {
return ecrRegion, nil
}

func getAuthFromIRSA(ctx context.Context, ref registry.Reference) (authn.AuthConfig, error) {
func filterAWSImages(image string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

@vishal-chdhry - I am not sure why this is necessary. We should be able to support images signed with notation and AWS signer but stored in registries other than ECR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JimBugwadia It is possible that someone is using an image like busybox/nginx for init containers, notation will try to verify those and fail

Besides, verification of images outside of ECR + AWS Signer context can be done using kyverno and I dont think it will be any helpful to try to verify them here as this focusses on AWS signer and ECR

Copy link
Contributor

Choose a reason for hiding this comment

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

@vishal-chdhry the question is on images signed by notation and the signer binary, but stored in registries other than ECR. Those cannot be verified without the extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JimBugwadia , we can add a field for matching image references like to filter images instead of making it strict

@JimBugwadia
Copy link
Contributor

@vishal-chdhry - that sounds good.

Should we add a imageReferences and skipImageReferences fields as variables in the policy that are passed to the extension?

Or can we get this from the configured TrustPolicy CR?

@vishal-chdhry
Copy link
Contributor Author

Yes we can configure trust policies to skip images that don't match.

And if the goal is to only mutate digests for verified image ( since Kyverno covers the other scenarios) then this change is not needed.

@JimBugwadia
Copy link
Contributor

JimBugwadia commented Oct 29, 2023

And if the goal is to only mutate digests for verified image ( since Kyverno covers the other scenarios) then this change is not needed.

So, the user would use 2 policies? Something like this:

  1. first policy: configure the TrustPolicy to only match images signed by notation & signer and mutate their digests
  2. second policy: match everything else and use required, verifyDigests, mutateDigests, and skipImageReferences to cover the rest of the images

Can you create samples for how to achieve this for the use case mentioned in #57? We can add that to the docs, as it seems to be a fairly common use case.

Reading the description in #57 - seems like we may still meed to change the response to indicate which images have been skipped, so we do not create a patch for those.

@vishal-chdhry
Copy link
Contributor Author

vishal-chdhry commented Oct 30, 2023

So, the user would use 2 policies? Something like this:

  1. For the first policy they will match and verify images based notation trust policy. The images that are skipped during verification will not be mutated.
  2. A kyverno policy that fetches and mutates digests.

Can you create samples for how to achieve this for the use case mentioned in #57? We can add that to the docs, as it seems to be a fairly common use case.

Sure

seems like we may still meed to change the response to indicate which images have been skipped, so we do not create a patch for those.

Adding a change where patch is not created if the image is skipped.

Signed-off-by: Vishal Choudhary <[email protected]>
@vishal-chdhry vishal-chdhry changed the title feat: skip image verification for non ecr images feat: add option to filter images for verification Oct 30, 2023
@vishal-chdhry
Copy link
Contributor Author

vishal-chdhry commented Oct 30, 2023

@JimBugwadia Even if the trust policy filters image, we will still need to filter images since artifact reference must contain a digest, therefore we have have to resolve the digest before passing it to notation.

Because of this we will try to resolve digest of every image and we will fail for those images that cannot be accessed by kyverno-notation-aws, so trust policy filtering is not enough.

(Notation has a function to check images skipped by the trust policy but I could not use it with an image with tag.)

@vishal-chdhry vishal-chdhry changed the title feat: add option to filter images for verification feat: fixes for release 1.0 Nov 1, 2023
@JimBugwadia JimBugwadia merged commit d437871 into main Nov 1, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants