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

signer: add SigstoreSigner from_priv_key_uri and import_ methods #535

Merged
merged 5 commits into from
Mar 16, 2023

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Mar 14, 2023

addresses most of #533

  • from_priv_key_urisupports token detection from ambient credentials (default), and token creation from OAuth2 + OpenID browser login (with '?ambient=false' URI param).
  • import_ creates sigstoreKey from passed identity and issuer, and signer URI based on passed bool to toggle ambient credential usage.

TODOs (not necessarily in this PR)

  • figure out appropriate CI trigger conditions
  • add tests
  • use identity/issuer from public key, passed to from_priv_key_uri, in the OAuth2 + OpenID flow
  • validate if public key and token passed to SigstoreSigner's from_priv_key_uri and constructor match
  • #533 also suggests adding an SigstoreSigner.import_github_action()

lukpueh added 3 commits March 14, 2023 11:04
Signed-off-by: Lukas Puehringer <[email protected]>
Supports token detection from ambient credentials (default),
and token creation from OAuth2 + OpenID browser login
(with '?ambient=false' URI param).

Updates usage example and smoke test accordingly.

Signed-off-by: Lukas Puehringer <[email protected]>
Creates SigstoreKey from passed identity and issuer, and signer URI
based on a bool to toggle ambient credential usage.

Other changes:
- _get_uri() helper
- _get_keyid() helper, the existing one would require many changes
  in the schema module
- update usage example and move from module to SigstoreSigner class
  docstring
- update smoke test

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh force-pushed the sigstore-signer-uri branch from 4356c1b to 5210a5e Compare March 14, 2023 14:48
@lukpueh
Copy link
Member Author

lukpueh commented Mar 14, 2023

@lukpueh lukpueh force-pushed the sigstore-signer-uri branch from 5210a5e to 46f2623 Compare March 14, 2023 14:51
@lukpueh lukpueh requested a review from jku March 14, 2023 14:51
Copy link
Collaborator

@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.

Looks great! Comments below should not block merging this IMO.

  • figure out appropriate CI trigger conditions

yes, pull_request trigger has to go I think.

  • use identity/issuer from public key, passed to from_priv_key_uri, in the OAuth2 + OpenID flow
  • validate if public key and token passed to SigstoreSigner's from_priv_key_uri and constructor match

I've asked about this in a couple of channels, let's see what happens

  • ... adding a SigstoreSigner.import_github_action()

no need to rush with this (since maybe there are alternatives like just providing some useful constants) but this might be useful -- producing the github action identity string is not completely trivial, and requiring the issuer string is just making the user do work we could avoid.

tests/check_sigstore_signer.py Outdated Show resolved Hide resolved
lukpueh and others added 2 commits March 16, 2023 09:30
Co-authored-by: Jussi Kukkonen <[email protected]>
Signed-off-by: Lukas Puehringer <[email protected]>
The sigstore workflow requires id-token: 'write' permission to sign
with ambient credentials. This permission is not available in a
PRs from forks.

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh force-pushed the sigstore-signer-uri branch from 92fa0fd to 2acf318 Compare March 16, 2023 08:56
@lukpueh
Copy link
Member Author

lukpueh commented Mar 16, 2023

Thanks for the review. I accepted your inline suggestion, disabled "on pr", and created separate issues for the remaining TODOs. CI failure is unrelated (#520). Merging...

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.

2 participants