Skip to content

Use trust config everywhere #1363

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

Merged
merged 24 commits into from
May 22, 2025
Merged

Conversation

jku
Copy link
Member

@jku jku commented May 5, 2025

Use ClientTrustConfig throughout the project. This makes it so that everything required to sign or verify using a specific Sigstore instance comes from a single config file: No hard coded URLs should now be used (except in some historical edge cases like fix-bundle).

Apologies for the seemingly large PR. The PR is actually net removal of code and fairly small (165 insertions(+), 180 deletions(-)) but there's a lot of asset updates: This is done to get test coverage for signingconfig handling (new root-signing-staging has a signingconfig 0.2 so I wanted to update the assets in testing and felt I should update _store/ as well at that point).

Fixes #1347, fixes #1371, fixes #1359

  • Use TrustConfig.signing_config to get the OIDC url
  • Provide the from_tuf() method for TrustConfig and not just Trustedroot: this matches the CLI where we chose to support --trust-config
  • Use ClientTrustConfig as the initialization source where that makes sense (OIDC, SigningContext)
  • There is a special case in ClientTrustConfig for Public Good production: prod does not have signingconfig yet so we hard code one for now
  • Refactor embedded asset lookup: directories are now based on repository URLs so there's less hard coded if-clauses for Public Good instance. This means we could now add a --tuf-url argument (to choose something that is not production or staging public good instance ), but I did not add it yet
  • Update the embedded staging assets and the testing staging assets: This means the staging signing config is used to test the code in unit tests. Doing this requires mocking datetime.now() since the current staging metadata actually expires, unlike the very old metadata that was used here before

@jku jku force-pushed the use-trust-config-everywhere branch 2 times, most recently from 6f7b8c7 to 262043f Compare May 5, 2025 08:33
@jku
Copy link
Member Author

jku commented May 6, 2025

I want to review the API a bit: Maybe the internal components (like SigningContext) don't actually have to know about ClientTrustConfig and could just get a SigningConfig and/or TrustedRoot instead...

It looks a bit messy but I think it's fine:

  • Verifier takes a TrustedRoot as argument: this makes some sense since it only needs the trust root
  • SigningContext takes a complete ClientTrustConfig since it at least currently requires data from both signing config and trust root
  • Issuer now just takes a url as argument (calling SigningConfig.get_oidc_url() is simple enough, no need to have a special constructor)

@jku
Copy link
Member Author

jku commented May 6, 2025

CC @ramonpetgrave64

@jku jku force-pushed the use-trust-config-everywhere branch from 7c2cdc5 to f9d380e Compare May 8, 2025 13:45
jku added 10 commits May 9, 2025 19:03
This is not super useful at this point as the TUF repositories do not
have the required signing config yet so we can't simplify the code yet:
The goal is still for trustconfig to be the only source configuration
like the OIDC URL.

Signed-off-by: Jussi Kukkonen <[email protected]>
We have previously done this for TrustedRoot but doing this for the whole
TrustConfig makes sense.

The only complication is that production instance does not have the
SigningConfig component yet so we need to provide a fallback for that.

Signed-off-by: Jussi Kukkonen <[email protected]>
This change makes almost all code paths now use TrustConfig to choose
the sigstore instance (urls, keys, validity periods, etc).

* OIDC url now comes from signingconfig too
* Some production()/staging() methods remain because they're used by
  tests or special cases like "fix-bundle"
* Likewise some hard coded urls are left in the code since they are used
  by some special case

Signed-off-by: Jussi Kukkonen <[email protected]>
Probably makes sense to handle this in ClientTrustConfig only: less code
that way.

The tests will start passing once staging TUF contains signingconfig
(and we have updated our test copies of staging TUF)

Signed-off-by: Jussi Kukkonen <[email protected]>
Commit is large just because the test and embedded assets for staging
are updated.

