Skip to content

fix: replace a shared key that cannot be an RSA ciphertext - #544

Closed
cconstab wants to merge 1 commit into
trunkfrom
fix/recreate-corrupt-shared-key
Closed

fix: replace a shared key that cannot be an RSA ciphertext#544
cconstab wants to merge 1 commit into
trunkfrom
fix/recreate-corrupt-shared-key

Conversation

@cconstab

@cconstab cconstab commented Jul 30, 2026

Copy link
Copy Markdown
Member

What this fixes

A sender keeps its own copy of the AES key it shares with a recipient — shared_key.<recipient>@<sender>, encrypted to the sender's own public key. If that stored record is damaged (a truncated or interrupted write), get_encryption_key_shared_by_me raises on it and there is no path back: the record is re-read on every send, so every later notify/put to that recipient fails identically until someone deletes it by hand.

Fix

RSA ciphertext is exactly as long as the key, so a stored value of a different length cannot be an RSA ciphertext for our key at all — the record is unusable, not merely undecryptable. It is now treated like a missing key and replaced, which is what the AtKeyNotFoundException branch a few lines above already does for an absent record. create_shared_encryption_key rewrites both copies, so the recipient's is refreshed too.

The check is deliberately narrow. A value of the correct length that still fails to decrypt is far more likely to mean the wrong keys are loaded, and replacing the shared key then would rotate a working key for both parties. That case keeps raising, and a test pins it.

Scope — please read before merging

The same error text (Ciphertext length must be equal to key size) also appears for a completely different reason that this PR does not address, and which callers should be aware of: if a read times out, the abandoned reply stays buffered in the socket and every later command on that connection reads the previous command's answer. A shared-key lookup then returns a non-ciphertext and reports the identical error — but nothing is wrong with the stored record, and reconnecting clears it.

I originally attributed a production incident to a damaged record; a restart cleared it, and I then reproduced the desynchronisation directly (asking for publickey returned the previous shared_key reply). So this PR is scoped to the genuinely-damaged case only, which is reproducible by overwriting the record with a valid-base64 value of the wrong length.

Worth considering separately: execute_command could mark a connection unusable after an abandoned read, so a desynchronised connection cannot be reused. Happy to raise that as its own PR if you agree.

Tests

test/shared_key_recovery_test.py, network-free:

Case Expected
readable record returned unchanged, no replacement
wrong-length record replaced once
correct length, undecryptable (wrong keys) still raises AtDecryptionException
the length check itself wrong length → true; valid → false; undecidable input → false

Diagnosing any of this was only possible because #525 made the underlying exception visible — before that the message ended in a literal e.

A sender keeps its own copy of the AES key it shares with a recipient, encrypted to
its own public key, in shared_key.<recipient>@<sender>. If that record is damaged —
for example a truncated or interrupted write — get_encryption_key_shared_by_me raises

    Failed to decrypt shared_key.<recipient>@<sender> - Ciphertext length must be equal
    to key size.

and because the record is re-read on every send, every later notify or put to that
recipient fails identically and restarting the process does not clear it. Observed in
a long-running deployment, where publishing to one recipient stopped permanently.

RSA ciphertext is exactly as long as the key, so a stored value of a different length
cannot be an RSA ciphertext for our key at all: the record is unusable rather than
merely undecryptable. It is now treated like a missing key and replaced, which is what
the AtKeyNotFoundException branch a few lines above already does.

The check is deliberately narrow. A value of the correct length that still fails to
decrypt is far more likely to mean the wrong keys are loaded, and replacing the shared
key then would rotate a working key for both parties, so that case keeps raising.

Adds network-free tests: a readable record is returned unchanged, a wrong-length record
is replaced, a correct-length undecryptable record still raises, and the length check
itself (including that undecidable input changes nothing).
@cconstab
cconstab marked this pull request as draft July 30, 2026 21:48
@cconstab cconstab changed the title fix: replace a damaged shared key instead of failing every send fix: replace a shared key that cannot be an RSA ciphertext Jul 30, 2026
@cconstab

Copy link
Copy Markdown
Member Author

Closing this in favour of fixing the cause rather than the symptom.

I opened this believing a production incident was a damaged shared_key record. It was not: restarting the app cleared it, which a damaged record cannot explain, and I then reproduced the real cause — after a read times out the abandoned reply stays buffered, so a later command reads the previous command's answer (asking for publickey returned the earlier shared_key reply). The stored record was healthy throughout.

That makes this change actively unsafe: the desynchronised case reports the identical error, so replacing the key would rotate a perfectly good shared key for both parties whenever a read had timed out. I have no evidence of a genuinely damaged record occurring naturally — I could only produce one by overwriting the value myself — so the guard is speculative while the false-positive risk is real.

Replacing it with a PR that discards a connection after an abandoned read, so the symptom cannot arise in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant