Skip to content

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

Closed
jku opened this issue May 7, 2021 · 9 comments
Closed

Metadata verification API tweaks #1377

jku opened this issue May 7, 2021 · 9 comments
Labels
backlog Issues to address with priority for current development goals

Comments

@jku
Copy link
Member

jku commented May 7, 2021

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:

@jku
Copy link
Member Author

jku commented May 7, 2021

I'll write down some ideas here.

  • using the words "sign*" and "hash" is useful: verify alone is really vague
  • sig verification and and the sig verification with threshold creates an annoying imbalance (in first case we ask metadata if it itself is signed, in second case we plan to ask metadata if some other metadata is signed with threshold of keys). Good naming helps but alternatively we could move sig verification to Key object
  • I don't have strong feelings on returning bool vs raising exceptions

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])

@joshuagl
Copy link
Member

The proposed API looks nice. Hard to reason about a Key object when that is not defined yet, but I think I prefer verification on the Metadata objects.

I'm in favour of raising exceptions if verification fails. It also seems to be what pyca/cryptography does, raising InvalidSignature if a signature is not valid.

@sechkova
Copy link
Contributor

sechkova commented May 13, 2021

Already implied by @jku's example but in addition current Metadata.verify() should be changed to take a Key as an argument and not a dictionary/mapping.

Looking at the current function implementation, 'key' which is a securesystemslib-style public key object, contains its keyid, while the Key class does not have such member. The class was designed to match the file format as described in the spec, but maybe Key should not be separated from its keyid?

@jku
Copy link
Member Author

jku commented May 13, 2021

The class was designed to match the file format as described in the spec, but maybe Key should not be separated by its keyid?

Great question, and I think you may be right: Key should contain keyid (but should just not serialize it).

@sechkova
Copy link
Contributor

sechkova commented May 14, 2021

* sig verification and and the sig verification with threshold creates an annoying imbalance (in first case we ask metadata if it itself is signed, in second case we plan to ask metadata if _some other_ metadata is signed with threshold of keys). Good naming helps but alternatively we could move sig verification to Key object

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.
If the common agreement is towards metadata.verify_delegate_threshold_of_signatures() then probably we can get by without metadata.verify as a public method and add a helper/wrapper of sslib.verify_signature() if needed.

Feels like this question links to #1306

@jku
Copy link
Member Author

jku commented May 21, 2021

Do we even need metadata to both verify itself and be able to verify other metadata?

This is good question: maybe it's worth it to reconsider moving verify_signature() into Key class:

  • Key verifies that it signs metadata (but this is typically not used by code outside the API)
  • Delegating Metadata verifies that delegate metadata is signed by correct threshold of keys (this is used by API users)

@sechkova sechkova added the backlog Issues to address with priority for current development goals label May 26, 2021
@jku
Copy link
Member Author

jku commented May 26, 2021

#1417 is about implementing verify_signature() in Key

@MVrachev
Copy link
Collaborator

MVrachev commented Aug 25, 2021

Do we have any to-do items left here? When can we close that issue?
All of the issues linked in the pr description or in this discussion here seem to be resolved.

@jku
Copy link
Member Author

jku commented Aug 25, 2021

Looks all done to me!

@jku jku closed this as completed Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals
Projects
None yet
Development

No branches or pull requests

4 participants