* Update the embedded data in sigstore/_store
* Also update the test assets in test/assets
* refactor the embedded asset lookup: use the URL to build the
  asset dir. This means less code duplication and easier to make this
  work with non-Public Good Instance TUF repos
* Make the tuf module work with non-PGI instances: if the local TUF
  metadata is initialized out of band, tuf module just works with it.
  If a root.json is provided in _store, it is still always used to
  initialize the client

Of special note is "signing_config.v0.2.json" for production: This does
not actually exist yet in the TUF repository but I've added one in
sigstore/_store and use it as a workaround in
ClientTrustConfig.from_tuf() -- this way the code can otherwise remain
identical for both staging and prod.

Signed-off-by: Jussi Kukkonen <[email protected]>
Default now comes from the trust config. The option is also not
especially in conflict with staging.

Signed-off-by: Jussi Kukkonen <[email protected]>
They will be if there are any rekor 2 instances, but currently
TSA is not strictly needed.

Signed-off-by: Jussi Kukkonen <[email protected]>
This is not a really a user visible change so I'm debating if the item
is even needed.

Signed-off-by: Jussi Kukkonen <[email protected]>
This is not a very useful helper: just use the url at callsite

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
Copy link
Member Author

jku commented May 9, 2025

ah, I forgot that if I update the staging test assets I need to handle the TUF timestamp expiring... I think that's just a minor monkeypatch (or at worst I need to generate a new tuf repo) but it does mean this is not yet ready for review

@jku jku force-pushed the use-trust-config-everywhere branch 2 times, most recently from dcf6234 to 0abec00 Compare May 9, 2025 16:51
The current staging TUF assets expire in 3 days: mock datetime.now()
so they seem valid in the tests.

This is absolutely quite sketchy but seems to work.

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku force-pushed the use-trust-config-everywhere branch from 0abec00 to a71f387 Compare May 9, 2025 16:53
@jku
Copy link
Member Author

jku commented May 9, 2025

sigh, now the tests are failing because timestamp-authority is doing a release and our tests are expecting it to be available already because the tags are there...

I will continue on monday

@jku jku marked this pull request as ready for review May 12, 2025 07:20
@jku
Copy link
Member Author

jku commented May 12, 2025

I've marked this ready for review. It's a big patch but only 200 lines are code changes (rest is assets). I can try to split this but it might not make it easier to manage.

jku added 2 commits May 13, 2025 16:20
* Also fix some typos in other changelog entries and remove "API"
  from the rekor trailing slash change (it's not actually a
  sigstore-python API change)

Signed-off-by: Jussi Kukkonen <[email protected]>
Copy link
Member Author

@jku jku left a comment

Choose a reason for hiding this comment

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

I should update make target update-embedded-root since I'm changing _store directory names

EDIT: fixed

jku added 4 commits May 16, 2025 10:18
The repository name is now based on the url

Signed-off-by: Jussi Kukkonen <[email protected]>
Signed-off-by: Jussi Kukkonen <[email protected]>
Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku requested a review from woodruffw May 16, 2025 08:36
@jku jku mentioned this pull request May 21, 2025
6 tasks
Copy link
Member

Choose a reason for hiding this comment

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

I don't love that we now have URL-encoded filenames checked into the project, but I don't have a great alternative here. Maybe we could trim the https:// prefix, although that could be a waste of time?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, it's not great.

I think I'll leave the https there (we don't currently test with a local http server but if someone ever wants to, assuming https would be a blocker). Not a strong opinion though, can do that change anyone really doesn't like it.

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!

I left one non-blocking comment about the URL-encoded filenames, but we can merge if you don't want to bother with that.

@jku jku merged commit 6ae464b into sigstore:main May 22, 2025
23 checks passed
@jku
Copy link
Member Author

jku commented May 22, 2025

Thank you 🙏

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.

OIDC URL is ignored in SigningConfig Use trustconfig throughout the project Use signingconfig from "--trust-config"
3 participants