Handle exceptions in verify

[jku]

#1417 has been merged so this is good to go.

We want to handle exceptions in verify_signature(): Callers are not going to be interested in the various details of why verification failed (or rather, good error messages are important but no-one is going to write code to handle the different cases).

This call is typically used on the client: the metadata comes from remote repository, and we're trying to verify it. We might try to handle a specific error situation by saying something specific like "we can't verify this sig because we're missing dependency X"... but this could also just be a mistake in the remote end. I feel like UnsignedMetadataError with descriptive message and exception history is probably the best we can do.

Still, discussing each error case here probably makes sense... I'll write a list.

Fixes #1351

[sechkova]

Can we use "e" to add these descriptive messages that are promised in the description?
Or I should assume that this is to be added since this is still a draft?

[jku]

Well, I said "error message" but mean the whole traceback that includes the securesystemslib error if e.g. key is malformed.

[jku]

The error cases that I know of are listed below. All lead to a UnsignedMetadataError in the PR:

• This key does not have a signature at all in the metadata
• UnsupportedAlgorithmError (signature algorithm is not supported)
• signature fails to verify
• CryptoError (key may be malformed)
• FormatError (signature is not hex encoded hash, or key fails a format check)

From a clients perspective I think these are all the same: the error content may be interesting but the client can only regard the signature as invalid and cannot "handle" the situations specifically -- and cannot tell when the error might be a broken client install or broken metadata from repository. I can see how the opposing argument could possibly be made at least for the first two items: but I'd like to see someone actually make those cases before we add separate handling for them.

It's also noteworthy that client code is unlikely to call this directly: they will call the "verify delegate with threshold of keys" function once it exists, and that function will make multiple calls to this one: so forwarding specific errors to client becomes a major pain even if we wanted to do that.

[jku]

Since last comment I've added some testing on Martins suggestions, and rebased on develop

[joshuagl]

Current repository_lib verifies signatures in _generate_and_write_metadata() before writing metadata to ensure that the required threshold of signatures is met. I don't know that this is desirable behaviour to replicate moving forward, because it's easy to imagine a scenario where one might want to incrementally sign metadata, but it does point to a possible use-case for more detailed exceptions in verification failures.

That being said, I'm all for adding these later when we know what we need and how we want to use them. It might be as simple as having more complex exceptions derived from UnsignedMetadataError?

[jku]

Current repository_lib verifies signatures in _generate_and_write_metadata() before writing metadata to ensure that the required threshold of signatures is met. I don't know that this is desirable behaviour to replicate moving forward, because it's easy to imagine a scenario where one might want to incrementally sign metadata, but it does point to a possible use-case for more detailed exceptions in verification failures.

Even with a repository tool, I can't imagine what the code could do differently based on the error type... but the possibility does exist of course.

As a side note, there's a lot of functionality that is "adjacent" to writing Metadata (version bumps, signing, choosing the filename (versioned or not), updating related MetaFiles). This sounds like a possible repository library component that is the equivalent of MetadataBundle in the ngclient code: Something that tracks a collection of metadata and makes sure things are written to disk in the correct order while handling all of the related changes.

That being said, I'm all for adding these later when we know what we need and how we want to use them. It might be as simple as having more complex exceptions derived from UnsignedMetadataError?

I believe this is correct.