Add functionality to sign DSSE envelopes with arbitrary payloads #1054

susperius · 2024-06-28T13:13:45Z

Summary

Added functionality to sign DSSE envelopes with arbitrary payloads. This is to ensure model signing can in the future use sigstore with its own manifest format inside of a DSSE envelope.

Closes #982

Release Note

sign.py: sign_dsse_envelope takes an envelope as input value and adds a signature for its contents to the envelope
dsse.py: Envelope provides a public classmethod to create an envelope based on an arbitrary payload type and payload

Signed-off-by: Martin Sablotny <[email protected]>

woodruffw

Thanks @susperius!

Did an initial pass -- structure LGTM overall, but I want to confirm that this is actually supported by the Sigstore PGI before moving forwards 🙂

woodruffw · 2024-06-28T15:11:08Z

sigstore/sign.py

@@ -229,6 +230,45 @@ def sign_dsse(

        return self._finalize_sign(cert, content, proposed_entry)

+    def sign_dsse_envelope(


I'm a little wary of a new top-level sign API, especially one that's so close to sign_dsse -- instead of making this a new function, what do you think about making sign_dsse take Statement | Envelope?

From there, we'd need to improve the docs. But I think that might be a little cleaner/easier to maintain long-term 🙂

woodruffw · 2024-06-28T15:11:45Z

sigstore/sign.py

+                ),
+            ),
+        )
+        return self._finalize_sign(cert, envelope, proposed_entry)


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.

CC @haydentherapper

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.

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 hashedrekord entry instead. I have no strong opinion on whether this is a good idea or not, but it'll likely cause a lot of incompatibilities with the other clients in the ecosystem.)

Oh ok, I missed that. Sorry!
We could log a warning when an arbitrary DSSE envelope is used and skip the Rekor post in finalize sign.
Would that make sense or would it mess up the verification itself?

Would that make sense or would it mess up the verification itself?

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.

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.

How could we proceed with this then?

Would it be possible to use the SHA256 of the payload as index key for arbitrary payloads?

Should we define our DSSE envelope payload type before and then add it to rekor? <- this wouldn't be a solution for arbitrary payloads but helps the model signing effort.

@haydentherapper I meant to bring your focus back to this by re-triggering the review ;)

sigstore/dsse.py

woodruffw · 2024-06-28T15:13:56Z

sigstore/dsse.py

@@ -210,6 +211,23 @@ def _from_json(cls, contents: bytes | str) -> Envelope:
        inner = _Envelope().from_json(contents)
        return cls(inner)

+    @classmethod
+    def from_payload(cls, payload_type: str, payload: bytes) -> Envelope:


Like elsewhere: we should start with this being a private API, before we make it public.

Suggested change

def from_payload(cls, payload_type: str, payload: bytes) -> Envelope:

def _from_payload(cls, payload_type: str, payload: bytes) -> Envelope:

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.

Hmm, yeah. The problem here is that from_payload is a little misleading as an API: it produces an Envelope, but that Envelope 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-level sign_dsse signature:

def sign_dsse( self, input_: dsse.Statement | tuple[str, bytes], ) -> Bundle:

...where tuple[str, bytes] is the envelope type and arbitrary contents. That can then be passed into dsse._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 🙂)

How about using a dataclass?

@dataclass class DSSEPayload: payload_type: str payload: bytes

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?

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

susperius · 2024-06-28T18:36:53Z

Thanks @susperius!

Did an initial pass -- structure LGTM overall, but I want to confirm that this is actually supported by the Sigstore PGI before moving forwards 🙂

Thank you for looking into this!

…gned and wrapped into a DSSE envelope Signed-off-by: Martin Sablotny <[email protected]>

woodruffw · 2024-07-12T18:18:28Z

sigstore/sign.py

+            )
+        elif type(input_) is dsse.RawPayload:
+            content = dsse._sign_payload(self._private_key, input_)
+            # TODO: figure out an entry that works.


NB: this is still a hard blocker: the client specification requires bundles to have a transparency log entry, so we can't just skip it when the signed-over input isn't a DSSE.

I think there are two possible resolutions here, both of which require some kind of upstream change:

Option 1: Rekor learns about non-JSON DSSE payloads. I don't know what the timeline for this would be 🙂

Option 2: The client spec clarifies/expresses a judgement about using dsse bundles with non-dsse Rekor entry types (such as hashedrekord, which would work fine here if the DSSE envelope is canonicalized).

In case it helps: the model signing use case would contain JSON as payload. However, as things stand now rekor solely supports in-toto statements.

woodruffw · 2024-07-12T18:19:50Z

sigstore/dsse.py

@@ -256,6 +289,19 @@ def _sign(key: ec.EllipticCurvePrivateKey, stmt: Statement) -> Envelope:
    )


+def _sign_payload(


Rather than a new _sign_payload function, can we make _sign take Statement | RawPayload? That'll require some internal checks but will be a bit shorter 🙂

woodruffw · 2024-07-12T18:21:27Z

sigstore/dsse.py

+    @classmethod
+    def _from_payload(
+        cls, payload: RawPayload, sigs: list[Signature]
+    ) -> Envelope:
+        """Return an unsigned DSSE envelope.
+
+        Args:
+            payload_type (str): The envelope's payload type
+            payload (bytes): The envelope's payload
+
+        Returns:
+            Envelope: An unsigned DSSE envelope
+        """
+        inner = _Envelope(
+            payload=payload.data,
+            payload_type=payload.type,
+            signatures=sigs,
+        )
+        return cls(inner)


Merging _sign_payload with _sign should eliminate the need for this new helper 🙂

susperius and others added 2 commits June 28, 2024 06:01

Adding functionality to sign DSSE envelopes with arbitrary payloads.

8285c67

Signed-off-by: Martin Sablotny <[email protected]>

Merge branch 'sigstore:main' into main

b5047c0

susperius force-pushed the main branch from 964da94 to 33ab1be Compare June 28, 2024 13:14

Added a docstring for the new signing method

beaa5f3

Signed-off-by: Martin Sablotny <[email protected]>

susperius force-pushed the main branch from 33ab1be to beaa5f3 Compare June 28, 2024 13:17

woodruffw reviewed Jun 28, 2024

View reviewed changes

susperius force-pushed the main branch from 2232df8 to 8249ee1 Compare July 1, 2024 16:51

change the envelope creation and add a RawPayload type that can be si…

53708cc

…gned and wrapped into a DSSE envelope Signed-off-by: Martin Sablotny <[email protected]>

susperius force-pushed the main branch from 8249ee1 to 53708cc Compare July 1, 2024 16:54

susperius requested review from woodruffw and haydentherapper July 12, 2024 18:11

woodruffw reviewed Jul 12, 2024

View reviewed changes

susperius closed this Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functionality to sign DSSE envelopes with arbitrary payloads #1054

Add functionality to sign DSSE envelopes with arbitrary payloads #1054

susperius commented Jun 28, 2024

woodruffw left a comment

woodruffw Jun 28, 2024

woodruffw Jun 28, 2024

haydentherapper Jun 28, 2024

woodruffw Jun 28, 2024

susperius Jun 28, 2024

woodruffw Jun 28, 2024

haydentherapper Jun 28, 2024

susperius Jul 1, 2024

susperius Jul 12, 2024

woodruffw Jun 28, 2024

susperius Jun 28, 2024

woodruffw Jun 28, 2024

susperius Jul 1, 2024

woodruffw Jul 1, 2024

susperius Jul 1, 2024

susperius commented Jun 28, 2024

woodruffw Jul 12, 2024

woodruffw Jul 12, 2024

susperius Jul 15, 2024

woodruffw Jul 12, 2024

woodruffw Jul 12, 2024

		@@ -229,6 +230,45 @@ def sign_dsse(

		return self._finalize_sign(cert, content, proposed_entry)

		def sign_dsse_envelope(

	def from_payload(cls, payload_type: str, payload: bytes) -> Envelope:
	def _from_payload(cls, payload_type: str, payload: bytes) -> Envelope:

		@@ -256,6 +289,19 @@ def _sign(key: ec.EllipticCurvePrivateKey, stmt: Statement) -> Envelope:
		)


		def _sign_payload(

Add functionality to sign DSSE envelopes with arbitrary payloads #1054

Add functionality to sign DSSE envelopes with arbitrary payloads #1054

Conversation

susperius commented Jun 28, 2024

Summary

Release Note

woodruffw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

susperius commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment