Skip to content

feat(dnssec): scaffold verifier module + canister-side trust-anchor wiring#3838

Merged
sea-snake merged 11 commits into
dfinity:mainfrom
sea-snake:feat/dnssec-verifier-scaffold
May 12, 2026
Merged

feat(dnssec): scaffold verifier module + canister-side trust-anchor wiring#3838
sea-snake merged 11 commits into
dfinity:mainfrom
sea-snake:feat/dnssec-verifier-scaffold

Conversation

@sea-snake
Copy link
Copy Markdown
Contributor

@sea-snake sea-snake commented May 4, 2026

Summary

First PR in the email-recovery stack (docs/ongoing/email-recovery.md §10 Phase 0). Lands a working RFC-4035-compliant DNSSEC verifier for caller-supplied DNS proof bundles, plus the trust-anchor wiring that drives it. PR 2 (DKIM verifier) and PRs 4–9 (storage + recovery methods) build on this.

What's in this PR

Verifier core

  • New dnssec/ module under src/internet_identity/src/:
    • types.rsDnsProofBundle, SignedRRset, DelegationLink, Rrsig, DnsName, DnssecError, VerifiedRecord.
    • canonical.rs — owner-name canonicalization, RR canonical form, RRSIG signed-data construction (RFC 4034 §3.1.8.1, §6.2, §6.3), DS digest input.
    • signature.rs — algorithm dispatch + DS digest matching (SHA-256).
    • verify.rs — four-step algorithm (root anchor match → chain walk → leaf RRSIG → freshness).
  • Algorithm coverage (RFC 8624 MUST set):
    • alg 8 — RSA-SHA256 (RFC 5702): root, com., most legacy zones.
    • alg 13 — ECDSA-P256-SHA256 (RFC 6605): most TLDs, Cloudflare, modern zones.
    • alg 15 — Ed25519 (RFC 8080): rare in production but mandatory.
    • Anything else returns UnsupportedAlgorithm.

Wiring

  • New DnssecConfig and DnssecRootAnchor types in internet_identity_interface, exposed at the top of InternetIdentityInit as dnssec_config: opt opt DnssecConfig (set/clear semantics matching analytics_config and dummy_auth).
  • Trust-anchor list plumbed through init/post_upgrade into PersistentState.dnssec_config (and StorablePersistentState for cross-upgrade persistence).
  • internet_identity.did updated.

Tests

13 unit tests in dnssec/ covering:

  • Real cloudflare.com chain verifies end-to-end (exercises alg 8 at root and alg 13 at com → cloudflare.com → leaf).
  • Empty trust-anchor list rejected with NoTrustAnchors.
  • Wrong trust anchor rejected with RootAnchorMismatch.
  • Flipped byte in root DNSKEY → RootAnchorMismatch or BadSignature.
  • Flipped byte in leaf signature → BadSignature.
  • Stale signature (clock advanced past expiration) → StaleOrFutureSignature.
  • Unsupported algorithm (alg 5 / RSA-SHA1) → UnsupportedAlgorithm(5).
  • Plus canonical-encoding + RFC 3110 RSA key parsing unit tests.

Test infrastructure

  • test_vectors/dnssec/cloudflare-com-2026-05.json — real DoH-captured chain (root DNSKEY + 2 delegation links + leaf TXT).
  • test_vectors/dnssec/iana-root-anchors-2026-05.json — IANA root KSK trust anchors (Klajeyz/2017 + Kmyv6jo/2024).
  • scripts/capture-dnssec-chain.py — reproducible capture script using dnspython + DoH wire format. Tests use a frozen now from the capture's metadata so freshness checks stay stable indefinitely.

New deps

  • domain (NLnet Labs, pure Rust) — referenced in docstrings for canonicalisation primitives; signature verification is hand-rolled on top of RustCrypto.
  • p256 — ECDSA P-256 verification.
  • ed25519-dalek — Ed25519 verification.

All three build cleanly for wasm32-unknown-unknown.

