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

Convert SigstoreKeys to protobuf-specs type #1303

Merged
merged 15 commits into from
Apr 4, 2024

Conversation

codysoyland
Copy link
Member

@codysoyland codysoyland commented Mar 12, 2024

Summary

This PR converts SigstoreKeys into the TrustedRoot type from protobuf-specs.

Additionally, this updates the TUF fetching logic to use trusted_root.json to fetch the trusted key material if it exists, and falls back to using the custom metadata and individual target files if that file is not included in given TUF repo.

This is the first step in modifying policy-controller to support bundle verification with sigstore-go using the OCI storage scheme detailed in Sigstore Bundle as OCI Artifact.

Fixes #1297

Release Note

Documentation

@hectorj2f
Copy link
Collaborator

@codysoyland Let me know when it is ready for a review. Good work :)!

@codysoyland codysoyland force-pushed the convert-sigstorekeys-to-protobuf branch from c867dd5 to f50ce6c Compare March 15, 2024 18:26
@codysoyland
Copy link
Member Author

codysoyland commented Mar 15, 2024

@codysoyland Let me know when it is ready for a review. Good work :)!

Thanks @hectorj2f! A few questions if you don't mind:

  • The serialized test data in the reconciler tests (e.g. marshalledEntry and marshalledEntryFromMirrorFS) need to be regenerated any time I make a change to SigstoreKeys. I was thinking about replacing these with JSON files using go:embed and writing a script (added to /hack) to generate them, if you'd be open to that?
  • I have the same question about the validRepository/rootJSON values in the tests (serialized TUF repos). You don't currently have a script to do this, do you? I see you updated them somewhat recently. If you have any notes, that would be helpful -- I'm assuming you used the go-tuf CLI? Edit: Regarding the first two bullets, I finally read the comment stating that these values were pulled from a scaffolding deployment on a kind cluster! I did go ahead and move these values to separate files in a testdata subdir and added a minimal script to regenerate the marshalledEntry which saved me some time debugging since the diff output from TableTest is hard to read.
  • I don't want this patch to break anyone's existing K8s setup, but it does change the structure of the cached keys in the ConfigMap (config-sigstore-keys). The TrustRoot reconciler doesn't run automatically when upgrading the webhook, right? How would you ensure that the ConfigMap gets updated when they upgrade?
  • To avoid breaking changes to the existing v1alpha.SigstoreKeys (k8s type), I was considering adding a new field to TrustRootSpec (e.g. TrustedRootJSON:) which would allow the user to just paste in a copy of a trusted_root.json (e.g. this one from root-signing). It seems simplest/safest to just accept it as a multiline string so that we don't need to maintain a yaml schema for it and the embedded mediaType could be used to deserialize it (within sigstore-go). Does this seem like a decent approach? Edit: This PR is getting large so I'll follow up with this in a new PR. I think it's worth considering if we should add a new version of the TrustRoot CRD to add this feature. For now, this PR will be constrained to only affecting internal data types and pulling the trusted root from TUF.

@hectorj2f
Copy link
Collaborator

have the same question about the validRepository/rootJSON values in the tests (serialized TUF repos). You don't currently have a script to do this, do you? I see you updated them somewhat recently. If you have any notes, that would be helpful -- I'm assuming you used the go-tuf CLI? Edit: Regarding the first two bullets, I finally read the comment stating that these values were pulled from a scaffolding deployment on a kind cluster! I did go ahead and move these values to separate files in a testdata subdir and added a minimal script to regenerate the marshalledEntry which saved me some time debugging since the diff output from TableTest is hard to read.

@codysoyland Would you mind creating a separate PR for this? So we can add these changes to main.

@hectorj2f
Copy link
Collaborator

The TrustRoot reconciler doesn't run automatically when upgrading the webhook, right? How would you ensure that the ConfigMap gets updated when they upgrade?

When you restart the webhook, it tries to reconcile all the TrustRoot resources.

If I understood you correctly. We have a watch on the configMap so every time it gets updated, we get the latest changes.

@hectorj2f
Copy link
Collaborator

For now, this PR will be constrained to only affecting internal data types and pulling the trusted root from TUF.

This sounds great.
For any breaking change, we probably want to open one issue and discuss any decision with @vaikas and others.

@codysoyland codysoyland force-pushed the convert-sigstorekeys-to-protobuf branch from 327aa13 to d28ef98 Compare March 20, 2024 19:47
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 25.80645% with 138 lines in your changes are missing coverage. Please review.

Project coverage is 52.92%. Comparing base (395638e) to head (ee576e2).
Report is 75 commits behind head on main.

Files Patch % Lines
pkg/apis/config/sigstore_keys.go 3.17% 122 Missing ⚠️
pkg/reconciler/trustroot/trustroot.go 70.83% 7 Missing and 7 partials ⚠️
pkg/webhook/validator.go 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1303      +/-   ##
==========================================
- Coverage   54.33%   52.92%   -1.41%     
==========================================
  Files          44       44              
  Lines        3839     3979     +140     
==========================================
+ Hits         2086     2106      +20     
- Misses       1539     1651     +112     
- Partials      214      222       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codysoyland codysoyland force-pushed the convert-sigstorekeys-to-protobuf branch from d28ef98 to e381716 Compare March 22, 2024 19:38
@codysoyland codysoyland changed the title WIP: Convert SigstoreKeys to protobuf-specs type Convert SigstoreKeys to protobuf-specs type Mar 22, 2024
@codysoyland
Copy link
Member Author

@hectorj2f I rebased this PR onto #1325, added coverage for the fetching of trusted_root.json from TUF, and spent some time (along with @bdehamer) fixing all the tests/linter issues so I will mark it ready for review. After #1325 is merged, the diff will become a little smaller. Thanks in advance for your review!

@codysoyland codysoyland marked this pull request as ready for review March 22, 2024 21:08
Makefile Outdated Show resolved Hide resolved
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.

Nice! Not too bad after all, love it when a plan comes together 👍 Just need to do some rebasing as Hector says, but looks good overall! Thanks for handling my TODOs :)

@codysoyland codysoyland force-pushed the convert-sigstorekeys-to-protobuf branch from 0ee701a to 266c96f Compare March 25, 2024 17:41
@codysoyland codysoyland force-pushed the convert-sigstorekeys-to-protobuf branch from 51aaec7 to 3846a34 Compare April 2, 2024 19:09
@codysoyland codysoyland force-pushed the convert-sigstorekeys-to-protobuf branch from f877ded to 18eea2b Compare April 2, 2024 19:48
@codysoyland
Copy link
Member Author

codysoyland commented Apr 2, 2024

I rebased again and also removed unnecessary go.mod updates, which in turn removed the commits related to re-running codegen, so I think this PR is in a good state to merge now. Would love to have this reviewed again @hectorj2f @vaikas 🙏🏻 Thanks!

hectorj2f
hectorj2f previously approved these changes Apr 3, 2024
Copy link
Collaborator

@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 overall

Signed-off-by: Cody Soyland <[email protected]>
hectorj2f
hectorj2f previously approved these changes Apr 3, 2024
@hectorj2f hectorj2f requested a review from vaikas April 3, 2024 13:27
Signed-off-by: Cody Soyland <[email protected]>
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!

@vaikas vaikas merged commit ba95f81 into sigstore:main Apr 4, 2024
74 of 76 checks passed
@github-actions github-actions bot added this to the v1 milestone Apr 4, 2024
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.

Convert SigstoreKeys to TrustedRoot from protobuf-specs
5 participants