Skip to content
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

Inconsistent handling of password parameters in FFI APIs #930

Open
dewyatt opened this issue Oct 24, 2019 · 1 comment
Open

Inconsistent handling of password parameters in FFI APIs #930

dewyatt opened this issue Oct 24, 2019 · 1 comment

Comments

@dewyatt
Copy link
Contributor

dewyatt commented Oct 24, 2019

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

  • rnp_key_protect - password parameter can NOT be NULL
  • rnp_key_unprotect - password parameter CAN be NULL, in which case the provider is used
  • rnp_key_unlock - (see rnp_key_unprotect)
  • rnp_generate_key_* - password parameter CAN be NULL, in which case the key is unprotected
  • rnp_op_generate_set_protection_password - password parameter can NOT be NULL, rnp_op_generate_set_request_password must be used to utilize provider. If neither is used, the generated key is not protected.
  • rnp_op_encrypt_add_password - password parameter CAN be NULL, in which case the provider is used (since recent PR Allow to use password provider in rnp_op_encrypt_add_password #925 )
@antonsviridenko
Copy link
Contributor

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

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

No branches or pull requests

2 participants