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
Sign and verify #2
Sign and verify #2
Changes from all commits
a4b920d
3129f96
020df8a
5a06521
a290b94
d7ec553
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.
Before appending maybe you will want to check if this signature doesn't exist?
Does a
set
makes sense forEnvelope.signatures
?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.
Not sure about it, according to current signature wrappers:
in-toto: signatures is a list and appends the signatures.
python-tuf: signatures is a dict and appends with
keyid
.Also, in DSSE
keyid
is optional. But what I can do is: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.
In
python-tuf
to add a new signature you should usesign
: https://github.com/theupdateframework/python-tuf/blob/8a03abfdeb5a6b6bc892859f65425be0834893a8/tuf/api/metadata.py#L342.In
sign
there is anappend
option which if used will add an additional signature.If
append
is True and a key is already used to sign a file, then the signature will exist in the dictionary will be replaced by a newSignature
instance with the same content: https://github.com/theupdateframework/python-tuf/blob/8a03abfdeb5a6b6bc892859f65425be0834893a8/tuf/api/metadata.py#L389.This removes the possibility of two existing
Signature
instances with the same content.The question is do we want to allow duplicating signatures?
I think duplicating signatures could be a problem because they artificially increase the numbers of signatures used and thus
threshold
loses its importance.I don't think an attacker can utilize that as for that he/she will need the private key used for signing (and if he has that he can break the system on multiple levels), so maybe I shouldn't worry.
@lukpueh any thoughts?
PS: If you want to remove an already existing signature you can directly use
==
and not rely onkeyid
existing in a signature as a__eq__
implementation exist for theSignature
class:securesystemslib/securesystemslib/signer.py
Line 40 in d3f4888
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 think in-toto's metadata wrapper do that.
@adityasaky whats your opinion about this.
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.
in-toto does allow multiple signatures from the same key on a particular document but that's because multiple signatures only really apply to the layout file. For a threshold k out of n keys for a particular step, you're not going to have one link with multiple signatures, but rather k different link files. We verify here to see we have k distinct link metadata files: https://github.com/in-toto/in-toto/blob/f6a4f6239ef28fc482adf147af79f030086f63f0/in_toto/verifylib.py#L506-L507. As for the layout, verification is dependent on each submitted public key.
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 still think that in order to escape possible problems regarding threshold computation it's better if we don't allow duplicating signatures. They just don't make sense to exist.
@adityasaky maybe before merging, we can wait for @lukpueh input 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.
We could, as long as we don't block @PradyumnaKrishna. I maintain that we shouldn't be getting too involved with duplicate signatures in the DSSE implementation. Perhaps this is even a hint that we shouldn't get too involved in the threshold aspects in the envelope either as each consumer may have different takes on how to handle thresholds. Looking for duplicate signatures gets messy here, IMO, for a number of reasons:
I think, then, trying to calculate thresholds and handling duplicate signatures gets very opinionated, and it's best to leave it to the consumers of the library, especially since each one may have their own take on threshold values. If we do want to, we'd have to map every key to every signature (which we more or less do when there are no keyids), calculate a consistent keyid value for every key, and then use that to track duplicates, all of which I think may be overkill. What do you think?
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 mind getting input from @lukpueh. I am working parallel on payload parser on my local.
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.
@adityasaky makes some good points. Let's not check for duplicates here, but leave it to the consumer/verifier.
Regarding...
I might be misunderstanding your thought, @MVrachev, but the premise of signature threshold verification is that the attacker can indeed control some private keys, but as long that number of keys is below the threshold the attacker can't compromise the data. So verifying the threshold does serve a purpose, but it has to happen at verification time (also see my comment below).
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.
Is it worth adding a TODO comment either here or in the docstring above that what we do here, i.e. mandating keyids in signatures in order to consider them for verification, is not dsse spec compliant?
see secure-systems-lab#416 (comment)
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.
FYI and because you asked about how to deal with exceptions that might be raised in
Key.verify
:theupdateframework/python-tuf@414dfc8 discusses returning boolean vs. raising exception in
Key.verify
and opts for the latter.I suggest we change our
Key
implementations to do the same thing, i.e. returnNone
if signature verification passes and raise an exceptions (or rather different exceptions with a common base) in all other cases. Then the caller does not have to case handle True/False/Error but only error type.I also suggest to do it in a separate PR.
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.
@PradyumnaKrishna, would you mind creating an issue for this?