-
Notifications
You must be signed in to change notification settings - Fork 278
Metadata verification API tweaks #1377
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
Comments
I'll write down some ideas here.
Potential API with bool return values (exceptions only for e.g. unsupported encryption algorithm): # Ask 'Metadata' if it is signed by 'key'
Metadata.is_signed(
key: Key, signed_serializer: Optional[SignedSerializer] = None
) -> bool
# Ask 'Metadata' if its delegated metadata 'delegate' is signed by
# correct threshold of keys
Metadata.is_delegate_signed_with_threshold(
role: str, delegate: Metadata
) -> bool
# Ask MetadataInfo/TargetInfo if given data matches the known hashes
MetadataInfo.hashes_match(data: Union[bytes, BinaryIO]) -> bool
TargetInfo.hashes_match(data: Union[bytes, BinaryIO]) -> bool Alternatively with exceptions, no return values # Ask 'metadata' to verify it is signed by 'key'
metadata.verify_signature(
key: Key, signed_serializer: Optional[SignedSerializer] = None
)
# Ask 'metadata' to verify that its delegated metadata 'delegate' is signed
# by correct threshold of keys
# The name is really long -- maybe verify_delegate_signatures() would be enough
metadata.verify_delegate_threshold_of_signatures(
role: str, delegate: Metadata
)
# Ask MetadataInfo/TargetInfo to verify that given data matches the known
# hashes
metadatainfo.verify_hashes(data: Union[bytes, BinaryIO])
targetinfo.verify_hashes(data: Union[bytes, BinaryIO]) |
The proposed API looks nice. Hard to reason about a I'm in favour of raising exceptions if verification fails. It also seems to be what pyca/cryptography does, raising |
Already implied by @jku's example but in addition current Looking at the current function implementation, |
Great question, and I think you may be right: Key should contain keyid (but should just not serialize it). |
Do we even need metadata to both verify itself and be able to verify other metadata? I think we should stick to one of the options. Feels like this question links to #1306 |
This is good question: maybe it's worth it to reconsider moving verify_signature() into Key class:
|
#1417 is about implementing verify_signature() in Key |
Do we have any to-do items left here? When can we close that issue? |
Looks all done to me! |
I'd like the API to handle all signature/hash verifications that it can (so that there's a single place that's considered "reference implementation").
I'd also like these to be consistent and easy to use correctly (and hard to use incorrectly). I'm filing this as an umbrella issue since this is harder to do bit-by-bit...
Issues:
verify()
(it returns a bool but multiple people have used the API incorrectly already, assuming it raises an exception)This doesn't mean it needs to raise: the issue could be fixed by renaming to
is_verified()
or evenis_signed()
The text was updated successfully, but these errors were encountered: