fix(libsignal): keep skipped message-key seeds projectable - #1210
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change persists skipped-message key seeds, validates seed-derived material, supports seed-backed legacy projection, rejects derived-only projection, and adds feature-gated v1 round-trip coverage. ChangesSkipped-key seed preservation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant SessionStore
participant LegacySession
Peer->>SessionStore: establish session and store skipped key
SessionStore->>LegacySession: export session to v1
LegacySession->>SessionStore: import v1 session record
Peer->>SessionStore: load skipped key and decrypt message
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| waproto/build.rs | Adds collision-checked descriptor splicing for the local skipped-key seed field and feeds the modified descriptor into code generation. |
| wacore/libsignal/src/protocol/ratchet/keys.rs | Persists a seed beside its derived message-key material while preserving seedless protobuf compatibility. |
| wacore/libsignal/src/protocol/record_components.rs | Uses fixed-width key material and validates that a persisted seed reproduces the derived keys before exposing it for projection. |
| wacore/libsignal/src/protocol/legacy_session.rs | Projects retained seeds into legacy skipped-message keys while continuing to reject genuinely seedless derived material. |
| wacore/libsignal/src/protocol/state/session.rs | Adds compatibility coverage showing older seedless persisted skipped keys still deserialize and decrypt. |
| wacore/libsignal/tests/legacy_session_skipped_keys.rs | Exercises out-of-order delivery, legacy projection and reimport, and eventual skipped-message decryption through public protocol APIs. |
| .github/workflows/main.yml | Runs the feature-gated legacy-session interoperability tests in CI. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["Out-of-order message delivery"] --> B["Derive skipped message key from seed"]
B --> C["Persist seed + cipher key + MAC key + IV"]
C --> D["Reload native session"]
D --> E["Validate seed derives stored key material"]
E --> F["Project seed into legacy v1 session"]
F --> G["Reimport and decrypt skipped message"]
Reviews (5): Last reviewed commit: "fix(libsignal): keep skipped message-key..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/libsignal/src/protocol/record_components.rs`:
- Around line 337-350: Update from_structure in record_components.rs so the
SessionMessageKeyMaterial::Seed path does not accept any valid-length seed
blindly; derive the cipher key, MAC key, and IV from the seed plus index and
verify they match the persisted derived triple before returning. Keep the
existing exact_bytes and invalid index handling, and add a regression test
covering a valid-length seed whose derived values do not match the stored
fields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4bb86b50-45c7-47cd-82a4-110cf6ab3a26
📒 Files selected for processing (10)
.github/workflows/main.ymlwacore/libsignal/src/protocol/legacy_session.rswacore/libsignal/src/protocol/ratchet/keys.rswacore/libsignal/src/protocol/record_components.rswacore/libsignal/src/protocol/state/session.rswacore/libsignal/tests/legacy_session_skipped_keys.rswaproto/build.rswaproto/src/whatsapp.descwaproto/src/whatsapp.desc.sha256waproto/src/whatsapp.proto
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
332927f to
fad650e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
fad650e to
e714a11
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/libsignal/src/protocol/ratchet/keys.rs`:
- Around line 501-516: Update
reloaded_keys_come_from_the_persisted_derived_material so the persisted cipher,
MAC, and IV values differ from MessageKeys::derive_keys(&seed, None, 4), while
retaining a valid seedless serialized record. Keep the existing reload
assertions, ensuring they verify the stored derived material rather than values
that could also result from re-deriving the seed.
In `@wacore/libsignal/src/protocol/state/session.rs`:
- Around line 734-738: Correct the documentation comment at the serialized
RecordStructure field decision point: state that
SessionStructure.Chain.MessageKey.seed is spliced into the descriptor by
waproto/build.rs via LOCAL_FIELDS, not declared in the upstream whatspec proto.
Review the full sentence to preserve the existing explanation that the field is
hand-encoded and deliberately assigned a high number.
In `@wacore/libsignal/tests/legacy_session_skipped_keys.rs`:
- Line 144: Update the registration_id initialization in the legacy session test
to always generate a non-zero value within the valid 14-bit range, using an
inclusive range such as 1 through 0x3FFF or an equivalent mask-plus-one
approach. Keep the generated value compatible with both Bundle::bundle() calls.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d2600b7-0ece-4696-9ab0-21b35da92d0e
📒 Files selected for processing (7)
.github/workflows/main.ymlwacore/libsignal/src/protocol/legacy_session.rswacore/libsignal/src/protocol/ratchet/keys.rswacore/libsignal/src/protocol/record_components.rswacore/libsignal/src/protocol/state/session.rswacore/libsignal/tests/legacy_session_skipped_keys.rswaproto/build.rs
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Merging this PR will degrade performance by 18.1%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | bench_message_key_eviction |
436 KB | 572.5 KB | -23.83% |
| ❌ | Memory | bench_out_of_order_decryption |
13.9 KB | 15.8 KB | -11.94% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/legacy-session-skipped-key-seed (d5feb45) with main (786c635)
Footnotes
-
2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e714a1163c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e714a11 to
7c88cbb
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
A session that skipped a message key could not be exported to the v1 model: v1 expresses a message key as its 32-byte seed, the native format persisted only the derived cipher/mac/iv, and that derivation has no inverse. The round trip was lossy from the first cycle, so importing a v1 record and exporting it back already failed with ChainNotRepresentable. The seed now rides along with the keys it derives, in a new local field on SessionStructure.Chain.MessageKey. It is written unconditionally: the persisted format must not vary with build flags, or a binary without legacy-session-interop would erase what one with it wrote. The derived triple stays, so a build that ignores the seed still loads the record and decrypts; only the export path reads it. The field is declared in build.rs and spliced into the descriptor rather than added to whatsapp.proto, which is regenerated wholesale from whatspec and would lose it on the next sync. Field number 100, because upstream appends low numbers without warning - kyberPreKeyId = 4 and kyberCiphertext = 5 landed on SessionStructure.PendingPreKey that way - and a collision would silently reinterpret the seed in every record already on disk. The splice now fails the build on such a collision instead of resolving it. SessionMessageKeyMaterial carries fixed-width arrays instead of Vec<u8>. The lengths were already invariants enforced at every conversion; in the type they cost no allocation and no runtime check. Writing the four persisted fields out of one buffer keeps the skipped-key path at a single allocation, down from three before this change.
7c88cbb to
d5feb45
Compare
1 participant
Summary
SessionRecord::into_legacy_session_v1_operational()failed withChainNotRepresentable { field: DerivedMessageKey }for any session holding a skipped message key. The v1 model expresses a message key as its 32-byte seed; our native format persisted only the derived cipher key / MAC key / IV, and that derivation is one-way. The round trip was lossy from the very first cycle: importing a v1 record with a skipped key and immediately exporting it back already failed. Skipped keys come from out-of-order delivery (counter > chain_index), so this hit exactly the sessions that had fallen behind.The seed now rides along with the keys it derives, in a new field on
SessionStructure.Chain.MessageKey. It is additive, not a replacement:MessageKeyGenerator::from_pbstill requires cipher+mac+iv, so a build that ignores the seed loads the record and decrypts normally. Only the export path reads it. Three places had to change together, since the seed was dropped ininto_pb, had nowhere to live in the schema, and was rebuilt asDerivedon every read.Changes
build.rsand spliced into the descriptor, not added towhatsapp.proto. That file andwhatsapp.descare regenerated wholesale from whatspec, so a field written there would be lost on the next sync and would conflict on every regeneration.LOCAL_FIELDSinbuild.rsholds it instead,apply_local_fieldssplices it into the decodedFileDescriptorSet, and codegen reads the spliced copy fromOUT_DIR. The whatspec-owned files are untouched by this PR.kyberPreKeyId = 4andkyberCiphertext = 5landed onSessionStructure.PendingPreKeyexactly that way — and a collision would silently reinterpret the seed in every record already on disk. 100 follows the rangelocal_field.rsalready reserves. The splice now fails the build if a sync lands on the number or the name, rather than resolving the race quietly.legacy-session-interop. The feature decides what is exportable, not what is persisted; gating it would make the on-disk format vary with build flags, and a binary without the feature would erase seeds written by one with it.MessageKeyGenerator::into_pbemits the seed, and theMessageKeyliteral stays exhaustive so a field removed by a future sync is a compile error rather than a silent drop. The four fields are now written out of one sharedBytesbuffer, so the skipped-key path does two allocations where it used to do three (the buffer, plus the refcount block the firstsplit_topromotes).MessageKeyGenerator::Keys(MessageKeys)removed. It was never constructed anywhere in the workspace — every skipped key comes fromstep_with_message_keys→new_from_seed→Seed. Keeping it would have left one variant able to produce a seedless key with nothing deciding what that means. Breaking for anyone matching on the enum; construct vianew_from_seed.SessionMessageKeyComponents::from_structurereturnsSeedwhen the field is present,Derivedwhen it is not, and verifies that the seed actually derives the triple stored beside it. The seed supersedes that triple on export, so one that reproduces something else would hand a consumer a key decrypting nothing, with no other signal. Both are written from the same material, so disagreement means a corrupt record and the projection fails closed — the same policy the projection already applies toDerivedmaterial.SessionMessageKeyMaterialcarries fixed-width arrays ([u8; 32]/[u8; 16]) instead ofVec<u8>. Those lengths were already invariants checked at every conversion; in the type they cost no allocation and no runtime check, andinto_structurestops being fallible. Breaking for consumers constructing or matching the enum.SessionMessageKeyMaterial's contract changed. The doc-comment used to promise "Exported records always useDerived" and no longer holds. Breaking: a record whose skipped keys retained their seed now exportsSeed. Migration: match both variants.Derivedis still what comes back for keys persisted before this change.project_chain_partsuses an exhaustivematch. It decided withmatches!and closed withunreachable!, so a newSessionMessageKeyMaterialvariant would have compiled without anyone deciding its v1 form. The refusal for seedless material is unchanged.RESERVED_SENDER_CHAIN_INDEX_FIELD's comment corrected. It claimed the upstream proto cannot carry local fields at all; that is now half true — they exist, just not in the checked-in.proto— so the comment says why that field is still written by the hand-rolled encoder.whatsapp.protono longer describes everything on disk, and it cannot say so itself since it is overwritten on every sync, so the pointer toLOCAL_FIELDSgoes in thebuild.rsmodule doc and in theAGENTS.mdgotchas.legacy-session-interop. The feature is off by default and no existing job enabled it for a test run, so every test inlegacy_session.rswas compiled away and never executed — including the one this PR had to change. Slightly outside the stated scope, but the new tests protect nothing without it.Release notes
waproto/build.rssetspreserve_unknown_fields(false), so a previous binary that reads a record and writes it back drops the seed silently. The affected keys go back to being unexportable; nothing corrupts and decrypt is unaffected.Cost
A saturated receiver chain (
MAX_MESSAGE_KEYS= 2000 skipped keys) serialized as aSessionRecord: 182211 → 252211 bytes, +70000 (+38%). Per skipped key that is +35 bytes — 32 of seed plus a 2-byte tag and a 1-byte length — against ~91 bytes before. The realistic case is single-digit skipped keys per chain, so a few hundred bytes; the saturated figure is the ceiling, and it only applies to a chain that hit the 2000-key cap.In memory, CodSpeed measures the same trade in both benchmarks that retain skipped keys: fewer allocation calls, more bytes.
bench_message_key_evictionbench_out_of_order_decryptionThe bytes are the design, not a surprise.
size_of::<MessageKey>()goes from 104 to 136 — the fourthOption<Bytes>costs 32 — and the seed itself is another 32 on the heap, so 64 bytes per retained skipped key. The eviction benchmark pre-fills 1999 keys and inserts 200, leaving ~2199 live: 2199 x 64 = 140736 predicted against 139696 measured, 0.7% off. The out-of-order benchmark holds ~30 skipped keys: 30 x 64 = 1920 against 1936 measured.Alloc calls fell by exactly 200 in a benchmark that inserts exactly 200 keys — one fewer per key written. Two is the floor for four views over one buffer; independent
Byteswould be four allocations for the same 112 bytes.from_structurenow allocates nothing at all for key material, where it used to allocate per field.That benchmark saturates
MAX_MESSAGE_KEYSby construction, so +31% is the ceiling. At the realistic handful of skipped keys per chain it is a few hundred bytes, and the alternatives are the ones already ruled out above: storing only the seed breaks downgrade, and gating on the feature makes the on-disk format vary with a build flag.Validation
New coverage:
a_skipped_key_survives_a_full_v1_projection_cycle(integration): establish a real session, cycle it through the v1 model, provoke a skipped key by delivering Bob's second message before his first over the realmessage_encrypt/message_decryptAPIs, cycle through v1 again, then decrypt the skipped message from the reimported record. Verified to fail withChainNotRepresentable { chain: 2, field: DerivedMessageKey }when the seed write is reverted.a_retained_skipped_key_projects_back_to_its_seed— named regression for the symptom.operational_projection_fails_closed_for_derived_skipped_keys— the contract change called out above. Its old setup (import a v1 record with a seed) now succeeds, which is the whole point of the PR, so it builds a genuinely seedless record instead and still asserts the same typed error.skipped_seed_uses_the_canonical_message_key_derivation— same reason: it asserted that import expands the seed intoDerived. It now checks the persisted structure directly, so it still pins the derivation, and additionally asserts the seed survives.seedless_persisted_message_keys_still_load_and_decrypt,reloaded_keys_come_from_the_persisted_derived_material,a_persisted_key_without_a_seed_projects_as_derived— read compatibility with records written before this change.a_persisted_seed_that_derives_other_keys_is_rejected,a_malformed_persisted_seed_is_rejected,from_pb_still_rejects_a_key_without_derived_material— failure paths.invalid_seed_length_is_rejecteddeleted: it constructed a 31-byteSeedto assert it was refused, which the fixed-width type no longer allows. The equivalent check on persisted input lives ina_malformed_persisted_seed_is_rejected.The build-time collision guard was checked by pointing the local field at an occupied number and confirming the build fails with the conflicting upstream field named.
Full matrix left to CI.