Bincode fixes #54
Open
insipx wants to merge 8 commits into
Open
Conversation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Critical
openmls/openmls/src/extensions/mod.rs
Line 138 in 8459a2e
The ImmutableMetadata variant is missing from ExtensionTypeId at line 272 and from EXTENSION_TYPE_VARIANTS at line 138, so any serialized data containing ExtensionType::ImmutableMetadata fails to deserialize with an unknown variant error.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file openmls/src/extensions/mod.rs around line 138:
The `ImmutableMetadata` variant is missing from `ExtensionTypeId` at line 272 and from `EXTENSION_TYPE_VARIANTS` at line 138, so any serialized data containing `ExtensionType::ImmutableMetadata` fails to deserialize with an unknown variant error.
Evidence trail:
openmls/src/extensions/mod.rs line 119: ImmutableMetadata variant in ExtensionType enum (unconditional). Lines 138-148: EXTENSION_TYPE_VARIANTS missing ImmutableMetadata. Lines 197-206: ExtensionTypeId enum missing ImmutableMetadata. Lines 150-183: Serialize impl match missing ImmutableMetadata arm. Lines 225-243: visit_u64 missing ImmutableMetadata. Lines 245-258: visit_str missing ImmutableMetadata. Lines 275-303: visit_enum missing ImmutableMetadata.
This avoids /library/std/src/sys/time/unsupported.rs:35:9:
time not implemented on this platform
`ProposalType`, `Proposal`, `Extension`, `ExtensionType`, and `CredentialType` all gained or lost variants since openmls-0.7.1 in positions that shifted the serde variant index. Self-describing formats (JSON, CBOR) deserialize enum variants by name and are unaffected, but non-self-describing formats like bincode and postcard decode the variant tag as a positional `u32`. A persisted `Proposal::Custom`, `Proposal::SelfRemove`, `Extension::Unknown`, `ExtensionType::Unknown`, or `CredentialType::Other` written by openmls-0.7.x deserializes as some other variant on the current main branch, silently corrupting state. Restore the original variant order and append all new variants (`Grease`, `AppEphemeral`, `AppDataUpdate`, `AppDataDictionary`, `ImmutableMetadata` where applicable, plus `Grease` for `CredentialType`) at the end of each enum. For the removed `AppAck` variant on `Proposal` and `ProposalType`, introduce a hidden `_AppAck` placeholder at index 7 (with `#[serde(rename = "AppAck")]` to preserve the JSON variant name) so subsequent variant indices stay stable. `AppAck` was never produced by the library so no real persisted data round-trips through this variant. Match arms in `messages/codec.rs`, `messages/proposals_in.rs`, and `group/mls_group/proposal_store.rs` updated accordingly.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. An unresolved critical review comment identifies a missing You can customize Macroscope's approvability policy. Learn more. |
…ucts `MessageSecrets.added_at` and `MemberStagedCommitState.application_export_tree` were added as trailing fields with `#[serde(default)]` after openmls-0.7.x. `#[serde(default)]` is honored by self-describing formats but not by positional formats like bincode, where the deserializer requires the field's bytes to be present. A group persisted by openmls-0.7.x fails to deserialize its `MessageSecretsStore` or its pending `MemberStagedCommitState` on current main because bincode hits EOF when trying to read the new tail field. Replace the derived `Serialize`/`Deserialize` impls with hand-written visitors that read the trailing field via `SeqAccess::next_element` and treat any failure (EOF from bincode, `Ok(None)` from formats that signal end of sequence) as "field absent, default to None". Persisted data that already contains the field round-trips its value unchanged; 0.7.x data that predates the field loads with the field as None. `visit_map` retains the standard `serde(default)` semantics for self-describing formats. Both fields are positioned as the last field of their struct, and each struct is the last field of its containing record (`MessageSecrets` is the final field of `MessageSecretsStore`; `MemberStagedCommitState` is the single payload of `StagedCommitState::GroupMember`), so the EOF-tolerant fallback is safe — there are no following bytes to misinterpret.
`usize` is 32 bits on `wasm32` and 64 bits on most host targets. Serde's default `usize` impl already routes through `serialize_u64` / `deserialize_u64` so common formats (JSON, bincode, postcard) produce identical wire bytes on both platforms — but PR openmls#1990's hand-written serializer for `PastEpochDeletionPolicy::KeepAll` wrote `usize::MAX as u64`, which differs by platform: * wasm32: `usize::MAX as u64 == u32::MAX == 4_294_967_295` * 64-bit: `usize::MAX as u64 == u64::MAX == 18_446_744_073_709_551_615` The deserializer compounded the problem by calling `usize::try_from(v)` before checking the sentinel, so a `KeepAll` policy written by a 64-bit host failed to deserialize at all on `wasm32` (since `u64::MAX` does not fit in a 32-bit `usize`). New code emits `u64::MAX` unconditionally and checks the sentinel *before* narrowing to `usize`, so cross-architecture group state with `KeepAll` now round-trips cleanly. Legacy `wasm32`-written `KeepAll` (encoded as `u32::MAX`) loads as `MaxEpochs(u32::MAX)` — functionally equivalent to "keep all" given the practical ceiling on past-epoch counts — so existing wasm32 data is not broken by this change. Defensively annotate every remaining persisted `usize` field with a new `usize_as_u64` serde adapter so any future format change cannot re-introduce platform-native encoding by accident: * `MlsGroupJoinConfig.padding_size` * `MlsGroupJoinConfig.number_of_resumption_psks` * `MessageSecretsStore.max_epochs` * `ResumptionPskStore.max_number_of_secrets` * `ResumptionPskStore.cursor` * `Pprf.width` These fields are bookkeeping (cache caps, ring-buffer cursor, message padding length, PPRF tree width); none feed key derivation, which is sized exclusively by ciphersuite constants and TLS-encoded length-prefixed byte slices.
The five persisted enums fixed by the previous commit (`CredentialType`, `ExtensionType`, `Extension`, `ProposalType`, `Proposal`) currently rely on the *declaration order* in source to determine their serde `variant_index`, because serde's derive macro uses `iter().enumerate()` to assign indices positionally — Rust's explicit discriminant syntax (`Basic = 1`) is ignored by the derive. That convention is fragile: a future PR that adds a variant in the middle of one of these enums silently shifts every following index and reintroduces the bincode/postcard wire-format corruption this branch just fixed. The documentation comment helps, but a one-line review miss is all it takes. Replace the derived `Serialize`/`Deserialize` impls with hand-written ones that pin each variant to an explicit `variant_index` via `serialize_unit_variant` / `serialize_newtype_variant` / `serialize_tuple_variant`. The wire bytes are byte-identical to the previous positional encoding — these impls produce the same calls the derive used to emit, just with the indices spelled out as numeric literals at the call site. The matching `Deserialize` impls use the standard `deserialize_enum` + `EnumAccess` + variant-identifier visitor pattern, accepting both u64 indices (for bincode/postcard) and variant name strings (for JSON/CBOR). The `_AppAck` placeholder on `Proposal` and `ProposalType` keeps its historical `"AppAck"` JSON name via the explicit string passed to `serialize_unit_variant`, replacing the previous `#[serde(rename = "AppAck")]` attribute on the variant. Future maintainers adding a variant to any of these enums must pick a new, never-reused index and add it to both the `Serialize` match and the variant-identifier visitor. The compiler's exhaustiveness check enforces that omissions are caught.
Add unit tests that pin the `(variant_index, variant_name)` pair emitted by
the manual `Serialize` impls for `CredentialType`, `ExtensionType`,
`Extension`, `ProposalType`, and `Proposal`. The variant index is the
bincode/postcard wire encoding of the enum tag; the variant name is the
JSON/CBOR encoding. Either drifting silently corrupts persisted group state
across openmls versions.
The mechanism is a minimal `serde::Serializer` (`utils::variant_index_probe`)
whose `Ok` type is `(u32, &'static str)` and whose only working methods are
`serialize_unit_variant`, `serialize_newtype_variant`, and
`serialize_tuple_variant`. They capture the `variant_index` and `variant`
arguments and return them directly; every other method returns an error so
misuse fails loudly. The probe never recurses into payloads, so the tests
construct each variant with the cheapest payload that satisfies its type.
For the tagged-union enums (`Extension`, `Proposal`), some payload variants
(those holding `Box<...Proposal>` with cryptographic content) are exercised
indirectly through the corresponding `ProposalType`/`ExtensionType` test
rather than directly, to avoid pulling key-package construction into the
test. The variants that historically drifted (the `_AppAck` placeholder,
`SelfRemove`, and `Custom`) are each pinned explicitly.
When adding a new variant to any of these enums:
* pick a brand-new `variant_index`, never reuse an existing one;
* append a `check(...)` line to the corresponding test;
* the compiler's exhaustive-match check on the `Serialize` impl will catch
omissions in the production code, and the test will catch omissions in
the wire-format documentation.
insipx
force-pushed
the
insipx/bincode-fixes
branch
5 times, most recently
from
af52a36 to
29040d1
Compare
insipx
force-pushed
the
insipx/bincode-fixes
branch
from
29040d1 to
624e59f
Compare
insipx
force-pushed
the
insipx/bincode-fixes
branch
from
15e59c7 to
e125674
Compare
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
2 participants
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.
Fix bincode/serde wire format stability for proposals, extensions, credentials, and usize fields
Serialize/DeserializeonProposal,ProposalType,Extension,ExtensionType, andCredentialTypewith manual impls that use fixed variant indices and names, stabilizing the bincode/postcard wire format._AppAckplaceholder variant toProposalandProposalTypeto preserve serde variant index ordering without exposing the type publicly; TLS codec handles it with explicit error or no-op arms.usizefields (padding_size,number_of_resumption_psks,cursor,width) asu64via a newusize_as_u64serde helper inutils.rsfor cross-arch portability.PastEpochDeletionPolicyto serializeKeepAllasu64::MAXandMaxEpochsasu64, replacing the previous platform-dependentusizeencoding.fluvio-wasm-timerwithweb-time(with serde support) for wasm32SystemTimeusage across the codebase.MessageSecrets.added_atout of the derived serde layout;MessageSecretsStoregains a custom tolerant serde impl that appends timestamps as trailing fields, remaining backward-compatible with older serialized data.Proposal,Extension, orCredentialTypewill not deserialize correctly with the new fixed-index format.Macroscope summarized e125674.