rnpgp / rnp

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

Questions: Searching keys, ambiguity, signing keys #915

Open kaie opened 4 years ago

kaie commented 4 years ago

I'd like to ask a few questions, but I couldn't find a mailing list, so excuse me if this isn't the best place for asking.

(1) Is function rnp_locate_key the correct API to search for a key by email adress?

(2) What happens if more than one key is available for that email address? API rnp_locate_key only allows returning a single key handle. Will RNP randomly return the first key it finds? Is there a way to obtain a list of all matching keys? (Using the first substring match might prevent finding the real key, e.g. searching for joe@example.com but finding smith.joe@example.com)

(3) Does RNP support key signing? Using your own private key to sign the key of someone else, and adding that signature to the other person's key?

kaie commented 4 years ago

Regarding (2), Looks like I could use rnp_identifier_iterator_create to iterate over all keys and compare all keys myself.

ni4 commented 4 years ago

Hi @kaie, This is the perfect place to ask questions, as most communication goes via issues. (1) This function is mostly for finding key via unique identifier (fingerprint, user id, etc), and it compares the whole identifier (i.e. it will not search by part of the userid). If you need more flexible search you should use rnp_identifier_iterator_create(). Also you may want to take a look at cli_rnp_get_keylist() in fficli.cpp. (2) You are correct, you should iterate over keys and do comparison in the way you need. While for somebody it would be enough to do simple substr() check on userid, other one would like to do regex() comparison, or do some more checks (key algorithm, for example). (3) While most of the underlying code is ready for this it still not exposed via API/CLI. However it is definitely planned for implementation (as other key editing functions).

kaie commented 4 years ago

Thanks for the quick answers! Some more questions:

(4) Which keystore format implementation do you consider the most stable? If an application wants to pick a keystore format for long term use (many years) which format would you expect to be future proof (with future RNP versions)?

(5) Have you considered allowing an application to configure the filenames used for the keystore, to avoid that users might decide to run gpg on the RNP managed keystore? I'm worried that GPG might potentially destroy the contents in a way that makes further operation with RNP impossible (such as potential in place migration performed by future versions of GPG).

ni4 commented 4 years ago

(4) I think that it would be good idea to stick to original OpenPGP key store format (transferable OpenPGP keys). However if you'll need to work with huge amount of keys then you'll need anyway some custom database-based solution for faster key lookup. Anyway we don't have our own format yet and just allow to use GnuPG key storages (which format definitely could be and will be changed in future).

(5) Yeah, that makes sense - while right now it is easy to import key data from GnuPG homedir, we should have some own storage format to not let it be mixed up with GnuPG. I'll file issue for this if there is no one yet.

kaie commented 4 years ago

Thanks. Have you considered to export the RNPERROR codes in a public header?

ni4 commented 4 years ago

@kaie I was also thinking about it - at least it is needed for signature verification status. Just didn't get to this task yet.

kaie commented 4 years ago

Thanks for exporting the header.

I have a comment regarding the callback functions defined in rnp.h

You defined callback rnp_password_cb the way I'd expect it to. However, for several others, you used a style that looked unexpected:

typedef ssize_t rnp_input_reader_t(void *app_ctx, void *buf, size_t len);
typedef void    rnp_input_closer_t(void *app_ctx);
typedef bool    rnp_output_writer_t(void *app_ctx, const void *buf, size_t len);
typedef void    rnp_output_closer_t(void *app_ctx, bool discard);

rnp_result_t rnp_input_from_callback(rnp_input_t *       input,
                                     rnp_input_reader_t *reader,
                                     rnp_input_closer_t *closer,
                                     void *              app_ctx);

I'd have expected those to be:

typedef ssize_t (*rnp_input_reader_t)(void *app_ctx, void *buf, size_t len);
typedef void    (*rnp_input_closer_t)(void *app_ctx);
typedef bool    (*rnp_output_writer_t)(void *app_ctx, const void *buf, size_t len);
typedef void    (*rnp_output_closer_t)(void *app_ctx, bool discard);

rnp_result_t rnp_input_from_callback(rnp_input_t *       input,
                                     rnp_input_reader_t reader,
                                     rnp_input_closer_t closer,
                                     void *              app_ctx);

Just giving you the feedback that this inconsistency confused me. It probably works both ways.

However, type rnp_password_cb can be directly used as a variable type for a function pointer. Type rnp_input_reader_t must be used as "rnp_input_reader_t *" when assigning a function pointer.

ni4 commented 4 years ago

@kaie I don't remember why it was written exactly this way, maybe @dewyatt has an idea. But does it give any problems? While more common usage is really (funcname), it's almost the same and difference is only where `` is written, or do I misunderstand something?

kaie commented 4 years ago

No problems right now :) Yes, only the place where the * is written.

kaie commented 4 years ago

