Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Move DSSE Implementation to securesystemslib #487
Move DSSE Implementation to securesystemslib #487
Changes from 19 commits
6a5efd2
0807b01
26de11e
58c7124
bcaca59
34fd48b
ca2b1ad
1ab712a
d1f9602
b8225f7
7ec955d
ba23d4c
cece2e9
7536f87
12c2690
1b40254
cd2381b
aade3f2
4c6be46
99f13cf
1ecb06b
c7602b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think the decision to internally store signatures as a dict in Metadata was a good idea. Is there a reason it's a list here (other than simpler de/serialize)?
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.
Yeah, I think we kept it a list for simplicity, and because that's what the DSSE spec describes. I'm fine with using a dict internally, if there's a clear usage advantage. Though I don't know if the same requirements apply as for TUF
Metadata
, since I expectEnvelope
objects to usually have a much shorter lifetime (also see theupdateframework/python-tuf#2246 (review)).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.
Any opinions about this, @PradyumnaKrishna?
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.
Let's not use schema/formats in any new code. The
from_dict
parser hierarchy should be format validation enough.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.
Ping
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.
SignatureVerificationError or VerificationError?
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.
Ping
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.
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.
Ping
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.
I don't object to it being a dict if that's useful for other purposes but looking at the code it looks more like a set
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.
Ping
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.
I resolved an earlier comment related to this... but I've decided I'm not happy about it: we should not return "accepted keys" that is not actually a complete list but just some of the valid keys.
This return value is also not part of the protocol spec.
To be clear: I don't object to the method returning a set of keys, but I do object to it returning a vaguely defined set of keys: returning None would be better than that.
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.
Let's return None then for the time being so that we provide a spec compliant implementation. Also, AFAICS, we don't currently need the return value in in-toto.