Skip to content

More proto updates #1358

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

More proto updates #1358

wants to merge 13 commits into from

Conversation

jku
Copy link
Member

@jku jku commented Apr 29, 2025 •
edited
Loading

This is built on top of #1276: I'm fine with merging that first or closing that one and merging all the proto changes at once in this PR.

This PR upgrades us to the most recent protobuf-specs. Changes include (on top of 1276):

  • A new signingconfig version is supported, and a SigningConfig class has been added to handle parsing the data in it
  • I'm removing support for SigningConfig 0.1 since that was not actually usable: I don't think anyone will mind (staging currently has 0.1 but we do not use it and 0.2 should be available in staging by next week)

I'd like to handle followups in other PRS, mainly I'm thinking of:

  • rekorclient is still a bit of a mess
  • the signingconfig service selectors are not fully supported yet
  • oidc provider is still hard coded when running CLI
  • signingconfig from TUF is not yet used: There is still a discussion on what the specific TUF targetpath is going to be

woodruffw and others added 10 commits January 10, 2025 17:04
@woodruffw
deps: bump protobuf-specs
a9f8052 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
more protobuf-specs hackery
caf2bfe 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
ignore another mypy finding
9a3bdf2 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
Merge branch 'main' into ww/bump-proto-specs
f3c66ad
@woodruffw
Merge branch 'main' into ww/bump-proto-specs
a623010 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
Merge branch 'main' into ww/bump-proto-specs
d88da82
@woodruffw
cherry-pick 7028110
27a29c6 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
_cli: fix proto mess
f8b08a8 
Signed-off-by: William Woodruff <william@trailofbits.com>
@jku
Merge branch 'main' into ww/bump-proto-specs
b555476
@jku
verifier: Remove unused argument
6b48b21 
Verifier only needs a Rekor client for the detached materials hack in
_cli... and it should not be using SigningConfig to get it.

* This is an API change for Verifier: I believe the proposed API should
  be stable now
* RekorClient definitely needs more work: I'm just punting the can down
  the road here.

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku
Copy link
Member Author

jku commented Apr 29, 2025

FYI @ramonpetgrave64:

  • this should work with new signing config format ....
  • but still doesn't make oidc provider selection use the signing config -- I believe that's a minor patch, CLI just needs a small refactor

BTW, now that I had a look at it, I think the current RekorClient is a bit of a mess: it probably needs a redesign even without rekor-tiles.

@jku jku force-pushed the more-proto-updates branch from e0c8f17 to a45d567 Compare April 29, 2025 13:35
@jku
_internal/trust: Upgrade to SigningConfig 0.2
669c49b 
* Let's forget that v0.1 ever existed (it was not really used): We could
  try to support both but since 0.1 does not really work, I won't bother
* Support signing config v0.2 in a minimal way (see note on selectors below)

Things that could be improved:
* Rekor client is still a bit of a hack: that area likely needs a
  redesign
* The "service selectors" in SigningConfig are not all yet supported:
  Only the ANY selector works (this is the one staging will use soon)
* The CLI does not yet use the OIDC provider specified in SigningConfig
  (this should be a small refactor)

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku jku force-pushed the more-proto-updates branch from a45d567 to 669c49b Compare April 29, 2025 13:35
@jku jku marked this pull request as ready for review April 29, 2025 13:39
@jku jku requested a review from woodruffw April 30, 2025 06:41
@woodruffw
Copy link
Member

Thanks @jku! I'll review now.

This is built on top of #1276: I'm fine with merging that first or closing that one and merging all the proto changes at once in this PR.

We can close #1276; you've made much more progress than I've had time to 🙂

  • I'm removing support for SigningConfig 0.1 since that was not actually usable: I don't think anyone will mind (staging currently has 0.1 but we do not use it and 0.2 should be available in staging by next week)

Sounds good to me!

@woodruffw woodruffw mentioned this pull request May 1, 2025
Closed
woodruffw
woodruffw previously approved these changes May 1, 2025
View reviewed changes
Copy link
Member

@woodruffw woodruffw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Thanks a ton for taking over the old PR and pushing this through + making the SigningConfig changes. If I'm not mistaken we shouldn't need a CHANGELOG entry here, since this should all be internal API changes 🙂

@jku
Copy link
Member Author

jku commented May 2, 2025 •
edited
Loading

If I'm not mistaken we shouldn't need a CHANGELOG entry here, since this should all be internal API changes 🙂

The one thing I now started thinging about is --trust-config: I just dropped the signingconfig 0.1 support in this PR so if anyone was using --trust-config already then the files they were previously using will not work anymore... I will add a Changelog mention

For reference: We could add some code to still handle v0.1: the problem is that it can't really be complete support (since 0.1 is not extensive enough) so I'd rather drop it assuming that it's not going annoy too many people

@jku jku requested a review from woodruffw May 2, 2025 06:41
@jku jku enabled auto-merge (squash) May 2, 2025 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woodruffw woodruffw Awaiting requested review from woodruffw

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jku @woodruffw