I'm trying to encrypt and have added the primary key (sign/certify) using rnp_op_encrypt_add_recipient. When executing, I get RNP_ERROR_NO_SUITABLE_KEY. I conclude I must search for the appropriate subkey myself, and use that when calling rnp_op_encrypt_add_recipient, right? I don't see an API like "find appropriate subkey for encrypting for this primary key", so I assume I have to write that loop myself. If I have missed or misunderstand something, I'll appreciate your advice :) Thanks

ni4 commented 4 years ago

@kaie yeah, now it will attempt to use main key if it is specified via key id/key fp. And it is subject for improvement, we should have an issue for this. However currently you can specify userid as recipient and rnp will find corresponding subkey automatically.

ni4 commented 4 years ago

@kaie Oops, I was wrong with the previous comment (was from phone so unable to check the codebase), sorry. Actually rnp_op_encrypt_add_recipient() checks whether primary key is suitable for operation and if not - then searches subkey list. Could you please provide more information about the case? Which flags are set for primary key and subkey(s)?

kaie commented 4 years ago

I think the cause for that problem was issue 1001. After I implemented the search for the subkey, I still got the same error, and the noticed it's about issue 1001. Once we have a fix, I can check if my loop is unnecessary.

kaie commented 4 years ago

With my initial hack from https://github.com/rnpgp/rnp/issues/1001, and after removing my code to search for a suitable subkey, (added primary key as recipient), encrypting still works.

ni4 commented 4 years ago

@kaie thanks for investigation. I'm trying to figure out which would be the correct behavior. Current thinking is that if key has valid self-signature and certification signature(s) from unknown key(s) then it still should be considered as valid. The same about invalid signatures - anybody can add invalid certification and mark key as invalid.

So logically third party certifications should be able to make impact only on key's trust settings (I trust Alice, Alice trust Bob, so I trust Bob).

kaie commented 4 years ago

How do I create a release archive that includes a version.h file (from a snapshot)

kaie commented 4 years ago

That is: a "source code" archive

ni4 commented 4 years ago

@kaie version.h is generated by cmake, and later stored in cmake build folders. But why do you need it?

kaie commented 4 years ago

Thanks. I think I misunderstood something. Our local build complained about the file being missing, but I should talk to our release engineer how to trigger a rebuild inside the thunderbird build.

kaie commented 4 years ago

I should have read https://hg.mozilla.org/comm-central/file/tip/third_party/README.rnp ... we're all good.

kaie commented 4 years ago

A question regarding multithreading. I couldn't find the terms thread or mutex mentioned anywhere in the RNP code base. Do you require that all RNP operations are done on a single thread, with no concurrency protection?

For Thunderbird's use, a single thread requirement might be acceptable. Although in the future, it might be nice to have support for key generation on a background thread.

kaie commented 4 years ago

FYI, this is simply a question about the current status, no changes are necessary right now.

ni4 commented 4 years ago

@kaie Currently there is no any locking/multithreading support in library so all objects should be used from the single thread. I don't remember whether we have any discussion about adding multi-threaded support, but probably we should.

Case with background key generation can be handled with creation of the separate FFI object, calling generate on it in a secondary thread, and then exporting-importing new key once operation is completed.

kaie commented 4 years ago

Thanks @ni4 - So RNP doesn't have any global state. Any state is contained within a FFI object?

Regardless of RNP's safety, another question is Botan's safety.

ni4 commented 4 years ago

@kaie Yeah, all the state information is stored in FFI object, however you may have any number of them. And current Botan's version as I'm aware is thread-safe by default. Maybe @dewyatt has some more information?

kaie commented 4 years ago

Which Botan version should we use with latest RNP ? We're still at 2.13.0, should we update to 2.14.0 ?

ni4 commented 4 years ago

@kaie While latest should be better, 2.14.0 had some problems with MinGW/thread pool (see the details here: https://github.com/rnpgp/rnp/issues/1104 ). Not sure whether this will affect your build. RNP itself will work the same way with both versions.

kaie commented 4 years ago

@ni4 When using rnp_key_get_signature_at to obtain a rnp_signature_handle_t - is it possible to ask RNP to verify that signature, and obtain the status of that signature verification? I cannot find an API for that, so I suspect that currently it isn't possible?

kaie commented 4 years ago

Actually, my question is about rnp_uid_get_signature_at

kaie commented 4 years ago

... or ... will rnp_uid_get_signature_count and rnp_uid_get_signature_at give just the subset of signatures that RNP considers valid?

ni4 commented 4 years ago

@kaie Unfortunately there is no such API yet. Currently all signatures are returned, and there is no way to validate those. Such API for userids is a part of issue #1022. Also it is logical to add function rnp_key_is_valid(). Is this of high priority?

kaie commented 4 years ago

@ni4 Thanks for the confirmation. I had intended to implement code that automatically accepts another public key as "ok to use", if it carries a signature from the user's own key (when migrating key ring from gnupg into TB). But if we cannot currently verify that the signatures are correct, we shouldn't rely on that signatures. We'll use the approach to require the user to manually mark a key as "ok to use". We can work on "automatic ok to use, if it has valid certification" later.

ni4 commented 4 years ago

@kaie Got it. Yeah, this functionality is must have and will be put in queue.