Skip to content

Signing: hardcoded audience value won't allow a custom sigstore clients audience claim #1401

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

Open
SequeI opened this issue May 21, 2025 · 9 comments · May be fixed by #1402
Open

Signing: hardcoded audience value won't allow a custom sigstore clients audience claim #1401

SequeI opened this issue May 21, 2025 · 9 comments · May be fixed by #1402
Labels
bug Something isn't working

Comments

@SequeI
Copy link

SequeI commented May 21, 2025

If a user wishes to use a custom OIDC client such as keycloak, alongside their own ClientID, signing will fail as the audience value is hardcoded to 'sigstore'.

This prevents custom OIDC usage

@SequeI SequeI added the bug Something isn't working label May 21, 2025
@jku
Copy link
Member

jku commented May 21, 2025

hmm, I'm not sure I follow: the token is used to authenticate to Fulcio, and Fulcio requires audience: "sigstore": https://docs.sigstore.dev/certificate_authority/oidc-in-fulcio/#oidc-token-requirements-with-extracted-claims

@SequeI
Copy link
Author

SequeI commented May 21, 2025

@jku

Using Redhats custom sigstore client, our audience is set to trusted-artifact-signer. We are currently working on making Model Transparency fully work with private sigstore instances using the Trustconfig setup you have also been working on, and the audience claim prevents us from fully using our own deployment of Sigstore.

@SequeI
Copy link
Author

SequeI commented May 21, 2025

I could be wrong here or there may be a discrepancy in documentation, but I see in fulcio that Audience defaults to clientID

https://github.com/sigstore/fulcio/blob/main/pkg/config/config.go#L265

@SequeI SequeI changed the title Signing: hardcoded audience value won't allow a custom clientID Signing: hardcoded audience value won't allow a custom sigstore clients audience claim May 21, 2025
@SequeI
Copy link
Author

SequeI commented May 21, 2025

I propose using clientID as the audience value, with a default value of 'sigstore' - I will change my PR to accommodate these changes if accepted and it would fall in line with how Fulcio defines the aud claim. wdyt?

@woodruffw
Copy link
Member

I could be wrong here or there may be a discrepancy in documentation, but I see in fulcio that Audience defaults to clientID

I think in practice clientID is assumed to always be "sigstore", so the configurability there might be a bit of a red herring.

I took a quick look at what other Sigstore clients do here:

Overall I'm not inherently opposed to making this flexible, but I'd like to have a good understanding of whether doing so is covered by Sigstore's own standards -- ideally it would be, so that we can justify and conformance-test this change. Similarly, ideally the trust config itself would provision this audience, so that users don't have to pass a new flag in (since the main point of the trust config is to reduce the number of distinct input states each Sigstore client needs to expose).

@SequeI
Copy link
Author

SequeI commented May 22, 2025

I agree with the points raised so far, but I still believe adding flexibility here makes sense. Right now, we let users provide a custom Client ID, but we hardcode the audience claim to "sigstore". If Fulcio defaults the audience to match the Client ID, then we're effectively locking users into using "sigstore" as their Client ID anyway. That undermines the purpose of allowing a custom Client ID in the first place.

Perhaps we could add flexibility currently, such as audience defaulting to clientID first if provided, then straight to 'sigstore' if not, and in the future allowing the trustconfig to be a better source of truth for these types of things and adding it as an option there? wdyt

@jku
Copy link
Member

jku commented May 22, 2025

sigstore-python has been relatively successful in avoiding too much CLI flexibility by design... I admit I don't know much about the OIDC side but because of that, personally I would like to see a bit of sigstore org level consensus that audience should indeed be configurable (like a documentation change or a signingconfig/trustedroot change proposal) before we add new CLI options to sigstore-python.

At API level, making IdentityToken more configurable seems totally reasonable right now.

Right now, we let users provide a custom Client ID, but we hardcode the audience claim to "sigstore". If Fulcio defaults the audience to match the Client ID, then we're effectively locking users into using "sigstore" as their Client ID anyway. That undermines the purpose of allowing a custom Client ID in the first place.

It does sound like exposing client id in CLI was not very useful... that is not in itself a strong argument for adding more options.

@SequeI
Copy link
Author

SequeI commented May 22, 2025

It does sound like exposing client id in CLI was not very useful... that is not in itself a strong argument for adding more options.

After taking a look at it again, I completely agree so my only current proposal would be having audience internally defaulting to clientID if supplied by the user (Within IdentityToken), and defaulting to 'sigstore' like it does currently. This would make any custom OIDC client setup work with sigstore-python, essentially behaving the same way Fulcio does internally. (And allowing some flexibility with custom sigstore setups, like in our case)

Would also remove the need for yet another CLI option.

@lukehinds
Copy link
Member

I expect it would make sense to have the aud default of sigstore , but allow a config override - this would be mirrored to what we do in fulcio:

https://github.com/sigstore/fulcio/blob/b2186c0/pkg/config/config.go#L182-L201

@SequeI are you thinking something along the lines of: --oidc-client-aud?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants