rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
198 stars 55 forks source link

Inconsistent handling of password parameters in FFI APIs #930

Open dewyatt opened 4 years ago

dewyatt commented 4 years ago

Description

We're a little inconsistent on how we deal with password parameters in some FFI APIs.

We have the password provider concept, which was planned for cases where a password may be required but useful context may not be available to the client at the call site.

Some APIs accept a NULL password parameter and this may do different things depending on the function (fallback to provider, etc).

Summary

Previously, the FFI password provider mostly performed a single function: "provide an existing password for this existing thing".

At this point the FFI password provider is often performing two functions: "provide an existing password for this existing thing, or provide a new password for this thing we're creating".

I think there are two main options:

  1. Use the FFI password provider only to retrieve existing passwords for existing things.
  2. Use the FFI password provider to retrieve existing passwords for existing things, and also to retrieve new passwords for new things.

I have always leaned towards 1, which #925 would violate along with some others. I don't think I agree with that PR in hindsight.

Regardless, I think we have some inconsistent things that are probably worth discussing.

Examples (of current usage)

Generating a primary key

rnp_op_generate_create(&op, ffi, "RSA");
// ...
rnp_op_generate_set_protection_password(op, getpass("Enter password: ")); // no provider used
rnp_op_execute(op);

Or

rnp_ffi_set_pass_provider(ffi, &string_copy_password_callback, getpass("Enter password: "));
rnp_op_generate_create(&op, ffi, "RSA");
// ...
rnp_op_generate_set_request_password(op, true);
rnp_op_execute(op); // provider used
// NOTE: should probably restore the old provider/ctx so we don't have weird bugs later
rnp_ffi_set_pass_provider(ffi, NULL, NULL);

Generating a subkey

rnp_key_handle_t primary = NULL;
rnp_locate_key(ffi, "userid", "uid", &primary);

rnp_op_generate_subkey_create(&op, ffi, primary, "RSA");
rnp_op_generate_set_protection_password(op, getpass("Enter password: ")); // no provider used
// NOTE: this will only call the provider to unlock the primary
rnp_op_generate_execute(op);
rnp_key_handle_t primary = NULL;
rnp_locate_key(ffi, "userid", "uid", &primary);

rnp_op_generate_subkey_create(&op, ffi, primary, "RSA");
rnp_op_generate_set_request_password(op, true);
// NOTE: This may call the provider twice, first to unlock the primary,
// and then to provide a new password for the new subkey. The provider
// will have to check "do I have a password for this key in my database/etc?".
// If not, it must be asking for a new password for a new key.
rnp_op_generate_execute(op); // provider used

Details

antonsviridenko commented 4 years ago

For example, in OpenSSL (as I remember) same password callback is used for both providing existing password and setting new one