What's deferred to later PRs in the stack

  • Captures for additional providers (proton.me, protonmail.com, tutanota.com — gmail.com / icloud.com / outlook.com / fastmail.com don't sign with DNSSEC; this is acknowledged in design doc §7.6).
  • Synthetic Ed25519 (alg 15) test vector — most production zones are alg 8 or 13; alg 15 is structurally exercised by the dispatch logic but doesn't have a real captured chain in this PR.

Test plan

  • cargo check -p internet_identity --target wasm32-unknown-unknown — clean (no warnings).
  • cargo test -p internet_identity --bin internet_identity — 238 tests pass (was 227 pre-PR).
  • cargo test -p internet_identity_interface --lib — 42 tests pass.
  • cargo clippy -p internet_identity --bin internet_identity --tests -- -D warnings — clean.
  • cargo fmt --check — clean (modulo a pre-existing diff in attributes.rs unrelated to this PR).
  • CI on feat(dnssec): scaffold verifier module + canister-side trust-anchor wiring #3838 — fully green.

Design doc

https://github.com/sea-snake/internet-identity/blob/design/email-recovery/docs/ongoing/email-recovery.md (PR pending review on dfinity/internet-identity)

Stack

This is PR 1 of a 12-PR series. Subsequent PRs:

  • PR 2 — mail-auth-backed DKIM verifier, consuming DnsProofBundle from this PR.
  • PR 3 — DMARC alignment.
  • PRs 4–9 — storage + Candid + behavior for email recovery.
  • PRs 10–12 — frontend (DoH walker, Manage UI, recovery wizard).

PR Stack

# PR Description Status
0 #3836 Design doc Open
1 #3838 DNSSEC verifier scaffold Open
2 #3839 DKIM verifier (RFC 6376) Open
3 #3840 DMARC alignment (RFC 7489) Open
4 #3841 DoH fallback Open
5+6 #3842 Setup flow (storage + smtp_request) Open
7 #3843 Recovery flow (delegation) Open
8 #3844 Frontend + feature flag Open
9 #3855 Deploy/upgrade scripts: dnssec_config + doh_config Open
10 #3857 Email-recovery UX overhaul Open

…iring

First PR in the email-recovery stack (docs/ongoing/email-recovery.md §10
Phase 0). Lands the structural pieces of the DNSSEC verifier so PR dfinity#2
(DKIM verifier) and PR dfinity#4-dfinity#9 (storage + recovery methods) can build
against the types. Cryptographic verification logic and real DoH-captured
test vectors arrive in PR #1b.

What's in this PR:

- New workspace crate internet_identity_email_test_vectors with a
  placeholder loader and a README explaining what arrives in PR #1b
  (DNSSEC chains, DKIM happy-path + tampering vectors, DMARC alignment
  matrix).
- New DnssecConfig and DnssecRootAnchor types in
  internet_identity_interface, exposed at the top of InternetIdentityInit
  as 'dnssec_config'. Not specific to email recovery — any feature that
  verifies DNS records against the IANA-rooted DNSSEC chain consumes the
  same anchors.
- New dnssec/ module under src/internet_identity/src/ with type
  definitions (DnsProofBundle, SignedRRset, DelegationLink, Rrsig,
  DnsName, DnssecError, VerifiedRecord) and a stub 'verify' that returns
  Err(DnssecError::NotImplemented). Step-by-step TODOs reference §7.3 of
  the design doc.
- Trust-anchor list plumbed through init/post_upgrade into
  PersistentState.dnssec_config (and through StorablePersistentState for
  cross-upgrade persistence).
- Two unit tests for the stub verifier (NoTrustAnchors path,
  NotImplemented path) — flip to positive assertions in PR #1b.
- internet_identity.did updated with DnssecConfig / DnssecRootAnchor and
  the new init field.

What's deferred to PR #1b:

- RRSIG / DS / DNSKEY canonicalization and signature verification per
  RFC 4034 §6.
- Crypto deps for ECDSA-P256-SHA256 (alg 13) and Ed25519 (alg 15). RSA-
  SHA256 (alg 8) deps already in the workspace.
- Real DoH-captured DNSSEC chains for gmail.com, icloud.com,
  outlook.com, fastmail.com, proton.me, plus deliberately-tampered
  negatives.

Build: cargo check --target wasm32-unknown-unknown clean; 227 internet_identity
bin tests pass; 42 internet_identity_interface unit tests pass.
Copilot AI review requested due to automatic review settings May 4, 2026 14:58
@sea-snake sea-snake requested a review from a team as a code owner May 4, 2026 14:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR bootstraps the DNSSEC verifier “surface area” (types + stub entry point) and wires a canister-configurable trust-anchor list through init/upgrade into stable-persisted state, so follow-up PRs in the email-recovery stack can build against the interface.

Changes:

  • Added DnssecConfig / DnssecRootAnchor to the public interface and plumbed dnssec_config through init/post_upgrade into PersistentState (including stable-memory persistence).
  • Introduced a new dnssec/ module in the canister crate with proof-bundle types and a stub verify() plus minimal unit tests.
  • Added a new workspace crate internet_identity_email_test_vectors as a placeholder for upcoming DNSSEC/DKIM/DMARC vectors.

Reviewed changes

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

Show a summary per file
File Description
src/internet_identity/src/storage/storable/storable_persistent_state.rs Persists dnssec_config in the stable-serialized persistent state and updates defaults/tests.
src/internet_identity/src/state.rs Extends PersistentState with dnssec_config and defaults it to None.
src/internet_identity/src/main.rs Exposes dnssec_config in config() and applies it from install/upgrade args.
src/internet_identity/src/dnssec/mod.rs Adds the new DNSSEC module and re-exports scaffolded types/APIs.
src/internet_identity/src/dnssec/types.rs Introduces canister-side DNSSEC proof bundle / RRset / error types.
src/internet_identity/src/dnssec/verify.rs Adds a stub verifier with NoTrustAnchors gating and tests.
src/internet_identity/internet_identity.did Updates the Candid interface with DnssecConfig / DnssecRootAnchor and init field.
src/internet_identity_interface/src/internet_identity/types/dnssec.rs Defines public DnssecConfig and DnssecRootAnchor types.
src/internet_identity_interface/src/internet_identity/types.rs Exposes the new DNSSEC types and adds dnssec_config to InternetIdentityInit.
src/internet_identity_email_test_vectors/src/lib.rs Adds placeholder API for future DNSSEC test vectors.
src/internet_identity_email_test_vectors/README.md Documents intent and what follow-up PRs will add.
src/internet_identity_email_test_vectors/Cargo.toml Adds the new (non-published) crate manifest.
Cargo.toml Registers the new workspace member and workspace path dependency.
Cargo.lock Includes the new local crate entry.

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

Comment thread src/internet_identity/src/dnssec/mod.rs Outdated
Comment thread src/internet_identity_interface/src/internet_identity/types.rs Outdated
sea-snake added 4 commits May 4, 2026 15:06
…scripts

The Dockerfile pre-builds workspace dependencies by COPY-ing each crate's
Cargo.toml and stubbing its lib.rs. Adding internet_identity_email_test_vectors
to Cargo.toml's workspace members without updating these hardcoded lists
causes 'docker-build-base' to fail at the cargo manifest discovery step.

Also add the new crate to the BACKEND_PATHS lists in
.github/actions/release/run.sh and scripts/make-upgrade-proposal so it
is included in release tarballs alongside canister_tests (also test-only).
- Drop the internet_identity_email_test_vectors crate. Test vectors will
  live as plain files at the repo root in PR 1b (still TBD which path);
  no separate crate needed for what is just data-on-disk shared between
  internet_identity unit tests and canister_tests integration tests via
  include_bytes! / fs::read.
- Revert the Dockerfile + release-script entries that registered the
  now-removed crate.
- Switch InternetIdentityInit.dnssec_config from Option<DnssecConfig>
  to Option<Option<DnssecConfig>> to match the same set/clear pattern
  as analytics_config and dummy_auth (per Copilot review). Outer None
  keeps the previously stored value; Some(None) clears; Some(Some(c))
  sets. Avoids a future breaking Candid change if operators ever need
  to detach trust anchors. Updates the consumer in apply_install_arg
  and the round-trip in config(), plus the .did declaration.
- Narrow the dnssec/mod.rs allow from `#![allow(dead_code,
  unused_imports)]` to just `#![allow(dead_code)]`, with per-item
  `#[allow(unused_imports)]` on the two re-exports that need it.
  This keeps unused_imports active so a real issue isn't masked when
  the verifier implementation lands in PR 1b.
Replaces the stub verify() with a working four-step DNSSEC validator
per docs/ongoing/email-recovery.md §7.3:

  1. Validate the bundle's root DNSKEY RRset against a configured trust
     anchor (matches digest of one DNSKEY KSK, then verifies the RRSIG
     covering the entire root DNSKEY RRset under that key).
  2. Walk the delegation chain top-down: each link's DS RRset verifies
     under the parent's DNSKEY, the child's DNSKEY RRset is self-signed
     by a KSK whose digest matches one of the parent's DS records.
  3. Verify the leaf RRset's RRSIG under the deepest zone's DNSKEY.
  4. Freshness check: every RRSIG's [inception, expiration] window must
     contain now ± 60s.

Algorithm coverage (RFC 8624 MUST):
  * 8 — RSA-SHA256, RFC 5702 (root, com, most legacy zones)
  * 13 — ECDSA-P256-SHA256, RFC 6605 (most TLDs, Cloudflare)
  * 15 — Ed25519, RFC 8080
Anything else is rejected with UnsupportedAlgorithm.

New deps: domain (NLnet Labs primitives — currently used for the
docstring/RFC reference frame; signature verification is hand-rolled
on RustCrypto), p256, ed25519-dalek. All wasm32-compatible.

New files:
  - src/internet_identity/src/dnssec/canonical.rs — owner-name
    canonicalization, RR canonical form, RRSIG signed-data
    construction (RFC 4034 §3.1.8.1, §6.2, §6.3), DS digest input.
  - src/internet_identity/src/dnssec/signature.rs — algorithm
    dispatch + DS digest matching.
  - src/internet_identity/src/dnssec/test_vectors.rs — cfg(test)
    JSON loader.
  - test_vectors/dnssec/cloudflare-com-2026-05.json — captured DoH
    chain for cloudflare.com TXT (root → com → cloudflare.com).
    Exercises both alg 8 and alg 13.
  - test_vectors/dnssec/iana-root-anchors-2026-05.json — IANA root
    KSK trust anchors (Klajeyz/2017 + Kmyv6jo/2024).
  - scripts/capture-dnssec-chain.py — reproducible DoH capture script.

Modified:
  - dnssec/types.rs: drop NotImplemented variant (now obsolete).
  - dnssec/verify.rs: replace stub with the real implementation; add
    7 unit tests covering happy path + 6 negative cases.
  - Cargo.toml + internet_identity/Cargo.toml: new deps.

Tests: 238 internet_identity bin tests pass (227 prior + 11 new dnssec
tests). Wasm32 build clean; full workspace cargo check clean (modulo the
unrelated frontend dist/ requirement).

Note on test data lifetime: the captured RRSIGs' validity windows expire
~2-4 weeks after capture. The tests use a 'frozen now' read from the
capture's _meta.captured_at_unix so freshness checks remain stable.
Re-run scripts/capture-dnssec-chain.py to refresh the captures when the
expiration approaches.
The clippy job flagged `#![cfg(test)]` on test_vectors.rs as a
duplicated attribute (the module declaration in dnssec/mod.rs already
gates it with `#[cfg(test)]`). Removed the redundant inner gate.

Also applied rustfmt to the four new dnssec module files so cargo-fmt
stays green.
- Drop unused domain workspace dep. The dep was added during planning when
  we considered using NLnet Labs' domain crate for canonicalization, but
  the verifier ended up hand-rolling everything (canonicalize_name,
  rrsig_rdata_for_signing, rr_canonical, ds_digest_input) on top of
  rsa/sha2/p256/ed25519-dalek alone. Removing it saves a 66-line
  Cargo.lock entry and shrinks dependency surface to no actual cost.
- scripts/capture-dnssec-chain.py docstring example: rename name/rdata/
  signature to name_hex/rdata_hex/signature_hex to match what the script
  actually emits (and what the canister-side test loader expects).
- scripts/capture-dnssec-chain.py --resolver help: was 'cloudflare-dns.com'
  but the actual default is https://1.1.1.1/dns-query. Make the help
  text match the default.
@aterga
Copy link
Copy Markdown
Collaborator

aterga commented May 11, 2026

Grok review

PR Review: #3838 feat(dnssec): scaffold verifier module + canister-side trust-anchor wiring

Status (as of May 11 2026): Still open, latest commit f177bc7 (May 5), CI fully green, 238 tests passing, no merge conflicts, no human reviews yet (only Copilot, which was fully addressed). This is PR 1 / Phase 0 of the 12-PR email-recovery stack.

Overall Assessment

This is a clean, focused, production-ready scaffold. It delivers exactly what the design doc (§10 Phase 0) requires: a full RFC-4035-compliant DNSSEC verifier (root-anchor → chain walk → leaf RRSIG + freshness) plus the minimal canister wiring to make trust anchors configurable and persistent. No user-visible change, no breakage, zero new attack surface in hot paths. The module is intentionally dead-code until the DKIM/DMARC PRs land.

No blockers. This PR is merge-ready today.

Major Security Gaps

None. The verifier is deliberately conservative and aligns perfectly with the threat model in the design doc:

  • DNS poisoning prevented by full chain validation to IANA root KSK DS records (deploy-arg controlled).
  • Stale/future signatures rejected (RRSIG inception/expiration ± 60 s skew for IC subnet jitter).
  • Only RFC 8624 MUST algorithms (8=RSA-SHA256, 13=ECDSA-P256-SHA256, 15=Ed25519); everything else → UnsupportedAlgorithm.
  • DS digests restricted to SHA-256 (type 2); SHA-1 rejected at boundary.
  • Granular DnssecError variants + fail-closed behavior.
  • Deterministic (caller-supplied DnsProofBundle only; no outcalls inside the verifier).
  • Real captured chains (Cloudflare + current IANA anchors) + explicit tampering tests (flipped bytes, wrong anchor, stale signature).

The only theoretical long-term risk is root KSK rollover (handled via dnssec_config upgrade arg, as noted in the design doc). Everything else is mitigated or explicitly out-of-scope for this PR.

Easy-to-Change Improvements (all < 30 min)

These are purely polish / future-proofing. None are required for merge.

  1. Remove #[allow(dead_code)] on dnssec/mod.rs (or gate the whole module behind a feature flag) once feat(dkim): hand-rolled RFC 6376 verifier (PR 2 of email-recovery stack) #3839 lands and the verifier is actually called. Current allowance is correct for a scaffold but should not become permanent.

  2. Add one tiny integration smoke test in main.rs (or state.rs tests) that calls dnssec::verify with the baked-in cloudflare-com-2026-05.json vector + real IANA anchors. This would catch any future persistence/upgrade-regression in the PersistentState path. Copy the frozen_now helper from the unit tests—trivial.

  3. Add a one-line comment next to current_time_secs() and in verify_with_clock explaining the 60 s skew (IC subnet time jitter). Makes the magic number self-documenting for whoever touches it in 2 years.

  4. Mark DnssecConfig / DnssecRootAnchor fields pub(crate) in internet_identity_interface unless the frontend needs them exposed in Candid right now. Least-privilege habit; zero functional impact.

  5. One-pass polish on scripts/capture-dnssec-chain.py docstring: make the example command + expected JSON fields (name_hex, rdata_hex, etc.) exactly match current output. Already improved, but a final sync would help future contributors.

Strategic Recommendations

  1. Merge this PR immediately. It is the bottom layer of the entire email-recovery stack. Every subsequent PR (feat(dkim): hand-rolled RFC 6376 verifier (PR 2 of email-recovery stack) #3839 DKIM verifier onward) depends on these types and the verify API. Bottom-up review is the right strategy—don’t let this block the rest.

  2. Add a synthetic Ed25519 (alg 15) test vector before the full stack ships. The design doc flags it as “rare in production but mandatory.” One extra JSON + one test would close the gap cheaply. (Current coverage is already excellent for real-world alg 8/13.)

  3. Keep the hand-rolled canonicalization/signature code for now. It keeps wasm size and dependency surface minimal (great win after dropping the domain crate). Only revisit if later phases need many more RR types.

  4. Ops/runbook item (post-merge): Add a short section on “updating root anchors on KSK rollover” with the exact upgrade-proposal snippet. Rollover is rare (last one 2018) but high-impact; make it boring.

  5. Broader stack hygiene: Once the whole feature lands, run the capture script against the remaining providers listed in the design doc (Proton, Tutanota, etc.). The infrastructure is already there—zero extra work.

Positive Callouts

  • Excellent test infrastructure (real DoH captures, frozen timestamps, explicit tampering coverage).
  • Config wiring is copy-paste consistent with analytics_config / dummy_auth (including the Option<Option<…>> set/clear semantics).
  • Dependency hygiene was handled immediately (domain crate removed as soon as it became unused).
  • RFC citations and threat-model alignment are first-class throughout.
  • Copilot feedback was actioned cleanly and quickly.

Verdict: Ship it. This is high-quality foundational work that advances a major new authentication primitive without introducing risk. The rest of the stack can now build on solid ground.

If you want a deeper dive on any specific file (e.g., full verify.rs logic, canonicalization edge cases, or persistence serialization) or a review of the next PR in the stack, just link it. Happy to help.

aterga
aterga previously approved these changes May 11, 2026
Comment thread Cargo.toml

# DNSSEC verifier deps (PR 1b on docs/ongoing/email-recovery.md §7)
p256 = { version = "0.13", default-features = false, features = ["ecdsa", "sha256"] }
ed25519-dalek = { version = "2.2", default-features = false }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please double check that these new dependencies can be trusted.

Copy link
Copy Markdown
Contributor Author

@sea-snake sea-snake May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Audit done — full per-crate writeup on the issue thread above. TL;DR:

  • rsa 0.9.10 (bumped from 0.9.7 in 2efff9ad): one open advisory (RUSTSEC-2023-0071, Marvin Attack) but it doesn't apply — Marvin is a private-key timing attack; this canister only does public-key verification.
  • p256 0.13.2: clean. zkSecurity audit (April 2025).
  • ed25519-dalek 2.2.0: clean. Quarkslab audit + production use across Solana / NEAR / Polkadot / Zcash.[


use super::types::{Rrsig, SignedRRset};

const CLASS_IN: u16 = 1;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a doc string

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e77f7997. The CLASS_IN const moved inline into rr_canonical() (single use, per the no-over-hoisting feedback) with a doc comment citing the two relevant RFC sections:

/// CLASS IN (Internet) per RFC 1035 §3.2.4. DNSSEC signatures
/// cover only records of a specific class (RFC 4034 §6.2), and
/// every record we look at is class IN.
const CLASS_IN: u16 = 1;

/// the top two bits set is a compression pointer (RFC 1035 §4.1.4) which
/// must NOT appear in canonical form, so we treat its presence as an
/// invalid input.
pub fn canonicalize_name(wire: &[u8]) -> Vec<u8> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use a library for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigated, decided to keep hand-rolled. Two candidates evaluated:

domain (NLnet Labs) — was originally in this PR, dropped before review. Signing / canonical-encoding sits behind the unstable-sign feature, which transitively requires unstable-crypto, bytes + std, and pulls smallvec, serde, tracing, secrecy — not wasm-friendly without giving up our minimal dep surface. More fundamentally, the maintainers acknowledge the RRset / canonical-order helpers are still missing (NLnetLabs/domain#606 — "Cascade does things manually"). Even after eating those deps we'd still hand-roll the sort and the TBS bytes.

hickory-proto — has the exact primitive (TBS::from_input) but the __dnssec feature requires std + a separate crypto backend (dnssec-ring / dnssec-aws-lc-rs). On wasm32-unknown-unknown only ring works and it bloats the canister substantially. We'd also carry two crypto stacks (theirs + our rsa/p256/ed25519-dalek). Three open RustSec advisories in 2025–2026 on hickory-proto (NSEC3 loop, name-compression DoS, DNSKEY self-sig); none hit our subset, but it shows the attack surface of pulling a full DNS protocol stack for ~90 lines of byte-pushing.

Hand-rolled state after e77f7997 + 8d770a20: ~90 lines of actual logic, zero new wasm deps. Three of the four functions are linear byte concatenation; the fourth is one sort_by on owned Vec<u8>. Every constant traces to a named RFC section (RFC 1035 §3.2.1 / §3.2.4, RFC 4034 §3.1.x / §6.2 / §6.3 / §5.1.4) and lives next to its using function.

If the rest of the email-recovery stack later needs many more RR types or a full resolver, we can revisit.

/// Build the canonical-form serialization of one RR (used for both
/// signed-data construction and DS digest input).
fn rr_canonical(name_canonical: &[u8], rtype: u16, original_ttl: u32, rdata: &[u8]) -> Vec<u8> {
let mut out = Vec::with_capacity(name_canonical.len() + 2 + 2 + 4 + 2 + rdata.len());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This capacity equation is highly error prone.

Please refactor it to avoid implicit references to constants, which don't even trivially map to the list of function args.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored in e77f7997. Field-size constants are now named and live inside rr_canonical (per the no-over-hoisting feedback):

fn rr_canonical(name_canonical: &[u8], rtype: u16, original_ttl: u32, rdata: &[u8]) -> Vec<u8> {
    /// Sum of the four fixed-width fields between the owner name and
    /// the RDATA in canonical form (TYPE | CLASS | TTL | RDLENGTH):
    /// 2 + 2 + 4 + 2 = 10 (RFC 1035 §3.2.1 + RFC 4034 §6.2).
    const FIXED_HEADER_LEN: usize = 10;
    /// CLASS IN (Internet) per RFC 1035 §3.2.4.
    const CLASS_IN: u16 = 1;

    let mut out = Vec::with_capacity(name_canonical.len() + FIXED_HEADER_LEN + rdata.len());
    out.extend_from_slice(name_canonical);
    out.extend_from_slice(&rtype.to_be_bytes());
    out.extend_from_slice(&CLASS_IN.to_be_bytes());
    out.extend_from_slice(&original_ttl.to_be_bytes());
    let rdlength = u16::try_from(rdata.len()).expect("RDATA longer than 65535 bytes");
    out.extend_from_slice(&rdlength.to_be_bytes());
    out.extend_from_slice(rdata);
    debug_assert_eq!(
        out.len(),
        name_canonical.len() + FIXED_HEADER_LEN + rdata.len(),
        "FIXED_HEADER_LEN out of sync with rr_canonical layout",
    );
    out
}

Same treatment applied to rrsig_rdata_for_signing (the bare 18 for the eight fixed RRSIG fields became FIXED_FIELDS_LEN with the same per-field RFC breakdown). Truly cross-module constants (DNSKEY_RDATA_HEADER_LEN, DS_RDATA_HEADER_LEN, DS_DIGEST_TYPE_SHA256) moved to a small wire.rs; everything else stays close to its using function.

@@ -0,0 +1,169 @@
//! Canonical encoding helpers for DNSSEC verification.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should ideally replace this thole file with a library

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigated, decided to keep hand-rolled. Two candidates evaluated:

domain (NLnet Labs) — was originally in this PR, dropped before review. Signing / canonical-encoding sits behind the unstable-sign feature, which transitively requires unstable-crypto, bytes + std, and pulls smallvec, serde, tracing, secrecy — not wasm-friendly without giving up our minimal dep surface. More fundamentally, the maintainers acknowledge the RRset / canonical-order helpers are still missing (NLnetLabs/domain#606 — "Cascade does things manually"). Even after eating those deps we'd still hand-roll the sort and the TBS bytes.

hickory-proto — has the exact primitive (TBS::from_input) but the __dnssec feature requires std + a separate crypto backend (dnssec-ring / dnssec-aws-lc-rs). On wasm32-unknown-unknown only ring works and it bloats the canister substantially. We'd also carry two crypto stacks (theirs + our rsa/p256/ed25519-dalek). Three open RustSec advisories in 2025–2026 on hickory-proto (NSEC3 loop, name-compression DoS, DNSKEY self-sig); none hit our subset, but it shows the attack surface of pulling a full DNS protocol stack for ~90 lines of byte-pushing.

Hand-rolled state after e77f7997 + 8d770a20: ~90 lines of actual logic, zero new wasm deps. Three of the four functions are linear byte concatenation; the fourth is one sort_by on owned Vec<u8>. Every constant traces to a named RFC section (RFC 1035 §3.2.1 / §3.2.4, RFC 4034 §3.1.x / §6.2 / §6.3 / §5.1.4) and lives next to its using function.

If the rest of the email-recovery stack later needs many more RR types or a full resolver, we can revisit.

Comment thread scripts/capture-dnssec-chain.py Outdated
@@ -0,0 +1,227 @@
#!/usr/bin/env python3
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only keep this if its not dead code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in e77f7997. Used it once more first, to generate four additional captures — proton.me, protonmail.com, tutanota.com (the email-recovery target domains from design doc §7.6) plus ed25519.nl (closes the real-data Ed25519 gap Grok flagged). With those five captures committed, the script's job is done; re-capture on a KSK rollover is a one-off ad-hoc effort if it ever becomes necessary.

@aterga
Copy link
Copy Markdown
Collaborator

aterga commented May 11, 2026

Please run appropriate agent queries to analyze the new library deps.

- Replace magic numbers across the dnssec module with named
  constants, co-located inside their using function when single-use
  and hoisted to wire.rs only when shared across modules. Every
  RDATA field offset and length now traces back to a named RFC
  reference.

- Beef up RFC citations on every public item: doc comments reference
  specific RFC 4033 / 4034 / 4035 / 5702 / 6605 / 8080 / 8624 sections
  rather than handwaving at "DNSSEC". Algorithm numbers in narrative
  text use the algorithm name (RSA-SHA256, ECDSA-P256-SHA256,
  Ed25519) instead of the bare IANA number.

- Add real-data test coverage for the four email-recovery target
  zones and the Ed25519 algorithm:
    * proton.me      — RSA-SHA256 end-to-end
    * protonmail.com — ECDSA-P256-SHA256 leaf
    * tutanota.com   — ECDSA-P256-SHA256 leaf
    * ed25519.nl     — Ed25519 leaf (closes the alg-15 real-data gap)

  Brings real-data algorithm coverage to the full RFC 8624 MUST set.
  Tests count: 243 (was 238).

- Remove scripts/capture-dnssec-chain.py — the captures committed in
  this change cover every algorithm and every email-recovery target
  the design doc names, so the script's job is done. Re-capture on
  root KSK rollover will be a one-off ad-hoc effort if it ever
  becomes necessary.
@github-actions github-actions Bot dismissed aterga’s stale review May 11, 2026 14:30

Review dismissed by automation script.

sea-snake added 4 commits May 11, 2026 14:35
Three small fixes that lived on feat/doh-fallback as part of a
combined "Copilot review" commit (516c48d) properly belong in
PR 1 since they all concern the dnssec scaffold:

- verify.rs: handle the multi-anchor case correctly. During a KSK
  rollover the operator configures both the rolling-out and
  rolling-in KSKs (RFC 5011 §2). The old "return on first digest
  match" strategy could short-circuit onto the inactive anchor and
  never try the active one. Track digest-match state, try every
  candidate, and surface the cryptographic failure (not the generic
  RootAnchorMismatch) when at least one anchor matched but no
  signature verified.

- mod.rs: doc-comment correctness. The verifier itself makes no
  outcalls, but the email-recovery stack uses DoH for unsigned
  domains (PR 4). The previous wording overclaimed.

- iana-root-anchors-2026-05.json: corrected stale "_comment" that
  said the historical 19036 KSK was included. It isn't, and
  shouldn't be — production deployments should configure 20326 +
  38696 only.
Three improvements that lived as commits on later branches in the
email-recovery stack are scaffold-level — they belong in PR 1 with
the rest of the verifier, not buried in feature PRs that happen to
need them. Back-port them here so PR 1 ships a complete scaffold
that downstream feature PRs can build on without extending the
verifier surface in passing.

- Two-phase API split (originally on feat/email-recovery-storage-and-smtp
  as cfcc99e): expose `verify_root_dnskey_with_clock`,
  `verify_chain_with_clock`, `verify_extra_chains_with_clock`,
  and `verify_hops_with_clock` as standalone entry points alongside
  the top-level `verify_bundle_with_clock`. Two-phase callers
  (prepare → submit-leaf) can now cache a validated `ZoneKeysMap`
  across calls and only validate the new chains a follow-up
  submission crossed into, avoiding 3-5 RSA verifies per call.

- Multi-zone + CNAME-aware bundle shape (originally feat/email-recovery-flow
  as 43a466e): real DKIM resolution often crosses zone boundaries via
  CNAME (proton.me → proton.ch, tutanota.com → tutanota.de, M365 custom
  domains). Replace the old single-chain `DnsProofBundle { root_dnskey,
  chain, leaf }` with `DnsProofBundle { root_dnskey, chains, hops }`
  where each chain pins a signing zone in the new `ZoneKeysMap` and
  each hop is verified under whichever zone its `RRSIG.signer_name`
  identifies. Adds CNAME-chain coherence checks (first owner matches
  requested name, intermediates are CNAMEs whose target equals the
  next owner, no loops, ≤ MAX_CNAME_HOPS = 4) and four new error
  variants (DuplicateZone, UnknownSigningZone, HopOwnerOutsideZone,
  BadCnameChain, TooManyHops).

- DNSKEY-RRset RRSIG fix (originally feat/email-recovery-flow as 6788020):
  loosen step 2c of `verify_link` to verify the child DNSKEY rrset
  against any key in itself, not just the DS-pinned KSK. Real-world
  zones (proton.me, proton.ch, …) publish a DNSKEY rrset signed by
  both the KSK and the ZSK, and resolvers return whichever RRSIG
  comes first; resolving this on the FE side isn't generally possible.
  Step 2b still pins the DS-referenced KSK as part of the rrset, so
  the chain of trust is intact regardless of which RRSIG we verify.

Test-vector JSONs migrated to the new `{root_dnskey, chains, hops}`
shape. Five new tests cover the new failure modes (duplicate zone,
too many hops, hop owner mismatch, wrong requested type, subdomain
boundary). All five real-data chains still verify end-to-end.

Counts: 248 tests pass (was 243); cargo clippy clean with -D warnings;
wasm32 build clean.
Two-phase consumers (prepare → submit-leaf in the email-recovery
flow, landing in PR 5+6) need to inspect the validated zones after
the chain walk — typically to extract the single zone DNSKEY when
the skeleton bundle is single-zone, or to enumerate the map when
caching it across calls. Adding a read-only iterator is the
minimum surface change to support both.

Insertion order matches the order delegation chains were verified
in, which is the order callers supplied them.
Minor-version bumps inside the 0.9 line — semver-compatible, no
API changes. Picks up the defensive validations added in 0.9.7
(`RsaPrivateKey::from_components` always validates keys, PKCS#1 v1.5
no longer panics on tiny keys) and 0.9.8.

Does not address RUSTSEC-2023-0071 (Marvin Attack timing
sidechannel) — there's still no patched version on the 0.9.x line
and 0.10.0 is in -rc. Note that the Marvin Attack is a private-key
recovery attack on RSA decryption/signing; this canister only does
`RsaPublicKey::verify(...)` of caller-supplied bytes, so the
threat doesn't apply. We can document that with a `cargo audit`
ignore-with-justification when we add the audit step.

ed25519-dalek 2.2.0 and p256 0.13.2 are already on their latest
0.x.y stable.
@sea-snake
Copy link
Copy Markdown
Contributor Author

Thanks. Round-2 push addresses the polish suggestions:

  1. #[allow(dead_code)] on dnssec/mod.rs — still there; removing it now would warning-spam the build because the public surface is genuinely unused at this point in the stack. Drops out automatically when the first consumer (PR 2's DKIM verifier / PR 5+6's recovery methods) lands.
  2. Tiny integration smoke test — went a bit further: five real-data chain captures now exercise the verifier end-to-end, covering every RFC 8624 MUST algorithm (RSA-SHA256, ECDSA-P256-SHA256, Ed25519) with real DoH-captured chains for every email-recovery target in design doc §7.6. 23 dnssec tests pass (was 18). [e77f7997, 8d770a20]
  3. CLOCK_SKEW_SECS rationale comment — added (BIND default + IC subnet-jitter explanation). [e77f7997]
  4. pub(crate) on DnssecConfig / DnssecRootAnchor — these are deploy/upgrade-arg fields, so they need to stay pub for Candid + the init-arg shape. The internet_identity_interface crate is the canonical place for caller-visible types.
  5. Capture-script polish — actually removed in e77f7997 after generating the four extra captures it was needed for. The five frozen captures cover all algorithms; re-capture on a KSK rollover is a one-off ad-hoc effort if it ever becomes necessary.

Strategic suggestions:

  • Synthetic Ed25519 vector — landed as ed25519.nl A real-data capture (chain: RSA-SHA256 root → ECDSA-P256-SHA256 nl → Ed25519 leaf).
  • Hand-rolled canonicalization kept — see the library-vs-hand-rolled response on the canonical.rs thread.
  • More provider captures — proton.me, protonmail.com, tutanota.com all in test_vectors/dnssec/.

@sea-snake
Copy link
Copy Markdown
Contributor Author

sea-snake commented May 11, 2026

Done — formal trust audit per crate:

Crate Verdict Note
rsa 0.9.10 (bumped from 0.9.7 in 2efff9ad) SHIP WITH CAVEAT RUSTSEC-2023-0071 / GHSA-c38w-74pg-36hr (Marvin Attack, CVSS 5.9 medium) — still no patched version on the 0.9.x line. Doesn't apply to this canister's threat model: Marvin is a private-key recovery attack via timing on RSA decryption/signing. We only call RsaPublicKey::verify(Pkcs1v15Sign, …) on caller-supplied bytes — pure public-key verification, no private keys, no decryption.
p256 0.13.2 SHIP Recently audited (zkSecurity, April 2025, commissioned by NEAR). Three findings; none affect ECDSA verification. RustCrypto, actively maintained.
ed25519-dalek 2.2.0 SHIP Quarkslab audit (2019, pre-2.x). Production use across Solana, NEAR, Polkadot, Zcash, Substrate. default-features = false is the canonical wasm config. curve25519-dalek 4.1.3+ (the timing fix for RUSTSEC-2024-0344) is what cargo tree picks up.

No IC-ecosystem swap is viable: ic-canister-sig-creation is IC-signature-scheme specific, ic-secp256k1 is the wrong curve (RFC 6605 mandates P-256). The RustCrypto/dalek crates are the standard pure-Rust, wasm-compatible implementations for DNSSEC's MUST algorithms.

Sources:

@sea-snake sea-snake enabled auto-merge May 11, 2026 17:24
@sea-snake sea-snake disabled auto-merge May 11, 2026 17:25
@sea-snake sea-snake added this pull request to the merge queue May 12, 2026
Merged via the queue into dfinity:main with commit 70e92cc May 12, 2026
43 checks passed
@sea-snake sea-snake deleted the feat/dnssec-verifier-scaffold branch May 12, 2026 09:13
sea-snake added a commit that referenced this pull request May 12, 2026
…l args

Email recovery (PRs #3838-#3844) needs two new BE init args set on
the canister: `dnssec_config.root_anchors` (the IANA root KSKs the
DNSSEC verifier trusts) and `doh_config.allowed_domains` (the
mailbox-provider domains that can use the DoH-fallback path). This
PR plumbs both through the three deploy scripts.

What landed:

- **`scripts/fetch-iana-root-anchors.bash`** — new module, sourced
  by the other deploy scripts. `fetch_and_review_iana_root_anchors`:
  - `curl`s `data.iana.org/root-anchors/root-anchors.xml` (retry
    + bounded timeout).
  - Parses every `<KeyDigest>` block with Node, filtering to those
    whose `validFrom`/`validUntil` window contains "now" — drops
    retired keys (Kjqmt7v expired 2019) and keeps the active KSKs
    (Klajeyz key_tag=20326, Kmyv6jo key_tag=38696 today).
  - Prints a one-line human summary per entry to stderr (id +
    key_tag + algo + digest_type + digest preview + validity
    window).
  - Prompts for confirmation on /dev/tty (gracefully falls back
    to "accept default" when there's no controlling terminal,
    so CI / sandboxes don't deadlock).
  - Writes Candid `vec record { key_tag = ...; algorithm = ...;
    digest_type = ...; digest = blob "..."; }` to stdout, ready
    to drop into `dnssec_config.root_anchors`.

- **`scripts/deploy-common.bash`** (consumed by `deploy-pr-to-beta`
  and `deploy-local-to-beta`):
  - New `DEFAULT_DOH_ALLOWED_DOMAINS` list — gmail, googlemail,
    outlook, hotmail, live, icloud, me, mac, yahoo, protonmail,
    proton.me, pm.me. Adding a domain is operator config, not
    code; override per-deploy with `--doh-domains <a,b,c,...>`.
    Empty list = DNSSEC-only, no DoH fallback.
  - New `--skip-email-recovery-init` flag — when set, leave both
    fields as `opt null` (preserve previous on-chain values).
    Default is to fetch fresh anchors + use the curated DoH
    allowlist, since the staging canisters are still in the
    "needs first-run init" stage.
  - `build_be_install_arg` now emits `dnssec_config = ...` and
    `doh_config = ...` alongside the existing trio (BE id,
    BE_URL, FE_URL).

- **`scripts/make-upgrade-proposal`** (production): didc-assist's
  interactive flow doesn't know how to type a 32-byte SHA-256
  digest as Candid blob, so we *post-process* the args.txt
  it produces:
  - After didc-assist writes the file (or the trivial `(null)`
    when the user accepts the no-update default), call
    `patch_email_recovery_init_fields`.
  - The patcher uses Node to find/replace the `dnssec_config` and
    `doh_config` fields in the Candid arg text — or insert them
    inside the outermost `record { ... }` if didc-assist didn't
    emit them — with values from the same IANA fetch + DoH
    allowlist. The user can opt out with
    `II_SKIP_EMAIL_RECOVERY_INIT=1` for upgrades where the on-chain
    values are already correct, and override the DoH allowlist
    via `II_DOH_DOMAINS=...`.
  - Both staging and production paths share the patcher; encoded
    Candid round-trips through `didc encode` cleanly in all three
    edge cases tested locally: existing fields = null, fields
    absent, args.txt = `(null)`.

The reason we don't bundle the IANA anchors into the wasm is that
they roll over every ~7 years; the next rollover becomes a
one-line change in the next upgrade arg instead of a code change
+ recompile.
sea-snake added a commit that referenced this pull request May 12, 2026
…l args

Email recovery (PRs #3838-#3844) needs two new BE init args set on
the canister: `dnssec_config.root_anchors` (the IANA root KSKs the
DNSSEC verifier trusts) and `doh_config.allowed_domains` (the
mailbox-provider domains that can use the DoH-fallback path). This
PR plumbs both through the three deploy scripts.

What landed:

- **`scripts/fetch-iana-root-anchors.bash`** — new module, sourced
  by the other deploy scripts. `fetch_and_review_iana_root_anchors`:
  - `curl`s `data.iana.org/root-anchors/root-anchors.xml` (retry
    + bounded timeout).
  - Parses every `<KeyDigest>` block with Node, filtering to those
    whose `validFrom`/`validUntil` window contains "now" — drops
    retired keys (Kjqmt7v expired 2019) and keeps the active KSKs
    (Klajeyz key_tag=20326, Kmyv6jo key_tag=38696 today).
  - Prints a one-line human summary per entry to stderr (id +
    key_tag + algo + digest_type + digest preview + validity
    window).
  - Prompts for confirmation on /dev/tty (gracefully falls back
    to "accept default" when there's no controlling terminal,
    so CI / sandboxes don't deadlock).
  - Writes Candid `vec record { key_tag = ...; algorithm = ...;
    digest_type = ...; digest = blob "..."; }` to stdout, ready
    to drop into `dnssec_config.root_anchors`.

- **`scripts/deploy-common.bash`** (consumed by `deploy-pr-to-beta`
  and `deploy-local-to-beta`):
  - New `DEFAULT_DOH_ALLOWED_DOMAINS` list — gmail, googlemail,
    outlook, hotmail, live, icloud, me, mac, yahoo, protonmail,
    proton.me, pm.me. Adding a domain is operator config, not
    code; override per-deploy with `--doh-domains <a,b,c,...>`.
    Empty list = DNSSEC-only, no DoH fallback.
  - New `--skip-email-recovery-init` flag — when set, leave both
    fields as `opt null` (preserve previous on-chain values).
    Default is to fetch fresh anchors + use the curated DoH
    allowlist, since the staging canisters are still in the
    "needs first-run init" stage.
  - `build_be_install_arg` now emits `dnssec_config = ...` and
    `doh_config = ...` alongside the existing trio (BE id,
    BE_URL, FE_URL).

- **`scripts/make-upgrade-proposal`** (production): didc-assist's
  interactive flow doesn't know how to type a 32-byte SHA-256
  digest as Candid blob, so we *post-process* the args.txt
  it produces:
  - After didc-assist writes the file (or the trivial `(null)`
    when the user accepts the no-update default), call
    `patch_email_recovery_init_fields`.
  - The patcher uses Node to find/replace the `dnssec_config` and
    `doh_config` fields in the Candid arg text — or insert them
    inside the outermost `record { ... }` if didc-assist didn't
    emit them — with values from the same IANA fetch + DoH
    allowlist. The user can opt out with
    `II_SKIP_EMAIL_RECOVERY_INIT=1` for upgrades where the on-chain
    values are already correct, and override the DoH allowlist
    via `II_DOH_DOMAINS=...`.
  - Both staging and production paths share the patcher; encoded
    Candid round-trips through `didc encode` cleanly in all three
    edge cases tested locally: existing fields = null, fields
    absent, args.txt = `(null)`.

The reason we don't bundle the IANA anchors into the wasm is that
they roll over every ~7 years; the next rollover becomes a
one-line change in the next upgrade arg instead of a code change
+ recompile.
sea-snake added a commit that referenced this pull request May 12, 2026
…l args

Email recovery (PRs #3838-#3844) needs two new BE init args set on
the canister: `dnssec_config.root_anchors` (the IANA root KSKs the
DNSSEC verifier trusts) and `doh_config.allowed_domains` (the
mailbox-provider domains that can use the DoH-fallback path). This
PR plumbs both through the three deploy scripts.

What landed:

- **`scripts/fetch-iana-root-anchors.bash`** — new module, sourced
  by the other deploy scripts. `fetch_and_review_iana_root_anchors`:
  - `curl`s `data.iana.org/root-anchors/root-anchors.xml` (retry
    + bounded timeout).
  - Parses every `<KeyDigest>` block with Node, filtering to those
    whose `validFrom`/`validUntil` window contains "now" — drops
    retired keys (Kjqmt7v expired 2019) and keeps the active KSKs
    (Klajeyz key_tag=20326, Kmyv6jo key_tag=38696 today).
  - Prints a one-line human summary per entry to stderr (id +
    key_tag + algo + digest_type + digest preview + validity
    window).
  - Prompts for confirmation on /dev/tty (gracefully falls back
    to "accept default" when there's no controlling terminal,
    so CI / sandboxes don't deadlock).
  - Writes Candid `vec record { key_tag = ...; algorithm = ...;
    digest_type = ...; digest = blob "..."; }` to stdout, ready
    to drop into `dnssec_config.root_anchors`.

- **`scripts/deploy-common.bash`** (consumed by `deploy-pr-to-beta`
  and `deploy-local-to-beta`):
  - New `DEFAULT_DOH_ALLOWED_DOMAINS` list — gmail, googlemail,
    outlook, hotmail, live, icloud, me, mac, yahoo, protonmail,
    proton.me, pm.me. Adding a domain is operator config, not
    code; override per-deploy with `--doh-domains <a,b,c,...>`.
    Empty list = DNSSEC-only, no DoH fallback.
  - New `--skip-email-recovery-init` flag — when set, leave both
    fields as `opt null` (preserve previous on-chain values).
    Default is to fetch fresh anchors + use the curated DoH
    allowlist, since the staging canisters are still in the
    "needs first-run init" stage.
  - `build_be_install_arg` now emits `dnssec_config = ...` and
    `doh_config = ...` alongside the existing trio (BE id,
    BE_URL, FE_URL).

- **`scripts/make-upgrade-proposal`** (production): didc-assist's
  interactive flow doesn't know how to type a 32-byte SHA-256
  digest as Candid blob, so we *post-process* the args.txt
  it produces:
  - After didc-assist writes the file (or the trivial `(null)`
    when the user accepts the no-update default), call
    `patch_email_recovery_init_fields`.
  - The patcher uses Node to find/replace the `dnssec_config` and
    `doh_config` fields in the Candid arg text — or insert them
    inside the outermost `record { ... }` if didc-assist didn't
    emit them — with values from the same IANA fetch + DoH
    allowlist. The user can opt out with
    `II_SKIP_EMAIL_RECOVERY_INIT=1` for upgrades where the on-chain
    values are already correct, and override the DoH allowlist
    via `II_DOH_DOMAINS=...`.
  - Both staging and production paths share the patcher; encoded
    Candid round-trips through `didc encode` cleanly in all three
    edge cases tested locally: existing fields = null, fields
    absent, args.txt = `(null)`.

The reason we don't bundle the IANA anchors into the wasm is that
they roll over every ~7 years; the next rollover becomes a
one-line change in the next upgrade arg instead of a code change
+ recompile.
aterga added a commit that referenced this pull request May 12, 2026
Addresses the four "easy-to-change improvements" from Grok's review on
#3877
(#3877 (review)).
Targets `feat/dkim-verifier` so it can be merged in as a follow-up
commit to that PR.

## What changed

1. **`types.rs` — `NoSignature` doc-comment.** The distinct variant Grok
asked for (`NoSignaturePresent` or equivalent) already exists as
`VerificationFailReason::NoSignature` at types.rs:84 and is returned
from `verify()` 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.

2. **`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 in `split_tag_list` → `get(name)`;
unknown tags pass straight through — the existing `happy_value()`
fixture even contains an unknown `q=dns/txt`). Pinned the behaviour with
a dedicated `unknown_tags_are_ignored` test covering `z=` and a
synthetic `zz=`.

3. **`verify.rs` — SECURITY comment at the trust boundary.** Added an
inline `// SECURITY:` block at the `parse_dkim_txt` call site stating
that `dkim_txt` is trusted, sourced from the DNSSEC verifier (#3838) or
pinned-host DoH outcall (#3879).

## Not actioned

- **CI job rename (`dkim-test-vectors` → `dkim-verifier-tests`).** No
such job exists in `.github/workflows/`; PR #3877 doesn't add one.
Nothing to rename.
- **`main.rs::handle_email_recovery` match 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 --check` on the three modified files — clean (other
pre-existing fmt drifts in `dnssec/`, `openid/`, `tests/integration/`
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-translation-bot pushed a commit to sea-snake-translation-bot/internet-identity that referenced this pull request May 12, 2026
…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>
sea-snake added a commit that referenced this pull request May 13, 2026
…l args

Email recovery (PRs #3838-#3844) needs two new BE init args set on
the canister: `dnssec_config.root_anchors` (the IANA root KSKs the
DNSSEC verifier trusts) and `doh_config.allowed_domains` (the
mailbox-provider domains that can use the DoH-fallback path). This
PR plumbs both through the three deploy scripts.

What landed:

- **`scripts/fetch-iana-root-anchors.bash`** — new module, sourced
  by the other deploy scripts. `fetch_and_review_iana_root_anchors`:
  - `curl`s `data.iana.org/root-anchors/root-anchors.xml` (retry
    + bounded timeout).
  - Parses every `<KeyDigest>` block with Node, filtering to those
    whose `validFrom`/`validUntil` window contains "now" — drops
    retired keys (Kjqmt7v expired 2019) and keeps the active KSKs
    (Klajeyz key_tag=20326, Kmyv6jo key_tag=38696 today).
  - Prints a one-line human summary per entry to stderr (id +
    key_tag + algo + digest_type + digest preview + validity
    window).
  - Prompts for confirmation on /dev/tty (gracefully falls back
    to "accept default" when there's no controlling terminal,
    so CI / sandboxes don't deadlock).
  - Writes Candid `vec record { key_tag = ...; algorithm = ...;
    digest_type = ...; digest = blob "..."; }` to stdout, ready
    to drop into `dnssec_config.root_anchors`.

- **`scripts/deploy-common.bash`** (consumed by `deploy-pr-to-beta`
  and `deploy-local-to-beta`):
  - New `DEFAULT_DOH_ALLOWED_DOMAINS` list — gmail, googlemail,
    outlook, hotmail, live, icloud, me, mac, yahoo, protonmail,
    proton.me, pm.me. Adding a domain is operator config, not
    code; override per-deploy with `--doh-domains <a,b,c,...>`.
    Empty list = DNSSEC-only, no DoH fallback.
  - New `--skip-email-recovery-init` flag — when set, leave both
    fields as `opt null` (preserve previous on-chain values).
    Default is to fetch fresh anchors + use the curated DoH
    allowlist, since the staging canisters are still in the
    "needs first-run init" stage.
  - `build_be_install_arg` now emits `dnssec_config = ...` and
    `doh_config = ...` alongside the existing trio (BE id,
    BE_URL, FE_URL).

- **`scripts/make-upgrade-proposal`** (production): didc-assist's
  interactive flow doesn't know how to type a 32-byte SHA-256
  digest as Candid blob, so we *post-process* the args.txt
  it produces:
  - After didc-assist writes the file (or the trivial `(null)`
    when the user accepts the no-update default), call
    `patch_email_recovery_init_fields`.
  - The patcher uses Node to find/replace the `dnssec_config` and
    `doh_config` fields in the Candid arg text — or insert them
    inside the outermost `record { ... }` if didc-assist didn't
    emit them — with values from the same IANA fetch + DoH
    allowlist. The user can opt out with
    `II_SKIP_EMAIL_RECOVERY_INIT=1` for upgrades where the on-chain
    values are already correct, and override the DoH allowlist
    via `II_DOH_DOMAINS=...`.
  - Both staging and production paths share the patcher; encoded
    Candid round-trips through `didc encode` cleanly in all three
    edge cases tested locally: existing fields = null, fields
    absent, args.txt = `(null)`.

The reason we don't bundle the IANA anchors into the wasm is that
they roll over every ~7 years; the next rollover becomes a
one-line change in the next upgrade arg instead of a code change
+ recompile.
aterga added a commit that referenced this pull request May 13, 2026
…of email-recovery stack) (#3878)

## 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

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

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

---------

Co-authored-by: Arshavir Ter-Gabrielyan <arshavir.ter.gabrielyan@dfinity.org>
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants