From b036c8f14bc6c15e5bda787a567024e102bdf05c Mon Sep 17 00:00:00 2001 From: Krista 'DarthMama' Bennett Date: Fri, 31 Jul 2020 15:04:07 +0200 Subject: [PATCH] ENGINE-781: hey, at least it doesn't break stuff. Existing tests pass. --- src/key_reset.c | 133 +++++++++++++++++++++++-------------------- src/keymanagement.c | 3 +- src/pEpEngine.c | 4 +- src/pEpEngine.h | 8 +-- src/pEp_internal.h | 2 +- test/src/Engine.cc | 2 +- test/src/SyncTest.cc | 2 +- 7 files changed, 83 insertions(+), 71 deletions(-) diff --git a/src/key_reset.c b/src/key_reset.c index 6cf61a7e..52d475ac 100644 --- a/src/key_reset.c +++ b/src/key_reset.c @@ -973,31 +973,50 @@ static PEP_STATUS _dup_grouped_only(identity_list* idents, identity_list** filte } static PEP_STATUS _check_own_reset_passphrase_readiness(PEP_SESSION session, - const char* key) { - // Before we do anything else, make sure the key to sign the - // revocation has the right passphrase set - PEP_STATUS status = probe_encrypt(session, key); - - // We only care if this has signing-related issues; key could - // already be revoked and it will be fine below. - if (PASS_ERROR(status)) - return status; - - // Because of the above, we CAN support a signing passphrase + const char* key) { + + // Check generation setup + // Because of the above, we can support a signing passphrase // that differs from the generation passphrase. We'll // just check to make sure everything is in order for // later use, however - if (session->new_key_pass_enable) { if (EMPTYSTR(session->generation_passphrase)) return PEP_PASSPHRASE_FOR_NEW_KEYS_REQUIRED; - - if (EMPTYSTR(session->curr_passphrase)) { - // We'll need it as the current passphrase to sign - // messages with the generated keys - session->curr_passphrase = strdup(session->generation_passphrase); - } - } + } + + stringlist_t* test_key = NULL; + + // Be sure we HAVE this key + PEP_STATUS status = find_keys(session, key, &test_key); + bool exists_key = test_key != NULL; + free_stringlist(test_key); + + if (!exists_key || status == PEP_KEY_NOT_FOUND) { + remove_fpr_as_default(session, key); + return PEP_KEY_NOT_FOUND; + } + if (status != PEP_STATUS_OK) + return status; + + ensure_passphrase_t ensure_key_cb = session->ensure_passphrase; + + // Check to see that this key has its passphrase set as the configured + // passphrase, IF it has one. If not, bail early. + status = probe_encrypt(session, key); + if (PASS_ERROR(status)) { + if (ensure_key_cb) + status = ensure_key_cb(session, key); + } + if (status != PEP_STATUS_OK) + return status; + + if (EMPTYSTR(session->curr_passphrase) && !EMPTYSTR(session->generation_passphrase)) { + // We'll need it as the current passphrase to sign + // messages with the generated keys + session->curr_passphrase = strdup(session->generation_passphrase); + } + return PEP_STATUS_OK; } @@ -1024,27 +1043,21 @@ static PEP_STATUS _key_reset_device_group_for_shared_key(PEP_SESSION session, if (!session || !key_idents || EMPTYSTR(old_key)) return PEP_ILLEGAL_VALUE; - - message* enc_msg = NULL; - message* outmsg = NULL; - + messageToSend_t send_cb = session->messageToSend; + if (!send_cb) return PEP_SYNC_NO_MESSAGE_SEND_CALLBACK; PEP_STATUS status = PEP_STATUS_OK; + + message* enc_msg = NULL; + message* outmsg = NULL; + stringlist_t* test_key = NULL; + - // N.B. The mechanism here is that we make the caller, if - // necessary, keep trying until they give us the - // right password for the key we're going to reset/revoke. - // The revocation does not happen until later, but basically, - // if the key is unrevoked and still requires a correct - // password, we need to stop this before we get there. - // - // This allows us to switch the correct passphrase in - // and out if there are different generation and old - // key signing passwords. (Not a concern if already revoked) - + // Make sure the signing password is set correctly and that + // we are also ready for keygen status = _check_own_reset_passphrase_readiness(session, old_key); if (status != PEP_STATUS_OK) return status; @@ -1127,12 +1140,12 @@ static PEP_STATUS _key_reset_device_group_for_shared_key(PEP_SESSION session, status = revoke_key(session, old_key, NULL); // again, we should not have key-related issues here, - // as we tested for the correct password earlier + // as we ensured the correct password earlier if (status != PEP_STATUS_OK) return status; // Ok, NOW - the current password needs to be swapped out - // because we're going to sign with keys using. + // because we're going to sign with keys using it. // // All new keys have the same passphrase, if any // @@ -1207,9 +1220,9 @@ static PEP_STATUS _key_reset_device_group_for_shared_key(PEP_SESSION session, return status; pEp_error: - // Just in case session->curr_passphrase = cached_passphrase; + free_stringlist(test_key); free_message(outmsg); free_message(enc_msg); return status; @@ -1228,8 +1241,6 @@ static PEP_STATUS probe_signing_for_keylist(PEP_SESSION session, return PEP_STATUS_OK; } -// This is simply NOT SAFE for multiple passwords on the extant -// keys. Cannot be called with multiple passwords for that purpose. DYNAMIC_API PEP_STATUS key_reset_own_grouped_keys(PEP_SESSION session) { assert(session); @@ -1247,12 +1258,7 @@ DYNAMIC_API PEP_STATUS key_reset_own_grouped_keys(PEP_SESSION session) { status = get_all_keys_for_user(session, user_id, &keys); // TODO: free - if (status == PEP_STATUS_OK) { - status = probe_signing_for_keylist(session, keys); - - if (PASS_ERROR(status)) - goto pEp_free; - + if (status == PEP_STATUS_OK) { stringlist_t* curr_key; for (curr_key = keys; curr_key && curr_key->value; curr_key = curr_key->next) { @@ -1269,13 +1275,16 @@ DYNAMIC_API PEP_STATUS key_reset_own_grouped_keys(PEP_SESSION session) { else goto pEp_free; - // Because of the key check above, this should not happen unless we were - // UNLESS we required a passphrase for keygen and it's not set. - if (PASS_ERROR(status)) - goto pEp_free; - - // FIXME: what about other statuses, though??? - + // This is in a switch because our return statuses COULD get more + // complicated + switch (status) { + case PEP_STATUS_OK: + case PEP_KEY_NOT_FOUND: // call removed it as a default + break; + default: + goto pEp_free; + } + free_identity_list(key_idents); } } @@ -1413,15 +1422,7 @@ PEP_STATUS key_reset( // case of own identities with private keys. if (is_own_private) { - - // Make sure we can even progress - if there are passphrase issues, - // bounce back to the caller now. - status = _check_own_reset_passphrase_readiness(session, fpr_copy); - - // These will always be passphrase errors. - if (status != PEP_STATUS_OK) - return status; - + // This is now the "is_own" base case - we don't do this // per-identity, because all identities using this key will // need new ones. That said, this is really only a problem @@ -1443,6 +1444,14 @@ PEP_STATUS key_reset( if (is_grouped) status = _key_reset_device_group_for_shared_key(session, key_idents, fpr_copy, false); else if (status == PEP_STATUS_OK) { + // KB: FIXME_NOW - revoked? + // Make sure we can even progress - if there are passphrase issues, + // bounce back to the caller now, because our attempts to make it work failed, + // even possibly with callback. + status = _check_own_reset_passphrase_readiness(session, fpr_copy); + if (status != PEP_STATUS_OK) + return status; + // now have ident list, or should identity_list* curr_ident; @@ -1453,7 +1462,7 @@ PEP_STATUS key_reset( // Do the full reset on this identity // Base case for is_own_private starts here // Note that we reset this key for ANY own ident that has it. And if - // tmp_ident did NOT have this key, it won't matter. We will reset the + // tmp_ident did NOT have this key, it won't matter. We will reset this // key for all idents for this user. status = revoke_key(session, fpr_copy, NULL); diff --git a/src/keymanagement.c b/src/keymanagement.c index a3066806..8c735d25 100644 --- a/src/keymanagement.c +++ b/src/keymanagement.c @@ -1358,7 +1358,8 @@ DYNAMIC_API PEP_STATUS do_keymanagement( { PEP_SESSION session; pEp_identity *identity; - PEP_STATUS status = init(&session, NULL, NULL); + // FIXME_NOW: ensure_decrypt callback??? + PEP_STATUS status = init(&session, NULL, NULL, NULL); assert(!status); if (status) return status; diff --git a/src/pEpEngine.c b/src/pEpEngine.c index 570165ad..37fd5122 100644 --- a/src/pEpEngine.c +++ b/src/pEpEngine.c @@ -947,7 +947,8 @@ static PEP_STATUS upgrade_revoc_contact_to_13(PEP_SESSION session) { DYNAMIC_API PEP_STATUS init( PEP_SESSION *session, messageToSend_t messageToSend, - inject_sync_event_t inject_sync_event + inject_sync_event_t inject_sync_event, + ensure_passphrase_t ensure_passphrase ) { PEP_STATUS status = PEP_STATUS_OK; @@ -997,6 +998,7 @@ DYNAMIC_API PEP_STATUS init( _session->version = PEP_ENGINE_VERSION; _session->messageToSend = messageToSend; _session->inject_sync_event = inject_sync_event; + _session->ensure_passphrase = ensure_passphrase; #ifdef DEBUG_ERRORSTACK _session->errorstack = new_stringlist("init()"); diff --git a/src/pEpEngine.h b/src/pEpEngine.h index 47745162..f4736789 100644 --- a/src/pEpEngine.h +++ b/src/pEpEngine.h @@ -205,7 +205,7 @@ DYNAMIC_API void free_Sync_event(SYNC_EVENT ev); typedef int (*inject_sync_event_t)(SYNC_EVENT ev, void *management); -// ensure_decrypt_key() - callee ensures correct password for (signing) key is configured in the session on +// ensure_passphrase() - callee ensures correct password for (signing) key is configured in the session on // return, or returns error when it is not found // parameters: //. session (in) session for which the guarantee is made @@ -221,7 +221,7 @@ typedef int (*inject_sync_event_t)(SYNC_EVENT ev, void *management); // The callee is responsible for iterating through passwords // to ensure signing/encryption can occur successfully. // -typedef PEP_STATUS (*ensure_decrypt_key_t)(PEP_SESSION session, const char* fpr); +typedef PEP_STATUS (*ensure_passphrase_t)(PEP_SESSION session, const char* fpr); // INIT_STATUS init() - initialize pEpEngine for a thread // @@ -231,7 +231,7 @@ typedef PEP_STATUS (*ensure_decrypt_key_t)(PEP_SESSION session, const char* fpr) // messageToSend (in) callback for sending message by the // application // inject_sync_event (in) callback for injecting a sync event -// ensure_decrypt_key (in) callback for ensuring correct password for key is set +// ensure_passphrase (in) callback for ensuring correct password for key is set // // return value: // PEP_STATUS_OK = 0 if init() succeeds @@ -266,7 +266,7 @@ DYNAMIC_API PEP_STATUS init( PEP_SESSION *session, messageToSend_t messageToSend, inject_sync_event_t inject_sync_event, - ensure_decrypt_key_t ensure_decrypt_key + ensure_passphrase_t ensure_passphrase ); diff --git a/src/pEp_internal.h b/src/pEp_internal.h index f6a0575d..e19e7809 100644 --- a/src/pEp_internal.h +++ b/src/pEp_internal.h @@ -250,7 +250,7 @@ struct _pEpSession { notifyHandshake_t notifyHandshake; inject_sync_event_t inject_sync_event; retrieve_next_sync_event_t retrieve_next_sync_event; - ensure_decrypt_key_t ensure_decrypt_key; + ensure_passphrase_t ensure_passphrase; // pEp Sync void *sync_management; diff --git a/test/src/Engine.cc b/test/src/Engine.cc index 3b577869..a73ffd97 100644 --- a/test/src/Engine.cc +++ b/test/src/Engine.cc @@ -90,7 +90,7 @@ void Engine::start() { unix_local_db(true); - PEP_STATUS status = init(&session, cached_messageToSend, cached_inject_sync_event); + PEP_STATUS status = init(&session, cached_messageToSend, cached_inject_sync_event, NULL); assert(status == PEP_STATUS_OK); assert(session); diff --git a/test/src/SyncTest.cc b/test/src/SyncTest.cc index df6efee4..357a3901 100644 --- a/test/src/SyncTest.cc +++ b/test/src/SyncTest.cc @@ -246,7 +246,7 @@ namespace { ST_fake_this = (void*)(&adapter); - status = init(&sync, ST_message_send_callback, ST_inject_sync_event_callback); + status = init(&sync, ST_message_send_callback, ST_inject_sync_event_callback, NULL); if (status != PEP_STATUS_OK) throw std::runtime_error((string("init returned ") + tl_status_string(status)).c_str());