chore(dkim): address Grok review feedback on PR #3877#3885
Merged
Conversation
- types.rs: expand `NoSignature` doc-comment to call out the frontend UX use case (distinct from a generic verification failure so the UI can render "this provider doesn't use DKIM"). The variant itself was already distinct — this just documents the intent. - parse.rs: add explicit `unknown_tags_are_ignored` test asserting that unknown tags (z=, future extensions) pass parsing per RFC 6376 §3.5. Behaviour was already correct (the parser is lookup-by-name, not reject-on-extra); the test pins it. - verify.rs: add SECURITY comment at the `parse_dkim_txt` call site making the trust boundary explicit — `dkim_txt` must come from a DNSSEC-validated chain (PR #3838) or a pinned-host DoH outcall (PR #3879).
6 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up to the DKIM verifier work that incorporates requested review tweaks to make intent/trust boundaries clearer and to pin RFC-required behavior with a dedicated test.
Changes:
- Clarified
VerificationFailReason::NoSignaturedoc-comment to reflect frontend UX intent. - Added a unit test asserting unknown DKIM-Signature tags are ignored (RFC 6376 §3.5).
- Added an explicit trust-boundary
SECURITY:comment at the DKIM TXT parsing call site.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/internet_identity/src/dkim/verify.rs |
Documents the trust boundary for dkim_txt at the DNS record parsing call site. |
src/internet_identity/src/dkim/types.rs |
Clarifies semantics/UX intent for the “no DKIM signature present” failure reason. |
src/internet_identity/src/dkim/parse.rs |
Adds a regression test ensuring unknown DKIM tags don’t break parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aterga
added a commit
that referenced
this pull request
May 12, 2026
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>
sea-snake
pushed a commit
that referenced
this pull request
May 13, 2026
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the four "easy-to-change improvements" from Grok's review on #3877 (#3877 (review)). Targets
feat/dkim-verifierso it can be merged in as a follow-up commit to that PR.What changed
types.rs—NoSignaturedoc-comment. The distinct variant Grok asked for (NoSignaturePresentor equivalent) already exists asVerificationFailReason::NoSignatureat types.rs:84 and is returned fromverify()when no DKIM-Signature headers are present (verify.rs:54, verify.rs:67). The frontend can already distinguish it from a generic failure. Expanded the doc-comment to make the UX intent ("this provider doesn't use DKIM") explicit so future readers (and AI reviewers) don't re-flag this.parse.rs— explicit unknown-tag-ignored test. RFC 6376 §3.5 requires implementations to ignore unrecognised tags. The parser already does this (it's lookup-by-name insplit_tag_list→get(name); unknown tags pass straight through — the existinghappy_value()fixture even contains an unknownq=dns/txt). Pinned the behaviour with a dedicatedunknown_tags_are_ignoredtest coveringz=and a syntheticzz=.verify.rs— SECURITY comment at the trust boundary. Added an inline// SECURITY:block at theparse_dkim_txtcall site stating thatdkim_txtis trusted, sourced from the DNSSEC verifier (feat(dnssec): scaffold verifier module + canister-side trust-anchor wiring #3838) or pinned-host DoH outcall (feat(doh): DoH fallback for DKIM/DMARC TXT records on unsigned domains #3879).Not actioned
dkim-test-vectors→dkim-verifier-tests). No such job exists in.github/workflows/; PR feat(dkim): hand-rolled RFC 6376 verifier (PR 2 of email-recovery stack) #3877 doesn't add one. Nothing to rename.main.rs::handle_email_recoverymatch arm. That endpoint hasn't been added yet (it's PRs 5–7 of the stack). No-op for this PR.Tests
cargo test -p internet_identity --bin internet_identity dkim::— 79 pass (was 78, +1 for the new 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 --checkon the three modified files — clean (other pre-existing fmt drifts indnssec/,openid/,tests/integration/are unrelated and predate this branch).Generated by Claude Code