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

Change securesystemslib.dsse.Envelope.signatures to dict upstream #743

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions securesystemslib/dsse.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,15 @@ class Envelope:
Attributes:
payload: Arbitrary byte sequence of serialized body.
payload_type: string that identifies how to interpret payload.
signatures: list of Signature.
signatures: dict of Signature key id and Signatures.

"""

def __init__(
self, payload: bytes, payload_type: str, signatures: List[Signature]
self,
payload: bytes,
payload_type: str,
signatures: Dict[str, Signature],
):
Copy link
Contributor Author

@NicholasTanz NicholasTanz Mar 9, 2024

Choose a reason for hiding this comment

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

I wasn't sure if I should change the last param from signatures: List[Signature] to signatures: Dict[str, Signature] or if it is fine to leave it

Copy link
Member

Choose a reason for hiding this comment

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

Good question! Please do change the constructor argument to Dict[str, Signature] and implement the translation in from_dict and to_dict. This is also what we do with python-tuf Metadata.

Copy link
Member

Choose a reason for hiding this comment

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

I also just saw that we raise, if there are duplicate (by keyid) signatures there. I suggest to do the same in Envelope.from_dict.

self.payload = payload
self.payload_type = payload_type
Expand Down Expand Up @@ -58,18 +61,23 @@ def from_dict(cls, data: dict) -> "Envelope":
payload = b64dec(data["payload"])
payload_type = data["payloadType"]

signatures = []
signatures = {}
for signature in data["signatures"]:
signature["sig"] = b64dec(signature["sig"]).hex()
signatures.append(Signature.from_dict(signature))
signature = Signature.from_dict(signature)
if signature.keyid in signatures:
raise ValueError(
f"Multiple signatures found for keyid {signature.keyid}"
)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the check. Would you mind adding a small test?

signatures[signature.keyid] = signature

return cls(payload, payload_type, signatures)

def to_dict(self) -> dict:
"""Returns the JSON-serializable dictionary representation of self."""

signatures = []
for signature in self.signatures:
for signature in list(self.signatures.values()):
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to convert to list? You could iterate directly over dict_values. Same comment applies below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah no - I just followed the format here comment . Will change to just iterating over dict_values

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that PR has a few such conversions for type checking reasons.

sig_dict = signature.to_dict()
sig_dict["sig"] = b64enc(bytes.fromhex(sig_dict["sig"]))
signatures.append(sig_dict)
Expand Down Expand Up @@ -101,7 +109,7 @@ def sign(self, signer: Signer) -> Signature:
"""

signature = signer.sign(self.pae())
self.signatures.append(signature)
self.signatures[signature.keyid] = signature

return signature

Expand Down Expand Up @@ -140,7 +148,7 @@ def verify(self, keys: List[Key], threshold: int) -> Dict[str, Key]:
if len(keys) < threshold:
raise ValueError("Number of keys can't be less than threshold")

for signature in self.signatures:
for signature in list(self.signatures.values()):
for key in keys:
# If Signature keyid doesn't match with Key, skip.
if not key.keyid == signature.keyid:
Expand Down
4 changes: 2 additions & 2 deletions tests/test_dsse.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def test_envelope_from_to_dict(self):

# create envelope object from its dict.
envelope_obj = Envelope.from_dict(envelope_dict)
for signature in envelope_obj.signatures:
for signature in list(envelope_obj.signatures.values()):
self.assertIsInstance(signature, Signature)

# Assert envelope dict created by to_dict will be equal.
Expand Down Expand Up @@ -105,7 +105,7 @@ def test_sign_and_verify(self):

# Check for signatures of Envelope.
self.assertEqual(len(self.key_dicts), len(envelope_obj.signatures))
for signature in envelope_obj.signatures:
for signature in list(envelope_obj.signatures.values()):
self.assertIsInstance(signature, Signature)

# Test for invalid threshold value for keys_list.
Expand Down