Filters on ValidCert, ValidUserIDAmalgamation, ValidKeyAmalgamation, etc. should be fallible
Consider the following code taken from sq:
fn get_keys(...) -> ...
{
...
for key in tsk.keys().with_policy(p, timestamp).alive().revoked(false)
.key_flags(flags.clone())
.supported()
...
return Err(anyhow::anyhow!(
format!("Found no suitable signing key on {}", tsk)));
This code looks for a valid signing or certification key and ignores keys that are not alive, are revoked, or are not supported. If no keys are found, then this returns a rather generic error message.
This error message confused me when I was testing sq certify with the new --time option. I knew the certificate in question had a signing-capable subkey (sq inspect showed it to me). Unfortunately, it took me a little to realize why the signing key was not suitable: it wasn't alive at the time that I had specified.
We can improve the message by checking the conditions in the loop. I've modified get_keys to do exactly that. The code is a lot more verbose, which isn't great. But more importantly, it violates one of our API design principles: the easier way should be the better way.
I propose that we change these methods to be fallible. That is, they should return an Iterator<Item=Result<Key>> rather than an Iterator<Item=Key> explaining why a key is inappropriate rather than silently skipping the key. Then it will be possible to still use the loop construct. And, to make it even easier to use a positive result and ignore any errors, we should have a sort method to buffer errors. Then it is possible to do something like: match vc.keys().for_signing().sort().next() { ... }.
Because we sometimes really want to ignore particular keys and a detailed error message is distracting, we should keep variants that just filter the results. In the previous example, it probably makes sense to silently ignore non-signing subkeys. So for each filter that we currently have we should have two variants: one that filters and one that is fallible. That is, for key_flags we'd have key_flags_filter and key_flags_fallible, for alive we'd have alive_filter and alive_fallible, etc.
I think that test_fallible should either be called test or be an alias for test. But, we can't do that for 1.0, because it would change the API. Instead, I think we should add both test_filter and test_fallible to 1.0, and make test an alias for test_filter in 1.0. Then, in 2.0 we should change test to be an alias for test_fallible. We should also add a big warning in the 1.0 documentation that test routes to test_filter and not test_fallible, which is often more appropriate.
Don't forget to apply the same logic to ValidComponentAmalgamation::certifications.