feat(cryptify): mint an upload challenge and verify it at finalize

[dobby-coder]

Closes #364. Part of #338.

What this does

upload_init mints 32 random bytes, stores them on the session and returns them hex-encoded as challenge beside max_chunk_size_bytes. upload_finalize reads an optional base64 X-PostGuard-Proof header and reduces it to a SenderClaim.

Before this, upload_init asked nothing of the uploader and upload_finalize read a sender identity out of the container and treated it as theirs. cryptify holds no decryption key and never opens an upload, so nothing inside the container can say who uploaded it. The proof is what binds the uploader to the identity their container claims.

Against the shipped pg-core API

Built on pg_core::challenge::verify_challenge from #368, not the sketch in the ticket body:

• the context is the uuid, passed as &str;
• the challenge is hex-decoded before verifying, so the signer signs the same bytes signChallenge(pubSignKey, uuid, challengeBytes) does in postguard-js#239;
• there is no error branch. false is the Unproven arm;
• no message construction happens here, and the domain separator is never passed in.

The signature bytes on the wire are bincode_compat::serialize of the signature, exactly SIG_BYTES long, which is what pg-wasm's signChallenge returns. decode_proof insists on that length, so extra bytes cannot ride along on a valid proof (the same check js_verify_challenge makes).

Two decisions worth your eye

sender_claim: Option<SenderClaim>, not a defaulted SenderClaim. The ticket asks that the claim be written exactly once and that no code path other than the verification produce a Proven. None means finalize has not run, exactly as sender already works; Some(Unproven) means it ran and settled the question. Init writes no SenderClaim at all.

Proven carries canonical values. derive_ibs canonicalizes, so a container spelling the address Alice@Example.COM verifies under a key issued for alice@example.com. The raw spelling is therefore not what the proof pinned, and putting it inside Proven would store an uploader-chosen string there. The claim is read off pub_id.canonical(). state.sender keeps the raw spelling, untouched, per the scope fence.

Say the word if you would rather have the raw spelling in the claim and I will change it.

Persistence and the migration

FileState gains challenge and sender_claim, so upload_sessions gains two nullable columns. CREATE TABLE IF NOT EXISTS leaves an existing table alone, so editing the create statement would be a silent no-op on every deployed database. StateDb::migrate_sessions ALTER TABLEs the missing columns on at startup, driven off pragma_table_info('upload_sessions') rather than a stored schema version, so there is no version counter to fall out of step with the columns. Both columns are nullable because ADD COLUMN cannot add NOT NULL without a default, and a row written before the column existed genuinely has no value: a restored session with no challenge finalizes as Unproven however it is presented.

Test 3 red, as asked

Making the verification ignore the derived identity (if !verify_challenge(...) becomes if false) turns a wrong-key signature into a Proven sender:

test integration::a_proof_from_another_identity_leaves_the_sender_unproven ... FAILED
  left: Some(Proven { email: "bob@example.com", attrs: [("pbdf.gemeente.personalData.name", "Bob")] })
 right: Some(Unproven)

test result: FAILED. 203 passed; 3 failed

The other two that went red with it are the wrong-challenge and wrong-context tests, which is what those two are for. Reverted.

Acceptance check

cargo test -p cryptify --all-targets   208 passed
cargo fmt --all -- --check             clean
cargo clippy -p cryptify --all-targets -- -D warnings   clean
cargo test --workspace                 all targets green

The oasdiff gate on cryptify/api-description.yaml reports no breaking changes at the flags CI uses, and with the engine installed locally the mutation test that normally skips in CI runs and passes too:

oasdiff breaking … --fail-on WARN --include-checks response-non-success-status-removed,response-property-enum-value-removed
  No breaking changes to report, but the specs are different.

api_gate_tests::the_api_gate_stops_breaking_changes_and_passes_additive_ones ... ok
api_gate_tests::every_api_gate_mutation_still_applies ... ok
api_gate_tests::the_workflow_uses_the_settings_this_module_pins ... ok

CI on the branch: PR Title, API diff, Continuous integration and Delivery all green.

