Skip to content

Add Rekor v2 client #1422

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
Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Add Rekor v2 client #1422

wants to merge 35 commits into from

Conversation

jku
Copy link
Member

@jku jku commented Jun 4, 2025 •
edited
Loading

This is a continuation of Ramons #1400 , I'm moving it here in hopes of getting staging tests running.

The PR adds a new RekorV2Client for adding entries to a rekor-tiles log.

  • The plan is that a followup PR starts using this client in the CLI when signingconfig contains a rekor v2 instance
  • The existing RekorClient is slightly modified so that there is a common abstraction that the signing code can use
    • the abstraction contains create_entry() and methods for constructing the requests for dsse/hashedrekord
    • name of the ABC is currently RekorLogSubmitter -- we could name it RekorClient but obviously would then need to rename the original V1 client...
  • Note that the client does not include functionality to lookup entries from the log (since a sigstore client does not strictly speaking need to do that). The feature could still be added
  • testing is very light but it does show that the client can add entries to a rekor-tiles log (the staging instance)

TODO:

  • Sort out the protobuf situation: currently the PR uses protobuf-specs from git

ramonpetgrave64 and others added 23 commits May 20, 2025 16:18
@ramonpetgrave64
add RekorV2Client and types
376361a 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
add rekorv2 client + tests
2983b4a 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
add lots of docstrings
3f94e7a 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
xfail on local and staging
d043256 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
add cahngelog
1a55117 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
Merge branch 'main' into rekov2-client
65ecf4e
@ramonpetgrave64
remove staging and production methods
3730364 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
add @pytest.mark.ambient_oidc
1e78607 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
send the cert, not only the public key
53c9113 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
abstract the signer fixture
79f967c 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
reorganize fixtures
832f87d 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
add tiemout
e6a6fe3 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
Merge branch 'main' into rekov2-client
c546aea
@ramonpetgrave64
no V002 workaround
5baeb8f 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
merge updates
4b2a03a 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
no V002 workaround
73eaf0e 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
regorganize tests
34043a2 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
use new methods for building requests
a733d5a 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
changelog
90d8244 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
cleanup comment
b49d8a7 
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64
future import
80422f2 
Signed-off-by: Ramon Petgrave <[email protected]>
@jku
Rekor: Tweak the log submitter abstraction
9b60fb6 
* Make sure the exposed signatures actually are abstract:
  the request payload can be just a dict so both clients actually
  implement the same API
* There is still a "EntryRequest" NewType being used instead of dict
  just to make the intent clear

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
tests: Simplify rekorv2 tests
d80c25c 
* Don't pretend these are unit tests: just have something simple
  that proves the client roughly does what it should
* We should have real unit tests (that don't require the whole
  staging infra as current tests do) and integration tests but this
  is what I have now

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku mentioned this pull request Jun 4, 2025
Closed
6 tasks
jku added 3 commits June 4, 2025 16:48
@jku
rekor: remove timeout for now
5746d0c 
* The timeout was a little misleading (since requests timeout is not the
  total maximum time of the request) and we don't have specific timeouts
  elsewhere either.
* Also remove some unused variables

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
Use rekor types from (unreleased) protobuf-specs
cba9507 
As of right now this is not in a released protobuf-specs: just preparing
for when it is

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
Merge remote-tracking branch 'origin/main' into rekov2-client
5c503a6
@jku
Copy link
Member Author

jku commented Jun 6, 2025

Sort out the protobuf situation

I've been testing with sigstore/protobuf-specs#661 -- looks fine so far

jku
jku commented Jun 6, 2025
View reviewed changes
@jku
rekor v2: Cope without CreateEntryRequest
3d9a1b8 
This was dropped from generated protobuf code

Signed-off-by: Jussi Kukkonen <[email protected]>
jku added 3 commits June 6, 2025 14:29
@jku
rekor v2: Use the correct keytype in the entry request
324647c 
We only generate secp256r1 so can skip checking all of the other types
for now.

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
rekor v2: Workaround mypys issue with enums
68633d4 
Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
tests: Add missing license
9cad1d1 
Signed-off-by: Jussi Kukkonen <[email protected]>
jku
jku commented Jun 6, 2025
View reviewed changes
jku
jku commented Jun 6, 2025
View reviewed changes
@jku
Copy link
Member Author

jku commented Jun 6, 2025 •
edited
Loading

I'm marking this ready for review: it depends on a protobuf-specs PR so is not ready to merge but should be otherwise reasonable

