Improve `rnp_op_verify_execute` semantics: return `RNP_SUCCESS` if any signature is valid.

[dkg]

As noted in #1257 RNP's default signature verification semantics appear to require every signature in a sequence of signatures to validate in order for the functions to return a success.

This implicit policy makes RNP effectively block evolution of the OpenPGP protocol because it means that updated signatures (of whatever form -- new keys, new algorithms, new signature versions) cannot be distributed alongside existing signatures to any consumer that uses RNP in this way to validate signatures.

See discussion on the IETF OpenPGP mailing list about why this seems to be necessary for interoperability and an ecosystem that is possible to evolve.

The ideal simple semantics would be to return RNP_SUCCESS if any of the signatures is valid, not all.

Slightly more sophisticated semantics might be to indicate that at least N distinct signers need to have offered signatures, where the user supplies N. But I also don't think is particularly clear: what counts as a distinct signer is a subtle question; and i don't know of many users who want this functionality. So starting with the simple semantics seems reasonable, and then let the users walk the list of signatures if they have more sophisticated requirements.

I understand the desire to avoid changing the semantics of an existing function, however harmful the existing semantics might be to the ecosystem. To avoid changing the existing semantics, I recommend defining a new operation (e.g. rnp_op_verify2_* instead of rnp_op_verify_*, though i'm sure you can do a better naming job). The new operation should offer the simple semantics; you could then also mark the old operation as deprecated, to help callers transition to the new operation.

In a future ABI break, you could remove the older, deprecated operation.

[ni4]

It could be easier to add some more general function rnp_op_verify_set_flags(op, RNP_VERIFY_REQUIRE_SINGLE_SIG) which could be reused for other behaviour-changing purposes later (we have rnp_op_encrypt_set_flags() already).

[dkg]

So the current default behavior is brittle and risks limiting ecosystem development because simple users will not know that they should be setting this flag.

Offering this kind of set_flags function also makes it trickier to look for instances where the default behavior is used; i want to find a way to figure out who i need to encourage to switch.

I'd be OK with the set_flags approach if the new release changes the default semantics to the non-brittle behavior, and the user has to rnp_op_verify_set_flags(op, RNP_VERIFY_REQUIRE_ALL_SIGS_VALID) to get back the current brittle behavior. Does that seem plausible?

[dkg]

I'm generally leery of setting stateful flags in a C FFI API. For one thing, it is not obvious that any given version of the library supports the flags you're trying to set (or that it knows to ignore the flags you're explicitly not setting). There are multiple ways for a library (and the user of a library) to get that subtly wrong or make it difficult to use. For example, in the current interface, how do i ensure that RNP_ENCRYPT_NOWRAP is not set for an encryption operation? The function's name is …_set_flags, not …_clear_flags, so it's not obvious to me that i'd be turning it off the flag if i just pass it in a 0. But if that works (if that's the way that i'm supposed to make sure that RNP_ENCRYPT_NOWRAP is not set), then if i pass a 0 then maybe i'm also turning off some other flag that i don't know about or understand. Maybe there's a flag in some new version of rnp that i'm running against without even knowing it (because the library was upgraded before my application was), and it's on by default and my passing a 0 is turning it off, yikes.

Stateful flags in a C API are tough to get right, tough to use, and tough to maintain over time. Going that route seems like inviting trouble.

Another possible approach:

• add rnp_op_verify_all_execute as an alias for the current rnp_op_verify_execute behavior.
• add rnp_op_verify_any_execute with the proposed semantics

Either explicitly deprecate rno_op_verify_execute, or change its behavior to be an alias of rnp_op_verify_any_execute.

[ni4]

I'd be OK with the set_flags approach if the new release changes the default semantics to the non-brittle behavior, and the user has to rnp_op_verify_set_flags(op, RNP_VERIFY_REQUIRE_ALL_SIGS_VALID) to get back the current brittle behavior. Does that seem plausible?

I don't think that behaviour should be changed in a such radical way. Already existing API users may rely on the default behaviour, described in the header. I.e. approach is future version should behave in the same way as previous versions unless something new is called.

For example, in the current interface, how do i ensure that RNP_ENCRYPT_NOWRAP is not set for an encryption operation?

Then library would be stick to the old behaviour. Probably this should be described more in the documentation (as well as what would happen if calling *_set_flags(0) ).

[dkg]

If you don't think it's appropriate to change the semantics of this function, then i recommend introducing a new function that has reasonable default semantics, and to deprecate this one.

I am completely sympathetic to your goal of maintaining ABI stability, as surprising changes can cause disruptions downstream. That said, i don't think that you want a new user of librnp to have to know that there's a specific dance you have to do to get it to behave in the way that most implementations are actively going to want. Can you point to an instance of a user of librnp that actually wants the current semantics where a single unknown signature will cause a valid signature to be ignored?

[ni4]

@dkg sorry, somehow missed to reply here in time. Finally this settled up in a head (mostly due to the security concerns), and we decided to change to the behaviour you suggested: return RNP_SUCCESS when there is at least one valid signature. It should not affect most of the users as most use-cases have just a single signature.

[ni4]

Fixed via #1896