New tests: init serves a fresh challenge and stores it; a retried finalize does not erase a proof and still upgrades an unproven one; a valid signature respelled in base64url proves nothing; a correct signature proves the sender and the claim names the container's identity; a signature from another identity, over another challenge, bound to another uuid, malformed, or absent each leave it Unproven and none refuses the upload; the challenge and both claim states survive a restart; a upload_sessions table without the columns gains them and keeps its rows; the migration is a no-op on a second boot; plus unit tests for hex_to_bytes and decode_proof.

Also run against the real binary

Booted target/debug/cryptify against the production PKG parameters endpoint, so this is the shipped launch path rather than a test harness:

• POST /fileupload/init answered "challenge": "3c9d749f…", 64 hex chars decoding to 32 bytes, and SQLite held that same value with sender_claim NULL.
• Dropped both columns to make the file look like a deployed pre-migration database and rebooted. The log shows Added column challenge to upload_sessions, Added column sender_claim to upload_sessions, then Restored 1 upload session(s). The old row came back with challenge NULL and a fresh init wrote its challenge into the migrated column.
• A finalize carrying a junk X-PostGuard-Proof reached the unsealer, so the header extractor takes it and the proof path costs the request nothing.

Review round 1

One bug and three nits, all addressed in 3fa1253.

The bug: a retried finalize erased a proof. Nothing marks a session finalized, so upload_finalize stays reachable while the session lives, and the claim was assigned unconditionally — a second finalize with no header recomputed Unproven and persist_session wrote that over a stored Proven. Both ways in are ordinary: a client retrying after a lost response, and a client resuming after a refresh, which no longer holds the challenge and so cannot rebuild the header at all. The assignment is now guarded, so Unproven to Proven still upgrades and Proven never degrades; only the verification produces a Proven, so nothing new can prove a sender. a_retried_finalize_does_not_erase_a_proof covers both directions, and was red before the guard:

left: Some(Unproven)
right: Some(Proven { email: "bob@example.com", attrs: [("pbdf.gemeente.personalData.name", "Bob")] })

The alphabet is now in the wire contract. decode_proof uses base64ct::Base64, so base64url — the reflex in web crypto code — is rejected, and the client half is a separate ticket in another repo. api-description.yaml says so, and a_base64url_proof_leaves_the_sender_unproven pins it: a valid signature respelled in base64url proves nothing. Making decode_proof fall back to Base64Url turns that test red with Some(Proven { .. }), so the alphabet is the only thing rejecting it.

The challenge field now tells the client to persist it, like its max_chunk_size_bytes sibling. GET /fileupload/{uuid}/status does not repeat it, so a client that lost it to a refresh finalizes unproven while still holding the signing key. Putting the challenge on the status response would be a wire-contract change for postguard-js#239 to agree to, so that is not in this PR.

The fourth was a banned word in cryptify/CLAUDE.md, cut. The pre-existing one at :200 is in a bullet this PR does not touch, so it is left for its own sweep.

Scope

No template, email.rs, EN_STRINGS/NL_STRINGS or sender_display change; no accounting-key change; nothing rejected; no config flag; nothing under .github/workflows/. api-description.yaml documents the new response field and header, and cryptify/CLAUDE.md records the migration gotcha and the proof wire format.

One dependency added: base64ct, the crate pg-core already encodes its artifacts with, so Cargo.lock gains only the edge and no new package.

[dobby-coder]

VERDICT: request-changes

Rules Dobby 2, cycle 1

One blocking issue, three non-blocking nits. I reproduced every finding on this HEAD before posting, and verified each suggested fix by applying it rather than by reading it.

What I verified

The blocking one reproduces. A scratch integration test driving two finalizes over real HTTP: the first, carrying a valid proof, returns 200 and stores Some(Proven { email: "bob@example.com", .. }). A second with the header omitted also returns 200 and leaves Some(Unproven). Details in the inline comment on main.rs:1216.

The suggested guard works and passes the gate. With it applied, the second finalize keeps its Proven claim, and the upgrade direction still works: an unproven finalize followed by one carrying a valid proof becomes Proven. On the fix alone, cargo fmt --manifest-path cryptify/Cargo.toml --all -- --check exits 0, cargo clippy -p cryptify --all-targets -- -D warnings is clean, and cargo test -p cryptify --all-targets is 206 passed. No existing test relied on the overwrite. The suggestion block below is what rustfmt emits.

