From 052c158447c301e0df8670620ceddb0c6f811f19 Mon Sep 17 00:00:00 2001 From: Krista 'DarthMama' Bennett Date: Thu, 3 Sep 2020 09:16:22 +0200 Subject: [PATCH] ENGINE-633: fixed some bugs with key removal for pEp users if expired, but am stuck on some TOFU cases, so progress is stalled somewhat. --- pEpErr.py | 41 ++++-- src/keymanagement.c | 9 +- src/message_api.c | 29 ++-- .../CheckRenewedExpiredKeyTrustStatusTest.cc | 136 +++++++++++++----- test/src/LeastColorGroupTest.cc | 10 +- test/src/OwnKeysRetrieveTest.cc | 2 +- test/src/test_util.cc | 8 +- 7 files changed, 173 insertions(+), 62 deletions(-) diff --git a/pEpErr.py b/pEpErr.py index 59b891cd..a51ce97c 100755 --- a/pEpErr.py +++ b/pEpErr.py @@ -10,7 +10,7 @@ import sys import argparse -def parse_enum_line(line, ct): +def parse_enum_line(line, ct, r): line = line.strip() if (line.startswith("//") or line == ""): return @@ -27,12 +27,14 @@ def parse_enum_line(line, ct): else: parts = line.split() - if len(parts) != 3 or parts[1] != '=': + if len(parts) < 3 or parts[1] != '=': return - if not ct and not parts[0].startswith("PEP_"): + if r and not parts[0].startswith("PEP_rating"): return elif ct and not parts[0].startswith("PEP_ct_"): return + elif not ct and not r and not parts[0].startswith("PEP_"): + return key = int(parts[2].strip(','), 0) value = parts[0] @@ -50,12 +52,20 @@ def get_error(code): parser = argparse.ArgumentParser() parser.add_argument("value", type=int) parser.add_argument("--comm_type", "-ct", help="number represents a comm type", action='store_true') +parser.add_argument("--rating", "-r", help="number represents a rating", action='store_true') args = parser.parse_args() error_val = args.value -input_fname = "src/pEpEngine.h" +input_fname = "" + +if not args.rating: + input_fname = "src/pEpEngine.h" +else: + input_fname = "src/message_api.h" +print(input_fname) +pEp_error = not (args.rating or args.comm_type) file = open(input_fname, 'r') content = file.readlines() @@ -68,20 +78,25 @@ valueDict = dict() # If another struct is added first, expect chaos! ;) # for line in content: - if not args.comm_type: - if line.startswith("} PEP_STATUS;"): + if args.rating: + if line.startswith("} PEP_rating;"): break - else: + elif args.comm_type: if line.startswith("} PEP_comm_type;"): + break + elif line.startswith("} PEP_STATUS;"): break - - if ((not args.comm_type and not line.startswith("typedef enum {")) or (args.comm_type and not line.startswith("typedef enum _PEP_comm_type {"))) and not inStruct: - continue + if not inStruct: - inStruct = True - continue + if (args.rating and not line.startswith("typedef enum _PEP_rating {")) or \ + (args.comm_type and not line.startswith("typedef enum _PEP_comm_type {")) or \ + (pEp_error and not line.startswith("typedef enum {")): + continue + else: + inStruct = True + continue - parse_enum_line(line, args.comm_type) + parse_enum_line(line, args.comm_type, args.rating) get_error(error_val) diff --git a/src/keymanagement.c b/src/keymanagement.c index 0ba01150..3aa2b51b 100644 --- a/src/keymanagement.c +++ b/src/keymanagement.c @@ -259,12 +259,17 @@ PEP_STATUS validate_fpr(PEP_SESSION session, } switch (ct) { - case PEP_ct_key_expired: - case PEP_ct_key_expired_but_confirmed: case PEP_ct_key_revoked: case PEP_ct_key_b0rken: // delete key from being default key for all users/identities status = remove_fpr_as_default(session, fpr); + // fallthrough intentional! + case PEP_ct_key_expired: + case PEP_ct_key_expired_but_confirmed: + // Note: we no longer remove expired keys as defaults; pEp users + // will either send us an updated key or a key reset, and OpenPGP + // users can either do the same or request a manual key reset. + // We don't want to upset the automated updating of expired keys. status = update_trust_for_fpr(session, fpr, ct); diff --git a/src/message_api.c b/src/message_api.c index fe75028d..b158d24f 100644 --- a/src/message_api.c +++ b/src/message_api.c @@ -4391,10 +4391,22 @@ static PEP_STATUS _decrypt_message( } } // else msg_major/minor_ver must have been set. - if (msg_major_ver > 2 || (msg_major_ver == 2 && msg_minor_ver > 0)) { + // Ok, this is actually tricky. Normally, we want to look at the message version. But because + // We are here, allegedly, on a 2.0+ message, if it was issued by a 2.1+ partner, it will still have + // X-pEp-Sender-FPR on it and we should take that! So we need to do this carefully and distinguish between + // things that vary here based on the SENDER'S client information (with the caveat that the format they are + // producing is 2.0 or greater because we're in this logical branch) and the things that vary based upon + // what version of the client the sender thinks we have here. + + // So first, let's grab a sender fpr if we have one. That depends on the sender'd CLIENT version. + if (major_ver > 2 || (major_ver == 2 && minor_ver > 0)) { stringpair_list_t* searched = stringpair_list_find(inner_message->opt_fields, "X-pEp-Sender-FPR"); inner_message->_sender_fpr = ((searched && searched->value && searched->value->value) ? strdup(searched->value->value) : NULL); - searched = stringpair_list_find(inner_message->opt_fields, X_PEP_MSG_WRAP_KEY); + } + + // Ok, now get the message wrapping info + if (msg_major_ver > 2 || (msg_major_ver == 2 && msg_minor_ver > 0)) { + stringpair_list_t* searched = stringpair_list_find(inner_message->opt_fields, X_PEP_MSG_WRAP_KEY); if (searched && searched->value && searched->value->value) { is_inner = (strcmp(searched->value->value, "INNER") == 0); if (!is_inner) @@ -4403,7 +4415,7 @@ static PEP_STATUS _decrypt_message( inner_message->opt_fields = stringpair_list_delete_by_key(inner_message->opt_fields, X_PEP_MSG_WRAP_KEY); } } - else if (msg_major_ver == 2 && msg_minor_ver == 0) { + else if (wrap_info && msg_major_ver == 2 && msg_minor_ver == 0) { is_inner = (strcmp(wrap_info, "INNER") == 0); if (!is_inner) is_key_reset = (strcmp(wrap_info, "KEY_RESET") == 0); @@ -4466,13 +4478,12 @@ static PEP_STATUS _decrypt_message( if (!breaks_protocol && inner_message->from && !is_me(session, inner_message->from)) { const char* sender_key = NULL; - if ((msg_major_ver == 2 && msg_minor_ver > 0) || msg_major_ver > 2) + + // Messages from clients 2.1 or greater should ALWAYS have sender_fpr on the inside. So... + if (!EMPTYSTR(inner_message->_sender_fpr)) sender_key = inner_message->_sender_fpr; - else { // !breaks_protocol is true, so this is a 2.0 message - if ((major_ver == 2 && minor_ver > 1)|| major_ver > 2) { - sender_key = imported_sender_key_fpr; // can be empty - } - else if (_keylist) { // signer key, 2.0 sent from 2.0 client + else { // !breaks_protocol is true, so this is a 2.0 message FROM a 2.0 client + if (_keylist) { // signer key, 2.0 sent from 2.0 client sender_key = _keylist->value; // can be empty } } diff --git a/test/src/CheckRenewedExpiredKeyTrustStatusTest.cc b/test/src/CheckRenewedExpiredKeyTrustStatusTest.cc index bd6bea0f..1f4b24d8 100644 --- a/test/src/CheckRenewedExpiredKeyTrustStatusTest.cc +++ b/test/src/CheckRenewedExpiredKeyTrustStatusTest.cc @@ -83,7 +83,8 @@ namespace { } // namespace -TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_status) { +// This is what happens if we receive a key renewal (e.g. by mail) before ever updating the OpenPGP identity after expiration. +TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_status_OpenPGP_no_update) { bool ok = false; ok = slurp_and_import_key(session, "test_keys/pub/pep-test-alice-0x6FF00E97_pub.asc"); ASSERT_TRUE(ok); @@ -99,7 +100,11 @@ TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_st ASSERT_OK; const char* inq_fpr = "8E8D2381AE066ABE1FEE509821BA977CA4728718"; - pEp_identity* expired_inquisitor = new_identity("inquisitor@darthmama.org", NULL, NULL, "Lady Claire Trevelyan"); + // KEY ELECTION: It is clear that leaving an address we have a key for as a TOFU address is going to continue to have implications in the + // event that usernames differ. Here, we're losing the key that has been set for this identity. I am not sure we want this. + // pEp_identity* expired_inquisitor = new_identity("inquisitor@darthmama.org", NULL, "TOFU_inquisitor@darthmama.org", "Lady Claire Trevelyan"); + pEp_identity* expired_inquisitor = new_identity("inquisitor@darthmama.org", NULL, "INQUISITOR", "Lady Claire Trevelyan"); + status = set_fpr_preserve_ident(session, expired_inquisitor, inq_fpr, false); ASSERT_OK; @@ -108,17 +113,19 @@ TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_st const string msg = slurp("test_mails/ENGINE-463-attempt-numero-dos.eml"); char* decrypted_msg = NULL; - stringlist_t* keylist_used = nullptr; - char* modified_src = NULL; + stringlist_t* keylist = NULL; PEP_rating rating; PEP_decrypt_flags_t flags = 0; - status = MIME_decrypt_message(session, msg.c_str(), msg.size(), &decrypted_msg, &keylist_used, &rating, &flags, &modified_src); - ASSERT_EQ(status , PEP_DECRYPTED); + message* dec_msg = NULL; + message* enc_msg = NULL; + status = mime_decode_message(msg.c_str(), msg.size(), &enc_msg, NULL); + ASSERT_OK; + + status = decrypt_message(session, enc_msg, &dec_msg, &keylist, &rating, &flags); + ASSERT_EQ(status, PEP_DECRYPTED); // ??? - free(decrypted_msg); - decrypted_msg = NULL; ok = slurp_and_import_key(session, "test_keys/pub/inquisitor-0xA4728718_renewed_pub.asc"); ASSERT_TRUE(ok); @@ -130,16 +137,79 @@ TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_st msg2->longmsg = strdup("Blahblahblah!"); msg2->attachments = new_bloblist(NULL, 0, "application/octet-stream", NULL); + // We don't keep OpenPGP expired keys. We'll have to receive the new one via mail. status = outgoing_message_rating(session, msg2, &rating); ASSERT_OK; - ASSERT_EQ(rating , PEP_rating_reliable); + ASSERT_EQ(rating, PEP_rating_reliable); +} - status = get_trust(session, expired_inquisitor); - ASSERT_EQ(expired_inquisitor->comm_type , PEP_ct_OpenPGP_unconfirmed); - free_message(msg2); +// If we updated an OpenPGP identity in the meantime, we will have removed the key. Too bad. +TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_status_OpenPGP_with_update) { + bool ok = false; + ok = slurp_and_import_key(session, "test_keys/pub/pep-test-alice-0x6FF00E97_pub.asc"); + ASSERT_TRUE(ok); + ok = slurp_and_import_key(session, "test_keys/priv/pep-test-alice-0x6FF00E97_priv.asc"); + ASSERT_TRUE(ok); + ok = slurp_and_import_key(session, "test_keys/pub/inquisitor-0xA4728718_full_expired.pub.asc"); + ASSERT_TRUE(ok); + + const char* alice_fpr = "4ABE3AAF59AC32CFE4F86500A9411D176FF00E97"; + pEp_identity* alice_from = new_identity("pep.test.alice@pep-project.org", alice_fpr, PEP_OWN_USERID, "Alice Cooper"); + + PEP_STATUS status = set_own_key(session, alice_from, alice_fpr); + ASSERT_OK; + + const char* inq_fpr = "8E8D2381AE066ABE1FEE509821BA977CA4728718"; + // KEY ELECTION: It is clear that leaving an address we have a key for as a TOFU address is going to continue to have implications in the + // event that usernames differ. Here, we're losing the key that has been set for this identity. I am not sure we want this. + // pEp_identity* expired_inquisitor = new_identity("inquisitor@darthmama.org", NULL, "TOFU_inquisitor@darthmama.org", "Lady Claire Trevelyan"); + pEp_identity* expired_inquisitor = new_identity("inquisitor@darthmama.org", NULL, "INQUISITOR", "Lady Claire Trevelyan"); + + status = set_fpr_preserve_ident(session, expired_inquisitor, inq_fpr, false); + ASSERT_OK; + + // Ok, so I want to make sure we make an entry, so I'll try to decrypt the message WITH + // the expired key: + const string msg = slurp("test_mails/ENGINE-463-attempt-numero-dos.eml"); + + status = update_identity(session, expired_inquisitor); + ASSERT_EQ(status, PEP_KEY_UNSUITABLE); + ASSERT_NULL(expired_inquisitor->fpr); + + char* decrypted_msg = NULL; + stringlist_t* keylist = NULL; + + PEP_rating rating; + PEP_decrypt_flags_t flags = 0; + + message* dec_msg = NULL; + message* enc_msg = NULL; + status = mime_decode_message(msg.c_str(), msg.size(), &enc_msg, NULL); + ASSERT_OK; + + status = decrypt_message(session, enc_msg, &dec_msg, &keylist, &rating, &flags); + ASSERT_EQ(status, PEP_DECRYPTED); // ??? + + ok = slurp_and_import_key(session, "test_keys/pub/inquisitor-0xA4728718_renewed_pub.asc"); + ASSERT_TRUE(ok); + + message* msg2 = new_message(PEP_dir_outgoing); + + msg2->from = alice_from; + msg2->to = new_identity_list(expired_inquisitor); + msg2->shortmsg = strdup("Blah!"); + msg2->longmsg = strdup("Blahblahblah!"); + msg2->attachments = new_bloblist(NULL, 0, "application/octet-stream", NULL); + + // We don't keep OpenPGP expired keys. We'll have to receive the new one via mail. + status = outgoing_message_rating(session, msg2, &rating); + ASSERT_OK; + ASSERT_EQ(rating, PEP_rating_unencrypted); } -TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_status_trusted_user) { +// If we updated an OpenPGP identity in the meantime, we will have removed the key. Too bad. +// Trust will have to be redone. +TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_status_trusted_user_OpenPGP) { bool ok = false; ok = slurp_and_import_key(session, "test_keys/pub/pep-test-alice-0x6FF00E97_pub.asc"); ASSERT_TRUE(ok); @@ -155,13 +225,11 @@ TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_st ASSERT_OK; const char* inquisitor_fpr = "8E8D2381AE066ABE1FEE509821BA977CA4728718"; - pEp_identity* expired_inquisitor = new_identity("inquisitor@darthmama.org", "8E8D2381AE066ABE1FEE509821BA977CA4728718", "TOFU_inquisitor@darthmama.org", "Lady Claire Trevelyan"); + pEp_identity* expired_inquisitor = new_identity("inquisitor@darthmama.org", "8E8D2381AE066ABE1FEE509821BA977CA4728718", "INQUISITOR", "Lady Claire Trevelyan"); status = set_fpr_preserve_ident(session, expired_inquisitor, inquisitor_fpr, false); ASSERT_OK; - - PEP_comm_type key_ct; status = get_key_rating(session, inquisitor_fpr, &key_ct); ASSERT_EQ(status, PEP_STATUS_OK); @@ -193,26 +261,29 @@ TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_st const string msg = slurp("test_mails/ENGINE-463-attempt-numero-dos.eml"); char* decrypted_msg = NULL; - stringlist_t* keylist_used = nullptr; - char* modified_src = NULL; + stringlist_t* keylist = NULL; +// char* modified_src = NULL; PEP_rating rating; PEP_decrypt_flags_t flags = 0; - status = MIME_decrypt_message(session, msg.c_str(), msg.size(), &decrypted_msg, &keylist_used, &rating, &flags, &modified_src); - ASSERT_EQ(status , PEP_DECRYPTED); + message* dec_msg = NULL; + message* enc_msg = NULL; + status = mime_decode_message(msg.c_str(), msg.size(), &enc_msg, NULL); + ASSERT_OK; + + status = decrypt_message(session, enc_msg, &dec_msg, &keylist, &rating, &flags); + ASSERT_EQ(status, PEP_DECRYPTED); // ??? - free(decrypted_msg); - decrypted_msg = NULL; ok = slurp_and_import_key(session, "test_keys/pub/inquisitor-0xA4728718_renewed_pub.asc"); ASSERT_TRUE(ok); - pEp_identity* expired_inquisitor1 = new_identity("inquisitor@darthmama.org", NULL, NULL, "Lady Claire Trevelyan"); + pEp_identity* expired_inquisitor1 = new_identity("inquisitor@darthmama.org", NULL, "INQUISITOR", "Lady Claire Trevelyan"); status = update_identity(session, expired_inquisitor1); ASSERT_OK; status = get_trust(session, expired_inquisitor1); - ASSERT_EQ(expired_inquisitor1->comm_type , PEP_ct_OpenPGP); + ASSERT_EQ(expired_inquisitor1->comm_type, PEP_ct_key_not_found); message* msg2 = new_message(PEP_dir_outgoing); @@ -224,7 +295,7 @@ TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_st status = outgoing_message_rating(session, msg2, &rating); ASSERT_OK; - ASSERT_GE(rating, PEP_rating_trusted); + ASSERT_EQ(rating, PEP_rating_unencrypted); free_message(msg2); } @@ -262,17 +333,18 @@ TEST_F(CheckRenewedExpiredKeyTrustStatusTest, check_renewed_expired_key_trust_st const string msg = slurp("test_mails/ENGINE-463-attempt-numero-dos.eml"); char* decrypted_msg = NULL; - stringlist_t* keylist_used = nullptr; - char* modified_src = NULL; - + stringlist_t* keylist = nullptr; PEP_rating rating; PEP_decrypt_flags_t flags = 0; - status = MIME_decrypt_message(session, msg.c_str(), msg.size(), &decrypted_msg, &keylist_used, &rating, &flags, &modified_src); - ASSERT_EQ(status , PEP_DECRYPTED); + message* dec_msg = NULL; + message* enc_msg = NULL; + status = mime_decode_message(msg.c_str(), msg.size(), &enc_msg, NULL); + ASSERT_OK; + + status = decrypt_message(session, enc_msg, &dec_msg, &keylist, &rating, &flags); + ASSERT_EQ(status, PEP_DECRYPTED); - free(decrypted_msg); - decrypted_msg = NULL; ok = slurp_and_import_key(session, "test_keys/pub/inquisitor-0xA4728718_renewed_pub.asc"); ASSERT_TRUE(ok); diff --git a/test/src/LeastColorGroupTest.cc b/test/src/LeastColorGroupTest.cc index a5a7fe46..3917ad16 100644 --- a/test/src/LeastColorGroupTest.cc +++ b/test/src/LeastColorGroupTest.cc @@ -120,10 +120,16 @@ TEST_F(LeastColorGroupTest, check_least_color_group) { pEp_identity * sender1 = new_identity("pep.color.test.V@kgrothoff.org", NULL, "TOFU_pep.color.test.V@kgrothoff.org", "Pep Color Test V (sender)"); + + status = set_fpr_preserve_ident(session, sender1, "AFC019B22E2CC61F13F285BF179B9DF271FC6D28", false); + ASSERT_OK; status = update_identity(session, sender1); - trust_personal_key(session, sender1); + ASSERT_OK; + status = trust_personal_key(session, sender1); + ASSERT_OK; status = update_identity(session, sender1); - + ASSERT_OK; + message* msg_ptr = nullptr; message* dest_msg = nullptr; message* final_ptr = nullptr; diff --git a/test/src/OwnKeysRetrieveTest.cc b/test/src/OwnKeysRetrieveTest.cc index 2ce96cb5..f2ae8f69 100644 --- a/test/src/OwnKeysRetrieveTest.cc +++ b/test/src/OwnKeysRetrieveTest.cc @@ -110,7 +110,7 @@ TEST_F(OwnKeysRetrieveTest, check_own_keys_retrieve_single_private_single_pub) { // Set up an own idea that only has a public key PEP_STATUS status = set_up_ident_from_scratch(session, "test_keys/pub/pep-test-bob-0xC9C2EE39_pub.asc", - "pep.test.bob@pep-project.org", NULL, PEP_OWN_USERID, "Bob's Burgers", + "pep.test.bob@pep-project.org", "BFCDB7F301DEEEBBF947F29659BFF488C9C2EE39", PEP_OWN_USERID, "Bob's Burgers", NULL, false ); ASSERT_OK; diff --git a/test/src/test_util.cc b/test/src/test_util.cc index 0687e9e7..bd8c3628 100644 --- a/test/src/test_util.cc +++ b/test/src/test_util.cc @@ -100,9 +100,11 @@ PEP_STATUS set_up_ident_from_scratch(PEP_SESSION session, status = myself(session, ident); } else { - status = set_fpr_preserve_ident(session, ident, fpr, false); - if (status != PEP_STATUS_OK) - goto pep_free; + if (!EMPTYSTR(fpr)) { + status = set_fpr_preserve_ident(session, ident, fpr, false); + if (status != PEP_STATUS_OK) + goto pep_free; + } status = update_identity(session, ident); } if (status != PEP_STATUS_OK)