-
Notifications
You must be signed in to change notification settings - Fork 36
feat: adding trust_config parameter for private sigstore instances #460
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
base: main
Are you sure you want to change the base?
Conversation
@@ -24,6 +24,7 @@ | |||
from sigstore import oidc as sigstore_oidc | |||
from sigstore import sign as sigstore_signer | |||
from sigstore import verify as sigstore_verifier | |||
from sigstore._internal.trust import ClientTrustConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we have no other choice than to take one of their _internal
classes.
Rest LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the type mismatch issue in sigstore-python
that was blocking this earlier is now resolved, so we should be good to go on making these parts of the API public.
Also, there's an upstream fix in progress for pulling the OIDC URL from trustconfig
. Once that's merged, I'll clean up the current workaround and refactor things properly to expose what we need in sigstore-python
. That way, we wont have to rely on internals in model-transparency
and have a cleaner setup overall.
Thanks for the review! (This does work in it's current form, just for stability purposes these changes will need to be introduced)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! Yes, let's wait for the upstream trustconfig
changes and then we can merge this.
Given the current changes, we could do a 1.1 release once the current PRs are merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mihaimaruseac The upstream change has been merged (commit)
The only real change needed in this PR to facilitate the upstream change, is removing the line where I manually pull in OIDC url from trustconfig. However ofcourse we won't have access to all these changes until they release a new version (Which would break this library currently since they get rid of .staging()
and .production
)
Do we wait until that point in time, or we can merge now and I will fix all the issues surround trustconfig changes when they do release. wdyt? (1.1 binary would be fine if we merge now and choose to release)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait a little bit more for the upstream release and be able to merge this as soon as the release occurs.
If the upstream release is delayed by a long time, then we can merge this and think of some compat layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ready to merge, except a change in changelog, a nit, and a wait for upstream.
@@ -24,6 +24,7 @@ | |||
from sigstore import oidc as sigstore_oidc | |||
from sigstore import sign as sigstore_signer | |||
from sigstore import verify as sigstore_verifier | |||
from sigstore._internal.trust import ClientTrustConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait a little bit more for the upstream release and be able to merge this as soon as the release occurs.
If the upstream release is delayed by a long time, then we can merge this and think of some compat layer.
3e4b874
to
96d1e1f
Compare
@@ -33,7 +33,7 @@ dependencies = [ | |||
"cryptography", | |||
"in-toto-attestation", | |||
"PyKCS11", | |||
"sigstore", | |||
"sigstore @ git+https://github.com/sigstore/sigstore-python.git@main", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mihaimaruseac Decided to work off of the main branch just to get this PR more ready for when a major release happens of sigstore-python soonish, and I also fixed the default sigstore signing + tests since large changes happened (ClientTrustConfig.staging()/.production()
now instead of SigningContext.staging()
etc.
All tests pass except these for EC & PKCS11:
FAILED tests/api_test.py::TestCertificateSigning::test_sign_and_verify - pydantic_core._pydantic_core.ValidationError: 1 validation error for Bundle
FAILED tests/api_test.py::TestKeySigning::test_sign_and_verify - pydantic_core._pydantic_core.ValidationError: 1 validation error for VerificationMaterial
Which I opened an issue for just to find a solution, however the actual sigstore signing should work just fine. Once a release comes, I can just clean up the pyproject.toml and everything should be ready to go.
8303e9a
to
7bfb2e2
Compare
`SigningContext.production()` and `SigningContext.staging()` have been removed, Using `SigningContext.from_trust_config()` instead now Using ClientTrustConfig now for the methods `production()` and `staging()` Signed-off-by: SequeI <[email protected]>
Summary
This change introduces the --trust_config option to enable the use of private Sigstore instances with custom trust roots. Currently, the tooling assumes the use of the public Sigstore infrastructure, which limits flexibility for organizations running their own Rekor, Fulcio, and CTLog instances. This PR addresses that limitation by allowing users to provide a custom trust configuration for both signing and verification operations.
I tested using the public goods instance and it's trust config, and also our own redhat TAS instance's trust config. Signed and verified with no issues :)
resolves #208
Checklist