The base64 alphabet nit reproduces too. Over 40 fresh signatures, the standard and base64url spellings differed in all 40 cases, decode_proof accepted all 40 standard ones and none of the 40 url ones. SIG_BYTES is 96, so the encoding is 128 characters with no padding, and unpadded standard base64 is accepted as well. Padding is not at issue; the alphabet is.

Both spec suggestions parse. I applied them to a scratch copy of api-description.yaml and loaded it: valid YAML, and both descriptions read correctly in context.

What the acceptance claims check out as

cargo test -p cryptify --all-targets is 206 passed on this HEAD, matching the PR body. Every CI check on the branch is green, including Format workspace (cryptify), Clippy workspace (cryptify), Test workspace (cryptify) and both API breaking changes jobs.

Rule sweep

Rules checked against this diff: the SQLite column-add and migration-test rules, the pre-existing cryptify Content-Range and synchronous-notification-mail conventions, the AEAD recipient-set and additive-security-fix rules, the one-way-security-switch rule, the Rust fmt/clippy gate, and the prose rules. All clean but one banned word, noted inline on CLAUDE.md:137.

Worth recording that the migration test is the shape the column-add rule asks for: a_sessions_table_without_the_proof_columns_gains_them builds the pre-change table explicitly, seeds a row with data in it, boots, and then checks all three things separately, including that a session written after the migration reaches the new columns. That is the case a fresh-schema test never sees.

The AEAD rule does not bite here. This change adds no copy of any value inside an AEAD plaintext and calls no multi-recipient encapsulation; it verifies an IBS signature over a server-minted random challenge, which is the sender-key binding that rule points at as the real remedy.

Not flipping out of draft, since this is a request for changes.

[dobby-coder]

VERDICT: request-changes

Rules + review sign-off — cycle 2

Re-swept at 3fa1253 (cycle 1 was 2a8a3bf). The cycle-1 blocking finding is fixed and load-bearing: the claim assignment at main.rs:1220 is now guarded, a_retried_finalize_does_not_erase_a_proof drives both directions over real HTTP, and the upgrade path (Unproven -> Proven) still works. The schema migration, the canonicalized claim, and the spec additions all check out.

Verified on HEAD myself: cargo test -p cryptify --all-targets -> 208 passed, 0 failed; cargo fmt --all -- --check clean; cargo clippy -p cryptify --all-targets -- -D warnings clean.

One blocking item remains, and it is the one the suite cannot show you by passing once.

Blocking

• a_base64url_proof_leaves_the_sender_unproven is flaky (main.rs:4375). Measured, not reasoned: 4 failures in 400 runs of this single test. ~1% is often enough to red CI on pushes that have nothing to do with this code. Fix + measurement in the inline comment; I verified the replacement at 0/400.

  Note for whoever applies it: the suggestion span in the previous review round was 4375-4378, which would have left url, standard, ... ); Some(url) dangling after the replacement. The block that has to go is 4375-4382, which is what the inline suggestion here covers.

Non-blocking

• Pre-existing, out of scope: a retried finalize also re-sends the recipient notification and double-charges the rolling limit (main.rs:1229, main.rs:1248). Real bug, not this PR's, and worth its own ticket rather than a change here — details inline at main.rs:1220.
• Two small wording/precision nits (api-description.yaml:381, main.rs:2736), both inline.

Rule sweep

Checked 17 rules selected for a Rust + SQLite + OpenAPI diff across four batches (security/correctness, tests, schema-migration/deps, spec-and-docs). Everything came back compliant except the items above. Specifically clean: a-recomputed-claim-downgrades-on-retry (the cycle-1 finding, now guarded on the assignment with the upgrade direction intact), automigrate-column-add-needs-a-nullable-field (both columns nullable, and a_sessions_table_without_the_proof_columns_gains_them builds the pre-migration schema explicitly, seeds a row with data, and asserts survival plus writability — which is exactly what that rule asks for), a-negative-test-behind-two-validators-pins-neither (the base64url test now uses a real signature respelled, not filler bytes), and rust-run-cargo-fmt-before-push.

Two Haiku candidates dropped as misapplied: the "AAAA" case in a_malformed_proof_header_leaves_the_sender_unproven asserts the end-to-end HTTP contract across three deliberately different junk shapes rather than one validator stage, and there is no AEAD binding change in this diff.

[dobby-coder]

Flaky test — reds CI on unrelated pushes. Confirmed independently: I ran this one test 400 times against the built test binary and got 4 failures (~1%), on the assert_ne! here:

assertion `left != right` failed: the two alphabets must disagree for this to test anything
  left: "PBKrustAX2odSlUvW1rWxGAeNQS7zJtUAs00MGNLPF3dyYzudn2f5VUzxUVrP6UlbBrTYVGWmn9PtmROZlOhCLbm5HXiqh37LMtLsDtynXgQ8qx6hPSNV5IXLDulow0F"

A signature is 96 bytes, so its standard base64 is 128 characters; when none of them happens to be + or / the base64url spelling is byte-identical and the assertion fires. Signing is randomised, so redrawing until the two spellings differ removes the flake without weakening what the test pins.

I applied the suggestion below and measured it: 0 failures in 400 runs, cargo build --tests -p cryptify clean, cargo fmt --all -- --check clean.

Suggested change

	let standard = proof_header(&setup.signing_keys[2], uuid, challenge);
	let bytes = Base64::decode_vec(&standard).expect("the header is standard base64");
	let url = Base64Url::encode_string(&bytes);
	assert_ne!(
	url, standard,
	"the two alphabets must disagree for this to test anything"
	);
	Some(url)
	// Roughly one signature in fifty encodes with neither `+` nor `/`,
	// and then both alphabets spell it identically and there is nothing
	// left to test. Signing is randomised, so draw again until the two
	// spellings actually differ.
	let url = loop {
	let standard = proof_header(&setup.signing_keys[2], uuid, challenge);
	let bytes = Base64::decode_vec(&standard).expect("the header is standard base64");
	let url = Base64Url::encode_string(&bytes);
	if url != standard {
	break url;
	}
	};
	Some(url)

[dobby-coder]

Pre-existing bug, not introduced here — out of scope, worth its own ticket (not a nit, and not a reason to hold this PR).

This PR promotes finalize's repeatability to a documented invariant (cryptify/CLAUDE.md: "Finalize is repeatable, so the claim only moves one way") and the guard on this line is correct. But two other things ride on that same repeatability and are not guarded:

• send_email (main.rs:1229) runs on every finalize, so a client retrying after a lost response sends the recipient a second notification.
• store.record_upload (main.rs:1248) pushes a fresh UploadRecord unconditionally — no per-uuid key (store.rs:1037) — and get_usage sums every record in the window (store.rs:1048), so a retried finalize charges the sender's 14-day rolling limit twice for one upload.

I verified both against HEAD. a_retried_finalize_does_not_erase_a_proof now drives exactly this path: both finalizes return 200 and run to the end of the handler. Neither function is touched by this diff, so neither is this PR's to fix — but the CLAUDE.md paragraph reads as though repeat finalizes are settled, and these two are the part that is not.

[dobby-coder]

Nit (wording). "the base64url alphabet is rejected" reads as an HTTP rejection, which is the one thing this endpoint never does — and the same description says so four lines later ("neither refuses the upload"). What actually happens is that the value fails to decode and the sender comes out Unproven, with a 200. Worth saying that directly, since this line is the client contract:

Suggested change

	standard base64 (RFC 4648 section 4, `+` and `/`) — the base64url
	standard base64 (RFC 4648 section 4, `+` and `/`); a value spelled in
	the base64url alphabet does not decode, and so proves nothing.

[dobby-coder]

Nit, low value — take it or leave it. The doc comment on decode_proof says "a value decoding to anything longer or shorter is refused rather than truncated", but neither case here exercises the explicit bytes.len() != SIG_BYTES check:

• long is caught earlier, by Base64::decode refusing to write past the SIG_BYTES buffer.
• short is caught later, by bincode_compat::deserialize.

I confirmed it by mutation: deleting the if bytes.len() != pg_core::ibs::gg::SIG_BYTES { return None; } block leaves this test green. So the check is defence in depth rather than something the suite pins. The trailing-bytes property the comment cares about is genuinely covered (by the buffer size), so this is a comment-precision point, not a hole.