rnpgp / rnp

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

FFI: add function to retrieve default subkey for the sign/encrypt operation. #1457

Closed ni4 closed 3 years ago

ni4 commented 3 years ago

Description

Most likely users will need some way to understand which subkey (or primary key itself) is picked for signing or encryption operation before executing it, so we should add function like rnp_key_get_default_key(primary_key, operation_flags).

antonsviridenko commented 3 years ago

So basically find_suitable_key() can be renamed to rnp_key_get_default_key() with some modifications?

ni4 commented 3 years ago

@antonsviridenko Not exactly, as new function should be a FFI one, i.e. set parameter to instance of rnp_key_handle_t (or NULL if there is no default key), and return RNP_SUCCESS/RNPERROR*

antonsviridenko commented 3 years ago

I've read related Thunderbird discussions, looks like more likely we need a way to configure subkey selection preferences. So users (API users and CLI users) can explicitly set how subkeys should be selected. Like most recent/most secure (based on key bits, not sure about sorting between different algorithms). Sorting order on algorithms can be also explicitly set by users. See https://www.openssl.org/docs/man1.1.1/man3/SSL_set_cipher_list.html as an example, OpenSSL users can set their preference order for algorithms using string parameter.

ni4 commented 3 years ago

I've read related Thunderbird discussions, looks like more likely we need a way to configure subkey selection preferences. So users (API users and CLI users) can explicitly set how subkeys should be selected. Like most recent/most secure (based on key bits, not sure about sorting between different algorithms).

Let's keep it simple right now, and use find_suitable_key() as it is at the moment. Just document it in the way "by default it selects latest valid subkey", and add some flags field (0 at the moment) so we may update the function later on. If API user needs more complicated checks/behaviour, he may implement in the way he want by using other API calls (actually, that's how Thunderbird works at the moment).

antonsviridenko commented 3 years ago

so we should add function like rnp_key_get_default_key(primary_key, operation_flags)

I see integer flags parameter in one API function https://github.com/rnpgp/rnp/blob/master/include/rnp/rnp.h#L1394 and in another case usage flag is a string parameter https://github.com/rnpgp/rnp/blob/master/include/rnp/rnp.h#L761

which one should I prefer for this new function?

ni4 commented 3 years ago

@antonsviridenko hm, in this case it would be better to use string as user would need key for a single operation. Also please add uint32_t flags field, which is now zero, so we may update the behaviour in future.

antonsviridenko commented 3 years ago

Not exactly, as new function should be a FFI one, i.e. set parameter to instance of rnp_key_handle_t (or NULL if there is no default key), and return RNP_SUCCESS/RNPERROR*

@ni4 let's clarify this part

or NULL if there is no default key

So should primary key parameter be optional here? I.e. if it is set to NULL, default primary key is selected first, and then its subkeys are searched for? Or did you mean returned subkey/primary key, i.e. pointer parameter which should be filled by found subkey/primary key (or NULL) on return?

rnp_result_t 
rnp_key_get_default_key(rnp_key_handle_t primary_key, const char *usage, uint32_t  flags, rnp_key_handle_t *default_key);

Does this declaration look as expected?

ni4 commented 3 years ago

So should primary key parameter be optional here? I.e. if it is set to NULL, default primary key is selected first, and then its subkeys are searched for?

No, we should not pick default key. primary key must be non-NULL.

Or did you mean returned subkey/primary key, i.e. pointer parameter which should be filled by found subkey/primary key (or NULL) on return?

Exactly.

Does this declaration look as expected?

Yeah. Also we may add first flag here - SUBKEYS_ONLY, as it was done internally for autocrypt.

antonsviridenko commented 3 years ago

ok, now I am stuck at the problem how to derive the first argument for find_suitable_key(pgp_op_t op, ...). We don't know which operation user wants to perform using requested default key. Should we add it as an argument to this new FFI function?

ni4 commented 3 years ago

@antonsviridenko const char *usage should be used for this, or do I miss something?

antonsviridenko commented 3 years ago

I mean there is no 1:1 mapping between

typedef enum pgp_op_t {
    PGP_OP_UNKNOWN = 0,
    PGP_OP_ADD_SUBKEY = 1,  /* adding a subkey, primary key password required */
    PGP_OP_SIGN = 2,        /* signing file or data */
    PGP_OP_DECRYPT = 3,     /* decrypting file or data */
    PGP_OP_UNLOCK = 4,      /* unlocking a key with key->unlock() */
    PGP_OP_PROTECT = 5,     /* adding protection to a key */
    PGP_OP_UNPROTECT = 6,   /* removing protection from a (locked) key */
    PGP_OP_DECRYPT_SYM = 7, /* symmetric decryption */
    PGP_OP_ENCRYPT_SYM = 8, /* symmetric encryption */
    PGP_OP_VERIFY = 9,      /* signature verification */
    PGP_OP_ADD_USERID = 10, /* adding a userid */
    PGP_OP_MERGE_INFO = 11, /* merging information from one key to another */
    PGP_OP_ENCRYPT = 12     /* public-key encryption */
} pgp_op_t;

and

static const pgp_bit_map_t key_usage_map[] = {{PGP_KF_SIGN, "sign"},
                                              {PGP_KF_CERTIFY, "certify"},
                                              {PGP_KF_ENCRYPT, "encrypt"},
                                              {PGP_KF_AUTH, "authenticate"}};

So "sign" => PGP_OP_SIGN, "encrypt" => PGP_OP_ENCRYPT, "certify" => PGP_OP_SIGN (?) but what should be mapped to "authenticate"?

ni4 commented 3 years ago

@antonsviridenko Ah, got it. Just using RNP_OP_UNKNOWN should not cause any problems as it is used just for key lookup. Default ffi_key_provider doesn't use this field at all.

antonsviridenko commented 3 years ago

What should be the return value if there is no default key with desired parameters, success and set default_key to NULL or RNP_ERROR_NO_SUITABLE_KEY?

ni4 commented 3 years ago

@antonsviridenko While in rnp_locate_key() we use NULL approach, I think it would be better to return RNP_ERROR_NO_SUITABLE_KEY and NULL in key, so inattentive user will not use NULL key on success.