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

sigstore: use our own Statement type #930

Merged
merged 11 commits into from
Mar 13, 2024
Merged

sigstore: use our own Statement type #930

merged 11 commits into from
Mar 13, 2024

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Mar 11, 2024

This solves our "which serialization is right" problem with DSSE by replacing our use of in_toto_attestation with our own Statement type, which is surfaced to the user as an opaque bytes container but ensures (internally) that the JSON object structure is as expected.

Long term, we'll probably want to reintroduce in_toto_attestation as a dep in some form, but keep our Statement as the public API type for interface stability reasons.

Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw self-assigned this Mar 11, 2024
Signed-off-by: William Woodruff <william@trailofbits.com>
)


class _StatementBuilder:
Copy link
Member Author

@woodruffw woodruffw Mar 11, 2024

Choose a reason for hiding this comment

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

NB: We will probably want to make this API public at some point, but not yet (it needs more design consideration).

except ValidationError as e:
raise ValueError(f"invalid statement: {e}")

return Statement(stmt.model_dump_json(by_alias=True).encode())
Copy link
Member Author

Choose a reason for hiding this comment

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

A consideration here: technically we make no canonical/serialization guarantees other than "it's valid JSON", but we could do RFC 8785 for good measure here.

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 woodruffw marked this pull request as ready for review March 11, 2024 19:31
@woodruffw woodruffw requested a review from jku March 11, 2024 19:31
@woodruffw woodruffw added component:signing Core signing functionality component:api Public APIs labels Mar 11, 2024
Signed-off-by: William Woodruff <william@trailofbits.com>
Logger objects are not part of the public API.

Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
return Statement(stmt.model_dump_json(by_alias=True).encode())


class Envelope:
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this as a thin wrapper for sigstore_protobuf_specs.io.intoto.Envelope, so that we don't leak a protobuf model into the public API here.

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.

Code looks good to me. I'm not too familiar with in-toto but since the signing is essentially still experimental/private API (because _StatementBuilder and _Statement are private and CLI does not expose this) seems fine to merge.

Are there conformance tests for statement bundles already?

Should we have a full round trip test with both sign & verify for the statement bundle?

@woodruffw
Copy link
Member Author

Are there conformance tests for statement bundles already?

I believe so, but we currently xfail them because we only support signing, not verifying. Once this is merged I can work on verification support.

Should we have a full round trip test with both sign & verify for the statement bundle?

Yep, I'll do this with the aforementioned followup 🙂

@woodruffw woodruffw merged commit afc14ba into main Mar 13, 2024
25 checks passed
@woodruffw woodruffw deleted the ww/internal-statement branch March 13, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:api Public APIs component:signing Core signing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants