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] Add signature pull secrets #1805

Merged
merged 10 commits into from
Apr 29, 2022

Conversation

DennyHoang
Copy link
Contributor

@DennyHoang DennyHoang commented Apr 26, 2022

Summary

  • Add signaturePullSecrets to ClusterImagePolicy's Authorities sources.
  • Allow users to define additional credentials for signature registry location. Previously, the same ImagePullSecret credentials used only therefore images and signatures needed to use the same credentials.

Ticket Link

Resolves #1655

Release Note

* Allow configurable signature pull secrets credentials for cosigned

Potential outstanding todo

  • Add integration test scenario (Requiring more investigation from my end on how to set up credentials for locally spun up registry in our github workflow)

cc: @coyote240 @hectorj2f @vaikas

@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2022

Codecov Report

Merging #1805 (350e17e) into main (db323cd) will increase coverage by 0.11%.
The diff coverage is 74.19%.

@@            Coverage Diff             @@
##             main    #1805      +/-   ##
==========================================
+ Coverage   32.73%   32.85%   +0.11%     
==========================================
  Files         147      147              
  Lines        9313     9346      +33     
==========================================
+ Hits         3049     3071      +22     
- Misses       5907     5919      +12     
+ Partials      357      356       -1     
Impacted Files Coverage Δ
...apis/cosigned/v1alpha1/clusterimagepolicy_types.go 0.00% <ø> (ø)
...kg/apis/cosigned/v1alpha1/zz_generated.deepcopy.go 0.00% <0.00%> (ø)
...cosigned/v1alpha1/clusterimagepolicy_validation.go 94.30% <100.00%> (+0.18%) ⬆️
pkg/cosign/kubernetes/webhook/validator.go 77.70% <100.00%> (+0.39%) ⬆️
cmd/cosign/cli/verify/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify_attestation.go 0.00% <0.00%> (ø)
cmd/cosign/cli/fulcio/fulcioroots/fulcioroots.go 36.36% <0.00%> (+0.42%) ⬆️
pkg/cosign/verify.go 29.95% <0.00%> (+0.47%) ⬆️
cmd/cosign/cli/fulcio/fulcioverifier/ctl/verify.go 49.26% <0.00%> (+1.40%) ⬆️

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 db323cd...350e17e. Read the comment docs.

kiiwyz
kiiwyz previously approved these changes Apr 28, 2022
@cpanato
Copy link
Member

cpanato commented Apr 28, 2022

@DennyHoang this is ready for review?

Signed-off-by: Denny Hoang <[email protected]>
@DennyHoang DennyHoang marked this pull request as ready for review April 28, 2022 14:59
Copy link
Contributor

@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, we can fix the comment in a followup. Thanks for doing this!
Thinking about how we want to add e2e tests for this. Thinking that we can create a local image in the registry we spin up, but then maybe push the signatures / attestations to another registry and use a different secret for that then?

@@ -314,7 +314,7 @@ func validatePolicies(ctx context.Context, ref name.Reference, policies map[stri
// signatures OR attestations if atttestations were specified.
// Returns PolicyResult, or errors encountered if none of the authorities
// passed.
func ValidatePolicy(ctx context.Context, ref name.Reference, cip webhookcip.ClusterImagePolicy, remoteOpts ...ociremote.Option) (*PolicyResult, []error) {
func ValidatePolicy(ctx context.Context, namespace string, ref name.Reference, cip webhookcip.ClusterImagePolicy, remoteOpts ...ociremote.Option) (*PolicyResult, []error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, we should document in the signature what namespace this is. It's the namespace where we allow the signature pull secrets to come from.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

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 4f02c2d into sigstore:main Apr 29, 2022
@github-actions github-actions bot added this to the v1.9.0 milestone Apr 29, 2022
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
* Add signaturePullSecrets support

Signed-off-by: Denny Hoang <[email protected]>

* Abstract signaturePullSecrets remoteOpts

Signed-off-by: Denny Hoang <[email protected]>

* Add validation and signaturePullSecrets test cases

Signed-off-by: Denny Hoang <[email protected]>

* Test Authorities RemoteOpts count

Signed-off-by: Denny Hoang <[email protected]>

* Comment on not storing in Authority RemoteOpts

Signed-off-by: Denny Hoang <[email protected]>

* Fix lint issue

Signed-off-by: Denny Hoang <[email protected]>

* Add podSpec signaturePullSecrets test

Signed-off-by: Denny Hoang <[email protected]>

* Add valid signaturePullSecrets test

Signed-off-by: Denny Hoang <[email protected]>

* early return err; add signaturePullSecrets comment

Signed-off-by: Denny Hoang <[email protected]>

* codegen update

Signed-off-by: Denny Hoang <[email protected]>
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.

Allow specifying ImagePullSecrets in ClusterImagePolicies
7 participants