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
Merged
merged 9 commits into from
May 9, 2025
Merged

More proto updates #1358

merged 9 commits into from
May 9, 2025

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

@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 2 times, most recently 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 require tweaking... 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
@jku jku mentioned this pull request May 5, 2025
Merged
@jku
Copy link
Member Author

jku commented May 7, 2025

@woodruffw or @di a final review would be welcome (I only added a changelog line after last review)

@jku jku requested a review from di May 7, 2025 07:25
woodruffw and others added 8 commits May 8, 2025 16:37
@woodruffw @jku
deps: bump protobuf-specs
72d7e64 
Signed-off-by: William Woodruff <[email protected]>
@woodruffw @jku
more protobuf-specs hackery
cc43ee0 
Signed-off-by: William Woodruff <[email protected]>
@woodruffw @jku
ignore another mypy finding
c05069e 
Signed-off-by: William Woodruff <[email protected]>
@woodruffw @jku
cherry-pick 7028110
000d196 
Signed-off-by: William Woodruff <[email protected]>
@woodruffw @jku
_cli: fix proto mess
8321730 
Signed-off-by: William Woodruff <[email protected]>
@jku
verifier: Remove unused argument
cc4c24c 
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 <[email protected]>
@jku
_internal/trust: Upgrade to SigningConfig 0.2
c0d3813 
* 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 <[email protected]>
@jku
CHANGELOG: Mention --trust-config changes
c59c200 
Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku force-pushed the more-proto-updates branch from 9387509 to c59c200 Compare May 8, 2025 13:38
@jku
Copy link
Member Author

jku commented May 8, 2025

rebased, fixed conflicts

woodruffw
woodruffw approved these changes May 9, 2025
View reviewed changes
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jku!

@jku jku merged commit ad9a001 into sigstore:main May 9, 2025
23 checks passed
@woodruffw
Copy link
Member

Related: are we considering this a breaking change, i.e. a runup for a v4 release? Technically it changes the --trust-config flag's behavior.

(I don't have a super strong opinion here, since ~nobody is probably using the current --trust-config flag.)

@ramonpetgrave64 ramonpetgrave64 mentioned this pull request May 9, 2025
Open
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woodruffw woodruffw woodruffw approved these changes

@di di Awaiting requested review from di

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