From bc51b6659332e5a0456a94afccbb30b90aeebe9e Mon Sep 17 00:00:00 2001 From: "dobby-yivi-agent[bot]" <275734547+dobby-yivi-agent[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 13:49:49 +0000 Subject: [PATCH 1/2] test: give the wire-compat sample set a non-canonical sender identity Every attribute value in the sample set was already canonical, so canonicalization was a no-op across the whole corpus and the gate could not tell canonical-values-on-the-wire from canonicalize-only-inside derive(). It would not have noticed a revert of either. The sender is the only role where a fixture can bite. RecipientHeader ::decaps is handed only the KEM ciphertext and the user secret key, so no reader derives from a recipient policy, and Header::new stores policy.to_hidden(), which blanks the value for any type outside HINT_TYPES. SignatureExt.pol, by contrast, is a full Policy and Unsealer::new derives from it. So: SENDER becomes " Sender@Sample.TEST " (trim and lowercase), the private sender policy gains pbdf.sidn-pbdf.mobilenumber.mobilenumber = "+31 (0)6 1234 5678" next to the existing fullname (the two policies are canonicalized by separate statements in the sealer, so one fixture leaves the other blind), and manifest() records .canonical() of each. The manifest is the expectation readers are checked against, so it promises the canonical form while the sealer is handed the raw one. pg-compat did not check the sender policy at all, while pg-compat-js has since #261; close that asymmetry. Manifest gains a `sender` field deserialized structurally, because each pinned pg-core is a distinct crate with a distinct Policy type. pg-core/tests/sample_sealer.rs had the same hole. Both halves get a gate-teeth case that rewrites the manifest's sender policy back to the raw value and requires a failure naming the mismatch, and the record is corrected in three places: Header::new's canonicalize is load-bearing for the hint a recipient is shown, not for what anyone derives, and the recipient side is not gate-observable. SCHEMA_VERSION, SEED, the five case names and the recipient policies are unchanged, and the non-privsig artifacts stay byte-identical because canonicalize(" Sender@Sample.TEST ") is the old value. Closes #355 Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 2 +- pg-compat-js/test/gate-teeth.test.mjs | 34 ++++ pg-compat/README.md | 16 +- pg-compat/src/lib.rs | 162 +++++++++++++++++- pg-compat/tests/gate_teeth.rs | 173 ++++++++++++++++++++ pg-core/examples/seal-samples/sample_set.rs | 40 ++++- pg-core/src/client/header.rs | 15 +- pg-core/src/identity.rs | 12 +- pg-core/tests/sample_sealer.rs | 32 ++++ 9 files changed, 466 insertions(+), 20 deletions(-) create mode 100644 pg-compat/tests/gate_teeth.rs diff --git a/CLAUDE.md b/CLAUDE.md index 7d6687ec..4aaed1a7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -15,7 +15,7 @@ Migrated from the dobby memory repo (`encryption4all/dobby`). This file is the h - Appending a field at the *end* of `Header` really is additive: the header is a length-prefixed region and `bincode` ignores trailing bytes, so published `pg-core` 0.6.3 still opens it. A field inserted anywhere else, a changed field type, or a reorder shifts every following byte and the containers stop opening, but *not* with a decode error: 0.6.3 reads a garbage length prefix, attempts a ~20 GiB allocation, and the process aborts (SIGABRT). Expect `reader died on signal 6 ... memory allocation of N bytes failed`, not a message naming the header. This is also why `pg-compat` opens each case in a child process (its `pg-compat-case` binary): an abort is not a panic, `catch_unwind` cannot contain it, and in one process the first broken case would take the run down before the others were tried. Don't reason about "additive" from the struct alone; run the compat gate. - **The AEAD plaintext has exactly one additive slot per mode, and the streaming one is not where you would put it** (#347). In memory mode the plaintext is one bincode struct (`MessageAndSignature`), so appending a field at its end is additive for the same reason it is on `Header` — but only for the *sealer*: a reader that decodes the wider struct fails on every container an older sealer wrote, since bincode is positional and has no notion of a missing trailing field. So the reader keeps a two-field prefix struct and uses `bincode_compat::deserialize_with_len` to find where it ended and inspect what follows itself. In streaming mode the first segment is `pol_len ‖ signing_pol ‖ pub_pol ‖ m₀ ‖ sig₀` (`pub_pol` added by #347), and the only additive slot is *inside* the `pol_len` region: the reader drains exactly `POL_SIZE_SIZE + pol_len` bytes and then splits the rest at `len - SIG_BYTES`, so anything appended after `sig₀` is consumed as signature or message bytes and every reader breaks, including new ones. Widening the region works because the reader bincode-decodes one policy out of it and ignores the rest. Whatever goes in there must stay out of the message signature — `sig₀` covers the message only, from `start` past the whole region. **And be clear about what putting something under the AEAD buys: it is authenticated against the DEM key, not against the sender's signing key.** Don't write "an attacker cannot forge this without the AEAD key" in a comment or a PR body; name the attacker the check covers and the one it does not. - **`wasm-pack test --node ./pg-wasm` runs most of the browser suite without a browser.** `pg-wasm/tests/tests.rs` carries `wasm_bindgen_test_configure!(run_in_browser)`, so `--node` reports `this test suite is only configured to run in a browser` and runs nothing; comment that one line out *locally* (never commit it) and Node 22 runs everything that does not touch `web_sys::window()` — `crypto` is a global binding (`pg-core/src/client/web/aesgcm.rs` imports it via `js_namespace = crypto` precisely so web workers work), and `ReadableStream`/`WritableStream` are Node globals, so `helpers.js` works too. What fails is the timing in `mem::test_web_to_web` and `stream::test_web_to_web`, which call `window().expect("no window")`; filter around them (`-- test_seal_unseal_web_to_rust`, `-- test_seal_unseal_rust_to_web`, or your own module name). That covers the web sealer and the web reader against the rust ones. Worth knowing because `Run wasm tests in browsers` is the only CI coverage of the web halves, it flakes (see below), and `dobby-coder` cannot re-run it. -- **Identity attribute values are canonicalized before they are hashed, and the canonical form is Yivi's, not ours** (#250). `Policy::derive` hashes exact value bytes and the PKG builds its `con` from Yivi's `raw_value` verbatim (`pg-pkg/src/middleware/auth.rs`), so a sender's policy only decrypts when its bytes match the disclosure exactly — which is why a capitalized email silently produced an undecryptable container. `pg-core/src/identity.rs` now carries `canonicalize`/`is_canonical` and a `RULES` table (email: trim + lowercase; mobile: drop grouping separators and a `(0)` trunk group, map a leading `00` to `+`). Four things about it are load-bearing. (1) **Both sides apply the same function, so canonicalization is a coarsening of equality** — it can only merge identities that used to differ, never split a pair that already matched, which is why this is a fix rather than a break and why no working container regressed. The one exception is **version skew**, and **no deploy order fixes it** — the obvious "PKG first" rule is wrong. Both directions break on exactly the same condition: a PKG-side value `D` for which `canon(D) != D`. New client + old PKG needs `canon(S) == D`, old client + new PKG needs `S == canon(D)`, and both held before only because `S == D`. Yivi's disclosures are canonical by premise so the IRMA/JWT path has no exposure, but the **API-key path builds its conjunction from hand-entered business-portal fields** (`pg-pkg/src/middleware/auth.rs`, `key_data.email` / `key_data.phone_number`, the latter typed `pbdf.sidn-pbdf.mobilenumber.mobilenumber`, which carries a rule), so a non-canonical `D` is ordinary there. The exposure is transient — it clears once both halves are deployed — and the durable fix is canonicalizing the stored portal data, not sequencing. The **signing** path is the one that *was* order-dependent and is now order-free: `pg-pkg/src/handlers/signing_key.rs` canonicalizes the policy it **returns**, not just the one it derives from, so the returned policy is a fixed point every client and verifier version agrees on. Getting that wrong is subtle and shipped in the first cut of #250 — `derive_ibs` canonicalizes internally, so a handler that returns the raw policy pairs a key for `derive(canon(pol))` with header bytes reading `pol`, and an un-upgraded verifier rejects signatures it accepts today. (2) It runs in **four** places on purpose — `Header::new` for recipient policies, `canonical_signing_key` for the client-side signing policy, `pg-pkg`'s `signing_key.rs` for the policy the PKG hands back, and defensively inside `derive` itself — because the stored policy is what an *un-upgraded* verifier reads back out of the header (`h_sig_ext.pol`), while `derive` is what catches a policy built by hand or assembled by the PKG from a disclosure. That triple application is why **idempotence is a correctness requirement, not a nicety**, and every vector asserts it. (3) The registry is keyed on the type's **tail** (`sidn-pbdf.email.email`), not the full literal, so `pbdf.`, `irma-demo.` and any future scheme match one entry — `HINT_TYPES` in the same file is keyed on full literals and is *missing* its `irma-demo` email row for exactly that reason. Note `test.test.email` (postguard-e2e's keyshare flow) deliberately does not match. (4) `pg-core` is `#![no_std]`, so a national number → E.164 (`0612345678` → `+31612345678`) is **not** implementable here: it needs libphonenumber's metadata database and a country hint. `canonicalize` is total and passes such a value through untouched; `is_canonical` is what reports it, and tb-addon's `libphonenumber-js/mobile` must stay. **The wire-compat gate is blind to all of this** — every value in `pg-core/examples/seal-samples/sample_set.rs` is already canonical, so the whole corpus is a no-op and the gate stays green whichever way the design went; a non-canonical sample would be needed to give it teeth (deliberately not added, see #250's resolution). +- **Identity attribute values are canonicalized before they are hashed, and the canonical form is Yivi's, not ours** (#250). `Policy::derive` hashes exact value bytes and the PKG builds its `con` from Yivi's `raw_value` verbatim (`pg-pkg/src/middleware/auth.rs`), so a sender's policy only decrypts when its bytes match the disclosure exactly — which is why a capitalized email silently produced an undecryptable container. `pg-core/src/identity.rs` now carries `canonicalize`/`is_canonical` and a `RULES` table (email: trim + lowercase; mobile: drop grouping separators and a `(0)` trunk group, map a leading `00` to `+`). Four things about it are load-bearing. (1) **Both sides apply the same function, so canonicalization is a coarsening of equality** — it can only merge identities that used to differ, never split a pair that already matched, which is why this is a fix rather than a break and why no working container regressed. The one exception is **version skew**, and **no deploy order fixes it** — the obvious "PKG first" rule is wrong. Both directions break on exactly the same condition: a PKG-side value `D` for which `canon(D) != D`. New client + old PKG needs `canon(S) == D`, old client + new PKG needs `S == canon(D)`, and both held before only because `S == D`. Yivi's disclosures are canonical by premise so the IRMA/JWT path has no exposure, but the **API-key path builds its conjunction from hand-entered business-portal fields** (`pg-pkg/src/middleware/auth.rs`, `key_data.email` / `key_data.phone_number`, the latter typed `pbdf.sidn-pbdf.mobilenumber.mobilenumber`, which carries a rule), so a non-canonical `D` is ordinary there. The exposure is transient — it clears once both halves are deployed — and the durable fix is canonicalizing the stored portal data, not sequencing. The **signing** path is the one that *was* order-dependent and is now order-free: `pg-pkg/src/handlers/signing_key.rs` canonicalizes the policy it **returns**, not just the one it derives from, so the returned policy is a fixed point every client and verifier version agrees on. Getting that wrong is subtle and shipped in the first cut of #250 — `derive_ibs` canonicalizes internally, so a handler that returns the raw policy pairs a key for `derive(canon(pol))` with header bytes reading `pol`, and an un-upgraded verifier rejects signatures it accepts today. (2) It runs in **four** places, but they are not four of the same thing, and reading them as equally load-bearing for derivation is wrong (corrected in #355). Three are: `canonical_signing_key` for the client-side signing policy, `pg-pkg`'s `signing_key.rs` for the policy the PKG hands back, and defensively inside `derive` itself — the stored *signing* policy is what an *un-upgraded* verifier reads back out of the header (`h_sig_ext.pol`) and derives from, while `derive` is what catches a policy built by hand or assembled by the PKG from a disclosure. The fourth, `Header::new` for recipient policies, is **not**: `derive` canonicalizes internally, so `derive_kem` reaches the same KEM identity with or without it, and what `Header::new` stores is `policy.to_hidden()`, which blanks the value outright for every type outside `HINT_TYPES`. Its only observable effect on the wire is the hint a recipient is *shown* for a hinted type — load-bearing for display, not for derivation. That also makes the **recipient side not gate-observable at all**: `RecipientHeader::decaps` is handed only the KEM ciphertext and the user secret key, so no reader ever derives from a recipient policy, and a non-canonical recipient value is invisible to every reader. Applying the rule at more than one site is why **idempotence is a correctness requirement, not a nicety**, and every vector asserts it. (3) The registry is keyed on the type's **tail** (`sidn-pbdf.email.email`), not the full literal, so `pbdf.`, `irma-demo.` and any future scheme match one entry — `HINT_TYPES` in the same file is keyed on full literals and is *missing* its `irma-demo` email row for exactly that reason. Note `test.test.email` (postguard-e2e's keyshare flow) deliberately does not match. (4) `pg-core` is `#![no_std]`, so a national number → E.164 (`0612345678` → `+31612345678`) is **not** implementable here: it needs libphonenumber's metadata database and a country hint. `canonicalize` is total and passes such a value through untouched; `is_canonical` is what reports it, and tb-addon's `libphonenumber-js/mobile` must stay. **The wire-compat gate had teeth added in #355** — it used to be blind to all of this, because every value in `pg-core/examples/seal-samples/sample_set.rs` was already canonical, making the whole corpus a no-op that stayed green whichever way the design went. The sender policies there now carry deliberately non-canonical values (`" Sender@Sample.TEST "` in the public one, `+31 (0)6 1234 5678` in the private one) while `manifest.json` records the *canonical* form of each, and both halves of the gate compare the recovered sender policy against it. Reverting `canonical_signing_key` therefore makes every published reader fail the header-signature check with `IncorrectSignature`, and dropping `with_priv_signing_key`'s `canonicalize` fails the `*-privsig` cases. Note `cargo test -p pg-core` stays **green** under both reverts, because HEAD's own `derive_ibs` canonicalizes — the failure is only visible to a *published* reader, which is what the fixture is for. The sender is the only role where such a fixture can bite, for the reason in (2). - CI's `Format workspace` matrix runs `cargo fmt --manifest-path /Cargo.toml --all -- --check` once per member directory over shared workspace files; always run `cargo fmt --all -- --check` from repo root before pushing, or one crate's drift fails the whole matrix. - `Run wasm tests in browsers` flakes, and the error names the wrong culprit. `Error: missing field 'chunk'` is `wasm-bindgen-test-runner` failing to parse a truncated webdriver reply; the cause is the line above it, `[SEVERE]: Timed out receiving message from renderer: 30.000`. Read the driver stderr before suspecting the test. The matrix is fail-fast, so one browser timing out reports the other two as failures when they were cancelled: check each job's own conclusion, not the summary. Seen on the same sha passing at 07:45 and failing at 07:48 (runs 30432815599 and 30432995207 on #269, a docs-only commit). Re-run rather than debug, and note that `dobby-coder` cannot: `POST /actions/runs/{id}/rerun-failed-jobs` is 403 for the App (`Resource not accessible by integration`), so a maintainer has to click it, or a fresh push has to supersede the run. `delivery.yml`'s GHCR jobs flake the same way and are worth the same treatment: every one of them starts with a `Log in to GHCR` step, and that step alone has failed twice on #347's PR, once as `denied: denied` and once as `Get "https://ghcr.io/v2/": net/http: request canceled while waiting for connection`, on two different jobs (`Scan cryptify image`, then `Finalize cryptify manifest`) and with the other passing. It is the registry, not the diff — check whether the same job is green on the last few `main` runs before reading a red `delivery.yml` check as yours, and note that a pg-core-only diff cannot reach a Docker job at all. None of these are required contexts; `Wire compat` is. - `scripts/semver-checks.sh` runs `cargo-semver-checks` over the two surfaces external consumers build against: `pg-core` against its crates.io release, and `pg-wasm` against `origin/main` (it has no crates.io release; the npm package is versioned from `pg-core`). The `semver-checks` job in `build.yml` calls it on any PR touching `pg-core`, `pg-wasm`, the root manifest or the script itself; run it yourself too before pushing such a change, since the job needs a wasm32 toolchain and a pinned cargo-semver-checks download and is therefore not the fastest feedback. Four things it encodes. (1) `pg-core` needs `--only-explicit-features --features test,rust,stream`, the same set the test and clippy matrices use: cargo-semver-checks otherwise enables everything that doesn't look unstable, which pulls in `web` and hits its `compile_error!`. (2) `pg-core`'s `web,stream` surface is deliberately not checked. `Unsealer` has two `unseal` methods there on different instantiations (owned `self` in `client/web/mod.rs`, `&mut self` in `client/web/stream.rs`) and cargo-semver-checks 0.49 pairs them by name alone, so it reports `method_receiver_mut_ref_became_owned` against byte-identical source; `rust,stream` is clean because both receivers are owned there. (3) Any wasm32 run needs `RUSTFLAGS=--cap-lints=warn`, because the `--cap-lints allow` cargo-semver-checks sets silences the "dropping unsupported crate type" warnings cargo reads back when probing rustc, and cargo then dies with "output of --print=file-names missing". (4) `cargo-semver-checks` splits its non-zero exits: `100` is a semver violation, `101` is the tool or the build failing (unresolvable baseline rev, missing rustup target, registry fetch failure, compile error in the crate). Never treat "non-zero" as "breaking change" here, because the advice a semver gate prints is "declare the break", and on this repo that means a `!` in the PR title and a spurious major release of `pg-core`. `scripts/semver-checks-test.sh` pins that mapping; it stubs `cargo`, so it runs in well under a second and needs neither cargo-semver-checks nor a wasm32 toolchain. Run it after touching the gate. diff --git a/pg-compat-js/test/gate-teeth.test.mjs b/pg-compat-js/test/gate-teeth.test.mjs index 4c1a2392..3502b1f4 100644 --- a/pg-compat-js/test/gate-teeth.test.mjs +++ b/pg-compat-js/test/gate-teeth.test.mjs @@ -100,3 +100,37 @@ test('a bumped wire version is reported once, before any ciphertext is opened', }, ); }); + +test('a manifest promising the raw sender value is reported, not opened cleanly', async () => { + // `sample_set.rs` hands the sealer this value and the manifest promises + // `canonicalize` of it, so writing it back into the manifest is what a sealer + // that stopped canonicalizing on its way to the wire would have produced. + const RAW_SENDER = ' Sender@Sample.TEST '; + + await withDamagedSet( + async (dir) => { + const path = join(dir, 'manifest.json'); + const manifest = JSON.parse(await readFile(path, 'utf8')); + assert.equal(typeof manifest.sender.public.con[0].v, 'string'); + manifest.sender.public.con[0].v = RAW_SENDER; + await writeFile(path, JSON.stringify(manifest)); + }, + (dir) => { + const failures = runCase(dir, WASM_READER, 'mem'); + assert.ok(failures.length > 0, 'a mismatched sender policy was reported as opening cleanly'); + assert.ok( + failures.every((f) => f.includes('public signing policy is')), + JSON.stringify(failures), + ); + // One per recipient, each naming which one it was. + assert.ok( + failures.some((f) => f.includes('mem/alice')), + JSON.stringify(failures), + ); + assert.ok( + failures.some((f) => f.includes('mem/bob')), + JSON.stringify(failures), + ); + }, + ); +}); diff --git a/pg-compat/README.md b/pg-compat/README.md index ccab2be4..c93344a1 100644 --- a/pg-compat/README.md +++ b/pg-compat/README.md @@ -122,13 +122,19 @@ stream-multi-segment.bin/.plain would drift. - `wireVersion`: the container version the bytes claim (`VERSION_V3`, `2`). - `sender.public`: the policy the sender signed the *header* with, visible to - anyone who has the bytes. This is what a reader checks the header signature - against, so a JS reader needs it as much as a Rust one. + anyone who has the bytes. Recorded in its **canonical** form, while the sealer + is handed a deliberately non-canonical value (`sample_set.rs`'s `SENDER`): that + disagreement is what makes the field a test rather than a copy of the input, so + a canonicalization that stops reaching the wire goes red here. Both halves of + the gate compare the sender policy a reader recovered against this. - `sender.private`: the policy the sender signed the *payload* with in the `*-privsig` cases. Despite the name it is not a secret key; it is the claims a - reader may only see after decrypting. It is present in the manifest for every - set, but only the cases with `privateSigning: true` were sealed with it, so - check it against `privateSigning` rather than against the case list. + reader may only see after decrypting. Canonical for the same reason as + `sender.public`, and non-canonical in the sealer for a different attribute + type, because the sealer canonicalizes the two policies in two separate + statements. It is present in the manifest for every set, but only the cases + with `privateSigning: true` were sealed with it, so check it against + `privateSigning` rather than against the case list. - `mode`: `"memory"` for `Sealer<_, SealerMemoryConfig>::seal` (what pg-wasm's `seal()` produces), `"stream"` for the segmented container (what cryptify stores). pg-js is stream mode in both directions — `toBytes()` seals with diff --git a/pg-compat/src/lib.rs b/pg-compat/src/lib.rs index f5ee41c8..16825719 100644 --- a/pg-compat/src/lib.rs +++ b/pg-compat/src/lib.rs @@ -21,6 +21,7 @@ use std::path::{Path, PathBuf}; use std::process::{Command, ExitStatus, Output}; use serde::Deserialize; +use serde_json::Value; pub mod support_window; @@ -42,10 +43,30 @@ pub struct Manifest { pub wire_version: u16, /// File holding the PKG parameters with the verifying key. pub verifying_key: String, + /// The policies every container in the set was signed under. + pub sender: Sender, /// The sealed containers. pub cases: Vec, } +/// The sender policies the manifest promises, in the canonical form the sealer +/// wrote them in. +/// +/// Held as raw JSON rather than as a `Policy`: every pinned `pg-core` is a +/// distinct crate with a distinct `Policy` type, so naming one of them in the +/// shared manifest would tie it to a single reader. The comparison is +/// structural, through [`describe_policy`]. +#[derive(Debug, Deserialize)] +pub struct Sender { + /// The policy the *header* signature was made under, visible to anyone who + /// has the bytes. + pub public: Value, + /// The policy the *payload* signature was made under in the `*-privsig` + /// cases. Present for every set; only checked for a case whose + /// `privateSigning` is true. + pub private: Option, +} + /// One sealed container plus everything needed to open and check it. #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] @@ -156,6 +177,56 @@ pub fn describe_plaintext_mismatch(got: &[u8], want: &[u8]) -> String { } } +/// A policy as a comparable string: attributes in a fixed order, and a missing +/// value distinguished from an empty one. +/// +/// Mirrors `describePolicy` in `pg-compat-js/src/failures.mjs`, so a mismatch +/// reads the same in either half of the gate. `con` is sorted here because the +/// order a reader hands the conjunction back in is not part of the wire +/// contract, and a reordering must not read as a break. +pub fn describe_policy(policy: &Value) -> String { + let mut con: Vec = policy + .get("con") + .and_then(Value::as_array) + .map(|attributes| { + attributes + .iter() + .map(|a| { + serde_json::json!({ + "t": a.get("t").cloned().unwrap_or(Value::Null), + "v": a.get("v").cloned().unwrap_or(Value::Null), + }) + }) + .collect() + }) + .unwrap_or_default(); + con.sort_by_cached_key(Value::to_string); + + serde_json::json!({ + "ts": policy.get("ts").cloned().unwrap_or(Value::Null), + "con": con, + }) + .to_string() +} + +/// Compare a signing policy a reader recovered against the one the manifest +/// says was signed. +/// +/// Recovering the right plaintext is not enough on its own: `SignatureExt.pol` +/// travels in full and a published reader derives the signer's identity from it, +/// so this is where a canonicalization that stops reaching the wire shows up. +pub fn describe_policy_mismatch(what: &str, got: &Value, want: &Value) -> Option { + let got = describe_policy(got); + let want = describe_policy(want); + if got == want { + return None; + } + + Some(format!( + "{what} signing policy is {got}, manifest says {want}" + )) +} + /// Open one case in a child process and turn its outcome into failure /// messages. /// @@ -268,7 +339,9 @@ macro_rules! reader { use serde::Deserialize; - use crate::{describe_plaintext_mismatch, read_file, Case, Manifest}; + use crate::{ + describe_plaintext_mismatch, describe_policy_mismatch, read_file, Case, Manifest, + }; /// The crates.io version this module reads with. pub const VERSION: &str = $version; @@ -339,6 +412,53 @@ macro_rules! reader { case.private_signing, )); } + failures.extend(check_sender(&label, &verified, manifest, case)); + } + } + } + + failures + } + + /// Hold the recovered sender identity to what the manifest + /// promises. The policies are serialized back to JSON because this + /// module's `Policy` is one pinned crate's type and the manifest is + /// shared by all of them. + fn check_sender( + label: &str, + verified: &VerificationResult, + manifest: &Manifest, + case: &Case, + ) -> Vec { + let mut failures = Vec::new(); + + match serde_json::to_value(&verified.public) { + Ok(got) => failures.extend( + describe_policy_mismatch("public", &got, &manifest.sender.public) + .map(|m| format!("{label}: {m}")), + ), + Err(e) => { + failures.push(format!("{label}: serialize the public signing policy: {e}")) + } + } + + // A `private` the reader did not surface is already reported as + // a presence mismatch by the caller, so only the both-present + // case is left to compare. + if case.private_signing { + if let Some(private) = &verified.private { + match (serde_json::to_value(private), &manifest.sender.private) { + (Ok(got), Some(want)) => failures.extend( + describe_policy_mismatch("private", &got, want) + .map(|m| format!("{label}: {m}")), + ), + (Ok(_), None) => failures.push(format!( + "{label}: recovered a private signing policy, the manifest names \ + none", + )), + (Err(e), _) => failures.push(format!( + "{label}: serialize the private signing policy: {e}", + )), } } } @@ -472,6 +592,46 @@ mod tests { assert!(message.contains("got 0x58, expected 0x64"), "{message}"); } + /// The order a reader hands the conjunction back in is not part of the wire + /// contract, so a reordering must not read as a break. + #[test] + fn a_reordered_conjunction_is_not_a_mismatch() { + let one = serde_json::json!({"ts": 1, "con": [{"t": "a", "v": "1"}, {"t": "b"}]}); + let other = + serde_json::json!({"ts": 1, "con": [{"t": "b", "v": null}, {"t": "a", "v": "1"}]}); + + assert_eq!(describe_policy(&one), describe_policy(&other)); + assert_eq!(describe_policy_mismatch("public", &one, &other), None); + } + + /// The non-canonical sender fixture (#355) turns on exactly this: the raw + /// value and the canonical one must not compare equal. + #[test] + fn a_non_canonical_value_is_reported_with_both_forms() { + let raw = serde_json::json!({"ts": 1, "con": [{"t": "e", "v": " Sender@Sample.TEST "}]}); + let canonical = + serde_json::json!({"ts": 1, "con": [{"t": "e", "v": "sender@sample.test"}]}); + + let message = describe_policy_mismatch("public", &canonical, &raw) + .expect("a raw value must not match its canonical form"); + assert!( + message.starts_with("public signing policy is "), + "{message}" + ); + assert!(message.contains("sender@sample.test"), "{message}"); + assert!(message.contains(" Sender@Sample.TEST "), "{message}"); + } + + /// A missing value is not an empty one: `Attribute.value` is an `Option`, + /// and `to_hidden` blanks a value to `""` rather than dropping it. + #[test] + fn an_absent_value_differs_from_an_empty_one() { + let absent = serde_json::json!({"ts": 1, "con": [{"t": "e"}]}); + let empty = serde_json::json!({"ts": 1, "con": [{"t": "e", "v": ""}]}); + + assert!(describe_policy_mismatch("public", &absent, &empty).is_some()); + } + #[test] fn a_truncated_plaintext_says_so() { let message = describe_plaintext_mismatch(b"abc", b"abcde"); diff --git a/pg-compat/tests/gate_teeth.rs b/pg-compat/tests/gate_teeth.rs new file mode 100644 index 00000000..31161b4c --- /dev/null +++ b/pg-compat/tests/gate_teeth.rs @@ -0,0 +1,173 @@ +//! Does the Rust half of the gate actually go red? +//! +//! `wire_compat.rs` only ever asserts that a good sample set opens, which is an +//! assertion a harness that has stopped reading also satisfies. This test hands +//! a published reader a copy of the set whose `manifest.json` promises the +//! *raw*, non-canonical sender value, and requires a failure naming the +//! mismatch. +//! +//! It is the counterpart of `pg-compat-js/test/gate-teeth.test.mjs`, and it +//! guards the newer of the two assertions: the JS half has compared the +//! recovered sender policy since #261, the Rust half only since #355. + +use std::fs; +use std::path::{Path, PathBuf}; + +use pg_compat::{artifacts_dir, read_manifest, readers, run_case}; + +/// The per-case child. Cargo builds it before this test runs. +const CASE_RUNNER: &str = env!("CARGO_BIN_EXE_pg-compat-case"); + +/// The value `sample_set.rs` hands the sealer. `manifest.json` promises +/// `canonicalize` of it, so writing this back into the manifest is what a +/// sealer that stopped canonicalizing on its way to the wire would have +/// produced. +const RAW_SENDER: &str = " Sender@Sample.TEST "; + +/// A throwaway copy of the sample set for `damage` to rewrite. +/// +/// Under `CARGO_TARGET_TMPDIR` rather than a random temp directory, so a +/// failing run leaves the damaged copy where a reviewer can look at it and no +/// dev-dependency is needed for the naming. +fn with_damaged_set(name: &str, damage: impl FnOnce(&Path), body: impl FnOnce(&Path)) { + let source = artifacts_dir(); + // Fail with read_manifest's "seal the sample set first" message rather than + // an ENOENT from the copy loop below. + read_manifest(&source); + let source = fs::canonicalize(&source).unwrap_or_else(|e| panic!("{}: {e}", source.display())); + + let dir = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(name); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap_or_else(|e| panic!("{}: {e}", dir.display())); + + for entry in fs::read_dir(&source).unwrap_or_else(|e| panic!("{}: {e}", source.display())) { + let entry = entry.unwrap_or_else(|e| panic!("{}: {e}", source.display())); + if entry.path().is_file() { + let to = dir.join(entry.file_name()); + fs::copy(entry.path(), &to).unwrap_or_else(|e| panic!("{}: {e}", to.display())); + } + } + + damage(&dir); + body(&dir); +} + +/// Rewrite `manifest.json` in place. +fn edit_manifest(dir: &Path, edit: impl FnOnce(&mut serde_json::Value)) { + let path = dir.join("manifest.json"); + let raw = fs::read(&path).unwrap_or_else(|e| panic!("read {}: {e}", path.display())); + let mut manifest: serde_json::Value = + serde_json::from_slice(&raw).unwrap_or_else(|e| panic!("parse {}: {e}", path.display())); + + edit(&mut manifest); + + fs::write( + &path, + serde_json::to_vec_pretty(&manifest).expect("serialize the manifest"), + ) + .unwrap_or_else(|e| panic!("write {}: {e}", path.display())); +} + +/// The reader the teeth are checked with: the newest pin, so a failure here is +/// about the assertion rather than about an old release. +fn newest_reader() -> String { + let readers = readers(); + let reader = readers.first().expect("no published readers configured"); + + reader.version.to_string() +} + +fn scratch() -> PathBuf { + let scratch = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join("cases"); + fs::create_dir_all(&scratch).unwrap_or_else(|e| panic!("{}: {e}", scratch.display())); + + scratch +} + +/// The assertion #355 added: a manifest promising the raw sender value must not +/// agree with the canonical policy the container carries. +#[test] +fn a_manifest_promising_the_raw_sender_value_is_reported() { + let version = newest_reader(); + + with_damaged_set( + "raw-sender", + |dir| { + edit_manifest(dir, |manifest| { + let attribute = &mut manifest["sender"]["public"]["con"][0]["v"]; + assert!( + attribute.is_string(), + "the manifest has no sender.public conjunction to rewrite", + ); + *attribute = serde_json::Value::String(RAW_SENDER.to_string()); + }); + }, + |dir| { + let failures = run_case(Path::new(CASE_RUNNER), dir, &scratch(), &version, "mem"); + + assert!( + !failures.is_empty(), + "a manifest promising {RAW_SENDER:?} was reported as opening cleanly", + ); + assert!( + failures + .iter() + .all(|f| f.contains("public signing policy is")), + "{failures:?}", + ); + // One per recipient, each naming which one it was. + assert!( + failures.iter().any(|f| f.contains("mem/alice")), + "{failures:?}" + ); + assert!( + failures.iter().any(|f| f.contains("mem/bob")), + "{failures:?}" + ); + }, + ); +} + +/// The private half is guarded by a separate statement in the sealer +/// (`with_priv_signing_key`), so it needs its own tooth. +#[test] +fn a_manifest_promising_the_wrong_private_sender_value_is_reported() { + let version = newest_reader(); + + with_damaged_set( + "wrong-private-sender", + |dir| { + edit_manifest(dir, |manifest| { + let con = manifest["sender"]["private"]["con"] + .as_array_mut() + .expect("the manifest has no sender.private conjunction to rewrite"); + let attribute = con + .iter_mut() + .find(|a| a["t"] == "pbdf.sidn-pbdf.mobilenumber.mobilenumber") + .expect("sender.private carries no mobile number"); + attribute["v"] = serde_json::Value::String("+31 (0)6 1234 5678".to_string()); + }); + }, + |dir| { + let failures = run_case( + Path::new(CASE_RUNNER), + dir, + &scratch(), + &version, + "mem-privsig", + ); + + assert!( + !failures.is_empty(), + "a manifest promising a non-canonical private value was reported as opening \ + cleanly", + ); + assert!( + failures + .iter() + .all(|f| f.contains("private signing policy is")), + "{failures:?}", + ); + }, + ); +} diff --git a/pg-core/examples/seal-samples/sample_set.rs b/pg-core/examples/seal-samples/sample_set.rs index 5435e398..9e1aa594 100644 --- a/pg-core/examples/seal-samples/sample_set.rs +++ b/pg-core/examples/seal-samples/sample_set.rs @@ -41,7 +41,13 @@ pub const SCHEMA_VERSION: u32 = 1; const TIMESTAMP: u64 = 1_704_067_200; /// The sender identity that signs every sample. -const SENDER: &str = "sender@sample.test"; +/// +/// Deliberately **not** in canonical form: the leading and trailing space and +/// the mixed case exercise both halves of the email rule. The sealer is handed +/// this value while the manifest promises `canonicalize` of it, so a reader that +/// derives the sender identity from the raw header bytes only agrees with the +/// signing key while canonicalization still reaches the wire. +const SENDER: &str = " Sender@Sample.TEST "; /// Filename of the manifest that ties the set together. pub const MANIFEST: &str = "manifest.json"; @@ -169,13 +175,27 @@ fn public_sender_policy() -> Policy { /// The policy the sender signs the payload with in the `*-privsig` cases /// (encrypted, only visible to a recipient who can decrypt). +/// +/// `fullname` carries no canonicalization rule and stays for the +/// different-attribute-type coverage the `*-privsig` cases have always had; the +/// mobile number is the non-canonical half. It covers the *phone* rule rather +/// than the email rule a second time, and picks the `(0)` trunk group on +/// purpose: dropping the parentheses while keeping the `0` yields +/// `+310612345678`, which passes an E.164 shape check and dials nowhere. +/// +/// `canonical_signing_key` guards the public policy and `with_priv_signing_key` +/// canonicalizes this one in a separate statement, so a non-canonical value in +/// only one of the two leaves the other as blind as an all-canonical corpus. fn private_sender_policy() -> Policy { Policy { timestamp: TIMESTAMP, - con: vec![Attribute::new( - "pbdf.gemeente.personalData.fullname", - Some("Sample Sender"), - )], + con: vec![ + Attribute::new("pbdf.gemeente.personalData.fullname", Some("Sample Sender")), + Attribute::new( + "pbdf.sidn-pbdf.mobilenumber.mobilenumber", + Some("+31 (0)6 1234 5678"), + ), + ], } } @@ -334,9 +354,15 @@ fn manifest(cases: &[Case], recipients: &[Recipient]) -> Vec { }, "wireVersion": VERSION_2, "verifyingKey": "vk.json", + // The *canonical* forms, not the raw ones the sealer is handed. The + // manifest is the expectation readers are checked against, so this + // disagreement between what the caller passes and what the manifest + // promises is what makes the non-canonical sender a test rather than + // decoration. Derived with `canonical()` rather than written out, so + // there is no second copy of the value to drift. "sender": { - "public": public_sender_policy(), - "private": private_sender_policy(), + "public": public_sender_policy().canonical(), + "private": private_sender_policy().canonical(), }, "cases": cases, }); diff --git a/pg-core/src/client/header.rs b/pg-core/src/client/header.rs index 48846b74..4f7ad233 100644 --- a/pg-core/src/client/header.rs +++ b/pg-core/src/client/header.rs @@ -141,9 +141,18 @@ impl Header { ) -> Result<(Self, SharedSecret), Error> { // Canonicalize before deriving *and* before storing, so the hidden // policies that go on the wire carry the same values the identities - // were derived from. A reader that predates the canonicalization rule - // then derives the same identity from this header as we did, which is - // what makes the fix reach consumers who never upgrade. + // were derived from. + // + // The KEM identities are unaffected either way: `Policy::derive` + // canonicalizes internally, so `derive_kem` below reaches the same + // identity from a raw policy. What this call changes is what the + // *stored* `HiddenPolicy` says, and `to_hidden` blanks the value of + // every attribute type outside `HINT_TYPES` — so its only observable + // effect on the wire is the hint a recipient is shown for a hinted + // type, not what anyone derives. That also means no wire-compat fixture + // can reach it: a non-canonical recipient value is invisible to every + // reader. The sender side is where a fixture bites, because + // `SignatureExt.pol` is a full `Policy` (see `canonical_signing_key`). let policies: EncryptionPolicy = policies .iter() .map(|(rid, policy)| (rid.clone(), policy.canonical())) diff --git a/pg-core/src/identity.rs b/pg-core/src/identity.rs index 1a706057..6ebe2849 100644 --- a/pg-core/src/identity.rs +++ b/pg-core/src/identity.rs @@ -543,9 +543,15 @@ mod tests { #[test] fn test_policy_canonicalization_reaches_the_wire() { - // `derive` canonicalizing is not enough on its own: the policy the - // sealer stores is what an older verifier reads, so the stored value - // has to move too. + // `derive` canonicalizing is not enough on its own: the *sender* + // signing policy travels in full as `SignatureExt.pol`, and an older + // verifier derives the signer's identity from those bytes, so the stored + // value has to move too. + // + // Recipient policies are the other case and they behave differently: a + // header stores `Policy::to_hidden`, which blanks the value outright for + // any type outside `HINT_TYPES`, so a recipient's value never reaches + // the wire unredacted and no reader derives from it. let mut policy = Policy { timestamp: 1_700_000_000, con: alloc::vec![ diff --git a/pg-core/tests/sample_sealer.rs b/pg-core/tests/sample_sealer.rs index 1dd69cf6..812f1015 100644 --- a/pg-core/tests/sample_sealer.rs +++ b/pg-core/tests/sample_sealer.rs @@ -22,6 +22,7 @@ use pg_core::client::rust::stream::UnsealerStreamConfig; use pg_core::client::rust::UnsealerMemoryConfig; use pg_core::client::Unsealer; use pg_core::consts::VERSION_2; +use pg_core::identity::Policy; use pg_core::kem::cgw_kv::CGWKV; use serde::Deserialize; @@ -34,9 +35,19 @@ struct Manifest { wire_version: u16, #[serde(rename = "verifyingKey")] verifying_key: String, + sender: Sender, cases: Vec, } +/// The sender policies the manifest promises. Unlike `pg-compat`, this test +/// links against the `pg_core` that wrote them, so it can name `Policy` +/// directly. +#[derive(Deserialize)] +struct Sender { + public: Policy, + private: Option, +} + #[derive(Deserialize)] struct Case { name: String, @@ -169,6 +180,27 @@ fn head_reads_back_every_case() { "{}: private signature presence", case.name ); + + // The sender policies travel in full and a reader derives the + // signer's identity from them, so what came back has to be what the + // manifest promises. The sample set signs with deliberately + // non-canonical values while the manifest records their canonical + // form (#355), which is what makes this an assertion about + // canonicalization reaching the wire rather than a tautology. + assert_eq!( + verified.public, m.sender.public, + "{}/{}: public signing policy", + case.name, recipient.id + ); + if case.private_signing { + assert_eq!( + verified.private.as_ref(), + m.sender.private.as_ref(), + "{}/{}: private signing policy", + case.name, + recipient.id + ); + } } } } From 9ccc934249e1785ebb3c145a898740d397390dcf Mon Sep 17 00:00:00 2001 From: "dobby-yivi-agent[bot]" <275734547+dobby-yivi-agent[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 17:29:50 +0000 Subject: [PATCH 2/2] test: sort the conjunction in the JS half too, and correct two claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #367. CLAUDE.md's identity-canonicalization bullet still ended with "cargo test -p pg-core stays green under both reverts". That was true of the tree the ticket was written against, but this PR's own step-3 assertion in head_reads_back_every_case invalidates it: with each canonicalization site reverted alone, cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream exits 101 on mem/alice: public signing policy and mem-privsig/alice: private signing policy respectively. The bullet is what a maintainer reads to decide whether a red Test workspace (pg-core) can be a canonicalization regression, and as written it told them no. describe_policy's doc claimed it mirrors describePolicy so a mismatch reads the same in either half. Two things were wrong. The text differs: serde_json here has no preserve_order, so its object keys come out alphabetically while the JS half emits ts before con. More than cosmetic, the halves disagreed on what a mismatch *is* — this half sorts con and the JS half did not. Until this PR sender.private held one attribute so the order could not differ; it now holds two, and Wire compat is the one required check, so a future reader handing the conjunction back in another order would redden it for something this half rules a non-break. describePolicy now sorts con on the same grounds (code-unit order, not localeCompare, so the order is the same everywhere the gate runs), and the doc says what actually holds. The JS half also only had a gate tooth on sender.public. The sealer canonicalizes the two policies in separate statements, which is the argument the Rust half's second tooth rests on, and it applies verbatim here; the private comparison is additionally the branch behind reader.privateSignatureVisible, so the wasm reader is the only path that reaches it. Adds the mem-privsig tooth and a unit test for the reorder. Both new tests were mutated to confirm they bite: removing verify.mjs's private comparison and removing the new sort each turn exactly one red. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 2 +- pg-compat-js/src/failures.mjs | 18 ++++++++++-- pg-compat-js/test/failures.test.mjs | 12 ++++++++ pg-compat-js/test/gate-teeth.test.mjs | 41 +++++++++++++++++++++++++++ pg-compat/src/lib.rs | 11 ++++--- 5 files changed, 76 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4aaed1a7..b8f3ab0a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -15,7 +15,7 @@ Migrated from the dobby memory repo (`encryption4all/dobby`). This file is the h - Appending a field at the *end* of `Header` really is additive: the header is a length-prefixed region and `bincode` ignores trailing bytes, so published `pg-core` 0.6.3 still opens it. A field inserted anywhere else, a changed field type, or a reorder shifts every following byte and the containers stop opening, but *not* with a decode error: 0.6.3 reads a garbage length prefix, attempts a ~20 GiB allocation, and the process aborts (SIGABRT). Expect `reader died on signal 6 ... memory allocation of N bytes failed`, not a message naming the header. This is also why `pg-compat` opens each case in a child process (its `pg-compat-case` binary): an abort is not a panic, `catch_unwind` cannot contain it, and in one process the first broken case would take the run down before the others were tried. Don't reason about "additive" from the struct alone; run the compat gate. - **The AEAD plaintext has exactly one additive slot per mode, and the streaming one is not where you would put it** (#347). In memory mode the plaintext is one bincode struct (`MessageAndSignature`), so appending a field at its end is additive for the same reason it is on `Header` — but only for the *sealer*: a reader that decodes the wider struct fails on every container an older sealer wrote, since bincode is positional and has no notion of a missing trailing field. So the reader keeps a two-field prefix struct and uses `bincode_compat::deserialize_with_len` to find where it ended and inspect what follows itself. In streaming mode the first segment is `pol_len ‖ signing_pol ‖ pub_pol ‖ m₀ ‖ sig₀` (`pub_pol` added by #347), and the only additive slot is *inside* the `pol_len` region: the reader drains exactly `POL_SIZE_SIZE + pol_len` bytes and then splits the rest at `len - SIG_BYTES`, so anything appended after `sig₀` is consumed as signature or message bytes and every reader breaks, including new ones. Widening the region works because the reader bincode-decodes one policy out of it and ignores the rest. Whatever goes in there must stay out of the message signature — `sig₀` covers the message only, from `start` past the whole region. **And be clear about what putting something under the AEAD buys: it is authenticated against the DEM key, not against the sender's signing key.** Don't write "an attacker cannot forge this without the AEAD key" in a comment or a PR body; name the attacker the check covers and the one it does not. - **`wasm-pack test --node ./pg-wasm` runs most of the browser suite without a browser.** `pg-wasm/tests/tests.rs` carries `wasm_bindgen_test_configure!(run_in_browser)`, so `--node` reports `this test suite is only configured to run in a browser` and runs nothing; comment that one line out *locally* (never commit it) and Node 22 runs everything that does not touch `web_sys::window()` — `crypto` is a global binding (`pg-core/src/client/web/aesgcm.rs` imports it via `js_namespace = crypto` precisely so web workers work), and `ReadableStream`/`WritableStream` are Node globals, so `helpers.js` works too. What fails is the timing in `mem::test_web_to_web` and `stream::test_web_to_web`, which call `window().expect("no window")`; filter around them (`-- test_seal_unseal_web_to_rust`, `-- test_seal_unseal_rust_to_web`, or your own module name). That covers the web sealer and the web reader against the rust ones. Worth knowing because `Run wasm tests in browsers` is the only CI coverage of the web halves, it flakes (see below), and `dobby-coder` cannot re-run it. -- **Identity attribute values are canonicalized before they are hashed, and the canonical form is Yivi's, not ours** (#250). `Policy::derive` hashes exact value bytes and the PKG builds its `con` from Yivi's `raw_value` verbatim (`pg-pkg/src/middleware/auth.rs`), so a sender's policy only decrypts when its bytes match the disclosure exactly — which is why a capitalized email silently produced an undecryptable container. `pg-core/src/identity.rs` now carries `canonicalize`/`is_canonical` and a `RULES` table (email: trim + lowercase; mobile: drop grouping separators and a `(0)` trunk group, map a leading `00` to `+`). Four things about it are load-bearing. (1) **Both sides apply the same function, so canonicalization is a coarsening of equality** — it can only merge identities that used to differ, never split a pair that already matched, which is why this is a fix rather than a break and why no working container regressed. The one exception is **version skew**, and **no deploy order fixes it** — the obvious "PKG first" rule is wrong. Both directions break on exactly the same condition: a PKG-side value `D` for which `canon(D) != D`. New client + old PKG needs `canon(S) == D`, old client + new PKG needs `S == canon(D)`, and both held before only because `S == D`. Yivi's disclosures are canonical by premise so the IRMA/JWT path has no exposure, but the **API-key path builds its conjunction from hand-entered business-portal fields** (`pg-pkg/src/middleware/auth.rs`, `key_data.email` / `key_data.phone_number`, the latter typed `pbdf.sidn-pbdf.mobilenumber.mobilenumber`, which carries a rule), so a non-canonical `D` is ordinary there. The exposure is transient — it clears once both halves are deployed — and the durable fix is canonicalizing the stored portal data, not sequencing. The **signing** path is the one that *was* order-dependent and is now order-free: `pg-pkg/src/handlers/signing_key.rs` canonicalizes the policy it **returns**, not just the one it derives from, so the returned policy is a fixed point every client and verifier version agrees on. Getting that wrong is subtle and shipped in the first cut of #250 — `derive_ibs` canonicalizes internally, so a handler that returns the raw policy pairs a key for `derive(canon(pol))` with header bytes reading `pol`, and an un-upgraded verifier rejects signatures it accepts today. (2) It runs in **four** places, but they are not four of the same thing, and reading them as equally load-bearing for derivation is wrong (corrected in #355). Three are: `canonical_signing_key` for the client-side signing policy, `pg-pkg`'s `signing_key.rs` for the policy the PKG hands back, and defensively inside `derive` itself — the stored *signing* policy is what an *un-upgraded* verifier reads back out of the header (`h_sig_ext.pol`) and derives from, while `derive` is what catches a policy built by hand or assembled by the PKG from a disclosure. The fourth, `Header::new` for recipient policies, is **not**: `derive` canonicalizes internally, so `derive_kem` reaches the same KEM identity with or without it, and what `Header::new` stores is `policy.to_hidden()`, which blanks the value outright for every type outside `HINT_TYPES`. Its only observable effect on the wire is the hint a recipient is *shown* for a hinted type — load-bearing for display, not for derivation. That also makes the **recipient side not gate-observable at all**: `RecipientHeader::decaps` is handed only the KEM ciphertext and the user secret key, so no reader ever derives from a recipient policy, and a non-canonical recipient value is invisible to every reader. Applying the rule at more than one site is why **idempotence is a correctness requirement, not a nicety**, and every vector asserts it. (3) The registry is keyed on the type's **tail** (`sidn-pbdf.email.email`), not the full literal, so `pbdf.`, `irma-demo.` and any future scheme match one entry — `HINT_TYPES` in the same file is keyed on full literals and is *missing* its `irma-demo` email row for exactly that reason. Note `test.test.email` (postguard-e2e's keyshare flow) deliberately does not match. (4) `pg-core` is `#![no_std]`, so a national number → E.164 (`0612345678` → `+31612345678`) is **not** implementable here: it needs libphonenumber's metadata database and a country hint. `canonicalize` is total and passes such a value through untouched; `is_canonical` is what reports it, and tb-addon's `libphonenumber-js/mobile` must stay. **The wire-compat gate had teeth added in #355** — it used to be blind to all of this, because every value in `pg-core/examples/seal-samples/sample_set.rs` was already canonical, making the whole corpus a no-op that stayed green whichever way the design went. The sender policies there now carry deliberately non-canonical values (`" Sender@Sample.TEST "` in the public one, `+31 (0)6 1234 5678` in the private one) while `manifest.json` records the *canonical* form of each, and both halves of the gate compare the recovered sender policy against it. Reverting `canonical_signing_key` therefore makes every published reader fail the header-signature check with `IncorrectSignature`, and dropping `with_priv_signing_key`'s `canonicalize` fails the `*-privsig` cases. Note `cargo test -p pg-core` stays **green** under both reverts, because HEAD's own `derive_ibs` canonicalizes — the failure is only visible to a *published* reader, which is what the fixture is for. The sender is the only role where such a fixture can bite, for the reason in (2). +- **Identity attribute values are canonicalized before they are hashed, and the canonical form is Yivi's, not ours** (#250). `Policy::derive` hashes exact value bytes and the PKG builds its `con` from Yivi's `raw_value` verbatim (`pg-pkg/src/middleware/auth.rs`), so a sender's policy only decrypts when its bytes match the disclosure exactly — which is why a capitalized email silently produced an undecryptable container. `pg-core/src/identity.rs` now carries `canonicalize`/`is_canonical` and a `RULES` table (email: trim + lowercase; mobile: drop grouping separators and a `(0)` trunk group, map a leading `00` to `+`). Four things about it are load-bearing. (1) **Both sides apply the same function, so canonicalization is a coarsening of equality** — it can only merge identities that used to differ, never split a pair that already matched, which is why this is a fix rather than a break and why no working container regressed. The one exception is **version skew**, and **no deploy order fixes it** — the obvious "PKG first" rule is wrong. Both directions break on exactly the same condition: a PKG-side value `D` for which `canon(D) != D`. New client + old PKG needs `canon(S) == D`, old client + new PKG needs `S == canon(D)`, and both held before only because `S == D`. Yivi's disclosures are canonical by premise so the IRMA/JWT path has no exposure, but the **API-key path builds its conjunction from hand-entered business-portal fields** (`pg-pkg/src/middleware/auth.rs`, `key_data.email` / `key_data.phone_number`, the latter typed `pbdf.sidn-pbdf.mobilenumber.mobilenumber`, which carries a rule), so a non-canonical `D` is ordinary there. The exposure is transient — it clears once both halves are deployed — and the durable fix is canonicalizing the stored portal data, not sequencing. The **signing** path is the one that *was* order-dependent and is now order-free: `pg-pkg/src/handlers/signing_key.rs` canonicalizes the policy it **returns**, not just the one it derives from, so the returned policy is a fixed point every client and verifier version agrees on. Getting that wrong is subtle and shipped in the first cut of #250 — `derive_ibs` canonicalizes internally, so a handler that returns the raw policy pairs a key for `derive(canon(pol))` with header bytes reading `pol`, and an un-upgraded verifier rejects signatures it accepts today. (2) It runs in **four** places, but they are not four of the same thing, and reading them as equally load-bearing for derivation is wrong (corrected in #355). Three are: `canonical_signing_key` for the client-side signing policy, `pg-pkg`'s `signing_key.rs` for the policy the PKG hands back, and defensively inside `derive` itself — the stored *signing* policy is what an *un-upgraded* verifier reads back out of the header (`h_sig_ext.pol`) and derives from, while `derive` is what catches a policy built by hand or assembled by the PKG from a disclosure. The fourth, `Header::new` for recipient policies, is **not**: `derive` canonicalizes internally, so `derive_kem` reaches the same KEM identity with or without it, and what `Header::new` stores is `policy.to_hidden()`, which blanks the value outright for every type outside `HINT_TYPES`. Its only observable effect on the wire is the hint a recipient is *shown* for a hinted type — load-bearing for display, not for derivation. That also makes the **recipient side not gate-observable at all**: `RecipientHeader::decaps` is handed only the KEM ciphertext and the user secret key, so no reader ever derives from a recipient policy, and a non-canonical recipient value is invisible to every reader. Applying the rule at more than one site is why **idempotence is a correctness requirement, not a nicety**, and every vector asserts it. (3) The registry is keyed on the type's **tail** (`sidn-pbdf.email.email`), not the full literal, so `pbdf.`, `irma-demo.` and any future scheme match one entry — `HINT_TYPES` in the same file is keyed on full literals and is *missing* its `irma-demo` email row for exactly that reason. Note `test.test.email` (postguard-e2e's keyshare flow) deliberately does not match. (4) `pg-core` is `#![no_std]`, so a national number → E.164 (`0612345678` → `+31612345678`) is **not** implementable here: it needs libphonenumber's metadata database and a country hint. `canonicalize` is total and passes such a value through untouched; `is_canonical` is what reports it, and tb-addon's `libphonenumber-js/mobile` must stay. **The wire-compat gate had teeth added in #355** — it used to be blind to all of this, because every value in `pg-core/examples/seal-samples/sample_set.rs` was already canonical, making the whole corpus a no-op that stayed green whichever way the design went. The sender policies there now carry deliberately non-canonical values (`" Sender@Sample.TEST "` in the public one, `+31 (0)6 1234 5678` in the private one) while `manifest.json` records the *canonical* form of each, and both halves of the gate compare the recovered sender policy against it. Reverting `canonical_signing_key` therefore makes every published reader fail the header-signature check with `IncorrectSignature`, and dropping `with_priv_signing_key`'s `canonicalize` fails the `*-privsig` cases. A revert is caught in `cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream` too — `head_reads_back_every_case` (`pg-core/tests/sample_sealer.rs`) compares the recovered sender policy against the manifest's canonical form, one CI job earlier than the gate. The gate is still what proves a *published* reader rejects it, which is the half HEAD's own `derive_ibs` cannot tell you, since it canonicalizes on both sides of the comparison. The sender is the only role where such a fixture can bite, for the reason in (2). - CI's `Format workspace` matrix runs `cargo fmt --manifest-path /Cargo.toml --all -- --check` once per member directory over shared workspace files; always run `cargo fmt --all -- --check` from repo root before pushing, or one crate's drift fails the whole matrix. - `Run wasm tests in browsers` flakes, and the error names the wrong culprit. `Error: missing field 'chunk'` is `wasm-bindgen-test-runner` failing to parse a truncated webdriver reply; the cause is the line above it, `[SEVERE]: Timed out receiving message from renderer: 30.000`. Read the driver stderr before suspecting the test. The matrix is fail-fast, so one browser timing out reports the other two as failures when they were cancelled: check each job's own conclusion, not the summary. Seen on the same sha passing at 07:45 and failing at 07:48 (runs 30432815599 and 30432995207 on #269, a docs-only commit). Re-run rather than debug, and note that `dobby-coder` cannot: `POST /actions/runs/{id}/rerun-failed-jobs` is 403 for the App (`Resource not accessible by integration`), so a maintainer has to click it, or a fresh push has to supersede the run. `delivery.yml`'s GHCR jobs flake the same way and are worth the same treatment: every one of them starts with a `Log in to GHCR` step, and that step alone has failed twice on #347's PR, once as `denied: denied` and once as `Get "https://ghcr.io/v2/": net/http: request canceled while waiting for connection`, on two different jobs (`Scan cryptify image`, then `Finalize cryptify manifest`) and with the other passing. It is the registry, not the diff — check whether the same job is green on the last few `main` runs before reading a red `delivery.yml` check as yours, and note that a pg-core-only diff cannot reach a Docker job at all. None of these are required contexts; `Wire compat` is. - `scripts/semver-checks.sh` runs `cargo-semver-checks` over the two surfaces external consumers build against: `pg-core` against its crates.io release, and `pg-wasm` against `origin/main` (it has no crates.io release; the npm package is versioned from `pg-core`). The `semver-checks` job in `build.yml` calls it on any PR touching `pg-core`, `pg-wasm`, the root manifest or the script itself; run it yourself too before pushing such a change, since the job needs a wasm32 toolchain and a pinned cargo-semver-checks download and is therefore not the fastest feedback. Four things it encodes. (1) `pg-core` needs `--only-explicit-features --features test,rust,stream`, the same set the test and clippy matrices use: cargo-semver-checks otherwise enables everything that doesn't look unstable, which pulls in `web` and hits its `compile_error!`. (2) `pg-core`'s `web,stream` surface is deliberately not checked. `Unsealer` has two `unseal` methods there on different instantiations (owned `self` in `client/web/mod.rs`, `&mut self` in `client/web/stream.rs`) and cargo-semver-checks 0.49 pairs them by name alone, so it reports `method_receiver_mut_ref_became_owned` against byte-identical source; `rust,stream` is clean because both receivers are owned there. (3) Any wasm32 run needs `RUSTFLAGS=--cap-lints=warn`, because the `--cap-lints allow` cargo-semver-checks sets silences the "dropping unsupported crate type" warnings cargo reads back when probing rustc, and cargo then dies with "output of --print=file-names missing". (4) `cargo-semver-checks` splits its non-zero exits: `100` is a semver violation, `101` is the tool or the build failing (unresolvable baseline rev, missing rustup target, registry fetch failure, compile error in the crate). Never treat "non-zero" as "breaking change" here, because the advice a semver gate prints is "declare the break", and on this repo that means a `!` in the PR title and a spurious major release of `pg-core`. `scripts/semver-checks-test.sh` pins that mapping; it stubs `cargo`, so it runs in well under a second and needs neither cargo-semver-checks nor a wasm32 toolchain. Run it after touching the gate. diff --git a/pg-compat-js/src/failures.mjs b/pg-compat-js/src/failures.mjs index f24a4311..e9aa20d3 100644 --- a/pg-compat-js/src/failures.mjs +++ b/pg-compat-js/src/failures.mjs @@ -105,8 +105,13 @@ function hex(byte) { } /** - * A policy as a comparable string: fixed key order, and a missing attribute - * value distinguished from an empty one. + * A policy as a comparable string: attributes in a fixed order, and a missing + * attribute value distinguished from an empty one. + * + * `con` is sorted because the order a reader hands the conjunction back in is + * not part of the wire contract, and a reordering must not read as a break. + * `describe_policy` in `pg-compat/src/lib.rs` sorts it on the same grounds, so + * both halves of the gate rule the same recovered policy a match. * * @param {{ts: number, con: Array<{t: string, v?: string}>}} policy * @returns {string} @@ -114,7 +119,14 @@ function hex(byte) { export function describePolicy(policy) { if (policy === null || typeof policy !== 'object') return JSON.stringify(policy ?? null); - const con = (policy.con ?? []).map(({ t, v }) => ({ t, v: v ?? null })); + const con = (policy.con ?? []) + .map(({ t, v }) => ({ t, v: v ?? null })) + .sort((a, b) => { + // Not `localeCompare`: the order has to be the same everywhere the gate + // runs, and code-unit order is what the Rust half's byte-wise sort does. + const [x, y] = [JSON.stringify(a), JSON.stringify(b)]; + return x < y ? -1 : x > y ? 1 : 0; + }); return JSON.stringify({ ts: policy.ts, con }); } diff --git a/pg-compat-js/test/failures.test.mjs b/pg-compat-js/test/failures.test.mjs index 2bbd5ab2..e3ba3d84 100644 --- a/pg-compat-js/test/failures.test.mjs +++ b/pg-compat-js/test/failures.test.mjs @@ -86,6 +86,18 @@ test('an attribute with no value is not the same as one with an empty value', () assert.ok(describePolicyMismatch('public', withValue, withoutValue)); }); +test('a conjunction handed back in another order is not a mismatch', () => { + // The order a reader returns the conjunction in is not part of the wire + // contract. `sender.private` carries two attributes, so this is reachable. + const con = [ + { t: 'pbdf.gemeente.personalData.fullname', v: 'Sample Sender' }, + { t: 'pbdf.sidn-pbdf.mobilenumber.mobilenumber', v: '+31612345678' }, + ]; + const got = { ts: 1704067200, con: [con[1], con[0]] }; + const want = { ts: 1704067200, con }; + assert.equal(describePolicyMismatch('private', got, want), null); +}); + test('a policy that matches reports nothing', () => { const policy = { ts: 1704067200, con: [{ t: 'pbdf.sidn-pbdf.email.email', v: 'a@b.test' }] }; assert.equal(describePolicyMismatch('public', policy, structuredClone(policy)), null); diff --git a/pg-compat-js/test/gate-teeth.test.mjs b/pg-compat-js/test/gate-teeth.test.mjs index 3502b1f4..d4789004 100644 --- a/pg-compat-js/test/gate-teeth.test.mjs +++ b/pg-compat-js/test/gate-teeth.test.mjs @@ -134,3 +134,44 @@ test('a manifest promising the raw sender value is reported, not opened cleanly' }, ); }); + +test('a manifest promising the wrong private sender value is reported', async () => { + // The sealer canonicalizes the two sender policies in separate statements + // (`canonical_signing_key` and `with_priv_signing_key`), so a tooth on the + // public one leaves the private one as unguarded as no tooth at all. This is + // also the only path that reaches the `privateSignatureVisible` branch in + // `verify.mjs`, which the wasm reader is alone in taking. + const RAW_MOBILE = '+31 (0)6 1234 5678'; + const MOBILE_TYPE = 'pbdf.sidn-pbdf.mobilenumber.mobilenumber'; + + await withDamagedSet( + async (dir) => { + const path = join(dir, 'manifest.json'); + const manifest = JSON.parse(await readFile(path, 'utf8')); + const attribute = manifest.sender.private.con.find((a) => a.t === MOBILE_TYPE); + assert.ok(attribute, 'sender.private carries no mobile number to rewrite'); + attribute.v = RAW_MOBILE; + await writeFile(path, JSON.stringify(manifest)); + }, + (dir) => { + const failures = runCase(dir, WASM_READER, 'mem-privsig'); + assert.ok( + failures.length > 0, + 'a mismatched private sender policy was reported as opening cleanly', + ); + assert.ok( + failures.every((f) => f.includes('private signing policy is')), + JSON.stringify(failures), + ); + // One per recipient, each naming which one it was. + assert.ok( + failures.some((f) => f.includes('mem-privsig/alice')), + JSON.stringify(failures), + ); + assert.ok( + failures.some((f) => f.includes('mem-privsig/bob')), + JSON.stringify(failures), + ); + }, + ); +}); diff --git a/pg-compat/src/lib.rs b/pg-compat/src/lib.rs index 16825719..27d6d5c7 100644 --- a/pg-compat/src/lib.rs +++ b/pg-compat/src/lib.rs @@ -180,10 +180,13 @@ pub fn describe_plaintext_mismatch(got: &[u8], want: &[u8]) -> String { /// A policy as a comparable string: attributes in a fixed order, and a missing /// value distinguished from an empty one. /// -/// Mirrors `describePolicy` in `pg-compat-js/src/failures.mjs`, so a mismatch -/// reads the same in either half of the gate. `con` is sorted here because the -/// order a reader hands the conjunction back in is not part of the wire -/// contract, and a reordering must not read as a break. +/// `con` is sorted because the order a reader hands the conjunction back in is +/// not part of the wire contract, and a reordering must not read as a break. +/// `describePolicy` in `pg-compat-js/src/failures.mjs` sorts it on the same +/// grounds, so both halves of the gate rule the same recovered policy a match +/// and name the same fields when they don't. The two texts are not identical: +/// `serde_json` here has no `preserve_order`, so object keys come out +/// alphabetically, while the JS half emits `ts` before `con`. pub fn describe_policy(policy: &Value) -> String { let mut con: Vec = policy .get("con")