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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Apr 3, 2024
Merged

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

merged 4 commits into from
Apr 3, 2024

Conversation

NicholasTanz
Copy link
Contributor

@NicholasTanz NicholasTanz commented Mar 9, 2024 •
edited
Loading

Fixes: theupdateframework/python-tuf#2564

Description of the changes being introduced by the pull request:

Please verify and check that the pull request fulfils the following requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@NicholasTanz NicholasTanz marked this pull request as draft March 9, 2024 18:00
NicholasTanz
NicholasTanz commented Mar 9, 2024
View reviewed changes
securesystemslib/dsse.py
@@ -26,7 +26,7 @@ def __init__(
):
Copy link
Contributor Author

@NicholasTanz NicholasTanz Mar 9, 2024 •
edited
Loading

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.

@NicholasTanz NicholasTanz marked this pull request as ready for review March 9, 2024 18:08
@NicholasTanz
Copy link
Contributor Author

Hey @lukpueh , I was wondering if you could take a look at this pr when you get a chance? thanks

lukpueh added a commit to lukpueh/tuf that referenced this pull request Mar 12, 2024
@lukpueh
Adopt upstream DSSE model change (WIP)
c4ea299 
secure-systems-lab/securesystemslib#743

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh added a commit to lukpueh/in-toto that referenced this pull request Mar 12, 2024
@lukpueh
Adopt upstream DSSE model change (WIP)
414f6d8 
secure-systems-lab/securesystemslib#743

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh
lukpueh requested changes Mar 12, 2024
View reviewed changes
Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Thanks for the clean patch! I replied to your question inline. Please address and we can merge this. FYI I already tried out what changes this will need in tuf and in-toto, and it looks reasonable (see referenced commits above).

securesystemslib/dsse.py
@@ -26,7 +26,7 @@ def __init__(
):
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.

securesystemslib/dsse.py
@@ -26,7 +26,7 @@ def __init__(
):
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.

lukpueh
lukpueh reviewed Mar 13, 2024
View reviewed changes
securesystemslib/dsse.py
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?

securesystemslib/dsse.py Outdated

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.

@NicholasTanz
add test
fa7c92d 
Signed-off-by: E3E <[email protected]>
lukpueh
lukpueh approved these changes Mar 13, 2024
View reviewed changes
Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

LGTM!

@lukpueh lukpueh merged commit e73758b into secure-systems-lab:main Apr 3, 2024
17 checks passed
lukpueh added a commit to lukpueh/tuf that referenced this pull request Apr 3, 2024
@lukpueh
Adopt upstream DSSE model change
8a52b47 
Adopt `securesystemslib.dsse.Envelope.signatures` type change
from list to dict (secure-systems-lab/securesystemslib/pull/743)
in `tuf.api.dsse.SimpleEnvelope` subclass.

fixes theupdateframework#2564

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh mentioned this pull request Apr 3, 2024
Closed
lukpueh added a commit to lukpueh/in-toto that referenced this pull request Apr 3, 2024
@lukpueh
Adopt upstream DSSE model change
6174597 
Adopt `securesystemslib.dsse.Envelope.signatures` type change
from list to dict (secure-systems-lab/securesystemslib/pull/743)
in `in_toto.models.metadata.Envelope` subclass.

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh mentioned this pull request Apr 3, 2024
Closed
lukpueh added a commit to lukpueh/in-toto that referenced this pull request Apr 3, 2024
@lukpueh
Adopt upstream DSSE model change
8835746 
Adopt `securesystemslib.dsse.Envelope.signatures` type change
from list to dict (secure-systems-lab/securesystemslib/pull/743)
in `in_toto.models.metadata.Envelope` subclass.

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh added a commit to lukpueh/in-toto that referenced this pull request Apr 29, 2024
@lukpueh
Adopt upstream DSSE model change
550a8d9 
Adopt `securesystemslib.dsse.Envelope.signatures` type change
from list to dict (secure-systems-lab/securesystemslib/pull/743)
in `in_toto.models.metadata.Envelope` subclass.

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh added a commit to lukpueh/in-toto that referenced this pull request Apr 30, 2024
@lukpueh
Adopt upstream DSSE model change
fe67c68 
Adopt `securesystemslib.dsse.Envelope.signatures` type change
from list to dict (secure-systems-lab/securesystemslib/pull/743)
in `in_toto.models.metadata.Envelope` subclass.

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh added a commit to lukpueh/in-toto that referenced this pull request May 2, 2024
@lukpueh
Adopt upstream DSSE model change
0075e9e 
Adopt `securesystemslib.dsse.Envelope.signatures` type change
from list to dict (secure-systems-lab/securesystemslib/pull/743)
in `in_toto.models.metadata.Envelope` subclass.

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh added a commit to lukpueh/in-toto that referenced this pull request May 9, 2024
@lukpueh
Adopt upstream DSSE model change
4ac19c8 
Adopt `securesystemslib.dsse.Envelope.signatures` type change
from list to dict (secure-systems-lab/securesystemslib/pull/743)
in `in_toto.models.metadata.Envelope` subclass.

Signed-off-by: Lukas Puehringer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukpueh lukpueh lukpueh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change securesystemslib.dsse.Envelope.signatures to dict upstream
2 participants
@NicholasTanz @lukpueh