feat(dmarc): RFC 7489 alignment + combined DKIM+DMARC verifier (PR 3 of email-recovery stack) by sea-snake · Pull Request #3878 · dfinity/internet-identity

sea-snake · 2026-05-12T11:52:23Z

Summary

PR 3 of the email-recovery stack (docs/ongoing/email-recovery.md §6). Stacks on top of #3877 (DKIM verifier). Lands a hand-rolled DMARC alignment check and reshapes the verifier API: dkim::verify_dkim becomes a DKIM-only primitive, and the new dmarc::verify_email is the public top-level entry point that produces the combined EmailVerificationStatus.

Note: This PR targets main but includes PRs 1+2's commits as its base. Review the DMARC-specific changes by looking at commits on top of ec371aae3 (PR 2's tip). Once PRs 1+2 merge, this PR's diff shrinks to just the DMARC additions.

What's in this PR

`src/internet_identity/src/dmarc/`

types.rs — DmarcOutcome (Aligned / Misaligned / NoRecord / Malformed), DmarcPolicy (None / Quarantine / Reject), AlignmentMode (Strict / Relaxed), DmarcRecord, plus the combined EmailVerificationStatus that carries both DKIM diagnostics and the DMARC outcome on success.
parse.rs (RFC 7489 §6.3) — DMARC TXT record parser. Enforces v=DMARC1 must be first, p= must be one of {none, quarantine, reject}, pct= 0..=100, rejects duplicate tags, ignores unknown / reporting tags. 12 unit tests.
from_header.rs (RFC 5322 / RFC 7489 §3.1.1) — single-mailbox From-header parser. Accepts bare addr-spec, name-addr, and quoted-display-name forms; rejects zero/multiple From: headers, address-lists, group syntax. Tolerates comma/colon inside quoted display names. 16 unit tests.
alignment.rs — strict (exact match) + relaxed (exact match OR label-aligned subdomain in either direction). Stricter than RFC-compliant relaxed alignment because we deliberately don't consult the PSL — design doc §6.4 documents the trust + asymmetric-failure-mode reasoning. The dot anchor on the subdomain check prevents evilexample.com from aliasing example.com. 8 unit tests.
verify.rs — orchestration. DKIM first; on failure, surface the DKIM reason verbatim. On DKIM pass, parse From and check DMARC alignment. Accepted iff Aligned, OR NoRecord with dkim_domain == from_domain. 8 unit tests.
test_vectors.rs — 5 end-to-end tests reusing PR 2's synthetic .eml fixtures.

`src/internet_identity/src/dkim/types.rs` (rename + new variants)

Renamed EmailVerificationStatus → DkimVerifyResult (DKIM-only). The combined verdict moved to dmarc::EmailVerificationStatus so it can carry the DmarcOutcome.
Added MalformedFromHeader(String), DmarcMalformed(String), DmarcMisaligned to VerificationFailReason.

`src/internet_identity/src/dkim/mod.rs`

Re-exports verify as verify_dkim so downstream callers (the dmarc layer) don't have to deal with both a dkim::verify and dmarc::verify in scope at the same time.

Test plan

cargo check -p internet_identity --target wasm32-unknown-unknown — clean.
cargo test -p internet_identity --bin internet_identity dmarc — 49 tests pass (12 parse + 16 from_header + 8 alignment + 8 verify + 5 e2e).
cargo test -p internet_identity --bin internet_identity — 365 tests pass total (was 313 with PR 2; +49 dmarc + 3 small reshape adjustments).
cargo clippy -p internet_identity --bin internet_identity --tests -- -D warnings — clean.
cargo fmt --check — clean (modulo pre-existing unrelated diffs).

PR Stack

#	PR	Description	Status
0	#3836	Design doc	Open
1	#3838	DNSSEC verifier scaffold	Open
2	#3877	DKIM verifier (RFC 6376)	Open
3	#3878	DMARC alignment (RFC 7489)	Open
4	#3879	DoH fallback	Open
5+6	#3880	Setup flow (storage + smtp_request)	Open
7	#3881	Recovery flow (delegation)	Open
8	#3882	Frontend + feature flag	Open
9	#3883	Deploy/upgrade scripts: dnssec_config + doh_config	Open
10	#3884	Email-recovery UX overhaul	Open

