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.

Sign up for GitHub

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

Jump to bottom

Adapt to new sigstore api #630

Merged
merged 7 commits into from
Oct 2, 2023
Merged

Adapt to new sigstore api #630

merged 7 commits into from
Oct 2, 2023

Conversation

jku
Copy link
Collaborator

@jku jku commented Aug 30, 2023 •
edited
Loading

Not ready for merging as sigstore 2.0 is not released yet

This adapts SigstoreSigner to sigstore 2.x API

  • tested on CI for ambient and manually for interactive signing
  • I found an unexpected issue where in the ambient case we cannot know what the signing identity is at signing time
  • this prevents actually implementing a SigstoreSigner import that would lookup the key content from the authenticated token (at least for ambient case) :(
  • however, this already prevents a user from signing with the wrong identity: this is great

Please verify and check that the pull request fulfils the following requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

This was referenced Aug 31, 2023
Closed
Merged
@jku
Copy link
Collaborator Author

jku commented Sep 7, 2023

however, this already prevents a user from signing with the wrong identity: this is great

I just added a SigstoreSigner.import_via_auth() which also allows importing a key without letting the user make copy paste mistakes: instead the user has to authenticate and then we get the identity details from the token.

This works for non-ambient cases only (because we don't know the signing identity in ambient cases)

@jku jku force-pushed the new-sigstore-api branch from 33de1dd to f82ea38 Compare September 7, 2023 12:00
@jku
Copy link
Collaborator Author

jku commented Sep 11, 2023

I've got a PR about making SigningResult._to_bundle() public: if that gets merged before sigstore releases this PR should be updated

@jku jku force-pushed the new-sigstore-api branch 3 times, most recently from 9ede80e to cdbeaf9 Compare September 12, 2023 08:47
@jku
Copy link
Collaborator Author

jku commented Sep 12, 2023

Updated to 2.0.0rc3: this is the first release candidate where we manage to use public API only

@jku jku mentioned this pull request Sep 13, 2023
Closed
@jku jku mentioned this pull request Oct 2, 2023
Closed
jku added 5 commits October 2, 2023 10:16
@jku
SigstoreSigner: Update to Sigstore 2.x
0ccb2c8 
* detect_credentials() requires no arguments
* token is now a IdentityToken instead of str
* signing now happens in a SigningContext for the token

The token improvement means that we could now also find out the
identity and issuer from the token
* would make sense to support this as a way to import a key
* we can verify the token against the public key in the constructor

The latter I intend to implement in a follow up commit, former is
likely future work (not hard, just needs some testing to see how it
should work).
@jku
SigstoreSigner: Verify that signer token and key match
f6da034 
This is now possible with sigstore 2.x.

The verification is in from_priv_key_uri(). It could be in
constructor but I was trying to avoid making the optional
imports even more complicated...

Note that there is a workaround for the fact that we do not know
the signing identity at signing time in the ambient case:
The signing certificate SAN is not the OIDC identity.
@jku
SigstoreSigner: lint workaround
95c3c79 
Cheeky rename of the unused argument: this takes us below the
too-many-locals limit.
@jku
Sigstore: Add an import method with no args
1434f83 
This way the user has to authenticate to the identity they want to
sign with later
* removes possibility of typos or misunderstanding
* Still allows storing the identity and issuer in the URI
  (this is not implemented here)
@jku
Sigstore: Use new public SigningResult.to_bundle()
4a938c8
@jku jku force-pushed the new-sigstore-api branch from cdbeaf9 to 4a938c8 Compare October 2, 2023 07:17
@jku
Copy link
Collaborator Author

jku commented Oct 2, 2023 •
edited
Loading

sigstore-python 2.0.0 has been released, this is good to review/merge.

Test run for ambient credentials: https://github.com/secure-systems-lab/securesystemslib/actions/runs/6377158699

Testing interactive credentials:

# Key creation: get key details by letting user authenticate
uri, key = SigstoreSigner.import_via_auth()

# Signing
# enable experimental signer
SIGNER_FOR_URI_SCHEME["sigstore"]= SigstoreSigner

signer = Signer.from_priv_key_uri(uri, key)
signature = signer.sign (b"")

# Verification
key.verify_signature(signature, b"")

@jku jku marked this pull request as ready for review October 2, 2023 07:28
jku
jku commented Oct 2, 2023
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
@jku
build: Fix sigstore compatible release" versioning
c01f957 
"~=2.0.0" matches 2.0.x but we actually want 2.x
lukpueh
lukpueh approved these changes Oct 2, 2023
View reviewed changes
Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Very cool! I tried your example snippet with GitHub auth and it works nicely. Code looks good!

securesystemslib/signer/_sigstore_signer.py Show resolved Hide resolved
securesystemslib/signer/_sigstore_signer.py Show resolved Hide resolved
securesystemslib/signer/_sigstore_signer.py Outdated Show resolved Hide resolved
@jku
sigstore: Tweak method signature
856aa57 
underscore prefix was used to avoid a too-many-locals warning:
Workaround that otherwise to make the method signature match the
parent class.
@jku jku merged commit 3ed46e5 into main Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukpueh lukpueh lukpueh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jku @lukpueh