Skip to content

Handle unknown pubkey algorithms safely (set RNP_LOAD_SAVE_PERMISSIVE by default?) #2204

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

Open
dkg opened this issue Mar 23, 2024 · 4 comments

Comments

@dkg
Copy link
Contributor

dkg commented Mar 23, 2024

The rnp-sop implementation used for the Interop Test Suite fails to ignore a subkey with an unknown algorithm. There is a request to make rnp-sop set RNP_LOAD_SAVE_PERMISSIVE by default, but it's not clear that it will be adopted, as it might not reflect the default behavior of a naive user of librnp. This makes it look like RNP is currently a blocker toward the ecosystem being able to adopt and distribute new pubkey algorithms, even if they are adopted in a backward-compatible way (i.e., widely-adopted algorithms are available in the same certificate as newer algorithms).

I see at least three ways to address this:

  • RNP could implement a native sop interface (Please implement a "sop" (Stateless OpenPGP Command Line) interface  #1760). In that case, you can set whatever flags you want to set for the test suite. That implementation can also serve as a guide to users who are trying to decide how to use the library
  • If RNP_LOAD_SAVE_PERMISSIVE isn't set, librnp could simply strip unknown public keys from a certificate, rather than rejecting the entire certificate. This would permit encryption to such a backwards-compatible certificate, even though the unknown subkey was discarded.
  • RNP could set RNP_LOAD_SAVE_PERMISSIVE by default, so that the unknown pubkey remains available in the storage for in case some future version of RNP could parse and use it.
@dkg
Copy link
Contributor Author

dkg commented Mar 23, 2024

Presumably a rigorous stateful OpenPGP implementation would want to do
something like this on certificate import (ignoring questions about third-party certifications for
the purposes of this discussion):

  • on ingesting a certificate with self-signed material, only ingest the
    self-signed components that actually cryptographically verify.

  • avoid discarding unknown-but-verified self-signed components (unless
    you run into storage constraints) -- those components might be useful
    later, after an upgrade

  • if the application layer wants to know whether the certificate is
    usable for a given purpose or not (e.g. signature verification,
    encryption), only report on the known components, even though the
    unknown components are stored.

  • on upgrade, a given certificate might suddenly become usable, that's good!

But the RNP_LOAD_SAVE_PERMISSIVE semantics are a strange combination
that doesn't really match the above reasoning that i can tell, whether
it is set or cleared.

@ni4
Copy link
Contributor

ni4 commented Mar 26, 2024

RNP library is designed with backward compatibility in mind, and from this point of view changing the default behaviour is wrong. That's actually why this flag was added at some point. So if implementation which use RNP library want to fail on unknown keys then it should not set this flag, and set otherwise. We do not dictate desired behaviour, and give a way to adobt any policy.

So if SOP implementers do not want to set this flag it's not a problem of RNP, but their intended behaviour.

@dkg
Copy link
Contributor Author

dkg commented Mar 26, 2024

This discussion reminds me of the discussion we had around #1820 , where backward compatibility was cited as the reason for not changing the default, even though the default was actually blocking ecosystem evolution (because of the behavior when an unknown signature was attached). I'm grateful that you ended up resolving that bug by changing the default in #1896 !

I am not claiming that the exact right answer here is to set the RNP_LOAD_SAVE_PERMISSIVE flag by default. maybe there are some more subtle semantics that would be a preferable default. But the basic shape of the problem is the same as #1820: rnp encounters an OpenPGP artifact that has some stuff it understands, and some stuff it doesn't understand. It rejects the entire artifact by default, unless the caller knows the right way to invoke it to get compatible, interoperable behavior.

The default should be compatible, interoperable behavior. whether that means ignoring and dropping the stuff it doesn't understand; or ignoring and saving the stuff it doesn't understand, i don't have much of a preference (though i think most rnp users might appreciate having the extra stuff stored as long as it has been cryptographically confirmed, so that they can use it after an rnp upgrade). But rejecting the stuff that rnp does understand means that no one else can introduce new OpenPGP material without worrying that it will break things for users of librnp.

@kaie
Copy link
Contributor

kaie commented Mar 27, 2024

Related Thunderbird discussion:
https://bugzilla.mozilla.org/show_bug.cgi?id=1884337

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants