diff --git a/CLAUDE.md b/CLAUDE.md index 7d6687ec..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 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. 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 4c1a2392..d4789004 100644 --- a/pg-compat-js/test/gate-teeth.test.mjs +++ b/pg-compat-js/test/gate-teeth.test.mjs @@ -100,3 +100,78 @@ 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), + ); + }, + ); +}); + +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/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..27d6d5c7 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,59 @@ 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. +/// +/// `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") + .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 +342,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 +415,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 +595,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 + ); + } } } }