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

feature: add TSA support when verifying authorities #468

Merged
merged 11 commits into from
Dec 29, 2022

Conversation

hectorj2f
Copy link
Collaborator

@hectorj2f hectorj2f commented Dec 23, 2022

Signed-off-by: Hector Fernandez hector@chainguard.dev

Summary

We are adding support to verify images using timestamp authorities. To test it, we use scaffolding and a working branch to install cosign v2.0.0-rc so we can use TSA verifications (sigstore/cosign-installer#105).

Release Note

Add support to verify images using timeStamp authorities.

Documentation

@hectorj2f hectorj2f self-assigned this Dec 23, 2022
@hectorj2f hectorj2f changed the title WIP: Add TSA support when verifying authorities Add TSA support when verifying authorities Dec 23, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2022

Codecov Report

Merging #468 (b5766e5) into main (9d7bafc) will decrease coverage by 0.92%.
The diff coverage is 15.17%.

@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
- Coverage   55.66%   54.73%   -0.93%     
==========================================
  Files          38       38              
  Lines        3992     4129     +137     
==========================================
+ Hits         2222     2260      +38     
- Misses       1598     1688      +90     
- Partials      172      181       +9     
Impacted Files Coverage Δ
pkg/apis/config/sigstore_keys.go 29.31% <ø> (ø)
...g/apis/policy/v1alpha1/clusterimagepolicy_types.go 0.00% <ø> (ø)
pkg/apis/policy/v1alpha1/zz_generated.deepcopy.go 12.65% <0.00%> (-0.40%) ⬇️
...kg/apis/policy/v1beta1/clusterimagepolicy_types.go 0.00% <ø> (ø)
pkg/apis/policy/v1beta1/zz_generated.deepcopy.go 19.93% <0.00%> (-1.02%) ⬇️
pkg/webhook/validator.go 61.91% <0.00%> (-3.56%) ⬇️
...s/policy/v1alpha1/clusterimagepolicy_validation.go 92.24% <57.14%> (-1.06%) ⬇️
...is/policy/v1beta1/clusterimagepolicy_validation.go 94.18% <62.50%> (-1.09%) ⬇️
...s/policy/v1alpha1/clusterimagepolicy_conversion.go 76.72% <100.00%> (+0.83%) ⬆️
pkg/apis/policy/v1alpha1/trustroot_validation.go 43.35% <0.00%> (+12.72%) ⬆️

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

name: my-sigstore-keys
spec:
sigstoreKeys:
timestampAuthorities:
Copy link
Collaborator

Choose a reason for hiding this comment

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

  The TrustRoot "my-sigstore-keys" is invalid: spec.sigstoreKeys.certificateAuthorities: Invalid value: "null": spec.sigstoreKeys.certificateAuthorities in body must be of type array: "null"
  Error: Process completed with exit code 1.

Don't we still need the signing CA added? It's currently listed as 'required', so if this is a bad assumption we need to relax it in the trustroot API.

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 am still working on this ... thanks for looking at it. I am adding a WIP.

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'll complete the tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh no worries, was just thinking through my assumptions on how things work :) My thinking has been that there always has to be a CA, but maybe that's wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't we still need the signing CA added? It's currently listed as 'required', so if this is a bad assumption we need to relax it in the trustroot API.

You can sign using the TSA as verification and a raw key without anything else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should relax the validation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@hectorj2f hectorj2f changed the title Add TSA support when verifying authorities WIP: Add TSA support when verifying authorities Dec 23, 2022
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>
@hectorj2f hectorj2f force-pushed the add_tsa_support branch 3 times, most recently from cb72c04 to f39b4e6 Compare December 27, 2022 09:38
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
@hectorj2f hectorj2f changed the title WIP: Add TSA support when verifying authorities feature: add TSA support when verifying authorities Dec 27, 2022
with:
mirror: mirror.gcr.io

- uses: hectorj2f/cosign-installer@c5ac9ce01eb4b0048c02123cb3721624c8f4dc55 # v2
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 am waiting for a decision so we can install cosign v2 and RC versions via sigstore/cosign-installer#105

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 created #478 to track this change.

Signed-off-by: Hector Fernandez <hector@chainguard.dev>
@hectorj2f hectorj2f force-pushed the add_tsa_support branch 2 times, most recently from 03147c8 to 7b585e6 Compare December 28, 2022 19:34
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Can you open tracking issues for updating the installer/tsa/cosign deps once they are released with the changes we need?

@hectorj2f
Copy link
Collaborator Author

@mattmoor Yes, I'll create one issue. I am working myself on the change too sigstore/cosign-installer#105.

@hectorj2f hectorj2f merged commit d6ef1f3 into sigstore:main Dec 29, 2022
@hectorj2f hectorj2f deleted the add_tsa_support branch December 29, 2022 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants