Context.decrypt() throws an error if *any* signature is bad
Open, Needs TriagePublic

Description

the logic behind Context.decrypt()'s signature verification looks like it's going to throw an error if *any* signature does not validate.

However, the logic for most applications that pass verify=True is that they need *one* signature to validate -- the rest could be garbage, or signatures using new algorithms that we don't know anything about, or signatures from keys we don't know about yet.

I think the current implementation is brittle, and is prone to cause problems, and the logic probably needs an overhaul.

furthermore, if a list of keys is passed to verify=, the logic says that a signature from each key is needed. that's fine, but the current logic is that *even if that's the case*, an additional (spurious, broken, or noisy) signature will cause the decrypt operation to throw an error.

This function should be less brittle, and the test suite should exercise it (maybe by making someting that has two signatures: one of them a deliberately broken, but the other one valid).

Details

Version
1.12.0
dkg created this task.Wed, Dec 5, 6:18 AM

Is this fixed now?

dkg added a comment.Wed, Dec 5, 2:04 PM

@aheinecke thanks for the merge of my other branch! sadly, that branch does *not* address this issue yet. It doesn't even test for it. :( I can work on trying to fix it (and test it) if there's a consensus that we want this particular change in behavior.

dkg added a comment.Wed, Dec 5, 6:30 PM

One more semantic question about how folks think Context.decrypt(verify=True) should work: if the decrypted thing has no signature at all, should the function succeed without throwing an exception? it currently does, but the returned verify_result has its signatures member set to the empty list.

I think this is correct ("i verified all signatures i found, and here they are (all none of them)"), but it might suprise someone who expects verify=True to throw an error if the data wasn't signed.

Anyway, if this is correct behavior (i think it is), then why should the existence of an invalid signature cause an exception to be thrown at all? If the bad signature was just stripped, no error would be thrown. I'm inclined to say that Context.decrypt() should just filter out (ignore) all bad signatures from its returned verify_result. That will avoid changing the current semantics too drastically -- the current function only ever returns lists of valid signatures -- while making it less brittle in the face of unevaluable signatures.