Skip to content

Pr/fredi/masquerade timeouts#1501

Closed
Fredi-raspall wants to merge 15 commits intopr/fredi/fix_dockerfilefrom
pr/fredi/masquerade_timeouts
Closed

Pr/fredi/masquerade timeouts#1501
Fredi-raspall wants to merge 15 commits intopr/fredi/fix_dockerfilefrom
pr/fredi/masquerade_timeouts

Conversation

@Fredi-raspall
Copy link
Copy Markdown
Contributor

@Fredi-raspall Fredi-raspall commented Apr 27, 2026

  • further cleanup stateful implementation
  • unify some types with port forwarding implementation
  • rewrite utils with new packet header matching support
  • add flowstatus + state machine for masqueraded flows
  • set timeouts for masqueraded flows accordingly.
  • The behavior is
    • configured timeout is ignored for ICMP. The timeout is set internally
    • UDP traffic: configured timeout is only applied until traffic gets to "established" state (2-way communication + 1 packet).
    • In the case of DNS (udp 53), the flow is closed (and flows cancelled) on the response.
    • TCP: user timeout ignored until flow becomes established.

Fixes: https://github.com/githedgehog/internal/issues/364

NOTE: this targets #1499

Nat the first packet of a port-forwarded flow using the state
created. This helps in consistency and in exposing a single method
to nat a packet.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Let masquerading and port forwarding use the same type for actions.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Write packet port-forwarding utils using the new pattern
matching. Return failures as errors and log them.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The utils:
  - use the new pattern matching
  - distinguish between source and destination address/port nat.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Use the new functions based on header pattern matching to masquerade
flows.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Rename:
  PortFwFlowStatus       -> NatFlowStatus
  AtomicPortFwFlowStatus -> AtomicNatFlowStatus

.. so that the same types can be used for masquerading

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Moves the types renamed in the prior commit to common/ so that
they can be used for masquerading. Note that while the types are
moved, the uses of the types and semantics may be NAT flavor
specific. In other words, only the types are moved but not some
of the implementations.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Add field status with type AtomicNatFlowStatus to the masquerade
states created for the flow pair to masquerade traffic.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Adds simple flow status state machines for masqueraded flows.
With the state machine defined:
  1) icmp traffic can only be oneway or twoway. The timeout for
     icmp traffic will not configurable by the user.
  2) UDP traffic can be in oneway, twoway or established (3 way).
     Only when reaching established flow timeouts will be the ones
     configured by the user.
  3) TCP traffic uses the same SM as for port-forwarding, reversed.
     Until flows reach established state, their timeouts will not
     be the user configured ones.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
- Set the initial timeout for a masqueraded flow.
- Update FlowStatus of masqueraded flows according to SM and proto.
- Set subsequent flow timeouts depending on the FlowStatus.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Allow "patching" the flow status depending on the application.
This is only implemented for DNS and relying on transport ports as
application identifiers.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Copilot AI review requested due to automatic review settings April 27, 2026 13:50
@Fredi-raspall Fredi-raspall requested a review from a team as a code owner April 27, 2026 13:50
@Fredi-raspall Fredi-raspall requested review from sergeymatov and removed request for a team April 27, 2026 13:50
@Fredi-raspall Fredi-raspall changed the base branch from main to pr/fredi/fix_dockerfile April 27, 2026 13:50
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 refactors stateful NAT masquerading to include a shared flow-status state machine and protocol-aware timeout behavior, while also unifying several NAT/port-forwarding types and packet header access patterns.

Changes:

  • Introduces shared NAT types (NatAction, NatFlowStatus, AtomicNatFlowStatus) and applies them across port-forwarding and masquerading.
  • Adds masquerade-specific protocol/state-machine logic and rewrites packet mangling using the newer header pattern-matching API.
  • Adjusts flow timeout behavior for masqueraded flows (pre-establishment vs established, DNS special-casing, etc.) and updates related formatting/logging.

Reviewed changes

