NAT: ICMP Error checksums validation clean-up

[qmonnet]

Some clean-ups for checksum validation for ICMP Error messages.

Please refer to individual commits for details.

Fixes: #1586

[Copilot]

Pull request overview

This PR refactors ICMP error handling around checksum validation and inner-packet translation, aiming to centralize validation in the ICMP error handler stage and simplify stateless NAT’s ICMP-specific logic (per issue #1586).

Changes:

• Added a Packet::is_icmp_error_with_embedded_packet() helper and simplified ICMP error detection.
• Moved ICMP checksum validation usage into IcmpErrorHandler via the shared validate_checksums_icmp() helper, removing the bespoke validation function.
• Simplified stateless NAT ICMP-inner-translation gating by computing an icmp_err boolean once and reusing it for both NAT directions.

Reviewed changes

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

File	Description
net/src/packet/utils.rs	Adds a packet helper to detect ICMP errors that include an embedded fragment.
nat/src/stateless/nf.rs	Simplifies ICMP inner translation gating and relies on earlier pipeline-stage validation.
nat/src/icmp_handler/nf.rs	Switches to shared ICMP checksum validation and removes the local validator.
nat/src/icmp_handler/icmp_error_msg.rs	Cleans up checksum-validation errors/mappings and adjusts tests for the new behavior.

[coderabbitai]

Warning

Review limit reached

@qmonnet, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 53 minutes and 22 seconds. Learn how PR review limits work.

To continue reviewing without waiting, enable usage-based billing in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ac39dcfa-746d-4207-aa08-ca99902dd2cc

📥 Commits

Reviewing files that changed from the base of the PR and between 67f56bd and 02e8ec7.

📒 Files selected for processing (10)

• nat/src/icmp_handler/icmp_error_msg.rs
• nat/src/icmp_handler/nf.rs
• net/src/flows/flow_key.rs
• net/src/headers/embedded.rs
• net/src/icmp4/truncated.rs
• net/src/icmp6/truncated.rs
• net/src/packet/icmp_err.rs
• net/src/packet/mod.rs
• net/src/tcp/truncated.rs
• net/src/udp/truncated.rs

📝 Walkthrough

Walkthrough

ICMP checksum validation now separates non-ICMP packets from ICMP error messages, and static NAT uses a precomputed embedded-ICMP flag for inner-packet translation. The ICMP handler now calls the shared validator, and packet utilities expose embedded-header detection.

Changes

ICMP validation and NAT flow updates

Layer / File(s)	Summary
Packet ICMP inspection helpers net/src/packet/utils.rs	is_icmp_error() is simplified, and is_icmp_error_with_embedded_packet() is added to detect ICMP error packets that include embedded headers.
ICMP error validation and outcomes nat/src/icmp_handler/icmp_error_msg.rs	IcmpErrorMsgError adds NotIcmp; DoneReason mappings change for non-ICMP and missing embedded headers; validate_checksums_icmp() returns Err(NotIcmp) for non-ICMP packets and Ok(()) for ICMP query messages, and tests cover the new checksum and embedded-header outcomes.
ICMP handler uses shared checksum validation nat/src/icmp_handler/nf.rs	The ICMP error handler imports validate_checksums_icmp, removes its local checksum helper, calls the shared validator in handle_icmp_error_msg(), and drops the explicit packet.update_checksums() call before static NAT metadata is set.
Static NAT threads embedded-ICMP flag nat/src/static_nat/nf.rs	Static NAT removes is_icmp_inner_translation_candidate, computes icmp_err once in translate() with is_icmp_error_with_embedded_packet(), passes it into source_nat() and destination_nat(), gates embedded inner translation on that flag, and narrows the reprocessing filter to embedded ICMP error packets.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: ICMP error checksum validation cleanup in NAT.
Description check	✅ Passed	The description is related to the checksum-validation cleanup and references the linked fix.
Linked Issues check	✅ Passed	The changes align with #1586 by validating ICMP error checksums once in shared handling before inner NAT translation.
Out of Scope Changes check	✅ Passed	The changed files stay focused on ICMP checksum handling and supporting NAT logic; no clear unrelated additions stand out.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

nat/src/stateless/nf.rs (1)

270-283: ICMP checksum validation is enforced before StatelessNat in the dataplane pipeline

dataplane/src/packet_processor/mod.rs adds icmp_error_handler before static_nat (StatelessNat), and IcmpErrorHandler::process drops packets that fail validate_checksums_icmp(...), so the skip of local checksum validation in nat/src/stateless/nf.rs (ICMP error NAT fast path) matches the pipeline contract.

Remaining gap: nat/src/stateless/test.rs calls StatelessNat::process directly for ICMP error cases (no IcmpErrorHandler stage), so invalid-checksum behavior depends on caller/test preconditions—document that precondition or add an end-to-end regression that confirms bad ICMP checksums are dropped before NAT.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/stateless/nf.rs` around lines 270 - 283, Tests call
StatelessNat::process directly for ICMP error packets but the real dataplane
guarantees validate_checksums_icmp runs in IcmpErrorHandler::process before
StatelessNat; update nat/src/stateless/test.rs to either (preferred) invoke the
full pipeline or explicitly run validate_checksums_icmp and drop packets with
bad checksums before calling StatelessNat::process so behavior matches
production, or at minimum add a regression test that runs
IcmpErrorHandler::process followed by StatelessNat::process to assert packets
with invalid ICMP checksums are dropped and not NATed; refer to
StatelessNat::process, IcmpErrorHandler::process, and validate_checksums_icmp
when making the change.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@nat/src/stateless/nf.rs`:
- Around line 270-283: Tests call StatelessNat::process directly for ICMP error
packets but the real dataplane guarantees validate_checksums_icmp runs in
IcmpErrorHandler::process before StatelessNat; update nat/src/stateless/test.rs
to either (preferred) invoke the full pipeline or explicitly run
validate_checksums_icmp and drop packets with bad checksums before calling
StatelessNat::process so behavior matches production, or at minimum add a
regression test that runs IcmpErrorHandler::process followed by
StatelessNat::process to assert packets with invalid ICMP checksums are dropped
and not NATed; refer to StatelessNat::process, IcmpErrorHandler::process, and
validate_checksums_icmp when making the change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bab16958-95ec-4b38-8970-64ce45da5bdb

📥 Commits

Reviewing files that changed from the base of the PR and between 58da74b and 5110c1f.

📒 Files selected for processing (4)

• nat/src/icmp_handler/icmp_error_msg.rs
• nat/src/icmp_handler/nf.rs
• nat/src/stateless/nf.rs
• net/src/packet/utils.rs

[Fredi-raspall]

I just have two minor comments, which are not blocking.