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

Consolidate the sign API #665

Closed
laurentsimon opened this issue Jun 2, 2023 · 5 comments
Closed

Consolidate the sign API #665

laurentsimon opened this issue Jun 2, 2023 · 5 comments
Labels
component:api Public APIs component:signing Core signing functionality enhancement New feature or request

Comments

@laurentsimon
Copy link
Contributor

Description

The existing sign() takes a limited number of inputs, and it may be useful to have an options input to add more flexible options. Example:

  • SIGSTORE_OAUTH_FORCE_OOB is currently an env variable which needs to be set prior to calling the API. Should it not be an input to the signing function instead?
  • Signing currently support hashedrekord, but this may change in the future to support intoto types

I wonder whether it'd make sense to consolidate the API.

@laurentsimon laurentsimon added the enhancement New feature or request label Jun 2, 2023
@woodruffw
Copy link
Member

  • SIGSTORE_OAUTH_FORCE_OOB is currently an env variable which needs to be set prior to calling the API. Should it not be an input to the signing function instead?

I might be misunderstanding what you mean, but I don't think this is true -- the current Signer.sign() API takes an identity token that's been pre-retrieved, so that environment variable shouldn't affect it. Also NB that #645 will change that API substantially in 2.0, but will still preserve the expectation that the user gets an identity token prior to calling the signing APIs.

That being said, I think it's a good idea to eliminate deep use of that SIGSTORE_OAUTH_FORCE_OOB env variable 🙂 -- it'd probably be good to have Issuer.identity_token() take that as a kwarg instead.

Signing currently support hashedrekord, but this may change in the future to support intoto types

Yeah, we should figure out what this will look like -- I'm not very familiar with in-toto, so I could use some help understanding what will need to change in sigstore-python to accommodate its Rekor types 🙂

I think #628 will also be relevant there (but could be wrong).

@woodruffw woodruffw added component:signing Core signing functionality component:api Public APIs labels Jun 2, 2023
@laurentsimon
Copy link
Contributor Author

  • SIGSTORE_OAUTH_FORCE_OOB is currently an env variable which needs to be set prior to calling the API. Should it not be an input to the signing function instead?

I might be misunderstanding what you mean, but I don't think this is true -- the current Signer.sign() API takes an identity token that's been pre-retrieved, so that environment variable shouldn't affect it.

The example shows using Issuer.get_identity_token() in https://github.com/sigstore/sigstore-python/blob/main/sigstore/sign.py#L26, which uses the env variable https://github.com/sigstore/sigstore-python/blob/main/sigstore/oidc.py#L264. If the token is passed as a string by a user, you're right it works. But if doing the OAuth flow, how should the caller create the token?

@woodruffw
Copy link
Member

Okay, I understand what you mean now -- you're talking about the identity_token() API, not the signing API. I don't think of the former as a part of the signing API, but it's reasonable to consider it part of it.

I agree that should become a parameter on identity_token(), rather than an environment variable. I'd be happy to accept a PR for it, otherwise I'll do it when I have a free moment 🙂

@laurentsimon
Copy link
Contributor Author

laurentsimon commented Jun 2, 2023

I'd be happy to accept a PR for it

See #667

@woodruffw
Copy link
Member

Done with #667 I believe. Please ping if I got this wrong!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:api Public APIs component:signing Core signing functionality enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants