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

@@ -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)
 

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);

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
                                     }
                                 }

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);
 

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;

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;

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)