@jku jku marked this pull request as ready for review June 6, 2025 12:05
jku
jku commented Jun 6, 2025
View reviewed changes
@jku jku force-pushed the rekov2-client branch from 8de5278 to 393275b Compare June 7, 2025 09:09
jku added 3 commits June 7, 2025 12:12
@jku
Revert "rekor v2: Cope without CreateEntryRequest"
b942f90 
This reverts commit 3d9a1b8.

protobuf-specs now contains CreateEntryRequest: no need to workaround

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
test: remove unnecessary change
4e9c733 
Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
rekor: Re-use the error class for both clients
afdebab 
Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku force-pushed the rekov2-client branch from 393275b to b6b2ad5 Compare June 7, 2025 09:12
jku added 2 commits June 7, 2025 12:21
@jku
sign: Tweak rekor annotation
f0b79f9 
This is a backwards compatible change (RekorClient is a
RekorLogSumbitter), but also allows using a RekorV2Client.

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
WIP: use protos from git to get test results
90d1ff7 
Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku force-pushed the rekov2-client branch from b6b2ad5 to 90d1ff7 Compare June 7, 2025 09:21
@jku jku requested review from ramonpetgrave64 and woodruffw June 9, 2025 07:15
This was referenced Jun 9, 2025
Closed
Closed
Draft
woodruffw
woodruffw reviewed Jun 9, 2025
View reviewed changes
CHANGELOG.md
Comment on lines +23 to +24
* Added a `RekorV2Client` for posting new entries to a Rekor V2 instance.
[#1400](https://github.com/sigstore/sigstore-python/pull/1400)
Copy link
Member

Choose a reason for hiding this comment

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

FWIW we technically don't need an entry here, since the Rekor client APIs are private. But we could also save the CL cleanup until release time 🙂

woodruffw
woodruffw reviewed Jun 9, 2025
View reviewed changes
pyproject.toml
@@ -38,7 +38,7 @@ dependencies = [
"rfc8785 ~= 0.1.2",
"rfc3161-client >= 1.0.2,< 1.1.0",
# NOTE(ww): Both under active development, so strictly pinned.
"sigstore-protobuf-specs == 0.4.2",
"sigstore-protobuf-specs @ git+https://github.com/sigstore/protobuf-specs.git@main#subdirectory=gen/pb-python",
Copy link
Member

Choose a reason for hiding this comment

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

Can we switch back to a release here? PyPI won't let us publish with a git dependency, so we'll need to at some point anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, absolutely -- this is the caveat I had for this PR: has to wait for protobuf-specs to release

Copy link
Member

Choose a reason for hiding this comment

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

Oh whoops, missed that in the PR desc. Sorry 🤦

woodruffw
woodruffw reviewed Jun 9, 2025
View reviewed changes
sigstore/_internal/rekor/client_v2.py
Comment on lines +93 to +95
"""Determine PublicKeyDetails from a certificate

We know that sign.Signer only uses secp256r1 so do not support anything else"""
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with the other docstrings:

Suggested change
"""Determine PublicKeyDetails from a certificate
We know that sign.Signer only uses secp256r1 so do not support anything else"""
"""
Determine PublicKeyDetails from a certificate
We know that sign.Signer only uses secp256r1 so do not support anything else
"""

woodruffw
woodruffw reviewed Jun 9, 2025
View reviewed changes
sigstore/sign.py
Comment on lines +216 to +217
proposed_entry = self._signing_ctx._rekor._build_dsse_request(
envelope=content, certificate=cert
Copy link
Member

Choose a reason for hiding this comment

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

This is a lot cleaner, thanks!

ramonpetgrave64
ramonpetgrave64 suggested changes Jun 9, 2025
View reviewed changes
sigstore/_internal/rekor/__init__.py

__all__ = [
"_hashedrekord_from_parts",
]

EntryRequest = NewType("EntryRequest", dict[str, Any])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EntryRequest = NewType("EntryRequest", dict[str, Any])
EntryRequestBody = NewType("EntryRequestBody", dict[str, Any])

test/unit/internal/rekor/test_client_v2.py
).build()

with sign_ctx.signer(identity) as signer:
bundle = signer.sign_dsse(stmt)
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I wanted to ensure that the certificate, not only the public key, was used in the request. It's no problem to not include that here, though.

@ramonpetgrave64 ramonpetgrave64 mentioned this pull request Jun 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woodruffw woodruffw woodruffw left review comments

@ramonpetgrave64 ramonpetgrave64 ramonpetgrave64 requested changes

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@jku @woodruffw @ramonpetgrave64