-
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
Add functionality to sign DSSE envelopes with arbitrary payloads #1054
Changes from 3 commits
8285c67
b5047c0
beaa5f3
53708cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
# Copyright 2022 The Sigstore Authors | ||
# Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
|
@@ -229,6 +230,45 @@ def sign_dsse( | |
|
||
return self._finalize_sign(cert, content, proposed_entry) | ||
|
||
def sign_dsse_envelope( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little wary of a new top-level sign API, especially one that's so close to From there, we'd need to improve the docs. But I think that might be a little cleaner/easier to maintain long-term 🙂 |
||
self, | ||
envelope: dsse.Envelope, | ||
) -> Bundle: | ||
""" | ||
Signs the provided envelope's payload, and returns a | ||
`Bundle containing the signed envelope and the verification | ||
material. | ||
|
||
Args: | ||
envelope (dsse.Envelope): The envelope to be signed. | ||
|
||
Returns: | ||
Bundle: The bundle containing the signed DSSE envelope. | ||
""" | ||
cert = self._signing_cert() | ||
|
||
b64_cert = base64.b64encode( | ||
cert.public_bytes(encoding=serialization.Encoding.PEM) | ||
) | ||
envelope = dsse._sign_envelope( | ||
self._private_key, | ||
envelope, | ||
) | ||
|
||
proposed_entry = rekor_types.Dsse( | ||
spec=rekor_types.dsse.DsseSchema( | ||
# NOTE: mypy can't see that this kwarg is correct due to two interacting | ||
# behaviors/bugs (one pydantic, one datamodel-codegen): | ||
# See: <https://github.com/pydantic/pydantic/discussions/7418#discussioncomment-9024927> | ||
# See: <https://github.com/koxudaxi/datamodel-code-generator/issues/1903> | ||
proposed_content=rekor_types.dsse.ProposedContent( # type: ignore[call-arg] | ||
envelope=envelope.to_json(), | ||
verifiers=[b64_cert.decode()], | ||
), | ||
), | ||
) | ||
return self._finalize_sign(cert, envelope, proposed_entry) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making sure I'm not missing anything: does this actually work yet? My understanding was that Rekor doesn't yet support arbitrary DSSE envelopes, since it expects a valid in-toto statement within the envelope. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is correct, https://github.com/sigstore/rekor/blob/main/pkg/types/dsse/v0.0.1/entry.go#L112-L146, also pointed out in #982 (comment). The reason is so that Rekor can extract index keys from the statement. We can add other payload types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @haydentherapper! I was feeling some deja vu at this 😅 Given the above, I'm not sure we can merge this/it makes sense to merge until Rekor supports other payload types (since users who attempt to use it will hit a hard error and think that sigstore-python is broken). (Another option here would be to use a DSSE envelope with an arbitrary payload, but to emit a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh ok, I missed that. Sorry! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That would mess up the verification itself, unfortunately -- that would result in a bundle with no transparency log entries, which would both be invalid and also incompatible with other clients. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if we made index keys optional for unparseable DSSEs, this would be a breaking change for log monitors that expect an intoto payload in order to extract subject values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How could we proceed with this then?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @haydentherapper I meant to bring your focus back to this by re-triggering the review ;) |
||
|
||
def sign_artifact( | ||
self, | ||
input_: bytes | sigstore_hashes.Hashed, | ||
|
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.
Like elsewhere: we should start with this being a private API, before we make it public.
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.
It is the only "valid" way to create an envelope for users (except of just calling the private method).
Surely, I can change this but just like to get your thoughts on how otherwise a user can sign an envelope.
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.
Hmm, yeah. The problem here is that
from_payload
is a little misleading as an API: it produces anEnvelope
, but thatEnvelope
is actually in an invalid state (since it's missing signatures).Instead of passing an incomplete
Envelope
in for signing, maybe we could adapt the top-levelsign_dsse
signature:...where
tuple[str, bytes]
is the envelope type and arbitrary contents. That can then be passed intodsse._sign
with some similar small tweaks.(I still don't love that signature, but I think it's a better starting point and avoids the need for a partial constructor here 🙂)
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.
How about using a dataclass?
I think at some point the
sign_dsse
might need to be renamed :) but for now a dataclass would make it clearer for users.WDYT?
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.
That works for me, although maybe
dsse.RawPayload
to make it clear that this is the "raw" variant of possible payload types (dsse.Statement
being another valid variant).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.
PTAL