-
Notifications
You must be signed in to change notification settings - Fork 49
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
API-level DSSE signing support #804
Conversation
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
CCing @laurentsimon, just so you're aware: this PR will change the |
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Not supported until 3.10+ Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Downstream bug: in-toto/attestation#300 |
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
For posterity: this is a small demo script that you can use to exercise the new DSSE functionality: #!/usr/bin/env python
import logging
from in_toto_attestation.v1.resource_descriptor import ResourceDescriptor
from in_toto_attestation.v1.statement import Statement
from sigstore.oidc import Issuer
from sigstore.sign import SigningContext
logging.getLogger().setLevel(logging.DEBUG)
ctx = SigningContext.staging()
with ctx.signer(identity_token=Issuer.staging().identity_token()) as signer:
stmt = Statement(
subjects=[
ResourceDescriptor(
name="null",
digest={
"sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
},
).pb,
],
predicate={"Data": "", "Timestamp": "2023-12-07T00:37:58Z"},
predicate_type="https://cosign.sigstore.dev/attestation/v1",
)
res = signer.sign(stmt)
print(res) |
R4R but marking as blocked, pending resolution of in-toto/attestation#315 |
Will get onto this at the beginning of next week. |
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.
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.
I have nothing to add here but left some high level questions in the issue
For tracking purposes: our upstream blocker is itself blocked on danielgtaylor/python-betterproto#551 |
Signed-off-by: William Woodruff <william@trailofbits.com>
This should be good to go -- I'm setting to auto-merge, pending review 🙂 |
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.
SigningResult removal means this has conflicts now. Apart from that:
- code seems solid, no notes
- would be great to see a test
- adding this signing capability without defining the verification side is a bit sketchy (even if I understand that it might not make sense to implement inside sigstore-python): I expect after this PR even the sigstore-python CLI might happily "verify" a bundle with an intoto statement? This is not entirely wrong but could maybe give the wrong impression to the user (implying that something in the statement has been verified, when it has not).
I think merging as is (after conflict fixes) should be ok though if the above items are in plans.
Signed-off-by: William Woodruff <william@trailofbits.com>
Yeah, I'm not sure what to do about this -- IMO sigstore-python itself should never attempt to verify these statements (that belongs at a higher-up level), so maybe this is an API documentation problem? Something like a big warning on
Yeah, I'd like to do an E2E test here once we have verification in place. I could probably hack it together as an integration-style test with another client for verification, but I figure it's not worth the effort versus merging now + working on a verification test in the follow-up. |
Maybe I'm missing something but couldn't you do a test that does signing like your demo and then verifies the bundle (leaving a note that the intoto statement is not verified) -- just to make sure the round trip works fine?
I wonder if I'm correct here and if we want to do something about this?
I'm really not sure, this is just hand waving |
Not with the current APIs -- I've only implemented DSSE signing, not verification. The verification APIs still expect only a "bare" signature, so attempting to verify a bundle containing a DSSE message will fail (probably with an abstruse Pydantic error).
Yes, this is why I don't want to expose these APIs via the CLI in the immediate term 🙂 -- my plan was to have the APIs themselves present in the 3.0 series, after which we can iterate on an appropriate CLI design. |
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.
Okay, sounds good then 👍
Adds support for DSSE signature generation (over an in-toto statement) at the API level. Not exposed via the
sigstore
CLI yet.See #628.