Check userids for validity. #1337

ni4 · 2020-10-31T11:49:22Z

This PR adds userid validity checks.
Userid is now considered as valid when:

it has valid, non-expired self-signature
this self-signature doesn't expire the key itself
there is no revocation for this userid.

This changes behavior to the following:

key cannot be searched by non-valid userid
primary key selection respects only valid userids
however, non valid userids still are enumerated, and should be checked for validity via rnp_uid_is_valid().

Also it adds few more functions which allows to retrieve uid-related data via uid handle.

Fixes #1022
Fixes #1126

ni4 · 2020-10-31T15:45:48Z

Single test is failing for ruby-rnp, it should be addressed via or together with rnpgp/ruby-rnp#69

rrrooommmaaa

LGTM, but I suggest to use std::set in places where we need to keep unique keys

rrrooommmaaa · 2020-11-06T13:14:38Z

src/tests/key-store-search.cpp

@@ -82,23 +83,22 @@ TEST_F(rnp_tests, test_key_store_search)
    for (size_t i = 0; i < ARRAY_SIZE(testdata); i++) {
        pgp_key_id_t keyid = {};
        assert_true(rnp_hex_decode(testdata[i].keyid, keyid.data(), keyid.size()));
-        list seen_keys = NULL;
+        std::vector<pgp_key_t *> seen_keys;


I think it's more convenient to use std::set here

@rrrooommmaaa That would require to satisfy https://en.cppreference.com/w/cpp/named_req/Compare, and if I understand correctly, pgp_key_t will not. Given that it's just for tests I don't see the reason to spend time on implement that. And, actually, now it works the same way as it worked before. What do you think?

@ni4 Aren't we comparing pointers?

@rrrooommmaaa Oh, my fault, somehow mislooked (how?) pointerness, sorry. Fixed and force-pushed.

rrrooommmaaa · 2020-11-06T13:15:08Z

src/tests/key-store-search.cpp

    }
    // keyid search (by_name)
    for (size_t i = 0; i < ARRAY_SIZE(testdata); i++) {
-        list       seen_keys = NULL;
-        pgp_key_t *key = NULL;
+        std::vector<pgp_key_t *> seen_keys;


I think it's more convenient to use std::set here

rrrooommmaaa · 2020-11-06T13:15:46Z

src/tests/key-store-search.cpp

-            pgp_key_t * key = NULL;
-            const char *userid = testdata[i].userids[uidn];
-            key = rnp_tests_key_search(store, userid);
+            std::vector<pgp_key_t *> seen_keys;


I think it's more convenient to use std::set here

rrrooommmaaa

LGTM!

ronaldtse · 2020-11-09T10:09:25Z

Seems that there are some failures with ruby-rnp. @dewyatt is there a way we can solve them once and for all?

dewyatt · 2020-11-11T11:13:09Z

It looks like this changes the behavior of rnp_locate_key so that searching via a userid without a valid cert would no longer return results where it may have before. Am I understanding that correctly?
Should we be doing that kind of filtering or should we let the API client decide what to do?

ni4 · 2020-11-11T11:56:13Z

It looks like this changes the behavior of rnp_locate_key so that searching via a userid without a valid cert would no longer return results where it may have before. Am I understanding that correctly?

Yes. Also userid will not be marked as primary if it is considered as invalid.

Should we be doing that kind of filtering or should we let the API client decide what to do?

Usage of non-signed userid (or one with invalid signature) should be considered as a security issue, so I think we should not allow it to be used by default. However, API user still able to list all userids and check their validities.

dewyatt · 2020-11-17T10:32:00Z

It looks like this changes the behavior of rnp_locate_key so that searching via a userid without a valid cert would no longer return results where it may have before. Am I understanding that correctly?

Yes. Also userid will not be marked as primary if it is considered as invalid.

Should we be doing that kind of filtering or should we let the API client decide what to do?

Usage of non-signed userid (or one with invalid signature) should be considered as a security issue, so I think we should not allow it to be used by default. However, API user still able to list all userids and check their validities.

Shouldn't we at least throw some documentation updates in here?

ronaldtse · 2020-11-20T04:16:38Z

Shouldn't we at least throw some documentation updates in here?

Agree

…uid_is_valid().

ni4 · 2020-11-20T10:57:12Z

Shouldn't we at least throw some documentation updates in here?

Agree

Agree as well, just got lost with those CI issues. Update documentation and force-pushed.

…d selection behavior.

ni4 · 2020-11-20T15:55:55Z

@dewyatt Should we merge this now? As I checked, only the single ruby-rnp test is failed. And CIFuzz shots it's leg on the container building steps.

dewyatt · 2020-11-20T16:22:12Z

@dewyatt Should we merge this now? As I checked, only the single ruby-rnp test is failed. And CIFuzz shots it's leg on the container building steps.

May as well yes

ni4 force-pushed the ni4-1022-userid-validity-checks branch 3 times, most recently from e140acf to 83e6b24 Compare October 31, 2020 15:05

ni4 mentioned this pull request Oct 31, 2020

Update spec to correctly handle userid validity checks. rnpgp/ruby-rnp#69

Merged

ni4 marked this pull request as ready for review October 31, 2020 15:44

ni4 requested review from rrrooommmaaa, dewyatt, antonsviridenko and ronaldtse October 31, 2020 15:45

antonsviridenko approved these changes Nov 2, 2020

View reviewed changes

rrrooommmaaa requested changes Nov 6, 2020

View reviewed changes

ni4 force-pushed the ni4-1022-userid-validity-checks branch from 83e6b24 to b916ada Compare November 7, 2020 19:09

rrrooommmaaa approved these changes Nov 8, 2020

View reviewed changes

ni4 mentioned this pull request Nov 13, 2020

Add FFI functions to retrieve key/uid revocation signature. #1342

Merged

ni4 added 5 commits November 20, 2020 12:48

Add FFI functions rnp_uid_get_type()/rnp_uid_get_data().

3345fca

Validate userids during key status refresh, and add FFI function rnp_…

27b1690

…uid_is_valid().

FFI: add function rnp_uid_is_primary().

1e5f2b9

Pick primary uid/search for uid only through valid ones.

31b2337

Add tests for new FFI uid-related functions.

646a4b3

ni4 force-pushed the ni4-1022-userid-validity-checks branch from b916ada to 0145ad8 Compare November 20, 2020 10:56

ni4 added 2 commits November 20, 2020 15:27

Update existing tests to match changed uid search/validity/primary ui…

07da1e9

…d selection behavior.

Run all centos7 GHA checks even if any of them failed.

5be4730

ni4 force-pushed the ni4-1022-userid-validity-checks branch from 958ddb9 to 5be4730 Compare November 20, 2020 13:27

dewyatt merged commit b72b1b7 into master Nov 20, 2020

dewyatt deleted the ni4-1022-userid-validity-checks branch November 20, 2020 16:22

ni4 added this to the v0.14.0 milestone Jan 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check userids for validity. #1337

Check userids for validity. #1337

ni4 commented Oct 31, 2020

ni4 commented Oct 31, 2020

rrrooommmaaa left a comment

rrrooommmaaa Nov 6, 2020

ni4 Nov 7, 2020

rrrooommmaaa Nov 7, 2020

ni4 Nov 7, 2020

rrrooommmaaa Nov 6, 2020

rrrooommmaaa Nov 6, 2020

rrrooommmaaa left a comment

ronaldtse commented Nov 9, 2020

dewyatt commented Nov 11, 2020

ni4 commented Nov 11, 2020

dewyatt commented Nov 17, 2020

ronaldtse commented Nov 20, 2020

ni4 commented Nov 20, 2020

ni4 commented Nov 20, 2020 •

edited

Loading

dewyatt commented Nov 20, 2020

Check userids for validity. #1337

Check userids for validity. #1337

Conversation

ni4 commented Oct 31, 2020

ni4 commented Oct 31, 2020

rrrooommmaaa left a comment

Choose a reason for hiding this comment

rrrooommmaaa Nov 6, 2020

Choose a reason for hiding this comment

ni4 Nov 7, 2020

Choose a reason for hiding this comment

rrrooommmaaa Nov 7, 2020

Choose a reason for hiding this comment

ni4 Nov 7, 2020

Choose a reason for hiding this comment

rrrooommmaaa Nov 6, 2020

Choose a reason for hiding this comment

rrrooommmaaa Nov 6, 2020

Choose a reason for hiding this comment

rrrooommmaaa left a comment

Choose a reason for hiding this comment

ronaldtse commented Nov 9, 2020

dewyatt commented Nov 11, 2020

ni4 commented Nov 11, 2020

dewyatt commented Nov 17, 2020

ronaldtse commented Nov 20, 2020

ni4 commented Nov 20, 2020

ni4 commented Nov 20, 2020 • edited Loading

dewyatt commented Nov 20, 2020

ni4 commented Nov 20, 2020 •

edited

Loading