From cbdf837e48fdc3c65130654865ac0d10624b670d Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Thu, 6 Aug 2026 14:49:37 +1000 Subject: [PATCH 1/9] tests: prove config re-store is byte-identical and seqno-neutral Config recovery re-stores an unchanged config to refresh its TTL rather than pushing a new revision, which requires that re-encrypting reproduces the received message byte for byte (the storage server hashes the ciphertext) and that a clean push is not a new revision. Both were established by reading only. Covers the full round trip -- push, receive, dump, reload, push -- rather than encrypting the same bytes twice, since recovery happens after a restart and it is the reload path that has to be reproducible: user configs (protobuf-wrapped), group configs (raw), multipart configs, and a read-only member reproducing an admin's bytes from a dump that retained the signature it cannot re-derive. Two behaviours worth knowing that the tests pin: a clean push consumes the obsolete-hash list, so a re-store must plumb it through to the delete call or leak those messages; and the hand-back is gated on !is_readonly() while the clear is not, so on the member path that list is always empty. The protobuf wrapper is additionally pinned by a golden digest: every other assertion compares bytes made within one process, so a wall-clock or random field added to the wrapper would break re-store idempotency while leaving them all green. --- tests/CMakeLists.txt | 1 + tests/test_config_determinism.cpp | 511 ++++++++++++++++++++++++++++++ 2 files changed, 512 insertions(+) create mode 100644 tests/test_config_determinism.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2ed0d1af5..8995fca36 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -15,6 +15,7 @@ set(LIB_SESSION_UTESTS_SOURCES test_configdata.cpp test_config_contacts.cpp test_config_convo_info_volatile.cpp + test_config_determinism.cpp test_config_local.cpp test_config_pro.cpp test_curve25519.cpp diff --git a/tests/test_config_determinism.cpp b/tests/test_config_determinism.cpp new file mode 100644 index 000000000..e77ddbbaf --- /dev/null +++ b/tests/test_config_determinism.cpp @@ -0,0 +1,511 @@ +// Round-trip determinism of config storage. +// +// Config recovery (re-storing an unchanged config to refresh its TTL, rather than pushing a new +// revision) depends on two properties that were previously only established by reading the code: +// +// 1. Loading a config from its stored dump and re-pushing it reproduces the message that was +// originally received from the swarm *byte for byte*. The storage server hashes the +// ciphertext, so byte-equality is what makes a re-store an idempotent TTL refresh instead of a +// second message. +// 2. `push()` on a clean config does not bump the seqno, so a re-store is not a new revision and +// provokes no merge. +// +// The tests below exercise the full round trip (push -> receive -> dump -> reload -> push), not +// just "encrypt the same bytes twice": recovery happens after a restart, so the reload path is the +// part that has to be reproducible — including a signature the reloading device cannot re-derive. + +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "utils.hpp" + +using namespace session; +using namespace session::config; + +namespace { + +// The user's ed25519 seed; also the config encryption key for user configs. +const auto user_seed = "0123456789abcdef0123456789abcdef00000000000000000000000000000000"_hexbytes; + +// Group identity keypair (the admin holds the secret key; members hold only the pubkey). +const auto group_seed = "0123456789abcdef0123456789abcdeffedcba9876543210fedcba9876543210"_hexbytes; + +// The group's symmetric config encryption key (normally handed out by groups::Keys). +const auto group_enc_key = + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"_hexbytes; + +struct GroupKeys { + std::array pk; + std::array sk; +}; + +GroupKeys group_keys() { + GroupKeys k{}; + crypto_sign_ed25519_seed_keypair(k.pk.data(), k.sk.data(), group_seed.data()); + return k; +} + +// A single incoming message as `merge()` wants it. +std::vector>> incoming( + std::string hash, std::vector data) { + std::vector>> configs; + configs.emplace_back(std::move(hash), std::move(data)); + return configs; +} + +// Strip the null prefix padding that `pad_message` adds, so the result is the bencoded config. +std::vector unpad(std::vector plain) { + auto it = std::find_if(plain.begin(), plain.end(), [](unsigned char c) { return c != 0; }); + plain.erase(plain.begin(), it); + return plain; +} + +} // namespace + +TEST_CASE("config re-store is byte-identical: user config", "[config][determinism][user]") { + + // === The device that creates the config pushes it to the swarm === + + UserProfile a{to_span(user_seed), std::nullopt}; + a.set_name("Determinism"); + a.set_profile_pic( + "http://example.com/12345", + "abcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcd"_hexbytes); + + auto [seqno, pushes, obs] = a.push(); + REQUIRE(pushes.size() == 1); + CHECK(seqno == 1); + // `received` stands in for the message as it lands on (and is fetched back from) the swarm. + const auto received = pushes[0]; + a.confirm_pushed(seqno, {"fakehash1"}); + + // User configs really are protobuf-wrapped: the stored bytes are not the config ciphertext, + // they unwrap *to* it. This is the layer most likely to smuggle in a timestamp. + CHECK_THROWS(decrypt(received, user_seed, "UserProfile")); + std::vector unwrapped; + REQUIRE_NOTHROW(unwrapped = protos::unwrap_config(user_seed, received, Namespace::UserProfile)); + CHECK_NOTHROW(decrypt(unwrapped, user_seed, "UserProfile")); + + SECTION("the pushing device reproduces it after a dump/reload") { + auto dump = a.dump(); + UserProfile reloaded{to_span(user_seed), to_span(dump)}; + + CHECK_FALSE(reloaded.needs_push()); + auto [s, p, o] = reloaded.push(); + CHECK(s == seqno); + REQUIRE(p.size() == 1); + CHECK(to_hex(p[0]) == to_hex(received)); + CHECK(o.empty()); + } + + SECTION("a receiving device reproduces it after a dump/reload") { + UserProfile b{to_span(user_seed), std::nullopt}; + CHECK(b.merge(incoming("fakehash1", received)) == std::unordered_set{{"fakehash1"s}}); + CHECK_FALSE(b.needs_push()); + CHECK(b.get_name() == "Determinism"); + + auto dump = b.dump(); + UserProfile reloaded{to_span(user_seed), to_span(dump)}; + + CHECK_FALSE(reloaded.needs_push()); + auto [s, p, o] = reloaded.push(); + CHECK(s == seqno); + REQUIRE(p.size() == 1); + CHECK(to_hex(p[0]) == to_hex(received)); + CHECK(o.empty()); + } +} + +TEST_CASE("config re-store is byte-identical: group config", "[config][determinism][groups]") { + + auto gk = group_keys(); + + groups::Info admin{gk.pk, to_span(gk.sk), std::nullopt}; + admin.add_key(group_enc_key, false); + admin.set_name("Determinism Group"); + admin.set_expiry_timer(1h); + admin.set_created(1682529839); + + auto [seqno, pushes, obs] = admin.push(); + REQUIRE(pushes.size() == 1); + CHECK(seqno == 1); + const auto received = pushes[0]; + admin.confirm_pushed(seqno, {"fakehash1"}); + + // Group configs go up raw — no protobuf wrapper — so the stored bytes decrypt straight to a + // bencoded config dict with the group's config key. + std::vector plain; + REQUIRE_NOTHROW(plain = unpad(decrypt(received, group_enc_key, "groups::Info"))); + REQUIRE_FALSE(plain.empty()); + CHECK(plain.front() == 'd'); + + SECTION("the pushing admin reproduces it after a dump/reload") { + auto dump = admin.dump(); + groups::Info reloaded{gk.pk, to_span(gk.sk), to_span(dump)}; + reloaded.add_key(group_enc_key, false); + + CHECK_FALSE(reloaded.needs_push()); + auto [s, p, o] = reloaded.push(); + CHECK(s == seqno); + REQUIRE(p.size() == 1); + CHECK(to_hex(p[0]) == to_hex(received)); + CHECK(o.empty()); + } + + SECTION("a receiving admin reproduces it after a dump/reload") { + groups::Info admin2{gk.pk, to_span(gk.sk), std::nullopt}; + admin2.add_key(group_enc_key, false); + CHECK(admin2.merge(incoming("fakehash1", received)) == std::unordered_set{{"fakehash1"s}}); + CHECK_FALSE(admin2.needs_push()); + + auto dump = admin2.dump(); + groups::Info reloaded{gk.pk, to_span(gk.sk), to_span(dump)}; + reloaded.add_key(group_enc_key, false); + + auto [s, p, o] = reloaded.push(); + CHECK(s == seqno); + REQUIRE(p.size() == 1); + CHECK(to_hex(p[0]) == to_hex(received)); + CHECK(o.empty()); + } +} + +TEST_CASE( + "config re-store is byte-identical: multipart config", "[config][determinism][multipart]") { + + // A config too big for one message is split into parts (and, note, skips protobuf wrapping + // entirely — see the `else` branch in ConfigBase::push()). Recovery has to re-store every + // part, so each one has to come back byte-identical after a dump/reload, including the + // reassembly state the dump carries in its "*" key. + Contacts contacts{to_span(user_seed), std::nullopt}; + + for (size_t i = 0; i < 3000; i++) { + // Random (i.e. poorly compressible) session ids, so the config gets big enough to split. + std::mt19937_64 rng{i}; + std::array random_sessionid; + random_sessionid[0] = 0x05; + for (int j = 1; j < 33; j += 8) + oxenc::write_host_as_little(rng(), random_sessionid.data() + j); + + auto c = contacts.get_or_construct(oxenc::to_hex(random_sessionid)); + c.nickname = "My friend {}"_format(i); + c.approved = true; + contacts.set(c); + } + + auto [seqno, pushes, obs] = contacts.push(); + REQUIRE(pushes.size() > 1); + const auto received = pushes; + + std::unordered_set hashes; + std::vector>> configs; + for (size_t i = 0; i < received.size(); i++) { + auto hash = "fakehash_part{}"_format(i); + hashes.insert(hash); + configs.emplace_back(std::move(hash), received[i]); + } + contacts.confirm_pushed(seqno, hashes); + + SECTION("the pushing device reproduces every part after a dump/reload") { + auto dump = contacts.dump(); + Contacts reloaded{to_span(user_seed), to_span(dump)}; + + CHECK_FALSE(reloaded.needs_push()); + auto [s, p, o] = reloaded.push(); + CHECK(s == seqno); + REQUIRE(p.size() == received.size()); + for (size_t i = 0; i < p.size(); i++) + CHECK(to_hex(p[i]) == to_hex(received[i])); + CHECK(o.empty()); + } + + SECTION("a receiving device reproduces every part after a dump/reload") { + Contacts b{to_span(user_seed), std::nullopt}; + CHECK(b.merge(configs) == hashes); + CHECK_FALSE(b.needs_push()); + + auto dump = b.dump(); + Contacts reloaded{to_span(user_seed), to_span(dump)}; + + CHECK_FALSE(reloaded.needs_push()); + auto [s, p, o] = reloaded.push(); + CHECK(s == seqno); + REQUIRE(p.size() == received.size()); + for (size_t i = 0; i < p.size(); i++) + CHECK(to_hex(p[i]) == to_hex(received[i])); + CHECK(o.empty()); + } +} + +TEST_CASE( + "config re-store is byte-identical: dump with a retained signature", + "[config][determinism][groups][signature]") { + + // The load-bearing case for group recovery: a non-admin member has no signing key, so it can + // only reproduce the admin's bytes if the signature survives dump -> reload (make_dump() + // serialises with signing disabled, but writes the stored signature; the reload trusts it). + auto gk = group_keys(); + + groups::Info admin{gk.pk, to_span(gk.sk), std::nullopt}; + admin.add_key(group_enc_key, false); + admin.set_name("Signed Group"); + admin.set_created(1682529839); + auto [seqno, pushes, obs] = admin.push(); + REQUIRE(pushes.size() == 1); + const auto received = pushes[0]; + admin.confirm_pushed(seqno, {"fakehash1"}); + + // The message is signed: the bencoded config ends with a "~" key holding the 64-byte + // signature, i.e. `1:~64:<64 bytes>` followed by the dict's closing `e`. + auto plain = unpad(decrypt(received, group_enc_key, "groups::Info")); + REQUIRE(plain.size() > 71); + CHECK(printable(std::span{plain}.subspan(plain.size() - 71, 6)) == "1:~64:"); + CHECK(plain.back() == 'e'); + + groups::Info member{gk.pk, std::nullopt, std::nullopt}; + member.add_key(group_enc_key, false); + CHECK(member.merge(incoming("fakehash1", received)) == std::unordered_set{{"fakehash1"s}}); + REQUIRE(member.is_readonly()); + CHECK(member.get_name() == "Signed Group"); + + auto dump = member.dump(); + groups::Info reloaded{gk.pk, std::nullopt, to_span(dump)}; + reloaded.add_key(group_enc_key, false); + REQUIRE(reloaded.is_readonly()); + + CHECK_FALSE(reloaded.needs_push()); + auto [s, p, o] = reloaded.push(); + CHECK(s == seqno); + REQUIRE(p.size() == 1); + CHECK(to_hex(p[0]) == to_hex(received)); + CHECK(o.empty()); + + // ...and the re-store is still a validly signed config, not merely the right length: another + // member verifies the signature against the group pubkey when it merges, and would throw + // (dropping the message) if it no longer matched. + groups::Info member2{gk.pk, std::nullopt, std::nullopt}; + member2.add_key(group_enc_key, false); + CHECK(member2.merge(incoming("fakehash1", p[0])) == std::unordered_set{{"fakehash1"s}}); + CHECK(member2.get_name() == "Signed Group"); +} + +TEST_CASE("push on a clean config does not bump the seqno", "[config][determinism][seqno]") { + + // Recovery must never dirty the config: dirtying bumps the seqno and turns an idempotent + // re-store into a new revision plus a merge. + + SECTION("user config") { + UserProfile a{to_span(user_seed), std::nullopt}; + a.set_name("Determinism"); + auto [seqno, pushes, obs] = a.push(); + a.confirm_pushed(seqno, {"fakehash1"}); + REQUIRE(a.is_clean()); + REQUIRE_FALSE(a.needs_push()); + + for (int i = 0; i < 3; i++) { + auto [s, p, o] = a.push(); + CHECK(s == seqno); + REQUIRE(p.size() == 1); + CHECK(to_hex(p[0]) == to_hex(pushes[0])); + CHECK(o.empty()); + CHECK(a.is_clean()); + CHECK_FALSE(a.is_dirty()); + CHECK_FALSE(a.needs_push()); + } + } + + SECTION("group config, admin") { + auto gk = group_keys(); + groups::Info admin{gk.pk, to_span(gk.sk), std::nullopt}; + admin.add_key(group_enc_key, false); + admin.set_name("Determinism Group"); + auto [seqno, pushes, obs] = admin.push(); + admin.confirm_pushed(seqno, {"fakehash1"}); + REQUIRE(admin.is_clean()); + REQUIRE_FALSE(admin.needs_push()); + + for (int i = 0; i < 3; i++) { + auto [s, p, o] = admin.push(); + CHECK(s == seqno); + REQUIRE(p.size() == 1); + CHECK(to_hex(p[0]) == to_hex(pushes[0])); + CHECK(o.empty()); + CHECK(admin.is_clean()); + CHECK_FALSE(admin.is_dirty()); + CHECK_FALSE(admin.needs_push()); + } + } + + SECTION("group config, read-only member") { + auto gk = group_keys(); + groups::Info admin{gk.pk, to_span(gk.sk), std::nullopt}; + admin.add_key(group_enc_key, false); + admin.set_name("Determinism Group"); + auto [seqno, pushes, obs] = admin.push(); + + groups::Info member{gk.pk, std::nullopt, std::nullopt}; + member.add_key(group_enc_key, false); + REQUIRE(member.merge(incoming("fakehash1", pushes[0])) == + std::unordered_set{{"fakehash1"s}}); + REQUIRE(member.is_readonly()); + REQUIRE_FALSE(member.needs_push()); + + for (int i = 0; i < 3; i++) { + auto [s, p, o] = member.push(); + CHECK(s == seqno); + REQUIRE(p.size() == 1); + CHECK(to_hex(p[0]) == to_hex(pushes[0])); + CHECK(o.empty()); + CHECK_FALSE(member.is_dirty()); + CHECK_FALSE(member.needs_push()); + } + } +} + +TEST_CASE( + "push on a clean config still consumes the obsolete-hash list", + "[config][determinism][seqno][obsolete]") { + + // A re-store leaves the seqno and the bytes alone, but it is NOT side-effect free: push() + // clears `_old_hashes` unconditionally (src/config/base.cpp:810-813), outside the + // `if (is_dirty())` guard above it. So a recovery push hands back the superseded hashes and + // forgets them. A caller that treats a recovery push as a no-op and discards its + // obsolete-hash return will leak those messages on the swarm: nothing reports them again, and + // the next dump no longer records them. + UserProfile a{to_span(user_seed), std::nullopt}; + a.set_name("First"); + auto [s1, p1, o1] = a.push(); + a.confirm_pushed(s1, {"fakehash1"}); + a.set_name("Second"); + auto [s2, p2, o2] = a.push(); + a.confirm_pushed(s2, {"fakehash2"}); + + // A device that fetches both messages at once: the seqno-2 message carries the seqno-1 diff, + // so it supersedes it and fakehash1 becomes obsolete. + UserProfile b{to_span(user_seed), std::nullopt}; + std::vector>> both; + both.emplace_back("fakehash1", p1[0]); + both.emplace_back("fakehash2", p2[0]); + CHECK(b.merge(both) == std::unordered_set{{"fakehash1"s, "fakehash2"s}}); + REQUIRE(b.is_clean()); + REQUIRE_FALSE(b.needs_push()); + + // The re-store itself is byte-identical and does not move the seqno, as everywhere above... + auto [s3, p3, o3] = b.push(); + CHECK(s3 == s2); + REQUIRE(p3.size() == 1); + CHECK(to_hex(p3[0]) == to_hex(p2[0])); + + // ...but it also hands back the obsolete hash and clears it. + CHECK(o3 == std::vector{"fakehash1"s}); + + auto [s4, p4, o4] = b.push(); + CHECK(s4 == s2); + REQUIRE(p4.size() == 1); + CHECK(to_hex(p4[0]) == to_hex(p2[0])); + CHECK(o4.empty()); // consumed by the previous push, not re-reported +} + +TEST_CASE( + "a read-only member re-stores but is never handed the obsolete hashes", + "[config][determinism][groups][obsolete]") { + + // The hand-back and the clear are gated differently (src/config/base.cpp:809-813): + // + // if (!is_readonly()) + // for (auto& old : _old_hashes) + // obs.push_back(std::move(old)); // <-- read-only: skipped + // _old_hashes.clear(); // <-- read-only: still happens + // + // So a read-only member — the actor that *can* re-store (see the retained-signature case) — + // never receives the superseded hashes at all. That is coherent rather than broken: a + // member's subaccount carries Write but not Delete, so it could not prune them anyway. But + // it means an EMPTY obsolete list is the *expected* result on the member path, not evidence + // that recovery failed, and superseded messages persist until an admin next pushes. + auto gk = group_keys(); + + groups::Info admin{gk.pk, to_span(gk.sk), std::nullopt}; + admin.add_key(group_enc_key, false); + admin.set_name("First"); + auto [s1, p1, o1] = admin.push(); + admin.confirm_pushed(s1, {"fakehash1"}); + admin.set_name("Second"); + auto [s2, p2, o2] = admin.push(); + admin.confirm_pushed(s2, {"fakehash2"}); + + std::vector>> both; + both.emplace_back("fakehash1", p1[0]); + both.emplace_back("fakehash2", p2[0]); + + SECTION("an admin that merged both is handed fakehash1") { + groups::Info admin2{gk.pk, to_span(gk.sk), std::nullopt}; + admin2.add_key(group_enc_key, false); + REQUIRE(admin2.merge(both) == std::unordered_set{{"fakehash1"s, "fakehash2"s}}); + REQUIRE_FALSE(admin2.is_readonly()); + + auto [s, p, o] = admin2.push(); + CHECK(s == s2); + REQUIRE(p.size() == 1); + CHECK(to_hex(p[0]) == to_hex(p2[0])); + CHECK(o == std::vector{"fakehash1"s}); + } + + SECTION("a read-only member that merged both is handed nothing") { + groups::Info member{gk.pk, std::nullopt, std::nullopt}; + member.add_key(group_enc_key, false); + REQUIRE(member.merge(both) == std::unordered_set{{"fakehash1"s, "fakehash2"s}}); + REQUIRE(member.is_readonly()); + + // The re-store still works — byte-identical, same seqno... + auto [s, p, o] = member.push(); + CHECK(s == s2); + REQUIRE(p.size() == 1); + CHECK(to_hex(p[0]) == to_hex(p2[0])); + + // ...but the obsolete hash is dropped rather than returned. + CHECK(o.empty()); + } +} + +TEST_CASE( + "the protobuf config wrapper contains no wall-clock or random data", + "[config][determinism][proto]") { + + // The wrapper is deterministic only because every non-deterministic element was deliberately + // pinned: Envelope.timestamp is hardcoded to 1, padding is a bare 0x80 with no random fill, + // and the inner encryption is encrypt_for_recipient_deterministic (src/config/protos.cpp). + // + // Adding a field carrying a wall-clock value or fresh randomness would silently break config + // re-store idempotency, and every test above would keep passing, because they only compare + // bytes produced within a single run. This golden digest is the tripwire for that: if it + // fails, check what changed in wrap_config before updating the constant. + const auto payload = "Hello from the other side"_bytes; + + const std::pair expected[] = { + {Namespace::UserProfile, + "e5bcbe0595e82d55e80079dc9cf8c8b6952c099ee8306c413833e0eecd45ce03"}, + {Namespace::Contacts, + "1573a2e233008a47f445a8f1b328fe37bcd49450c94b743202b68902a1a94a89"}, + {Namespace::ConvoInfoVolatile, + "121dabefb251fa586c14f7e9c82590361c2db4efcb9817771bb4d9b0aaba9362"}, + {Namespace::UserGroups, + "b78bd9b62719db76ff7a9ec59eef3de0b4aaee4e4d55b515f70b04d674f8b8a9"}, + }; + + for (const auto& [ns, digest] : expected) { + auto wrapped = protos::wrap_config(user_seed, payload, 1, ns); + CHECK(to_hex(session::hash::hash(32, wrapped)) == digest); + } +} From a18b0f08f3b1e6be97951f9125cb5e8dcbbf7c46 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Thu, 6 Aug 2026 14:52:45 +1000 Subject: [PATCH 2/9] groups::Keys: flag a dump when a known key arrives under a new hash insert_key's early-return path -- we already hold this key, but this is a different message carrying it -- recorded the new hash in active_msgs_ without setting needs_dump_, unlike both of the other recording paths. The hash was therefore lost on the next restart, and that message stopped having its TTL renewed, so it could expire from the swarm while still being a live copy of a current key. Reachable whenever an admin issues a supplemental carrying a key the recipient already holds. --- src/config/groups/keys.cpp | 6 ++++- tests/test_group_keys.cpp | 51 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/config/groups/keys.cpp b/src/config/groups/keys.cpp index cc42ab8d3..29685a4d9 100644 --- a/src/config/groups/keys.cpp +++ b/src/config/groups/keys.cpp @@ -843,7 +843,11 @@ void Keys::insert_key(std::string_view msg_hash, key_info&& new_key) { }); for (auto it = gen_begin; it != gen_end; ++it) if (it->key == new_key.key) { - active_msgs_[new_key.generation].emplace(msg_hash); + // We already have this key, but this may be a *different* message carrying it, in + // which case we want to keep renewing this copy too. Flag a dump only when the hash + // is actually new, so re-loading a message we already know stays free. + if (active_msgs_[new_key.generation].emplace(msg_hash).second) + needs_dump_ = true; return; } diff --git a/tests/test_group_keys.cpp b/tests/test_group_keys.cpp index 0af36f311..82c51e795 100644 --- a/tests/test_group_keys.cpp +++ b/tests/test_group_keys.cpp @@ -1037,3 +1037,54 @@ TEST_CASE("Group Keys promotion", "[config][groups][keys][promotion]") { CHECK(admin.info.get_name() == "new name"); } + +TEST_CASE( + "Group Keys - the same key under a second message hash", + "[config][groups][keys][recovery]") { + + // insert_key's early-return path: we already hold this key, but this is a *different* message + // carrying it. Both copies want renewing, so the new hash has to be recorded -- and persisted, + // or it is silently lost on the next restart and that message stops being renewed. + const std::vector group_seed = + "0123456789abcdeffedcba98765432100123456789abcdeffedcba9876543210"_hexbytes; + const std::vector admin_seed = + "0123456789abcdef0123456789abcdeffedcba9876543210fedcba9876543210"_hexbytes; + const std::vector member_seed = + "000111222333444555666777888999aaabbbcccdddeeefff0123456789abcdef"_hexbytes; + + std::array group_pk; + std::array group_sk; + crypto_sign_ed25519_seed_keypair(group_pk.data(), group_sk.data(), group_seed.data()); + + pseudo_client admin{admin_seed, true, group_pk.data(), group_sk.data()}; + pseudo_client member{member_seed, false, group_pk.data(), std::nullopt}; + + for (const auto* c : {&admin, &member}) { + auto m = admin.members.get_or_construct(c->session_id); + m.admin = (c == &admin); + admin.members.set(m); + } + auto rekey1 = session::to_vector(admin.keys.rekey(admin.info, admin.members)); + + constexpr int64_t t0 = 1'700'000'000'000; + REQUIRE(admin.keys.load_key_message("keyhash1", rekey1, t0, admin.info, admin.members)); + REQUIRE(member.keys.load_key_message("keyhash1", rekey1, t0, member.info, member.members)); + + member.keys.dump(); // clear needs_dump + REQUIRE_FALSE(member.keys.needs_dump()); + + CHECK(member.keys.load_key_message("keyhash1b", rekey1, t0, member.info, member.members)); + CHECK(member.keys.active_hashes().count("keyhash1b")); + CHECK(member.keys.needs_dump()); + + // Re-loading a message we already know changes nothing, so it must not ask for a dump. + member.keys.dump(); + CHECK(member.keys.load_key_message("keyhash1b", rekey1, t0, member.info, member.members)); + CHECK_FALSE(member.keys.needs_dump()); + + // The second copy survives the round trip, so it is still renewable after a restart. + auto dump = member.keys.dump(); + pseudo_client reloaded{ + member_seed, false, group_pk.data(), std::nullopt, std::nullopt, std::nullopt, dump}; + CHECK(reloaded.keys.active_hashes().count("keyhash1b")); +} From 97aafbbd0bac481a9d845716c54d7dd5088b285b Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Thu, 6 Aug 2026 14:54:17 +1000 Subject: [PATCH 3/9] groups::Keys: retain active keys message bytes so they can be re-stored A keys message that expires from the swarm is currently unrecoverable: it is signed by an admin and padded from the group secret key, so a member cannot regenerate one, and only an admin rekey repairs the group. Retain the raw bytes of each message named in active_msgs_ so recovery can push them back verbatim, landing on the same message hash. Stored by hash rather than generation because one generation carries the full rekey plus every supplemental issued against it; a member that receives only some of them does not get the key. Recovery must therefore re-store every held message for a generation, not one of them. The cache is pruned from active_msgs_ as the authority in a single place rather than alongside each site that drops hashes, so a future way of dropping a hash cannot leak bytes by forgetting to prune too. That pruning is what bounds this to the same KEY_EXPIRY window as the keys themselves, on disk as well as in memory. Adds Keys::active_key_messages() and a groups_keys_active_message() C shim returning borrowed bytes, following pending_config(). The dump gains a "C" key, which sorts between the existing "A" and "L"; old dumps load without it and old code skips it. Only messages loaded after this ships are retained, so existing groups are unchanged and a hash may legitimately have no bytes behind it. --- include/session/config/groups/keys.h | 28 +++ include/session/config/groups/keys.hpp | 48 ++++- src/config/groups/keys.cpp | 79 ++++++- tests/test_group_keys.cpp | 278 +++++++++++++++++++++++++ 4 files changed, 425 insertions(+), 8 deletions(-) diff --git a/include/session/config/groups/keys.h b/include/session/config/groups/keys.h index 33c42368b..0a029d564 100644 --- a/include/session/config/groups/keys.h +++ b/include/session/config/groups/keys.h @@ -293,6 +293,34 @@ LIBSESSION_EXPORT bool groups_keys_load_message( /// to the caller and must be free()d when done. LIBSESSION_EXPORT config_string_list* groups_keys_active_hashes(const config_group_keys* conf); +/// API: groups/groups_keys_active_message +/// +/// Retrieves the raw bytes of an active keys message previously loaded by this device, by its +/// message hash. Use this together with `groups_keys_active_hashes` to re-store a keys message +/// that has expired from the swarm: the bytes must be pushed back *unchanged*, which is what lets +/// a non-admin do it at all (a keys message is admin-signed and cannot be regenerated by a member). +/// +/// Only messages loaded since this device began retaining them are available, so a hash returned by +/// `groups_keys_active_hashes` may legitimately have no bytes here. That means "cannot recover +/// this one", not an error. +/// +/// Inputs: +/// - `conf` -- [in] Pointer to the keys config object +/// - `msg_hash` -- [in] Null-terminated C string containing the message hash +/// - `data` -- [out] Set to a pointer to the message bytes, if found +/// - `datalen` -- [out] Set to the length of `data`, if found +/// +/// Outputs: +/// - `true` if the message was found, with `data`/`datalen` set. The pointer belongs to `conf` and +/// must NOT be free()d; it is invalidated by anything that modifies `conf` (loading a message, +/// rekeying, etc.), so copy the bytes if you need to keep them. +/// - `false` if no bytes are retained for that hash, leaving `data`/`datalen` untouched. +LIBSESSION_EXPORT bool groups_keys_active_message( + const config_group_keys* conf, + const char* msg_hash, + const unsigned char** data, + size_t* datalen); + /// API: groups/groups_keys_needs_rekey /// /// Checks whether a rekey is required (for instance, because of key generation conflict). Note diff --git a/include/session/config/groups/keys.hpp b/include/session/config/groups/keys.hpp index 5eaa27bb4..115ac1719 100644 --- a/include/session/config/groups/keys.hpp +++ b/include/session/config/groups/keys.hpp @@ -108,6 +108,18 @@ class Keys : public ConfigSig { /// Hashes of messages we have successfully parsed; used for deciding what needs to be renewed. std::map> active_msgs_; + /// The raw bytes of the messages named in `active_msgs_`, keyed by hash so that one generation + /// can hold both the full rekey message and every supplemental issued against it. + /// + /// We keep these so that an expired keys message can be re-stored *verbatim*: a keys message + /// carries an admin signature that a member cannot produce, and its junk padding derives from + /// `_sign_sk`, so re-pushing bytes we already hold is the only way a non-admin can put one + /// back. It is not secret — the same bytes sit on the swarm — so this is a plain vector + /// rather than a `sodium_vector`. + /// + /// Kept in lockstep with `active_msgs_` by `remove_expired()`; nothing else may add to it. + std::map> key_msgs_; + sodium_cleared> pending_key_; sodium_vector pending_key_config_; int64_t pending_gen_ = -1; @@ -125,11 +137,20 @@ class Keys : public ConfigSig { // Checks for and drops expired keys. void remove_expired(); + // Drops any retained message bytes whose hash is no longer in `active_msgs_`. Derived from + // `active_msgs_` rather than repeated at each place that drops hashes, so that a new way of + // dropping a hash cannot leak bytes by forgetting to prune here as well. + void prune_key_msgs(); + // Loads existing state from a previous dump of keys data void load_dump(std::span dump); - // Inserts a key into the correct place in `keys_`. - void insert_key(std::string_view message_hash, key_info&& key); + // Inserts a key into the correct place in `keys_`. `message_data` is the raw message the key + // came from, retained alongside the hash so it can be re-stored verbatim later. + void insert_key( + std::string_view message_hash, + std::span message_data, + key_info&& key); // Returned the blinding factor for a given session X25519 pubkey. This depends on the group's // seed and thus is only obtainable by an admin account. @@ -596,6 +617,29 @@ class Keys : public ConfigSig { /// - vector of message hashes std::unordered_set active_hashes() const; + /// API: groups/Keys::active_key_messages + /// + /// Returns the raw bytes of the currently active keys messages, keyed by message hash. These + /// are the same messages `active_hashes()` names; this gives you the contents as well, so that + /// a message which has expired from the swarm can be re-stored *verbatim*. + /// + /// Re-storing the bytes unchanged is what makes this usable by a non-admin: a keys message is + /// signed by an admin and padded from the group secret key, so it cannot be regenerated by a + /// member, but bytes already held can be pushed back as-is and land on the same message hash. + /// + /// Only messages loaded by this device since it began retaining them appear here, so a group + /// whose keys messages all predate that support will return fewer entries than + /// `active_hashes()` — or none at all. Callers must treat a missing entry as "cannot recover + /// this one" rather than as an error. + /// + /// Inputs: none + /// + /// Outputs: + /// - map of message hash to the message bytes. The spans point at data owned by this object + /// and are invalidated by anything that modifies it (e.g. `load_key_message`, `rekey`), + /// exactly as for `pending_config()`. + std::map> active_key_messages() const; + /// API: groups/Keys::needs_rekey /// /// Returns true if the key list requires a new key to be generated and pushed to the server (by diff --git a/src/config/groups/keys.cpp b/src/config/groups/keys.cpp index 29685a4d9..81e0fe532 100644 --- a/src/config/groups/keys.cpp +++ b/src/config/groups/keys.cpp @@ -86,6 +86,15 @@ std::vector Keys::make_dump() const { } } + { + // Raw bytes of the messages named in "A", so they can be re-stored verbatim if they expire + // from the swarm. `key_msgs_` is a std::map, so this comes out in the sorted order bt_dict + // requires. Old dumps simply won't have this key, and old code skips it. + auto msgs = d.append_dict("C"); + for (const auto& [hash, data] : key_msgs_) + msgs.append(hash, to_string_view(data)); + } + { auto keys = d.append_list("L"); for (auto& k : keys_) { @@ -123,6 +132,16 @@ void Keys::load_dump(std::span dump) { throw config_value_error{"Invalid Keys dump: `active` not found"}; } + // Optional: absent in dumps written before we retained message bytes, in which case we simply + // have hashes we cannot re-store. Not an error. + if (d.skip_until("C")) { + auto msgs = d.consume_dict_consumer(); + while (!msgs.is_finished()) { + auto [hash, data] = msgs.next_string(); + key_msgs_.emplace(hash, to_vector(data)); + } + } + if (d.skip_until("L")) { auto keys = d.consume_list_consumer(); while (!keys.is_finished()) { @@ -176,6 +195,17 @@ void Keys::load_dump(std::span dump) { std::to_string(pk.size()) + ")"}; std::memcpy(pending_key_.data(), pk.data(), pending_key_.size()); } + + // `key_msgs_` must never outlive the hashes in `active_msgs_`; enforce that on the way in too, + // so a hand-written or corrupted dump can't seed an entry that nothing will ever prune. + prune_key_msgs(); +} + +void Keys::prune_key_msgs() { + if (key_msgs_.empty()) + return; + auto keep = active_hashes(); + std::erase_if(key_msgs_, [&](const auto& item) { return !keep.count(item.first); }); } size_t Keys::size() const { @@ -833,7 +863,8 @@ std::optional> Keys::pending_config() const { return std::span{pending_key_config_.data(), pending_key_config_.size()}; } -void Keys::insert_key(std::string_view msg_hash, key_info&& new_key) { +void Keys::insert_key( + std::string_view msg_hash, std::span msg_data, key_info&& new_key) { // Find all keys with the same generation and see if our key is in there (that is: we are // deliberately ignoring timestamp so that we don't add the same key with slight timestamp // variations). @@ -843,10 +874,14 @@ void Keys::insert_key(std::string_view msg_hash, key_info&& new_key) { }); for (auto it = gen_begin; it != gen_end; ++it) if (it->key == new_key.key) { - // We already have this key, but this may be a *different* message carrying it, in - // which case we want to keep renewing this copy too. Flag a dump only when the hash - // is actually new, so re-loading a message we already know stays free. - if (active_msgs_[new_key.generation].emplace(msg_hash).second) + // We already have this key, but this may be a *different* message carrying it (the + // same key can arrive again under another hash), in which case we want to renew and be + // able to re-store this copy too. Flag a dump only when something actually changed, + // so re-loading a message we already know stays free. + bool new_hash = active_msgs_[new_key.generation].emplace(msg_hash).second; + bool new_bytes = + key_msgs_.try_emplace(std::string{msg_hash}, to_vector(msg_data)).second; + if (new_hash || new_bytes) needs_dump_ = true; return; } @@ -860,6 +895,7 @@ void Keys::insert_key(std::string_view msg_hash, key_info&& new_key) { return; active_msgs_[new_key.generation].emplace(msg_hash); + key_msgs_.insert_or_assign(std::string{msg_hash}, to_vector(msg_data)); keys_.insert(it, std::move(new_key)); remove_expired(); needs_dump_ = true; @@ -1106,14 +1142,18 @@ bool Keys::load_key_message( if (!new_keys.empty()) { for (auto& k : new_keys) - insert_key(hash, std::move(k)); + insert_key(hash, data, std::move(k)); auto new_key_list = group_keys(); members.replace_keys(new_key_list, /*dirty=*/false); info.replace_keys(new_key_list, /*dirty=*/false); return true; } else if (max_gen) { + // A valid keys message that held no key for us — a supplemental aimed at other members, + // typically. Still worth retaining: it is part of the generation, and a member who gets + // only some of a generation's messages doesn't get the key. active_msgs_[*max_gen].emplace(hash); + key_msgs_.insert_or_assign(std::string{hash}, to_vector(data)); remove_expired(); needs_dump_ = true; } @@ -1128,6 +1168,13 @@ std::unordered_set Keys::active_hashes() const { return hashes; } +std::map> Keys::active_key_messages() const { + std::map> msgs; + for (const auto& [hash, data] : key_msgs_) + msgs.emplace(hash, std::span{data.data(), data.size()}); + return msgs; +} + void Keys::remove_expired() { if (keys_.size() >= 2) { // When we're done, this will point at the first element we want to keep (i.e. we want to @@ -1174,6 +1221,11 @@ void Keys::remove_expired() { // something) and so it isn't really up to us to keep them alive, since that's a history of // the group we apparently don't have access to. active_msgs_.clear(); + + // Retained message bytes follow the hashes exactly, for both of the above branches. This is + // what bounds the size of `key_msgs_` (and hence of the dump) to the same KEY_EXPIRY window as + // the keys themselves; without it an expired generation's bytes would live forever on disk. + prune_key_msgs(); } bool Keys::needs_rekey() const { @@ -1470,6 +1522,21 @@ LIBSESSION_C_API config_string_list* groups_keys_active_hashes(const config_grou return make_string_list(unbox(conf).active_hashes()); } +LIBSESSION_C_API bool groups_keys_active_message( + const config_group_keys* conf, + const char* msg_hash, + const unsigned char** data, + size_t* datalen) { + assert(msg_hash && data && datalen); + auto msgs = unbox(conf).active_key_messages(); + if (auto it = msgs.find(msg_hash); it != msgs.end()) { + *data = it->second.data(); + *datalen = it->second.size(); + return true; + } + return false; +} + LIBSESSION_C_API bool groups_keys_needs_rekey(const config_group_keys* conf) { return unbox(conf).needs_rekey(); } diff --git a/tests/test_group_keys.cpp b/tests/test_group_keys.cpp index 82c51e795..72456c668 100644 --- a/tests/test_group_keys.cpp +++ b/tests/test_group_keys.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -1088,3 +1089,280 @@ TEST_CASE( member_seed, false, group_pk.data(), std::nullopt, std::nullopt, std::nullopt, dump}; CHECK(reloaded.keys.active_hashes().count("keyhash1b")); } + +// Recovery support: `Keys` retains the raw bytes of each active keys message so that a message +// which has expired from the swarm can be re-stored *verbatim*. Verbatim is the whole point — a +// keys message carries an admin signature and junk padding derived from the group secret key, so a +// member cannot regenerate one, but it can push back bytes it already holds. +namespace { + +// Rebuilds a Keys dump with the "C" (retained message bytes) key stripped, i.e. the format written +// before this feature existed. Used to check an old dump still loads. +std::vector strip_retained_messages(std::span dump) { + // Re-emit the raw sub-encodings rather than re-serialising values, so what comes out is + // byte-for-byte what the old code would have written. + oxenc::bt_dict_consumer d{dump}; + std::string out = "d"; + REQUIRE(d.skip_until("A")); + out += "1:A"; + out += d.consume_list_data(); + if (d.skip_until("C")) + d.consume_dict_data(); // drop it + REQUIRE(d.skip_until("L")); + out += "1:L"; + out += d.consume_list_data(); + if (d.skip_until("P")) { + out += "1:P"; + out += d.consume_dict_data(); + } + out += "e"; + return session::to_vector(out); +} + +} // namespace + +TEST_CASE("Group Keys - retained message bytes", "[config][groups][keys][recovery]") { + + const std::vector group_seed = + "0123456789abcdeffedcba98765432100123456789abcdeffedcba9876543210"_hexbytes; + const std::vector admin_seed = + "0123456789abcdef0123456789abcdeffedcba9876543210fedcba9876543210"_hexbytes; + const std::vector member_seed = + "000111222333444555666777888999aaabbbcccdddeeefff0123456789abcdef"_hexbytes; + const std::vector member2_seed = + "00011122435111155566677788811263446552465222efff0123456789abcdef"_hexbytes; + + std::array group_pk; + std::array group_sk; + crypto_sign_ed25519_seed_keypair(group_pk.data(), group_sk.data(), group_seed.data()); + + pseudo_client admin{admin_seed, true, group_pk.data(), group_sk.data()}; + pseudo_client member{member_seed, false, group_pk.data(), std::nullopt}; + pseudo_client member2{member2_seed, false, group_pk.data(), std::nullopt}; + + // Put the admin and member1 in the group, then rekey so member1 can actually read the message. + for (const auto* c : {&admin, &member}) { + auto m = admin.members.get_or_construct(c->session_id); + m.admin = (c == &admin); + admin.members.set(m); + } + auto rekey1 = session::to_vector(admin.keys.rekey(admin.info, admin.members)); + + constexpr int64_t t0 = 1'700'000'000'000; + + // The admin has to load its own rekey back to confirm it; until it does, the key is only + // pending and the generation doesn't advance. + REQUIRE(admin.keys.load_key_message("keyhash1", rekey1, t0, admin.info, admin.members)); + REQUIRE(member.keys.load_key_message("keyhash1", rekey1, t0, member.info, member.members)); + + SECTION("bytes are retained verbatim, and survive a dump/reload") { + auto held = member.keys.active_key_messages(); + REQUIRE(held.size() == 1); + REQUIRE(held.count("keyhash1")); + CHECK(to_hex(session::to_vector(held.at("keyhash1"))) == to_hex(rekey1)); + + // Every hash we advertise for renewal has bytes behind it. + CHECK(as_set(member.keys.active_hashes()) == std::set{{"keyhash1"s}}); + + auto dump = member.keys.dump(); + pseudo_client reloaded{ + member_seed, + false, + group_pk.data(), + std::nullopt, + std::nullopt, + std::nullopt, + dump}; + + auto held2 = reloaded.keys.active_key_messages(); + REQUIRE(held2.size() == 1); + REQUIRE(held2.count("keyhash1")); + CHECK(to_hex(session::to_vector(held2.at("keyhash1"))) == to_hex(rekey1)); + } + + SECTION("a supplemental is retained alongside the full message of the same generation") { + // This is why storage is keyed by hash rather than generation: one generation carries the + // full rekey plus every supplemental issued against it, and a member that receives only + // the supplemental does not get the key. + auto supp = admin.keys.key_supplement(member2.session_id); + // False here just means "no key in it for us" -- it is addressed to member2. We still + // retain it, which is the point: this is the path where a message is recorded against the + // generation without contributing a key, and a member who receives only the supplemental + // needs the full message too. + CHECK_FALSE(member.keys.load_key_message( + "keyhash2", supp, t0 + 1000, member.info, member.members)); + + auto held = member.keys.active_key_messages(); + REQUIRE(held.size() == 2); + REQUIRE(held.count("keyhash1")); + REQUIRE(held.count("keyhash2")); + CHECK(to_hex(session::to_vector(held.at("keyhash1"))) == to_hex(rekey1)); + CHECK(to_hex(session::to_vector(held.at("keyhash2"))) == to_hex(supp)); + + // Both belong to the same generation, and both are advertised for renewal. + CHECK(as_set(member.keys.active_hashes()) == + std::set{{"keyhash1"s, "keyhash2"s}}); + + // ...and both survive a dump/reload, which is what recovery after a restart depends on. + auto dump = member.keys.dump(); + pseudo_client reloaded{ + member_seed, + false, + group_pk.data(), + std::nullopt, + std::nullopt, + std::nullopt, + dump}; + auto held2 = reloaded.keys.active_key_messages(); + REQUIRE(held2.size() == 2); + CHECK(to_hex(session::to_vector(held2.at("keyhash1"))) == to_hex(rekey1)); + CHECK(to_hex(session::to_vector(held2.at("keyhash2"))) == to_hex(supp)); + } + + SECTION("pruning: bytes are dropped when their generation expires") { + // The generation-erase branch of remove_expired(). Expiry is driven by the swarm-provided + // message timestamps, not the system clock, so we just hand it messages far enough apart. + auto rekey2 = session::to_vector(admin.keys.rekey(admin.info, admin.members)); + REQUIRE(admin.keys.load_key_message( + "keyhash2", rekey2, t0 + 1000, admin.info, admin.members)); + REQUIRE(member.keys.load_key_message( + "keyhash2", rekey2, t0 + 1000, member.info, member.members)); + CHECK(member.keys.active_key_messages().size() == 2); + + // A third generation, more than KEY_EXPIRY (60d) after the second, retires the first. + constexpr int64_t past_expiry = t0 + 1000 + 61 * 24 * 60 * 60 * 1000LL; + auto rekey3 = session::to_vector(admin.keys.rekey(admin.info, admin.members)); + REQUIRE(admin.keys.load_key_message( + "keyhash3", rekey3, past_expiry, admin.info, admin.members)); + REQUIRE(member.keys.load_key_message( + "keyhash3", rekey3, past_expiry, member.info, member.members)); + + auto held = member.keys.active_key_messages(); + CHECK_FALSE(held.count("keyhash1")); + + // The invariant that bounds this cache: it holds exactly the hashes we still advertise. + std::set held_hashes; + for (const auto& [h, _] : held) + held_hashes.insert(h); + CHECK(held_hashes == as_set(member.keys.active_hashes())); + + // And the shrink is real on disk, not just in memory. + auto dump = member.keys.dump(); + pseudo_client reloaded{ + member_seed, + false, + group_pk.data(), + std::nullopt, + std::nullopt, + std::nullopt, + dump}; + CHECK_FALSE(reloaded.keys.active_key_messages().count("keyhash1")); + } + + SECTION("pruning: bytes are dropped when we hold no keys at all") { + // The other branch of remove_expired(): `keys_` empty means we aren't keeping any of the + // group's history, so active_msgs_ is cleared outright. member2 was never given the keys, + // so it reaches this by loading a supplemental addressed to somebody else. + auto supp = admin.keys.key_supplement(member.session_id); + + REQUIRE(member2.keys.size() == 0); + CHECK_FALSE(member2.keys.load_key_message( + "keyhash9", supp, t0 + 1000, member2.info, member2.members)); + + CHECK(member2.keys.active_hashes().empty()); + CHECK(member2.keys.active_key_messages().empty()); + } + + SECTION("dump compatibility, both directions") { + auto supp = admin.keys.key_supplement(member2.session_id); + // False here just means "no key in it for us" -- it is addressed to member2. We still + // retain it, which is the point: this is the path where a message is recorded against the + // generation without contributing a key, and a member who receives only the supplemental + // needs the full message too. + CHECK_FALSE(member.keys.load_key_message( + "keyhash2", supp, t0 + 1000, member.info, member.members)); + auto dump = member.keys.dump(); + + SECTION("an old dump loads under new code") { + auto old_dump = strip_retained_messages(dump); + pseudo_client reloaded{ + member_seed, + false, + group_pk.data(), + std::nullopt, + std::nullopt, + std::nullopt, + old_dump}; + + // The hashes still load; we simply have no bytes for them, which is exactly the + // position of every group that predates this feature. + CHECK(as_set(reloaded.keys.active_hashes()) == + std::set{{"keyhash1"s, "keyhash2"s}}); + CHECK(reloaded.keys.active_key_messages().empty()); + + // Still fully functional: it can decrypt with the keys it loaded. + CHECK(reloaded.keys.size() == member.keys.size()); + } + + SECTION("a new dump loads under old code") { + // Old code reads with skip_until over "A", "L", "P" and never asks for "C", so the + // unknown key is skipped. Parsing the new dump the way the old code does must still + // yield the same "A" and "L". + oxenc::bt_dict_consumer d{dump}; + REQUIRE(d.skip_until("A")); + auto active = d.consume_list_consumer(); + std::set hashes; + while (!active.is_finished()) { + auto lst = active.consume_list_consumer(); + lst.consume_integer(); // generation + while (!lst.is_finished()) + hashes.insert(lst.consume_string()); + } + CHECK(hashes == std::set{{"keyhash1"s, "keyhash2"s}}); + + // "C" sorts between "A" and "L", so a consumer that skips straight to "L" must still + // find it -- this is the assertion that would fail if the new key were misplaced. + REQUIRE(d.skip_until("L")); + auto keys = d.consume_list_consumer(); + CHECK_FALSE(keys.is_finished()); + } + } + + SECTION("C API") { + auto dump = member.keys.dump(); + config_group_keys* conf; + config_object* info_conf; + config_object* mem_conf; + char err[256]; + REQUIRE(groups_info_init(&info_conf, group_pk.data(), nullptr, nullptr, 0, err) == 0); + REQUIRE(groups_members_init(&mem_conf, group_pk.data(), nullptr, nullptr, 0, err) == 0); + + auto member_sk = sk_from_seed(member_seed); + REQUIRE(groups_keys_init( + &conf, + member_sk.data(), + group_pk.data(), + nullptr, + info_conf, + mem_conf, + dump.data(), + dump.size(), + err) == 0); + + const unsigned char* data = nullptr; + size_t datalen = 0; + CHECK(groups_keys_active_message(conf, "keyhash1", &data, &datalen)); + REQUIRE(data); + CHECK(to_hex(std::vector{data, data + datalen}) == to_hex(rekey1)); + + // A hash we don't hold bytes for is a plain "no", not an error. + const unsigned char* missing = nullptr; + size_t missinglen = 0; + CHECK_FALSE(groups_keys_active_message(conf, "nosuchhash", &missing, &missinglen)); + CHECK(missing == nullptr); + + groups_keys_free(conf); + config_free(info_conf); + config_free(mem_conf); + } +} From ab75f54b6ddec67092cc306af353bddf97a00003 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 21 Aug 2026 11:11:46 +1000 Subject: [PATCH 4/9] groups::Keys: correct two comments that claimed more than they bound From a cold review of #123; neither changes behaviour. The key_msgs_ comment said nothing else may add to it, but four sites do: load_dump, both recording paths in insert_key, and load_key_message's no-key-for-us path. Replaced with the invariant that is actually true and checkable -- entries are added only while loading, and removed only by prune_key_msgs(), which derives the survivors from active_msgs_. The prune comment claimed to bound the dump size. It bounds the window. Every rekey and every supplemental inside KEY_EXPIRY is held verbatim, and a rekey message is 177 + 48*N bytes for N members padded to a multiple of MESSAGE_KEY_MULTIPLE, so a large group that rekeys often can still carry a sizeable dump. The size argument this feature was accepted on rests on "does not grow without limit", not on "stays small". --- include/session/config/groups/keys.hpp | 6 +++++- src/config/groups/keys.cpp | 12 +++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/include/session/config/groups/keys.hpp b/include/session/config/groups/keys.hpp index 115ac1719..e36314654 100644 --- a/include/session/config/groups/keys.hpp +++ b/include/session/config/groups/keys.hpp @@ -117,7 +117,11 @@ class Keys : public ConfigSig { /// back. It is not secret — the same bytes sit on the swarm — so this is a plain vector /// rather than a `sodium_vector`. /// - /// Kept in lockstep with `active_msgs_` by `remove_expired()`; nothing else may add to it. + /// Invariant: every key here is also a hash in `active_msgs_`. Entries are added only while + /// loading -- `load_key_message()` (via `insert_key()`, or directly for a message that carried + /// no key for us) and `load_dump()` when restoring -- and removed only by `prune_key_msgs()`, + /// which derives the survivors from `active_msgs_`. Nothing outside those load paths may add + /// to it. std::map> key_msgs_; sodium_cleared> pending_key_; diff --git a/src/config/groups/keys.cpp b/src/config/groups/keys.cpp index 81e0fe532..71fa3de13 100644 --- a/src/config/groups/keys.cpp +++ b/src/config/groups/keys.cpp @@ -1222,9 +1222,15 @@ void Keys::remove_expired() { // the group we apparently don't have access to. active_msgs_.clear(); - // Retained message bytes follow the hashes exactly, for both of the above branches. This is - // what bounds the size of `key_msgs_` (and hence of the dump) to the same KEY_EXPIRY window as - // the keys themselves; without it an expired generation's bytes would live forever on disk. + // Retained message bytes follow the hashes exactly, for both of the above branches, so they + // expire on the same schedule as the keys; without this an expired generation's bytes would sit + // on disk forever. + // + // Note what that does and does not bound: it bounds the WINDOW, not the size within it. Every + // rekey and every supplemental inside KEY_EXPIRY is held verbatim, and a full rekey message is + // 177 + 48*N bytes for N members rounded up to a multiple of MESSAGE_KEY_MULTIPLE (see the + // arithmetic in keys.hpp), so a large group that rekeys often can carry a sizeable dump. + // Bounded and predictable, not necessarily small. prune_key_msgs(); } From 9ed5822e961b01913c8b229d94f7a6641f847e23 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 14 Sep 2026 15:39:36 +1000 Subject: [PATCH 5/9] groups::Keys: look a retained message up without building a map groups_keys_active_message went through active_key_messages(), which allocates a fresh map holding a copy of every retained hash -- on every lookup. The header documents the usage as "use this together with groups_keys_active_hashes", so the documented pattern is quadratic: on a 100-message store that is ~6.6us per lookup against ~30ns for a direct find. active_key_messages() becomes public API once the wrapper pins move, so the shape is cheaper to settle now than later. --- include/session/config/groups/keys.hpp | 19 +++++++++++++++++++ src/config/groups/keys.cpp | 14 ++++++++++---- tests/test_group_keys.cpp | 5 +++++ 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/include/session/config/groups/keys.hpp b/include/session/config/groups/keys.hpp index e36314654..65abcaba5 100644 --- a/include/session/config/groups/keys.hpp +++ b/include/session/config/groups/keys.hpp @@ -644,6 +644,25 @@ class Keys : public ConfigSig { /// exactly as for `pending_config()`. std::map> active_key_messages() const; + /// API: groups/Keys::active_key_message + /// + /// Returns the raw bytes of a single active keys message, by its message hash, or + /// `std::nullopt` if we retain nothing for that hash. See `active_key_messages()` for what + /// the bytes are for and why a hash named by `active_hashes()` may legitimately have none. + /// + /// Prefer this over `active_key_messages()` when looking one hash up: the latter builds a new + /// map on every call. + /// + /// Inputs: + /// - `msg_hash` -- the message hash to look up + /// + /// Outputs: + /// - the message bytes, or `std::nullopt`. The span points at data owned by this object and + /// is invalidated by anything that modifies it (e.g. `load_key_message`, `rekey`), exactly + /// as for `pending_config()`. + std::optional> active_key_message( + std::string_view msg_hash) const; + /// API: groups/Keys::needs_rekey /// /// Returns true if the key list requires a new key to be generated and pushed to the server (by diff --git a/src/config/groups/keys.cpp b/src/config/groups/keys.cpp index 71fa3de13..eea5afbb3 100644 --- a/src/config/groups/keys.cpp +++ b/src/config/groups/keys.cpp @@ -1175,6 +1175,13 @@ std::map> Keys::active_key_messages( return msgs; } +std::optional> Keys::active_key_message( + std::string_view msg_hash) const { + if (auto it = key_msgs_.find(std::string{msg_hash}); it != key_msgs_.end()) + return std::span{it->second.data(), it->second.size()}; + return std::nullopt; +} + void Keys::remove_expired() { if (keys_.size() >= 2) { // When we're done, this will point at the first element we want to keep (i.e. we want to @@ -1534,10 +1541,9 @@ LIBSESSION_C_API bool groups_keys_active_message( const unsigned char** data, size_t* datalen) { assert(msg_hash && data && datalen); - auto msgs = unbox(conf).active_key_messages(); - if (auto it = msgs.find(msg_hash); it != msgs.end()) { - *data = it->second.data(); - *datalen = it->second.size(); + if (auto msg = unbox(conf).active_key_message(msg_hash)) { + *data = msg->data(); + *datalen = msg->size(); return true; } return false; diff --git a/tests/test_group_keys.cpp b/tests/test_group_keys.cpp index 72456c668..28c0db7f3 100644 --- a/tests/test_group_keys.cpp +++ b/tests/test_group_keys.cpp @@ -1161,6 +1161,11 @@ TEST_CASE("Group Keys - retained message bytes", "[config][groups][keys][recover REQUIRE(held.count("keyhash1")); CHECK(to_hex(session::to_vector(held.at("keyhash1"))) == to_hex(rekey1)); + auto one = member.keys.active_key_message("keyhash1"); + REQUIRE(one); + CHECK(to_hex(session::to_vector(*one)) == to_hex(rekey1)); + CHECK_FALSE(member.keys.active_key_message("nosuchhash")); + // Every hash we advertise for renewal has bytes behind it. CHECK(as_set(member.keys.active_hashes()) == std::set{{"keyhash1"s}}); From 127e9c23a83ba40a89baad1b9e8ffee6dac2d4f6 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 14 Sep 2026 15:40:07 +1000 Subject: [PATCH 6/9] groups::Keys: copy a retained message once per hash, not once per key key_supplement packs every key in keys_, so load_key_message calls insert_key once per generation the supplemental carried -- same hash, same bytes each time. to_vector(msg_data) was evaluated as a call argument on each of those, so an N-generation supplemental copied the whole message N times and kept one. The new test pins the property that makes this shape necessary: a hash is named by every generation the message carried, so its bytes have to outlive the oldest of them expiring. --- src/config/groups/keys.cpp | 11 ++++-- tests/test_group_keys.cpp | 78 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/src/config/groups/keys.cpp b/src/config/groups/keys.cpp index eea5afbb3..60c0fc067 100644 --- a/src/config/groups/keys.cpp +++ b/src/config/groups/keys.cpp @@ -879,8 +879,10 @@ void Keys::insert_key( // able to re-store this copy too. Flag a dump only when something actually changed, // so re-loading a message we already know stays free. bool new_hash = active_msgs_[new_key.generation].emplace(msg_hash).second; - bool new_bytes = - key_msgs_.try_emplace(std::string{msg_hash}, to_vector(msg_data)).second; + auto key = std::string{msg_hash}; + bool new_bytes = !key_msgs_.contains(key); + if (new_bytes) + key_msgs_.emplace(std::move(key), to_vector(msg_data)); if (new_hash || new_bytes) needs_dump_ = true; return; @@ -895,7 +897,10 @@ void Keys::insert_key( return; active_msgs_[new_key.generation].emplace(msg_hash); - key_msgs_.insert_or_assign(std::string{msg_hash}, to_vector(msg_data)); + // A supplemental carries every key in `keys_`, so one message reaches this once per generation + // it brought that we didn't already hold -- same hash, same bytes each time. + if (auto key = std::string{msg_hash}; !key_msgs_.contains(key)) + key_msgs_.emplace(std::move(key), to_vector(msg_data)); keys_.insert(it, std::move(new_key)); remove_expired(); needs_dump_ = true; diff --git a/tests/test_group_keys.cpp b/tests/test_group_keys.cpp index 28c0db7f3..c2246f5f0 100644 --- a/tests/test_group_keys.cpp +++ b/tests/test_group_keys.cpp @@ -1371,3 +1371,81 @@ TEST_CASE("Group Keys - retained message bytes", "[config][groups][keys][recover config_free(mem_conf); } } + +TEST_CASE("Group Keys - one message, several generations", "[config][groups][keys][recovery]") { + + // `key_supplement` packs every key in `keys_`, so a member that can decrypt one ends up + // recording the same hash against each generation it carried. Retained bytes are therefore + // shared between generations, and must outlive the *oldest* of them expiring -- pruning per + // dropped generation would strand a hash that is still active under a newer one. + const std::vector group_seed = + "0123456789abcdeffedcba98765432100123456789abcdeffedcba9876543210"_hexbytes; + const std::vector admin_seed = + "0123456789abcdef0123456789abcdeffedcba9876543210fedcba9876543210"_hexbytes; + const std::vector member_seed = + "000111222333444555666777888999aaabbbcccdddeeefff0123456789abcdef"_hexbytes; + + std::array group_pk; + std::array group_sk; + crypto_sign_ed25519_seed_keypair(group_pk.data(), group_sk.data(), group_seed.data()); + + pseudo_client admin{admin_seed, true, group_pk.data(), group_sk.data()}; + pseudo_client member{member_seed, false, group_pk.data(), std::nullopt}; + + for (const auto* c : {&admin, &member}) { + auto m = admin.members.get_or_construct(c->session_id); + m.admin = (c == &admin); + admin.members.set(m); + } + + constexpr int64_t t0 = 1'700'000'000'000; + + auto rekey1 = session::to_vector(admin.keys.rekey(admin.info, admin.members)); + REQUIRE(admin.keys.load_key_message("keyhash1", rekey1, t0, admin.info, admin.members)); + REQUIRE(member.keys.load_key_message("keyhash1", rekey1, t0, member.info, member.members)); + + auto rekey2 = session::to_vector(admin.keys.rekey(admin.info, admin.members)); + REQUIRE(admin.keys.load_key_message("keyhash2", rekey2, t0 + 1000, admin.info, admin.members)); + REQUIRE(member.keys.load_key_message( + "keyhash2", rekey2, t0 + 1000, member.info, member.members)); + + // Addressed to the member itself, so it decrypts and yields a key per generation the admin + // holds -- all of which the member already has, i.e. insert_key's early-return path, twice. + REQUIRE(admin.keys.size() == 2); + auto supp = admin.keys.key_supplement(member.session_id); + CHECK(member.keys.load_key_message("supphash", supp, t0 + 2000, member.info, member.members)); + + REQUIRE(member.keys.active_key_message("supphash")); + CHECK(to_hex(session::to_vector(*member.keys.active_key_message("supphash"))) == to_hex(supp)); + + // A third generation far enough ahead to retire the first. + constexpr int64_t past_expiry = t0 + 1000 + 61 * 24 * 60 * 60 * 1000LL; + auto rekey3 = session::to_vector(admin.keys.rekey(admin.info, admin.members)); + REQUIRE(admin.keys.load_key_message( + "keyhash3", rekey3, past_expiry, admin.info, admin.members)); + REQUIRE(member.keys.load_key_message( + "keyhash3", rekey3, past_expiry, member.info, member.members)); + + // The first generation's own message is gone... + CHECK_FALSE(member.keys.active_key_message("keyhash1")); + + // ...but the supplemental is still named by a generation we keep, so its bytes must survive. + CHECK(member.keys.active_hashes().count("supphash")); + REQUIRE(member.keys.active_key_message("supphash")); + CHECK(to_hex(session::to_vector(*member.keys.active_key_message("supphash"))) == to_hex(supp)); + + // The invariant, stated as the tests above state it: what we advertise is what we can re-store. + std::set held; + for (const auto& [h, _] : member.keys.active_key_messages()) + held.insert(h); + CHECK(held == as_set(member.keys.active_hashes())); + + // And the same after a restart. + auto dump = member.keys.dump(); + pseudo_client reloaded{ + member_seed, false, group_pk.data(), std::nullopt, std::nullopt, std::nullopt, dump}; + REQUIRE(reloaded.keys.active_key_message("supphash")); + CHECK(to_hex(session::to_vector(*reloaded.keys.active_key_message("supphash"))) == + to_hex(supp)); + CHECK_FALSE(reloaded.keys.active_key_message("keyhash1")); +} From aa42ee8dc2807c3753fbcfe712c6c3df03bc5e86 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 14 Sep 2026 15:40:27 +1000 Subject: [PATCH 7/9] groups::Keys: prune retained bytes only when a hash was dropped prune_key_msgs() runs from remove_expired(), i.e. on every message load, and built an unordered_set copy of every active hash in order to decide -- almost always -- that there was nothing to drop. Every path that adds bytes adds the hash too, so nothing can be orphaned unless a hash was just dropped; run it only then, and hold views into active_msgs_ rather than copies when it does run. The derivation itself stays across all generations. One hash is routinely named by several, so erasing per dropped generation would strand a hash that is still active under a newer one -- which the test added in the previous commit catches. --- include/session/config/groups/keys.hpp | 14 ++++++++---- src/config/groups/keys.cpp | 31 ++++++++++++++++++-------- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/include/session/config/groups/keys.hpp b/include/session/config/groups/keys.hpp index 65abcaba5..2763f75cc 100644 --- a/include/session/config/groups/keys.hpp +++ b/include/session/config/groups/keys.hpp @@ -141,10 +141,16 @@ class Keys : public ConfigSig { // Checks for and drops expired keys. void remove_expired(); - // Drops any retained message bytes whose hash is no longer in `active_msgs_`. Derived from - // `active_msgs_` rather than repeated at each place that drops hashes, so that a new way of - // dropping a hash cannot leak bytes by forgetting to prune here as well. - void prune_key_msgs(); + // Drops any retained message bytes whose hash is no longer in `active_msgs_`, returning true + // if anything was dropped. Derived from `active_msgs_` rather than repeated at each place + // that drops hashes, so that a new way of dropping a hash cannot leak bytes by forgetting to + // prune here as well. + // + // The survivors have to be taken across *all* generations, not just the ones being dropped: a + // supplemental carries every key in `keys_`, so one hash is routinely named by several + // generations, and erasing per dropped generation would strand a hash that is still active + // under a newer one. + bool prune_key_msgs(); // Loads existing state from a previous dump of keys data void load_dump(std::span dump); diff --git a/src/config/groups/keys.cpp b/src/config/groups/keys.cpp index 60c0fc067..2010d489d 100644 --- a/src/config/groups/keys.cpp +++ b/src/config/groups/keys.cpp @@ -201,11 +201,16 @@ void Keys::load_dump(std::span dump) { prune_key_msgs(); } -void Keys::prune_key_msgs() { +bool Keys::prune_key_msgs() { if (key_msgs_.empty()) - return; - auto keep = active_hashes(); - std::erase_if(key_msgs_, [&](const auto& item) { return !keep.count(item.first); }); + return false; + // Views, not copies: `active_msgs_` owns these strings and outlives the lookup. + std::unordered_set keep; + for (const auto& [gen, hashes] : active_msgs_) + keep.insert(hashes.begin(), hashes.end()); + auto dropped = + std::erase_if(key_msgs_, [&](const auto& item) { return !keep.contains(item.first); }); + return dropped > 0; } size_t Keys::size() const { @@ -1225,14 +1230,18 @@ void Keys::remove_expired() { } // Drop any active message hashes for generations we are no longer keeping around - if (!keys_.empty()) - active_msgs_.erase( - active_msgs_.begin(), active_msgs_.lower_bound(keys_.front().generation)); - else + bool dropped_hashes = false; + if (!keys_.empty()) { + auto keep_from = active_msgs_.lower_bound(keys_.front().generation); + dropped_hashes = keep_from != active_msgs_.begin(); + active_msgs_.erase(active_msgs_.begin(), keep_from); + } else { // Keys is empty, which means we aren't keep *any* keys around (or they are all invalid or // something) and so it isn't really up to us to keep them alive, since that's a history of // the group we apparently don't have access to. + dropped_hashes = !active_msgs_.empty(); active_msgs_.clear(); + } // Retained message bytes follow the hashes exactly, for both of the above branches, so they // expire on the same schedule as the keys; without this an expired generation's bytes would sit @@ -1243,7 +1252,11 @@ void Keys::remove_expired() { // 177 + 48*N bytes for N members rounded up to a multiple of MESSAGE_KEY_MULTIPLE (see the // arithmetic in keys.hpp), so a large group that rekeys often can carry a sizeable dump. // Bounded and predictable, not necessarily small. - prune_key_msgs(); + // + // Every path that adds bytes adds the hash too, so nothing can be orphaned unless a hash was + // just dropped -- which is the uncommon case, and this runs on every message load. + if (dropped_hashes) + prune_key_msgs(); } bool Keys::needs_rekey() const { From fb5f03048dc7fdc57d32e1b575a02adb0507be92 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 14 Sep 2026 15:40:48 +1000 Subject: [PATCH 8/9] groups::Keys: write back a dump whose retained bytes were pruned on load load_dump() prunes orphaned "C" entries so that a hand-written or corrupted dump cannot seed an entry nothing will ever prune again, but left needs_dump_ false -- so the orphan stayed on disk until something unrelated happened to flag a dump. --- src/config/groups/keys.cpp | 6 ++- tests/test_group_keys.cpp | 104 +++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/src/config/groups/keys.cpp b/src/config/groups/keys.cpp index 2010d489d..36bd54884 100644 --- a/src/config/groups/keys.cpp +++ b/src/config/groups/keys.cpp @@ -197,8 +197,10 @@ void Keys::load_dump(std::span dump) { } // `key_msgs_` must never outlive the hashes in `active_msgs_`; enforce that on the way in too, - // so a hand-written or corrupted dump can't seed an entry that nothing will ever prune. - prune_key_msgs(); + // so a hand-written or corrupted dump can't seed an entry that nothing will ever prune. What + // we just pruned is still on disk, so the cleaned state has to be written back. + if (prune_key_msgs()) + needs_dump_ = true; } bool Keys::prune_key_msgs() { diff --git a/tests/test_group_keys.cpp b/tests/test_group_keys.cpp index c2246f5f0..340ef0892 100644 --- a/tests/test_group_keys.cpp +++ b/tests/test_group_keys.cpp @@ -1449,3 +1449,107 @@ TEST_CASE("Group Keys - one message, several generations", "[config][groups][key to_hex(supp)); CHECK_FALSE(reloaded.keys.active_key_message("keyhash1")); } + +namespace { + +// Rebuilds a Keys dump with an extra "C" entry whose hash is not named in "A" -- the sort of orphan +// a hand-written or corrupted dump can carry, and which nothing would ever prune again. +std::vector add_orphan_retained_message( + std::span dump, std::string_view hash) { + oxenc::bt_dict_consumer d{dump}; + std::string out = "d"; + + REQUIRE(d.skip_until("A")); + out += "1:A"; + out += d.consume_list_data(); + + std::map msgs; + if (d.skip_until("C")) { + auto c = d.consume_dict_consumer(); + while (!c.is_finished()) { + auto [h, v] = c.next_string(); + msgs.emplace(h, v); + } + } + REQUIRE(msgs.emplace(hash, "not a keys message at all").second); + { + oxenc::bt_dict_producer c; + for (const auto& [h, v] : msgs) + c.append(h, v); + out += "1:C"; + out += c.view(); + } + + REQUIRE(d.skip_until("L")); + out += "1:L"; + out += d.consume_list_data(); + if (d.skip_until("P")) { + out += "1:P"; + out += d.consume_dict_data(); + } + out += "e"; + return session::to_vector(out); +} + +} // namespace + +TEST_CASE("Group Keys - orphaned retained messages", "[config][groups][keys][recovery]") { + + const std::vector group_seed = + "0123456789abcdeffedcba98765432100123456789abcdeffedcba9876543210"_hexbytes; + const std::vector admin_seed = + "0123456789abcdef0123456789abcdeffedcba9876543210fedcba9876543210"_hexbytes; + const std::vector member_seed = + "000111222333444555666777888999aaabbbcccdddeeefff0123456789abcdef"_hexbytes; + + std::array group_pk; + std::array group_sk; + crypto_sign_ed25519_seed_keypair(group_pk.data(), group_sk.data(), group_seed.data()); + + pseudo_client admin{admin_seed, true, group_pk.data(), group_sk.data()}; + pseudo_client member{member_seed, false, group_pk.data(), std::nullopt}; + + for (const auto* c : {&admin, &member}) { + auto m = admin.members.get_or_construct(c->session_id); + m.admin = (c == &admin); + admin.members.set(m); + } + + constexpr int64_t t0 = 1'700'000'000'000; + auto rekey1 = session::to_vector(admin.keys.rekey(admin.info, admin.members)); + REQUIRE(admin.keys.load_key_message("keyhash1", rekey1, t0, admin.info, admin.members)); + REQUIRE(member.keys.load_key_message("keyhash1", rekey1, t0, member.info, member.members)); + + SECTION("an orphan in the dump is dropped, and the cleaned dump is written back") { + auto tampered = add_orphan_retained_message(member.keys.dump(), "zorphanhash"); + pseudo_client reloaded{ + member_seed, + false, + group_pk.data(), + std::nullopt, + std::nullopt, + std::nullopt, + tampered}; + + CHECK_FALSE(reloaded.keys.active_key_message("zorphanhash")); + CHECK(reloaded.keys.active_key_message("keyhash1")); + + // Pruning it in memory is only half the job: the orphan is still on disk until something + // asks for a dump, and nothing else here would. + CHECK(reloaded.keys.needs_dump()); + + auto cleaned = reloaded.keys.dump(); + pseudo_client again{ + member_seed, + false, + group_pk.data(), + std::nullopt, + std::nullopt, + std::nullopt, + cleaned}; + CHECK_FALSE(again.keys.active_key_message("zorphanhash")); + CHECK(again.keys.active_key_message("keyhash1")); + // A dump with nothing to prune must not ask to be rewritten on every load. + CHECK_FALSE(again.keys.needs_dump()); + } +} From 7586325e73b8aaad9029df04667aed705ca97fe2 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 14 Sep 2026 16:34:58 +1000 Subject: [PATCH 9/9] groups::Keys: guard the retained-bytes copy in the no-key branch too load_key_message's no-key branch still went through insert_or_assign, re-copying the whole message on every re-delivery, unlike the two insert_key sites. Same hash means same ciphertext, so the copy we already hold is the only one worth keeping. needs_dump_ stays unconditional here: remove_expired() runs on this path and can prune without flagging a dump of its own. --- src/config/groups/keys.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/config/groups/keys.cpp b/src/config/groups/keys.cpp index 36bd54884..1d7146be5 100644 --- a/src/config/groups/keys.cpp +++ b/src/config/groups/keys.cpp @@ -1165,8 +1165,14 @@ bool Keys::load_key_message( // typically. Still worth retaining: it is part of the generation, and a member who gets // only some of a generation's messages doesn't get the key. active_msgs_[*max_gen].emplace(hash); - key_msgs_.insert_or_assign(std::string{hash}, to_vector(data)); + // Same hash means same ciphertext (the storage server derives one from the other) so a + // re-delivery brings bytes we already hold. Keeping the first copy skips re-copying them; + // the keys namespace is re-read in full whenever a device is missing retained bytes, so + // this is not a rare path. + if (auto key = std::string{hash}; !key_msgs_.contains(key)) + key_msgs_.emplace(std::move(key), to_vector(data)); remove_expired(); + // Unconditional: `remove_expired()` above can prune without flagging a dump of its own. needs_dump_ = true; }