Copilot reviewed 21 out of 22 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
net/src/packet/mod.rs Updates TryHeaders/TryHeadersMut impls to return concrete Headers.
net/src/headers/mod.rs Changes TryHeaders/TryHeadersMut traits to return &Headers / &mut Headers.
net/src/flows/flow_key.rs Removes Uni wrapper doc example.
net/src/flows/flow_info.rs Adds tracectl trace target initialization; adjusts flow invalidation logging.
net/src/flows/display.rs Tweaks flow display formatting strings.
net/Cargo.toml Adds linkme and tracectl dependencies for net crate usage.
nat/src/stateful/state.rs Unifies masquerade action type with NatAction and adds shared atomic flow status + translation view helper.
nat/src/stateful/protocol.rs New masquerade flow-status state machine (UDP/TCP/ICMP + DNS special handling).
nat/src/stateful/packet.rs New masquerade packet rewrite implementation using header pattern matching.
nat/src/stateful/nf.rs Integrates masquerade status machine and internal timeouts into the stateful NAT datapath.
nat/src/stateful/mod.rs Wires in new packet and protocol modules.
nat/src/stateful/flows.rs Updates to use unified NatAction.
nat/src/portfw/test.rs Updates tests to use unified NatFlowStatus instead of portfw-specific status type.
nat/src/portfw/protocol.rs Replaces portfw-specific status machine types with NatFlowStatus (and updates logic).
nat/src/portfw/packet.rs Refactors portfw NAT packet mangling to return typed errors and use header pattern matching.
nat/src/portfw/nf.rs Updates port-forwarding NF to handle new nat_packet() error-returning API and unified action type.
nat/src/portfw/icmp_handling.rs Updates ICMP error handling to use unified NatAction and new nat_packet() Result API.
nat/src/portfw/flow_state.rs Unifies port-forward flow state action/status types with new shared NAT types.
nat/src/port.rs Adjusts NatPort display formatting.
nat/src/lib.rs Adds new common module and removes Clone from NatTranslationData.
nat/src/common/mod.rs New shared NAT action/status/atomic types for both masquerade and port-forwarding.
Dockerfile Changes runtime user directive from USER root to numeric USER 0.
Cargo.lock Updates lockfile for new dependencies.

Comment thread nat/src/portfw/protocol.rs
Comment thread nat/src/portfw/packet.rs
Comment on lines 133 to 141
/// Perform src or dst nat for a packet, depending on the action indicated in state
pub(crate) fn nat_packet<Buf: PacketBufferMut>(
pub fn nat_packet<Buf: PacketBufferMut>(
packet: &mut Packet<Buf>,
state: &PortFwState,
) -> bool {
) -> Result<bool, NatPacketError> {
match state.action() {
PortFwAction::DstNat => dnat_packet(packet, state.use_ip().inner(), state.use_port()),
PortFwAction::SrcNat => snat_packet(packet, state.use_ip(), state.use_port()),
NatAction::DstNat => dnat_packet(packet, state.use_ip().inner(), state.use_port()),
NatAction::SrcNat => snat_packet(packet, state.use_ip(), state.use_port()),
}
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

confidence: 10
tags: ["logic", "style"]

nat_packet is declared pub but lives in a private module (portfw::packet is not public). This will trigger the unreachable_pub lint (and fail under -D warnings). Make the function pub(crate)/pub(super) (or expose the module intentionally) to match its actual visibility requirements.

Copilot uses AI. Check for mistakes.
Comment thread nat/src/stateful/nf.rs
Comment thread net/src/flows/flow_info.rs Outdated
Comment thread nat/src/stateful/packet.rs
) -> Result<(), NatPacketError> {
debug!("Natting packet using {xlate} (masquerading flow)");
match xlate.action {
NatAction::SrcNat => snat(packet, xlate.use_ip.try_into().unwrap(), xlate.nat_port), // FIXME
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

confidence: 9
tags: ["logic", "style"]

masquerade() uses xlate.use_ip.try_into().unwrap() to obtain a UnicastIpAddr for SNAT. This can panic if use_ip isn’t unicast (and the comment indicates this is known). Prefer making unicast-ness an invariant (e.g., store UnicastIpAddr in NatTranslate/MasqueradeState for SNAT) or return a typed error instead of unwrapping.

Suggested change
NatAction::SrcNat => snat(packet, xlate.use_ip.try_into().unwrap(), xlate.nat_port), // FIXME
NatAction::SrcNat => {
let src_ip: UnicastIpAddr = xlate
.use_ip
.try_into()
.map_err(|_| NatPacketError::UnsupportedTraffic)?;
snat(packet, src_ip, xlate.nat_port)
}

Copilot uses AI. Check for mistakes.
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.

Yup, forgot to address the fixme

Comment thread nat/src/portfw/protocol.rs
Comment thread nat/src/stateful/nf.rs
Comment thread nat/src/stateful/packet.rs
Comment thread nat/src/stateful/protocol.rs
- fix displays for clearer logs
- remove duplicate logs
- add tracing target to net/flows. We can't have linkme as a dependency
  of net, so we declare the target in flow-entry on behalf of net.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
@Fredi-raspall Fredi-raspall force-pushed the pr/fredi/masquerade_timeouts branch from be4f7fc to 4d91336 Compare April 27, 2026 14:35
@Fredi-raspall Fredi-raspall reopened this Apr 27, 2026
@Fredi-raspall Fredi-raspall added ci:+vlab Enable VLAB tests labels Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:+vlab Enable VLAB tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants