feat(dmarc): RFC 7489 alignment + combined DKIM+DMARC verifier (rebased #3878) by aterga · Pull Request #3887 · dfinity/internet-identity

aterga · 2026-05-12T15:30:55Z

Rebased version of #3878 with conflicts resolved against the latest main.

What changed in the rebase

PR #3878 was stacked on top of #3877 (DKIM verifier). Now that #3877 has merged into main (commit 74ad617), the 10 DKIM-related commits in this PR's history are obsolete — main carries them in squashed form. The rebase drops those duplicate commits so the branch contains only the 4 DMARC-specific commits on top of the new main:

feat(dmarc): RFC 7489 alignment check + combined DKIM+DMARC verifier
feat(dmarc): end-to-end verification tests + rustfmt
fix(dmarc): spec compliance + parser fixes from PR review
chore(dmarc): address Grok review feedback on PR #3878 (#3886)

Mechanically this was git rebase --onto origin/main 23597e1 (the last DKIM commit on the original branch). No conflicts — every dropped commit was already represented in main by the squashed #3877 merge.

Verification

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 — 376 tests pass (up from 365 in the original PR; main has moved)

Note: cargo clippy surfaces one warning in src/internet_identity/src/storage.rs:768 (redundant_iter_cloned), but it's pre-existing on main (introduced in commit f297260, unrelated to this stack) — it appears because the local clippy toolchain is newer than CI's. Not addressed here to keep the diff focused on DMARC.

Original PR description

See #3878 for the full DMARC feature description, file-by-file breakdown, and PR-stack context.

https://claude.ai/code/session_01WQSFHkbjuZttf6tGGbbjff

Generated by Claude Code

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>

Copilot

Pull request overview

This PR adds a DMARC alignment layer on top of the existing DKIM verifier, introducing a combined verify_email entry point that returns a unified DKIM+DMARC verification verdict for the email-recovery flow.

Changes:

Introduces a new dmarc module implementing DMARC TXT parsing, From-header domain extraction, alignment logic, and a combined DKIM+DMARC verifier.
Refactors DKIM’s public result type to be DKIM-only (DkimVerifyResult) and re-exports the DKIM entry point as verify_dkim for clearer layering.
Adds unit + end-to-end test coverage for DMARC parsing, From-header handling, alignment, and combined verification behavior.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/internet_identity/src/main.rs	Wires the new `dmarc` module into the binary crate.
src/internet_identity/src/dmarc/mod.rs	Defines DMARC module structure, exports, and security model notes.
src/internet_identity/src/dmarc/types.rs	Adds DMARC/combined verdict types (`DmarcOutcome`, combined `EmailVerificationStatus`).
src/internet_identity/src/dmarc/parse.rs	Implements DMARC TXT record parsing with validation and tests.
src/internet_identity/src/dmarc/from_header.rs	Implements a minimal single-mailbox `From:` parser for alignment inputs.
src/internet_identity/src/dmarc/alignment.rs	Implements strict/relaxed alignment checks (without PSL) plus tests.
src/internet_identity/src/dmarc/verify.rs	Implements the combined DKIM+DMARC orchestration entry point and unit tests.
src/internet_identity/src/dmarc/test_vectors.rs	Adds end-to-end tests using existing DKIM `.eml` fixtures plus synthetic DMARC records.
src/internet_identity/src/dkim/types.rs	Renames DKIM verdict type to `DkimVerifyResult` and adds DMARC-related failure reasons.
src/internet_identity/src/dkim/verify.rs	Updates DKIM verifier to return `DkimVerifyResult` and adjusts tests/docs accordingly.
src/internet_identity/src/dkim/mod.rs	Re-exports DKIM entry point as `verify_dkim` and re-exports `DkimVerifyResult`.
src/internet_identity/src/dkim/test_vectors.rs	Updates DKIM end-to-end tests to use `DkimVerifyResult`.

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

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

+        if b == b'>' {
+            angle_end = Some(i);
+            i += 1;
+            continue;
+        }


Fixed in 7d157bd on #3878 (this PR is closed in its favor).

find_address_spec now rejects:

> without a matching < ("'>' without matching '<'")

Multiple > ("multiple '>'")

Non-whitespace content after the closing > ("trailing content after '>'")

Added regression tests: rejects_stray_closing_angle_without_opening, rejects_content_after_closing_angle, rejects_multiple_closing_angles.

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

 //! ```ignore
 //! pub fn verify(
 //!     email: &SmtpRequest,
 //!     dkim_txt: &str,
 //!     now_secs: u64,
-//! ) -> EmailVerificationStatus
+//! ) -> DkimVerifyResult


Fixed in 7d157bd on #3878 (this PR is closed in its favor). Module doc snippet now references verify_dkim and notes the re-export.

- from_header: reject stray `>` without a prior `<` and reject multiple `>` so malformed `From:` headers like `alice@example.com>` fail closed at parse time instead of leaking through to alignment with a `>` glued onto the extracted domain. Add tests for both. - dkim/verify: clarify the module-level doc snippet — the function is defined here as `verify` but re-exported by the parent `dkim` module as `verify_dkim`, which is what callers actually see. https://claude.ai/code/session_01WQSFHkbjuZttf6tGGbbjff

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:53:41Z

Closing in favor of #3878, which has now been rebased onto main and addresses all five Copilot inline comments from both PRs (whitespace in bare addr-spec, stray >, defensive None branch, all_checks accumulation, doc snippet). See 7d157bd on feat/dmarc-alignment.

sea-snake and others added 4 commits May 12, 2026 15:25

aterga marked this pull request as ready for review May 12, 2026 15:33

aterga requested a review from a team as a code owner May 12, 2026 15:34

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

aterga enabled auto-merge May 12, 2026 15:34

aterga disabled auto-merge May 12, 2026 15:34

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

Copilot AI reviewed May 12, 2026

View reviewed changes

sea-snake closed this May 13, 2026

sea-snake mentioned this pull request May 13, 2026

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

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

feat(dmarc): RFC 7489 alignment + combined DKIM+DMARC verifier (rebased #3878)#3887
aterga wants to merge 5 commits into
mainfrom
claude/resolve-pr-conflicts-Pdoeu

aterga commented May 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

sea-snake May 13, 2026

Uh oh!

sea-snake May 13, 2026

Uh oh!

sea-snake commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

aterga commented May 12, 2026

What changed in the rebase

Verification

Original PR description

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

sea-snake May 13, 2026

Choose a reason for hiding this comment

Uh oh!

sea-snake May 13, 2026

Choose a reason for hiding this comment

Uh oh!

sea-snake commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants