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

Signing should support issuer and identity arguments too #567

Closed
jku opened this issue Mar 16, 2023 · 6 comments
Closed

Signing should support issuer and identity arguments too #567

jku opened this issue Mar 16, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@jku
Copy link
Member

jku commented Mar 16, 2023

Issue

Situation is this

  • application that uses sigstore-python knows that it wants a signature from a specific certificate identity/issuer
  • triggering the browser authentication via sigstore-python forces the user choose the issuer/identity
  • user can easily select the wrong one
  • it is difficult for the application to check if the token is for the correct identity (as the token is just a blob from application perspective)

Proposal

EDIT: Please see comment below for maybe a better proposal

Signing should support issuer and identity arguments just like verification does.

In practice:
* Issuer.identity_token() could accept optional arguments identity and issuer
* Issuer.identity_token() could check the token contents and fail if they do not match the arguments (note that issuer here refers to the issuer that will be used in the certificate so the check needs to be against the federated issuer if there is one, not the oidc issuer)
* this could be extended to the UI as well: sigstore sign could have --cert-oidc-issuer and --cert-identity options

In future identity_token() could make things easier for user too:
* could use the OIDC optional argument login_hint=identity -- unfortunately this is not supported by dex (the sigstore oidc proxy) currently
* dex apparently has a non-standard connector which could be used to pre-select the issuer

@jku
Copy link
Member Author

jku commented Apr 5, 2023

Issuer.identity_token() could accept optional arguments identity and issuer

Thinking about this further, the same issue exists in the ambient credential case... which the identity_token() change would not help with.

So either id.detect_credentials() should have a similar change or the optional arguments actually belong to Signer.sign() which could then check that the provided token matches optional identity arguments before requesting a signing certificate.

Modifying the Signer API has the downside that the potential future improvements to the OIDC proxy would still require separate changes in Issuer... but conceptually it feels correct that Signer would check that the token is what it should be: it doesn't matter if in the future a "login_hint" is passed to the OIDC proxy or not, Signer should still check that the token is as expected.

@jku
Copy link
Member Author

jku commented Apr 24, 2023

I have an actual proposal now. Wrote a whole document to make it clearer to myself: https://docs.google.com/document/d/1VNCXhUW_DSm0jG8ZmzhY2AfZo8KUUTDv4KbvHqI1yH8/edit?usp=sharing

The summary is that I think token should be a type that exposes the certificate identity and issuer to the calling application. That would allow the application to make decisions based on the token identity like this:

issuer = Issuer.production()
token = issuer.identity_token()
id, issuer = token.get_certificate_identity()
if id != signing_id or issuer != signing_issuer:
    raise CredentialError("Unexpected signing token")

# continue signing with token ...
 

Improving the ability to constrain the actual web login process can still happen separately (to prevent authentication with "wrong" identity), but I think the above is useful in any case.

@woodruffw
Copy link
Member

Thank you for writing this up @jku!

I think this looks very reasonable, and fits nicely into the other breaking changes we're making before a 2.0 release of sigstore-python.

@woodruffw woodruffw self-assigned this Apr 26, 2023
@woodruffw woodruffw added this to the 2.0 milestone Apr 26, 2023
@woodruffw
Copy link
Member

Working on this now. What I'm going to do is re-use our existing oidc.Identity type and make more parts of the API work with it.

@haydentherapper
Copy link
Contributor

I think this is a good idea. Summarizing a few comments I've made across issues and PRs:

  • Federated issuer needs to be considered. As a first pass, it might be sufficient to simply check if the issuer matches the public Sigstore Dex instance. However, to make it more resilient to future updates, it might be good to also let the user provide which claim the verifier should look in to get the issuer. This is the same for subject - It could be in sub, it could be in email. You could either try both, or let the user configure it.
  • What about signing on CI platforms? We have a mapping between claims per CI platform and extension. Is this mapping something we would need to expose outside of Fulcio?

@woodruffw
Copy link
Member

#635.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants