Skip to content

Conversation

@hectorj2f
Copy link
Collaborator

@hectorj2f hectorj2f commented Jan 20, 2023

closes #458

Summary

I am proposing a solution for setting cue-rego policies via a URL. This PR parses the url during creation time, and fetches the content of the URL to policy.Data during reconciliation.

Release Note

Documentation

@hectorj2f hectorj2f self-assigned this Jan 20, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #518 (19a6dc8) into main (cf7b97a) will decrease coverage by 0.29%.
The diff coverage is 44.26%.

@@            Coverage Diff             @@
##             main     #518      +/-   ##
==========================================
- Coverage   56.21%   55.93%   -0.29%     
==========================================
  Files          42       42              
  Lines        4454     4505      +51     
==========================================
+ Hits         2504     2520      +16     
- Misses       1747     1778      +31     
- Partials      203      207       +4     
Impacted Files Coverage Δ
...econciler/clusterimagepolicy/clusterimagepolicy.go 55.55% <3.44%> (-8.62%) ⬇️
...s/policy/v1alpha1/clusterimagepolicy_validation.go 91.51% <81.25%> (-0.80%) ⬇️
...is/policy/v1beta1/clusterimagepolicy_validation.go 93.35% <81.25%> (-0.88%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hectorj2f hectorj2f added enhancement New feature or request feature labels Jan 20, 2023
@hectorj2f hectorj2f marked this pull request as ready for review January 20, 2023 12:08
Copy link
Collaborator

@vaikas vaikas left a comment

Choose a reason for hiding this comment

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

Took a quick pass, couple of thoughts! Thanks for working on this!

if err != nil {
return fmt.Errorf("failed to read policy url response: %w", err)
}
policyRef.Data = string(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we really should add the status field into the CIPs sooner rather than later, this one (not picking on it just reminds me of it again), that if the policy is invalid it will fail at run time. I understand that even when creating inlined CIPs and configmap refs, we need to do a better job validating the policy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we need to take over the status work again. I can start writing something down for this.

}
if p.URL != nil {
url := *p.URL
_, err := apis.ParseURL(url.String())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't recall if this allows for things like relative URLs? Also, wonder if this should be required to be https?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can easily improve the validation here. Yes, i like the https requirement here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added some additional validations.

@hectorj2f
Copy link
Collaborator Author

@vaikas I made some changes. Please, take a look when possible.

Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
@znewman01
Copy link
Contributor

Is this a good idea?

I worry about an attacker just giving you the "everything passes" policy. Should we check signatures on remote policies?

@hectorj2f
Copy link
Collaborator Author

@znewman01 Yes, we should check signatures, I'd like to get the policies from an OCI registry. Perhaps we could use sget logic here.
But, wouldn't it be sha256sum avoid anyone trying to change the content of a remote url without been detected by the respective sha256sum ? Obviously we should detect the identity of the policy.

@znewman01
Copy link
Contributor

@znewman01 Yes, we should check signatures, I'd like to get the policies from an OCI registry. Perhaps we could use sget logic here.
But, wouldn't it be sha256sum avoid anyone trying to change the content of a remote url without been detected by the respective sha256sum ? Obviously we should detect the identity of the policy.

D'oh, should have actually read the PR. 🤦

Yes, you don't need signing if you're checking the sha256. But if you know the checksum, why not just inline the policy? For size or something?

Copy link
Collaborator

@vaikas vaikas left a comment

Choose a reason for hiding this comment

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

Thanks!!! I'll rebase after merge to add these to status stuff.

@hectorj2f hectorj2f merged commit 776b1dd into sigstore:main Jan 27, 2023
@hectorj2f hectorj2f deleted the policy_url branch January 27, 2023 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement policy.URL

4 participants