thoughts on pr #1548#1553
Draft
daniel-noland wants to merge 23 commits into
Draft
Conversation
For consistency with masquerading and port forwarding, move the code for the network function and for the main logic for static NAT to a dedicated nf.rs file, only leaving dependency inclusion and re-exports in mod.rs. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Avoid running the transformation of the IP into a validated unicast address at the last moment; do it as soon as we return the mapping from the table for static NAT. This allows working with a UnicastIpAddr all along, ensuring we've got the right type. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Move the check on ports being non-zero for PAT, and pass NonZero<u16> to follow-up methods rather than working with u16 that have a risk of being zero. This allows some minor simplification of the translation code. Signed-off-by: Quentin Monnet <qmo@qmon.net>
I wanted a view on a transport header that would use ports (meaning, TCP or UDP, only, at the moment). After various manual attempts, I just let Claude run its magic. We get a TcpUdp enum that is very similar to the IcmpAny objects. We can get ports, and set them too for the mutable variant. This should help simplify some portions of the NAT code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
Use the new TcpUdp view to simplify port translation in static NAT, and get rid of some unnecessary error variants. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Instead of repeating the "if () insert else remove" for each flag, let's add a set_flag() private method to PacketMeta, to simplify setting the different flags. Signed-off-by: Quentin Monnet <qmo@qmon.net>
In preparation for finer-grained flagging for static NAT, with separation for source and destination NAT, split requires_stateless_nat() into a source- and a destination-based version. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Instead of just marking packets for static NAT, extend the flags to specify individually for source and destination whether the packet should go through static NAT. This will allow to handle packets more efficiently when using static NAT in addition to masquerading or port forwarding in the future. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Use a finer-grained marking for packets after NAT-ing them, to account for whether the source or destination address has been translated. This will help processing packets when supporting static NAT in addition to masquerading or port forwarding in the future. Signed-off-by: Quentin Monnet <qmo@qmon.net>
We mark the packet independently for source or destination static NAT, so we can leverage this in the static NAT code to only apply the translation to the relevant direction(s), and only if translation has not been applied for that direction already. Signed-off-by: Quentin Monnet <qmo@qmon.net>
So far, using NAT on both sides of a peering was supported only if both
sides used static NAT. Combining static NAT and masquerading or port
forwarding for a connection was not possible. We want to support it now,
and for this, we need to have the static NAT stage coming first.
Example setup
-------------
Here are some detailed explanations, and that the packet goes through.
Let's imagine two endpoints communicating through the gateway, and both
using NAT. For this connection, endpoint 1 has IP address "a", NAT-ed to
"A"; and endpoint 2 has IP address "b", exposed with NAT as "B". Here's
a simple diagram:
+------------+ +---------+ +------------+
| | a A | | B b | |
| Endpoint 1 |------------| Gateway |------------| Endpoint 2 |
| | | | | |
+------------+ +---------+ +------------+
Let's look at a packet leaving endpoint 1 for endpoint 2:
- Initially: src: a, dst: B
- After translation: src: A, dst: b
Now the reply:
- Initially: src: b, dst: A
- After translation: src: B, dst: a
With static NAT on both sides, this doesn't present any major
difficulty, we can just do source and destination NAT in the static NAT
pipeline stage, the order is not really important.
Port forwarding then static NAT
-------------------------------
Now if we want to have masquerading or port forwarding on one end of the
peering, and static NAT on the other, the situation is different. Let's
look at the case where endpoint 1 uses port forwarding, and endpoint 2
uses static NAT. Let's consider the first packet. If we apply port
forwarding first, then the port forwarding stage will create the
following entries in the flow table:
- Key: (src: a, dst: B), translation value: (src: a, dst: b) (forward)
- Key: (src: b, dst: a), translation value: (src: B, dst: a) (reverse)
But this is not what we want! The reply will come as (src: b, dst: A)
and the packet will go through the flow table lookup stage with these
IPs, before any NAT operation. The lookup will fail, given that the
table entry has "a" as destination address and not "A".
Static NAT then port forwarding
-------------------------------
If we do static NAT first, the situation doesn't look better. The port
forwarding stage creates the flows based on the packet with the
translated source address, so we get:
- Key: (src: A, dst: B), translation value: (src: A, dst: b) (forward)
- Key: (src: b, dst: A), translation value: (src: B, dst: A) (reverse)
This works for replies, but not for subsequent packets in the forward
direction.
However, we do want static NAT first: in a follow-up commit we will keep
track of the initial source and destination IP for the packets so that
the port forwarding stage can create the relevant flow for the reverse
direction. We'll get (after this follow-up commit):
- Key: (src: B, dst: A), translation value: (src: B, dst: a) (reverse)
However, for the stage to be able to use the translated address and to
create the right entry for the forward direction, it needs to know "A",
in other words: it needs to process the packet _after_ static NAT has
been applied. Then we'll get (after this follow-up commit):
- Key: (src: a, dst: B), translation value: (src: A, dst: B) (forward)
The same also applies to masquerading with static NAT (swapping source
and destination), but static NAT already comes before masquerading.
Changes
-------
So here we move the static NAT stage before the port forwarding stage,
so that in the future we can make sure to create the write flow table
entries when combining NAT modes.
We also add debug asserts to ensure that we never attempt to NAT an IP
address twice.
We also allow the masquerading stage to process packets that have
already been through other NAT modes (in practice, this can only be
static NAT, because we never mark packets as requiring both masquerade
and port forwarding in flow-filter).
Signed-off-by: Quentin Monnet <qmo@qmon.net>
We want to support static NAT in addition with masquerading (or port forwarding), on opposite ends of a peering. But we've got an issue: when a stateful flow is established, the flow-filter stage bypass mechanism derives the NAT requirements from the existing flow information, without going through the flow-filter table lookup. As a result we don't mark the packet for static NAT, and the packet does not go through the static NAT stage! As a workaround, store the static NAT requirements as new bitflag fields for the flow information, and use these fields when setting NAT requirements from the flow information to packet metadata. Signed-off-by: Quentin Monnet <qmo@qmon.net>
In preparation for future change where updating the src_vpcd will require updating another field, make PacketMeta's src_vpcd field private and only accessible for update via the dedicated set_src_vpcd() method. Signed-off-by: Quentin Monnet <qmo@qmon.net>
And make sure to update it when we update the src_vpcd for the metadata. This will help us handle NAT mode combinations, where we need to keep track of the initial packet's addresses before the first NAT pass. Signed-off-by: Quentin Monnet <qmo@qmon.net>
So far, using NAT on both sides of a peering was supported only if both
sides used static NAT. Combining static NAT and masquerading or port
forwarding for a connection was not possible. We want to support it now,
and for this, we need to change the way we create flow table entries.
Here are some detailed explanations, and that the packet goes through.
Let's imagine two endpoints communicating through the gateway, and both
using NAT. For this connection, endpoint 1 has IP address "a", NAT-ed to
"A"; and endpoint 2 has IP address "b", exposed with NAT as "B". Here's
a simple diagram:
+------------+ +---------+ +------------+
| | a A | | B b | |
| Endpoint 1 |------------| Gateway |------------| Endpoint 2 |
| | | | | |
+------------+ +---------+ +------------+
Let's look at a packet leaving endpoint 1 for endpoint 2:
- Initially: src: a, dst: B
- After translation: src: A, dst: b
Now the reply:
- Initially: src: b, dst: A
- After translation: src: B, dst: a
Let's consider a setup where endpoint 1 uses static NAT, and endpoint 2
uses port forwarding. When we process the packet in the port forwarding
stage, we want to create the following flow table entries:
- Key: (src: a, dst: B), translation value: (src: A, dst: b) (forward)
- Key: (src: b, dst: A), translation value: (src: B, dst: a) (reverse)
However, because the packet has already been through the static NAT
stage before reaching the port forwarding stage, we collect the
translated source IP address from the packet and create the following
entries:
- Key: (src: A, dst: B), translation value: (src: A, dst: b) (forward)
- Key: (src: b, dst: A), translation value: (src: B, dst: A) (reverse)
This is fine for the return path, but subsequent packets in the forward
direction will not match: the flow lookup stage comes before the static
NAT stage and "a" hasn't been translated to "A" yet before the flow
lookup. Flow lookup fails, we find no flow.
So instead, we want to create the forward-direction entry based not on
the address observed by the port forwarding stage, but on the initial
source IP address that the packet came with. In order to keep track of
that address, we associate a flow key for the packet's metadata. We can
use this flow key to create flow table entries for the forward
direction, but we keep using the addresses from the packet, after static
NAT has occurred, for the reverse direction.
Signed-off-by: Quentin Monnet <qmo@qmon.net>
Now that we support using static NAT on one side of a peering and masquerading or port forwarding on the other end, let's allow users to deploy such configurations. Also adjust relevant tests accordingly. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Add unit tests to validate the use in a pipeline of static NAT + masquerade, or static NAT + port forwarding, on opposite sides of a peering. We set up a pipeline, create a packet, process it, and check for its output; then we create a fake reply, process it, check as well; at last, we re-process the original packet, to make sure that the connection still works after the flow table entry has been created. Signed-off-by: Quentin Monnet <qmo@qmon.net>
daniel-noland
force-pushed
the
fixups/pr-1548
branch
from
7e80420 to
dc05ef7
Compare
`Tcp`, `Udp`, `Icmp4`, and `Icmp6` already carry hand-written
`TypeGenerator` impls under `#[cfg(any(test, feature = "bolero"))]`.
The `Transport` enum is structurally simple -- one tuple variant per
type, no extra state -- so the bolero derive macro produces a uniform
choice over the four variants for free, without touching the existing
per-variant generators.
Unblocks property tests that need to range over arbitrary L4 protocols
(e.g. checking the `TcpUdp{,Mut}` wrapper's "succeeds iff TCP or UDP"
invariant, or any test that wants `with_type::<Transport>()` rather
than picking a single variant).
Add a smoke test asserting all four variants are reached so a future
refactor cannot silently regress to generating only a subset.
…update Use pat_mut chain in static NAT source/destination
daniel-noland
force-pushed
the
fixups/pr-1548
branch
from
dc05ef7 to
11ceb61
Compare
Property test mirror invariant on compute_flow_flags_*
Property test full NAT combination matrix against spec
Property tests for TcpUdp/TcpUdpMut wrapper invariants
`Packet::new()` populates `PacketMeta::flow_key` from whatever headers
were parsed at NIC ingress -- for a VXLAN-encapped frame, that means the
outer (underlay) IP/UDP. `vxlan_decap()` then replaces `self.headers`
with the inner packet but does not refresh `flow_key`; `set_src_vpcd()`
only rewrites the discriminant on the cached key and preserves the stale
src/dst/proto.
As a result, the value consumed as `initial_flow_key` by
`nat::portfw::flow_state::build_portfw_flow_keys` and
`nat::stateful::nf::StatefulNat::process_packet` describes the outer
underlay flow, not the inner overlay flow the rest of the pipeline
operates on. Forward-direction flow table entries are installed under a
key that no subsequent inner packet will match against the key
`flow_lookup` recomputes from current headers. Existing NAT tests don't
catch this because they construct packets without VXLAN.
Add two tests that pin down the current behaviour:
* `flow_key_reflects_outer_headers_before_decap` -- the baseline,
confirming the cached key mirrors the outer underlay at parse time.
* `flow_key_is_stale_after_vxlan_decap` -- runs `vxlan_decap()`,
asserts the cached key is unchanged, and compares it against a
fresh `FlowKey::try_from(Uni(&p))` computed from the inner packet.
Both pass today because the bug is present. When `vxlan_decap()` is
taught to refresh `flow_key`, the final `assert_ne!` in the second test
must flip to `assert_eq!` (and the function name should drop the
"stale" framing).
daniel-noland
force-pushed
the
fixups/pr-1548
branch
from
11ceb61 to
c3f131f
Compare
qmonnet
force-pushed
the
pr/qmonnet/split-static-nat
branch
2 times, most recently
from
4bea4c4 to
1d0ea0e
Compare
qmonnet
force-pushed
the
pr/qmonnet/split-static-nat
branch
2 times, most recently
from
55f2af7 to
76c39fc
Compare
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
2 participants
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.
Just organizing my thoughts on #1548