aterga · 2026-05-12T15:00:08Z

Grok's review

PR Review: #3878 – Combined DKIM + DMARC email verification for recovery flow

High-level verdict
This is a high-quality, security-focused PR. It meaningfully hardens the email recovery path by layering proper DMARC alignment (RFC 7489) on top of the existing DKIM verifier. The implementation is deliberate, fail-closed where it matters, and comes with excellent test coverage (≈127 total email-verification tests). No blockers. Ship after the two easy fixes below.

Major Security Assessment

No critical or high-severity gaps found.

The verifier now only accepts an email when DKIM passes and the DKIM-signed domain aligns with the From: header under the DMARC policy (or exact-match fallback when no DMARC record exists). This directly mitigates domain-spoofing attacks in the recovery flow.
From-header parser is robust (rejects multi-address, group syntax, malformed quoted strings; added backslash-escape support in iteration 3).
Alignment logic deliberately avoids Public Suffix List (PSL) lookups → prevents suffix attacks (evil-example.com masquerading as example.com). This is the correct security trade-off.
All parsing is defensive and returns structured VerificationFailReason / DmarcOutcome instead of panicking.
No new dependencies, no crypto changes, no canister storage/UI impact.

The only minor design decisions (no SPF, stricter-than-RFC parser) are explicitly called out in the code/comments and are acceptable for this use-case.

Blockers

None.
Tests pass, diff is clean, no breaking changes to public API surface (existing verify_dkim alias preserved).

Easy-to-change improvements (do these before merge – all <30 min)

DMARC parser must ignore unknown tags (RFC compliance)
Current code rejects unknown tags. RFC 7489 §6.3 explicitly says: “Tags not understood by the verifier MUST be ignored.”
Action: In src/internet_identity/src/dmarc/parse.rs, change unknown-tag handling from error to skip (keep the strict duplicate and ordering checks).
This prevents future breakage on legitimate records that add new tags (e.g. fo=, ri=, or vendor extensions). Tests will still catch truly malformed records.
Add security-model comments (one-liners)
In dmarc/verify.rs, dmarc/alignment.rs, and the new verify_email function, add a short block explaining:
- Why no PSL (suffix-attack prevention).
- Why SPF is omitted (design choice per §6.5, DKIM-only flow).
- Fail-closed philosophy.
  This will save future maintainers time and makes the security intent explicit in code review / audit.
Minor test-vector hygiene
The new dmarc/test_vectors.rs re-uses DKIM fixtures. Add one explicit “DMARC record with unknown tag” vector (after the parser change) to prove the ignore-unknown behavior.

Strategic recommendations (nice-to-have, low effort)

Logging / observability: When DmarcOutcome::Misaligned or NoRecord occurs in production, emit a structured metric (domain + reason). This will tell us quickly if the stricter alignment is rejecting legitimate recovery emails (e.g. gmail.com vs googlemail.com edge cases).
Future-proofing the recovery flow: With II v2 on the horizon (discoverable passkeys, multiple accounts), consider exposing verify_email result details to the frontend so users see a clear “Email domain not properly authenticated” message instead of a generic failure.
Optional SPF alignment later: The code already parses aspf= and sp= – wiring in SPF alignment (when the flow eventually needs it) will be a tiny delta on top of this PR.
Documentation: Add a one-paragraph section to the email-recovery README (or inline in dmarc/mod.rs) titled “DKIM + DMARC threat model”. It will be the canonical reference for security auditors.

Positive notes

Excellent separation of concerns (new dmarc/ module is clean, tiny, and well-tested).
Type renaming (EmailVerificationStatus → DkimVerifyResult + combined EmailVerificationStatus in dmarc) is the right long-term move.
Test count and realism (real .eml fixtures + 44 unit tests) set a high bar for this codebase.
Iterations (PR 2/3 fixes) show good responsiveness.

Recommendation: Approve with the two easy parser/docs changes above. This PR is a clear net security win for Internet Identity’s recovery path with zero downside. Great work.

aterga · 2026-05-12T15:07:18Z

Took a pass at Grok's three "easy-to-change improvements" — opened #3886 (draft, targets feat/dmarc-alignment) so they can land as a follow-up commit on this PR.

(Tried to push directly to feat/dmarc-alignment per the note from #3877 about not stacking PRs, but my account got a 403 against that branch — falling back to a stacked draft for now.)

Summary:

#	Grok's suggestion	Action
1	DMARC parser must ignore unknown tags	Already implemented & unit-tested (`dmarc::parse::unknown_tags_are_ignored`). The parser is lookup-by-name; the new e2e test below is the explicit pin Grok asked for under #3.
2	Add security-model comments (no PSL / no SPF / fail-closed)	Added a consolidated `# Security model` docblock in `dmarc/mod.rs` — one paragraph each for the three deliberate deviations.
3	Add a "DMARC record with unknown tag" e2e test vector	Added `verifies_end_to_end_when_dmarc_record_has_unknown_tags` in `dmarc/test_vectors.rs` — runs the full `verify_email` with `rua=`, `ruf=`, `fo=`, and a synthetic `vendorext=` tag.

The four strategic recommendations (observability, frontend error surface, SPF wiring, README) are explicitly "nice-to-have" / "future" and belong in later PRs of the stack.

Tests: 49 dmarc tests pass (+1). Clippy clean. wasm32 clean.

Generated by Claude Code

Addresses the two actionable "easy-to-change improvements" from Grok's review on #3878 (#3878 (comment)). Targets `feat/dmarc-alignment` so it can land as a follow-up commit on that PR. (Tried to push directly to `feat/dmarc-alignment` per the maintainer's note from #3877 / #3885, but my account got a 403 against that branch — falling back to a stacked draft PR.) ## What changed 1. **`dmarc/mod.rs` — consolidated `# Security model` docblock.** Calls out the three deliberate deviations from "stock" DMARC, each with a one-paragraph rationale: - **No Public Suffix List** — label-anchored suffix check prevents `evilexample.com`-style spoofs at the cost of closing multi-domain orgs (`gmail.com` ↔ `googlemail.com`). Safe direction for a recovery surface. Design doc §6.4. - **No SPF** — recovery proves mailbox control, not path-of-delivery. DKIM gives the cryptographic binding; SPF would need a source IP the gateway payload doesn't carry. Design doc §6.5. - **Fail-closed everywhere** — every malformed/unknown step collapses to `Unverified`. No quarantine, no downgrade. 2. **`dmarc/test_vectors.rs` — `verifies_end_to_end_when_dmarc_record_has_unknown_tags`.** Runs the full `verify_email` against a DMARC record carrying `rua=`, `ruf=`, `fo=`, and a synthetic `vendorext=` tag. Pins the ignore-unknown-tags behaviour through the public entry point, not just the parser unit (which already has a `unknown_tags_are_ignored` test in `dmarc/parse.rs`). ## Not actioned - **Grok item #1 ("parser must ignore unknown tags").** Already implemented and unit-tested before this PR. `dmarc/parse.rs` is lookup-by-name, so unknown tags pass through silently. The PR description even calls this out: "ignores unknown / reporting tags". The new end-to-end test is the explicit confirmation Grok asked for under item #3. The four strategic recommendations (observability metrics, frontend error surface, SPF wiring, README) are explicitly "nice-to-have" / "future" and belong in later PRs of the stack. ## Tests - `cargo test -p internet_identity --bin internet_identity dmarc::` — 49 pass (was 48, +1 e2e unknown-tag test). - `cargo clippy -p internet_identity --bin internet_identity --tests -- -D warnings` — clean. - `cargo check -p internet_identity --target wasm32-unknown-unknown` — clean. - `cargo fmt --check` on the two touched files — clean (other pre-existing fmt drifts in `dnssec/`, `openid/`, `tests/integration/`, `types/attributes.rs` are unrelated and predate this branch). --- _Generated by [Claude Code](https://claude.ai/code/session_01NZmvbHgzN5NQc7Hqx9ZzpP)_ Co-authored-by: Claude <noreply@anthropic.com>

Copilot

Pull request overview

Adds a DMARC alignment layer (RFC 7489 subset) on top of the existing DKIM verifier, introducing a new top-level dmarc::verify_email entry point that produces a combined DKIM+DMARC EmailVerificationStatus for the email-recovery verification pipeline.

Changes:

Introduces a new dmarc/ module: DMARC TXT parsing, From-header single-mailbox parsing, alignment logic, and verification orchestration (+ unit/e2e tests).
Refactors the DKIM verifier API to be DKIM-only (DkimVerifyResult) and re-exports it as verify_dkim for use by the DMARC layer.
Adds synthetic DKIM test-vector fixtures (.eml + TXT record) and wires SMTP gateway Candid types into the interface crate.

Reviewed changes

Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test_vectors/dkim/synth-rsa-test1._domainkey.test.example.com.txt	DKIM public-key TXT fixture for synthetic vectors.
test_vectors/dkim/synth-rsa-simple-simple.eml	Synthetic DKIM-signed email fixture (simple/simple).
test_vectors/dkim/synth-rsa-relaxed-simple.eml	Synthetic DKIM-signed email fixture (relaxed/simple).
test_vectors/dkim/synth-rsa-relaxed-relaxed.eml	Synthetic DKIM-signed email fixture (relaxed/relaxed).
test_vectors/dkim/README.md	Documentation for DKIM test-vector fixtures and provenance.
src/internet_identity/src/main.rs	Registers new `dkim` and `dmarc` modules in the canister binary.
src/internet_identity/src/dmarc/verify.rs	New top-level combined DKIM+DMARC verification orchestration.
src/internet_identity/src/dmarc/types.rs	DMARC types + combined `EmailVerificationStatus` for callers/UI.
src/internet_identity/src/dmarc/test_vectors.rs	End-to-end tests for combined verification using DKIM fixtures.
src/internet_identity/src/dmarc/parse.rs	DMARC TXT record parser (tag/value list).
src/internet_identity/src/dmarc/mod.rs	DMARC module wiring, exports, and security-model notes.
src/internet_identity/src/dmarc/from_header.rs	From-header single-mailbox parser for DMARC alignment.
src/internet_identity/src/dmarc/alignment.rs	Strict/relaxed (PSL-free) domain alignment logic + tests.
src/internet_identity/src/dkim/verify.rs	DKIM verification orchestration (multi-signature loop, enforcement).
src/internet_identity/src/dkim/types.rs	DKIM-only result type rename and new failure reasons for DMARC layer.
src/internet_identity/src/dkim/test_vectors.rs	DKIM end-to-end tests consuming `.eml`/TXT fixtures.
src/internet_identity/src/dkim/signature.rs	RSA/Ed25519 DKIM signature verification + body hash helper.
src/internet_identity/src/dkim/parse.rs	DKIM-Signature header parser (tag list, base64 handling).
src/internet_identity/src/dkim/mod.rs	DKIM module wiring and `verify_dkim` re-export.
src/internet_identity/src/dkim/dns_record.rs	DKIM DNS TXT record parser (`p=`, `k=`, `t=` flags).
src/internet_identity/src/dkim/canonicalize.rs	Relaxed canonicalization implementations for DKIM.
src/internet_identity_interface/src/internet_identity/types/smtp.rs	SMTP gateway Candid protocol types + validation helpers.
src/internet_identity_interface/src/internet_identity/types.rs	Exposes/re-exports SMTP types from the interface crate.
Cargo.lock	Dependency lockfile updates.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ck) (dfinity#3877) ## Summary PR 2 of the email-recovery stack (`docs/ongoing/email-recovery.md` §10 Phase 0). Stacks on top of PR 3838 (DNSSEC verifier). Lands a hand-rolled RFC 6376 DKIM verifier that consumes a parsed `SmtpRequest` plus an already-trusted DKIM TXT record and returns a per-step `EmailVerificationStatus`. **Note:** This PR targets `main` but includes PR 3838's commits (DNSSEC verifier) as its base. Review the DKIM-specific changes by looking at commits after `9bbd8717` (the last PR 3838 commit). Once PR 3838 merges, this PR's diff will shrink to just the DKIM additions. ## Why hand-rolled The design originally specified `mail-auth` (Stalwart's well-tested DKIM library), but mail-auth pulls a non-optional `hickory-resolver` dep that fails to compile for `wasm32-unknown-unknown` (transitive: tokio + mio). Forking + patching mail-auth would be possible but creates perpetual rebase burden. We hand-roll instead — "the right way, no shortcuts" was the explicit guidance. ## What's in this PR ### `src/internet_identity_interface/src/internet_identity/types/smtp.rs` Brings forward the SMTP gateway protocol types from PoC PR 3760: `SmtpRequest`/`SmtpResponse`/`SmtpHeader`/`SmtpMessage`/`SmtpAddress`/`SmtpEnvelope`, the size bounds, and the input-bound validation (`format_address` lowercases both halves; `truncate_at_char_boundary` clamps to the previous UTF-8 boundary so a multi-byte subject can't trap the canister). Drops postbox-specific bits (PostboxEmail, ValidatedSmtpRequest, anchor-number parser). ### `src/internet_identity/src/dkim/` - **`types.rs`** — Algorithm (RsaSha256, Ed25519Sha256), HeaderCanon/BodyCanon (Relaxed, Simple), DkimCheck/DkimCheckName/DkimCheckStatus per-step diagnostics, EmailVerificationStatus / VerificationFailReason result shape. - **`parse.rs`** (RFC 6376 §3.5) — DKIM-Signature header tag-list parser. Splits structurally on `;` first then on the *first* `=` per element, so a literal `b=` substring inside another tag's base64 doesn't get misread as a new tag start (the bug class the PoC PR review specifically flagged). Folded whitespace inside base64 values is stripped before decoding. Tag names case-insensitive; duplicates rejected. - **`canonicalize.rs`** (§3.4.2 / §3.4.4) — relaxed header canon (lowercase name, unfold continuations, collapse WSP+ to single SP, strip trailing WSP, strip WSP around colon) and relaxed body canon (per-line WSP cleanup, drop trailing empty lines, ensure non-empty output ends in exactly one CRLF). - **`dns_record.rs`** (§3.6.2) — DKIM TXT record parser. Tag names case-insensitive (`P=` vs `p=` was a PoC bug), whitespace inside `p=` tolerated (multi-chunk DNS TXT records), `t=y`/`t=s` flags honoured, unknown tags ignored. - **`signature.rs`** — RSA-SHA256 (RFC 5702 / RFC 8301) and Ed25519-SHA256 (RFC 8463) signature verification on top of `rsa`+`sha2`+`ed25519-dalek` from PR 1's deps. Enforces 1024-bit RSA minimum per design §5.6. Ed25519 path wraps in SHA-256 per RFC 8463. Plus `body_hash_sha256` with optional `l=` truncation per §3.4.5. - **`verify.rs`** — orchestration. Multi-signature loop per §5.5 (accept on first pass), tag enforcement per design §5.4 (c=relaxed/* only, x= expiration, i= alignment with d=, k= match, t=y testing-mode), bottom-up header selection per §5.4 when h= lists a name multiple times, b=value blanking that's structural-position-aware so it doesn't mis-target an internal substring. - **`test_vectors.rs`** — `#[cfg(test)]` .eml loader + 8 end-to-end tests against committed fixtures. ### `test_vectors/dkim/` - 3 synthetic .eml files generated offline with dkimpy + a 2048-bit RSA key (`relaxed/relaxed`, `relaxed/simple`, `simple/simple`). - The matching DKIM TXT record (public key only). - README documenting provenance — the throwaway private key is **not** committed. ## Test plan - [x] `cargo check -p internet_identity --target wasm32-unknown-unknown` — clean. - [x] `cargo test -p internet_identity --bin internet_identity dkim` — 75 tests pass (parse 14, canonicalize 18, dns_record 16, signature 7, verify 12, end-to-end 8). - [x] `cargo test -p internet_identity --bin internet_identity` — 313 tests pass total (was 238 before this PR; +75 DKIM, plus a few in smtp types). - [x] `cargo test -p internet_identity_interface --lib` — 52 tests pass (was 42; +10 SMTP type tests). - [x] `cargo clippy -p internet_identity --bin internet_identity --tests -- -D warnings` — clean. - [x] `cargo fmt --check` — clean (modulo pre-existing diffs unrelated to this PR). ## Stack This is PR 2 of a 12-PR series. Includes PR 3838's commits as its base; once PR 3838 merges, the diff shrinks to just the DKIM additions. Subsequent PRs: - **PR 3** — DMARC alignment. - **PR 4** — DoH outcall fallback for unsigned domains (Gmail / Outlook / iCloud — see the design doc §7.6 and the team Slack writeup). - **PRs 5–9** — storage + Candid + behavior for email recovery. - **PRs 10–12** — frontend. ## PR Stack | # | PR | Description | Status | |---|---|---|---| | 0 | [dfinity#3836](dfinity#3836) | Design doc | Open | | 1 | [dfinity#3838](dfinity#3838) | DNSSEC verifier scaffold | Open | | 2 | [dfinity#3877](dfinity#3877) | DKIM verifier (RFC 6376) | Open | | 3 | [dfinity#3878](dfinity#3878) | DMARC alignment (RFC 7489) | Open | | 4 | [dfinity#3879](dfinity#3879) | DoH fallback | Open | | 5+6 | [dfinity#3880](dfinity#3880) | Setup flow (storage + smtp_request) | Open | | 7 | [dfinity#3881](dfinity#3881) | Recovery flow (delegation) | Open | | 8 | [dfinity#3882](dfinity#3882) | Frontend + feature flag | Open | | 9 | [dfinity#3883](dfinity#3883) | Deploy/upgrade scripts: dnssec_config + doh_config | Open | | 10 | [dfinity#3884](dfinity#3884) | Email-recovery UX overhaul | Open | --------- Co-authored-by: Arshavir Ter-Gabrielyan <arshavir.ter.gabrielyan@dfinity.org> Co-authored-by: Claude <noreply@anthropic.com>

PR 3 of the email-recovery stack (docs/ongoing/email-recovery.md §6). Stacks on top of PR 2 (DKIM verifier) and reshapes the verifier API: dkim::verify_dkim now returns a DKIM-only DkimVerifyResult, and the new dmarc::verify_email is the public top-level entry point that produces the combined EmailVerificationStatus. New module src/internet_identity/src/dmarc/: - types.rs: DmarcOutcome (Aligned / Misaligned / NoRecord / Malformed), DmarcPolicy, AlignmentMode, DmarcRecord, plus the combined EmailVerificationStatus that carries both the DKIM diagnostic and the DMARC outcome on success. - parse.rs: DMARC TXT record parser per RFC 7489 §6.3. Enforces v=DMARC1 must be first, p= must be one of {none, quarantine, reject}, pct= 0..=100, rejects duplicate tags, ignores unknown tags. 12 unit tests. - from_header.rs: RFC 5322 single-mailbox From: parser. Accepts bare addr-spec, name-addr, and quoted-display-name forms; rejects zero/multiple From: headers, address-lists, group syntax. Tolerates comma/colon inside quoted display names. 16 unit tests. - alignment.rs: strict (exact match) + relaxed (exact match OR label-aligned subdomain in either direction). Stricter than RFC- compliant relaxed alignment because we deliberately don't consult the PSL — see design doc §6.4. The dot anchor on the subdomain check prevents 'evilexample.com' from aliasing 'example.com'. 8 unit tests. - verify.rs: orchestration. DKIM first; on failure, surface the DKIM reason verbatim. On DKIM pass, parse From and check DMARC alignment. Accepted iff Aligned, OR NoRecord with dkim_domain == from_domain. 8 unit tests. dkim/types.rs: - Renamed EmailVerificationStatus -> DkimVerifyResult (DKIM-only). The combined verdict moved to dmarc::EmailVerificationStatus so it can carry the DmarcOutcome. - Added MalformedFromHeader, DmarcMalformed, DmarcMisaligned to VerificationFailReason. 44 DMARC tests pass (12 parse + 16 from_header + 8 alignment + 8 verify), on top of the existing 78 DKIM tests. Wasm32 build clean.

5 end-to-end tests in dmarc/test_vectors.rs reusing the synthetic .eml fixtures from PR 2 (alice@test.example.com signed with d=test.example.com, exact match → trivially aligned regardless of mode). Covers: - no DMARC record + dkim==from → Verified(NoRecord) - aligned DMARC strict → Verified(Aligned, Strict) - aligned DMARC relaxed (default) → Verified(Aligned, Relaxed) - malformed DMARC TXT → Unverified(DmarcMalformed) - From: with address-list → Unverified 49 dmarc tests total (44 unit + 5 e2e). 365 total in the II suite. The parse_eml helper is duplicated from dkim/test_vectors.rs because the original is #[cfg(test)] and not pub-visible across modules; the duplication is contained to test code.

Three fixes originally landed bundled with DoH review feedback (commit 5d5f686 on PR #3879). They belong here in PR 3's DMARC module since they're parser / spec-compliance fixes: - parse.rs: enforce p= immediately after v=DMARC1 per RFC 7489 §6.3. Real-world records all do this; OpenDMARC and other reference parsers reject the alternative. - from_header.rs: honour backslash escapes inside quoted-string display names so `"Alice \"Ops, Inc\"" <a@e.com>` parses correctly. - verify.rs: removed empty placeholder test that asserted nothing; the e2e path is exercised by test_vectors::*.

Addresses the two actionable "easy-to-change improvements" from Grok's review on #3878 (#3878 (comment)). Targets `feat/dmarc-alignment` so it can land as a follow-up commit on that PR. (Tried to push directly to `feat/dmarc-alignment` per the maintainer's note from #3877 / #3885, but my account got a 403 against that branch — falling back to a stacked draft PR.) ## What changed 1. **`dmarc/mod.rs` — consolidated `# Security model` docblock.** Calls out the three deliberate deviations from "stock" DMARC, each with a one-paragraph rationale: - **No Public Suffix List** — label-anchored suffix check prevents `evilexample.com`-style spoofs at the cost of closing multi-domain orgs (`gmail.com` ↔ `googlemail.com`). Safe direction for a recovery surface. Design doc §6.4. - **No SPF** — recovery proves mailbox control, not path-of-delivery. DKIM gives the cryptographic binding; SPF would need a source IP the gateway payload doesn't carry. Design doc §6.5. - **Fail-closed everywhere** — every malformed/unknown step collapses to `Unverified`. No quarantine, no downgrade. 2. **`dmarc/test_vectors.rs` — `verifies_end_to_end_when_dmarc_record_has_unknown_tags`.** Runs the full `verify_email` against a DMARC record carrying `rua=`, `ruf=`, `fo=`, and a synthetic `vendorext=` tag. Pins the ignore-unknown-tags behaviour through the public entry point, not just the parser unit (which already has a `unknown_tags_are_ignored` test in `dmarc/parse.rs`). ## Not actioned - **Grok item #1 ("parser must ignore unknown tags").** Already implemented and unit-tested before this PR. `dmarc/parse.rs` is lookup-by-name, so unknown tags pass through silently. The PR description even calls this out: "ignores unknown / reporting tags". The new end-to-end test is the explicit confirmation Grok asked for under item #3. The four strategic recommendations (observability metrics, frontend error surface, SPF wiring, README) are explicitly "nice-to-have" / "future" and belong in later PRs of the stack. ## Tests - `cargo test -p internet_identity --bin internet_identity dmarc::` — 49 pass (was 48, +1 e2e unknown-tag test). - `cargo clippy -p internet_identity --bin internet_identity --tests -- -D warnings` — clean. - `cargo check -p internet_identity --target wasm32-unknown-unknown` — clean. - `cargo fmt --check` on the two touched files — clean (other pre-existing fmt drifts in `dnssec/`, `openid/`, `tests/integration/`, `types/attributes.rs` are unrelated and predate this branch). --- _Generated by [Claude Code](https://claude.ai/code/session_01NZmvbHgzN5NQc7Hqx9ZzpP)_ Co-authored-by: Claude <noreply@anthropic.com>

Addresses the five inline Copilot review comments across #3878 and its rebased twin #3887. dmarc/from_header.rs - Reject internal whitespace in the bare addr-spec form (no angle brackets). Previously `"Alice alice@example.com"` and `"alice @example.com"` slipped through with domain=example.com because only the post-`@` domain part was whitespace-checked. - Reject `>` without a matching `<`, multiple `>`, and content after the closing `>`. Previously `alice@example.com>` was parsed as a bare addr-spec with the `>` ending up in the extracted domain. - 5 new regression tests covering the above. dmarc/verify.rs - The `email.message == None` branch after a successful DKIM is dead in practice (verify_dkim returns Unverified(NoSignature) otherwise), but we keep the defensive branch rather than .expect()/unreachable!() — canister code on the IC must never trap, an invariant violation should surface as a structured fail-closed verdict instead. Updated the inline comment and switched the placeholder reason from the misleading MalformedFromHeader to NoSignature. dkim/verify.rs - On a successful signature, return only that signature's per-step checks in DkimVerifyResult::Verified.checks, matching the doc ("checks for the winning signature"). Previously the verifier concatenated every failed attempt's checks onto the winner's. - Update the module-level doc snippet to reference `verify_dkim` (the re-exported public name) instead of `verify`.

sea-snake · 2026-05-13T10:59:49Z

Thanks for the thorough Grok review @aterga — the three actionable items were all addressed:

Unknown tags — already handled by the lookup-by-name parser; now pinned with an explicit end-to-end test vector (`verifies_end_to_end_when_dmarc_record_has_unknown_tags`).
Security-model docblock — consolidated in `dmarc/mod.rs` (no PSL, no SPF, fail-closed).
Unknown-tag e2e test — added in `dmarc/test_vectors.rs`.

These landed via #3886 (merged into `feat/dmarc-alignment`). The branch has also been rebased onto current `main` (PR #3877 merged), and Copilot's five inline comments from #3878 and #3887 are addressed in 7d157bd.

Strategic recommendations (observability, frontend error surface, SPF wiring, README) are tracked for later PRs in the stack.

sea-snake requested a review from a team as a code owner May 12, 2026 11:52

sea-snake force-pushed the feat/dkim-verifier branch from 23985ad to bcf8233 Compare May 12, 2026 11:58

sea-snake force-pushed the feat/dmarc-alignment branch 3 times, most recently from 15811ad to 9208444 Compare May 12, 2026 13:01

aterga mentioned this pull request May 12, 2026

chore(dmarc): address Grok review feedback on PR #3878 #3886

Merged

Base automatically changed from feat/dkim-verifier to main May 12, 2026 15:10

Copilot AI review requested due to automatic review settings May 12, 2026 15:23

Copilot started reviewing on behalf of aterga May 12, 2026 15:24 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

Comment thread src/internet_identity/src/dkim/verify.rs Outdated

Comment thread src/internet_identity/src/dmarc/from_header.rs

Comment thread src/internet_identity/src/dmarc/verify.rs Outdated

aterga mentioned this pull request May 12, 2026

feat(dmarc): RFC 7489 alignment + combined DKIM+DMARC verifier (rebased #3878) #3887

Closed

3 tasks

sea-snake and others added 5 commits May 13, 2026 10:41

sea-snake force-pushed the feat/dmarc-alignment branch from 271edb2 to 7d157bd Compare May 13, 2026 10:53

sea-snake requested a review from aterga May 13, 2026 11:03

aterga approved these changes May 13, 2026

View reviewed changes

aterga added this pull request to the merge queue May 13, 2026

aterga removed this pull request from the merge queue due to the queue being cleared May 13, 2026

aterga enabled auto-merge (squash) May 13, 2026 14:07

aterga merged commit b30fd7c into main May 13, 2026
47 checks passed

aterga deleted the feat/dmarc-alignment branch May 13, 2026 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dmarc): RFC 7489 alignment + combined DKIM+DMARC verifier (PR 3 of email-recovery stack)#3878

feat(dmarc): RFC 7489 alignment + combined DKIM+DMARC verifier (PR 3 of email-recovery stack)#3878
aterga merged 5 commits into
mainfrom
feat/dmarc-alignment

sea-snake commented May 12, 2026 •

edited

Loading

Uh oh!

aterga commented May 12, 2026

Uh oh!

aterga commented May 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sea-snake commented May 13, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

sea-snake commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What's in this PR

src/internet_identity/src/dmarc/

src/internet_identity/src/dkim/types.rs (rename + new variants)

src/internet_identity/src/dkim/mod.rs

Test plan

PR Stack

Uh oh!

aterga commented May 12, 2026

Major Security Assessment

Blockers

Easy-to-change improvements (do these before merge – all <30 min)

Strategic recommendations (nice-to-have, low effort)

Positive notes

Uh oh!

aterga commented May 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sea-snake commented May 13, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sea-snake commented May 12, 2026 •

edited

Loading

`src/internet_identity/src/dmarc/`

`src/internet_identity/src/dkim/types.rs` (rename + new variants)

`src/internet_identity/src/dkim/mod.rs`