Retain keys-message bytes so an expired GroupKeys can be put back - #123
Open
mpretty-cyro wants to merge 3 commits into
Open
Retain keys-message bytes so an expired GroupKeys can be put back#123mpretty-cyro wants to merge 3 commits into
mpretty-cyro wants to merge 3 commits into
Conversation
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this exists
Config messages have a 30-day TTL, refreshed by the
expirebump clients already piggyback on everypoll. A device offline past the TTL loses its config from the swarm silently. Clients are gaining
detection and re-store for the configs they can rebuild; this PR is the piece libSession has to
provide for the one they cannot.
A
GroupKeysmessage is not reproducible by the device that needs it. It carries an adminsignature, and its junk padding derives from the group secret key with the count structurally enforced
at verification. A member cannot regenerate one, so today an expired keys message means the group is
unrecoverable until an admin happens to rekey.
The way out is to never regenerate it: keep the bytes we received and push those back unchanged.
They land on the same message hash — the storage server hashes the ciphertext — so the re-store is
idempotent and needs no signature the pusher does not have. That is what lets a member repair the
group, which is the point of the change.
What it adds
Keys::active_key_messages()→map<hash, span<const unsigned char>>groups_keys_active_message(conf, msg_hash, &data, &len)— borrowed bytes, followinggroups_keys_pending_config's convention; returns false when we hold nothing for that hash,which means cannot recover this one, not an error
"C"key in the dumpThree decisions worth reviewing
Keyed by hash, not generation. One generation carries the full rekey plus every supplemental
issued against it, and a member that receives only some of them does not get the key. So recovery
must re-store every held message for a generation, not one — which a generation-keyed cache could not
express.
Pruned from
active_msgs_as the single authority, in one place, rather than beside each site thatdrops hashes.
remove_expired()already drops hashes in two branches; duplicating the erase is exactlyhow the two get out of step, and it would leave a third branch — added later — equally exposed. This
also bounds the cache to the same
KEY_EXPIRYwindow as the keys themselves, on disk as well as inmemory, which is the whole size argument.
Dump compatibility is by construction and tested both ways.
"C"sorts between the existing"A"and
"L", andload_dumpreads withskip_until— so an old dump loads under new code and a new dumploads under old code. Both directions have tests rather than a comment; the old-code case is built by
stripping
"C"from a real dump and re-emitting the raw sub-encodings, so it is byte-for-byte what oldcode would have written.
What it deliberately does not do
Retention happens on load, not on create. The device that authors a keys message holds no bytes
for it until it loads its own message back from the swarm — so an admin immediately after a rekey is
the device least able to repair. It converges within a poll cycle, and it is stated at the accessor
because the natural reading is the opposite.
Only messages loaded after this ships are retained. Existing groups are unchanged; a hash returned
by
active_hashes()may legitimately have no bytes behind it. In short: this does not fix currentlybroken groups, it prevents future groups from being broken.
A pre-existing
needs_dump_bug, fixed separatelyinsert_key's early-return path — we already hold this key, but this is a different message carryingit — recorded the new hash in
active_msgs_without settingneeds_dump_, unlike both of theother recording paths.
So the hash was lost on the next restart, and that message stopped having its TTL renewed — 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.
Independent of this feature and separately revertible, but it would also have holed it: the retained
bytes would be dropped with the hash, making that message unrecoverable.
Determinism tests
test_config_determinism.cppis the evidence for a property the clients depend on and that had onlyever been established by reading: re-encrypting an unchanged config reproduces the received message
byte for byte, and a clean push is not a new revision.
Covers the full round trip — push, receive, dump, reload, push — rather than encrypting the same bytes
twice, because recovery happens after a restart and it is the reload path that has to be reproducible.
User configs (protobuf-wrapped), group configs (raw), multipart, and a read-only member reproducing an
admin's bytes from a dump that retained a signature it cannot re-derive.
Two behaviours it pins, both of which caught client bugs: 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 isalways empty — which is expected, not a failure.
The protobuf wrapper additionally carries a golden digest, because every other assertion compares
bytes made within one process: a wall-clock or random field added to the wrapper would break re-store
idempotency while leaving all of them green.
Testing
Full suite 138 cases / 25,756,113 assertions,
[recovery]99 assertions / 2 cases,format.sh -cclean.The pruning is mutation-verified rather than argued: disabling the single
prune_key_msgs()call fails3 assertions across both branches of
remove_expired()— in memory, after a dump/reload, and in thekeys_-empty path.Downstream
The NodeJS and Android wrappers have branches waiting on this; both expose the accessor and neither
compiles until this merges and their pin moves. Nothing else depends on it.