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

API-level DSSE signing support #804

Merged
merged 28 commits into from
Jan 17, 2024
Merged

API-level DSSE signing support #804

merged 28 commits into from
Jan 17, 2024

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Oct 18, 2023

Adds support for DSSE signature generation (over an in-toto statement) at the API level. Not exposed via the sigstore CLI yet.

See #628.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw self-assigned this Oct 18, 2023
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>
@woodruffw
Copy link
Member Author

CCing @laurentsimon, just so you're aware: this PR will change the sign(...) API a bit, so #666 may be affected by it 🙂

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>
@woodruffw
Copy link
Member Author

Downstream bug: in-toto/attestation#300

@woodruffw woodruffw added component:signing Core signing functionality component:api Public APIs blocked labels Dec 7, 2023
@woodruffw woodruffw marked this pull request as ready for review December 7, 2023 22:39
@woodruffw woodruffw marked this pull request as draft December 7, 2023 22:40
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
sigstore/sign.py Outdated Show resolved Hide resolved
@woodruffw
Copy link
Member Author

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)

@woodruffw
Copy link
Member Author

R4R but marking as blocked, pending resolution of in-toto/attestation#315

@woodruffw woodruffw added upstream Requires upstream work or coordination blocked labels Dec 19, 2023
@tetsuo-cpp
Copy link
Collaborator

Will get onto this at the beginning of next week.

tetsuo-cpp
tetsuo-cpp previously approved these changes Jan 2, 2024
Copy link
Collaborator

@tetsuo-cpp tetsuo-cpp left a comment

Choose a reason for hiding this comment

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

LGTM!

sigstore/sign.py Show resolved Hide resolved
sigstore/sign.py Outdated Show resolved Hide resolved
Copy link
Member

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

I have nothing to add here but left some high level questions in the issue

sigstore/sign.py Outdated Show resolved Hide resolved
@woodruffw
Copy link
Member Author

For tracking purposes: our upstream blocker is itself blocked on danielgtaylor/python-betterproto#551

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
Copy link
Member Author

This should be good to go -- I'm setting to auto-merge, pending review 🙂

Copy link
Member

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

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>
@woodruffw
Copy link
Member Author

  • 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).

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 verify(...) saying that the API only verifies the signature itself modulo VerificationPolicy, which only concerns the Sigstore bits and not any arbitrary metadata in the payload itself?

I think merging as is (after conflict fixes) should be ok though if the above items are in plans.

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.

@woodruffw woodruffw added this to the 3.0 milestone Jan 16, 2024
@jku
Copy link
Member

jku commented Jan 17, 2024

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

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 expect after this PR even the sigstore-python CLI might happily "verify" a bundle with an intoto statement?

I wonder if I'm correct here and if we want to do something about this?

  • should the CLI not succeed when the payload is a intoto statement (until we know how to expose this)
  • should there should be some output on CLI when the payload is a intoto statement

I'm really not sure, this is just hand waving

@woodruffw
Copy link
Member Author

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?

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

I wonder if I'm correct here and if we want to do something about this?

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.

@woodruffw woodruffw requested a review from jku January 17, 2024 15:25
Copy link
Member

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

Okay, sounds good then 👍

@woodruffw woodruffw merged commit bb9b1a0 into main Jan 17, 2024
25 checks passed
@woodruffw woodruffw deleted the ww/dsse branch January 17, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked component:api Public APIs component:signing Core signing functionality upstream Requires upstream work or coordination
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants