Implement nat46 by kloenk · Pull Request #74 · atenart/sniproxy

kloenk · 2025-09-22T17:23:47Z

Implement a nat46 mode similar to https://github.com/AGWA/snid/tree/main?tab=readme-ov-file#nat46-mode but using config instead of DNS records

Update edition to 2024, this allows to use if let chains, and similar features, that got introduced with edition 2024.

Add a `nat46_prefix` to the Backend struct. This allows to configure a nat46 prefix to use when encoding the v4 address in a v6 destination address. Co-authored-by: Schrottkatze <git@schrottkatze.de>

Run cargo fmt to cleanup further commits when running fmt localy

Add a nat46 feature similar to the one implemented in snid[0]. [0]: https://github.com/AGWA/snid/tree/main?tab=readme-ov-file#nat46-mode

atenart

Thanks for the PR! I believe this will need some revisions but I also don't see a blocker about the feature itself.

A few global comments:

All commits should have a proper Signed-off-by tag (some are missing and I'm not sure about the one in patch 2). Sorry, this should be in a contributing guide.
What are the advantages in using NAT46 instead of using a public v4 address + v6 backend + proxy protocol header? Is it only about conveying the client IP at a lower level?
No need to reference third party projects in commit logs, NAT46 is well defined in an RFC which can be referenced directly instead.
Please update the documentation as well (README.md), this also helps while reviewing as it shows the intent.
Please put the formatting patch before introducing new code.

atenart · 2025-09-23T19:36:37Z

+        let mut ifname_c = [0u8; libc::IF_NAMESIZE];
+        (&mut ifname_c[..if_name.len()]).copy_from_slice(if_name.as_bytes());


I believe you can use a CString and its to_ptr helper instead here.

CString is heap based and bit more annoying to work with. as there is a max name that is enforced by linux I found this approach much nicer to work with and it is stack based

There's already a check on the name length so that should be fine. By regardless of that I'd prefer to stick to higher level interfaces for easier maintenance and potential issues.

atenart · 2025-09-23T19:37:44Z

+impl Nat46Address {
+    /// Convert ifname to scope_id
+    #[cfg(not(test))]
+    fn if_nametoindex(if_name: &str) -> std::io::Result<u32> {


Please use anyhow::Result instead and e.g. the bail! macro for returning early. There are other examples.

Really not a fan of anyhow for the type erasure. as this is just a wrapper around libc using errno I would favour sticking with io::Result. but can change if you exists but might lose debugging info for the errno context things

The errors are ultimately being converted to anyhow, so let's not split the project in this regards.

FYI I believe using the anyhow::Context trait the underlying error can be shown if needed.

atenart · 2025-09-23T19:39:27Z

+#[derive(Debug)]
+pub(crate) struct Nat46Address {
+    /// V6 address prefix
+    pub(crate) address: std::net::Ipv6Addr,


There are many examples of fully qualified names in the PR. Please avoid them (except for the libc:: ones).

atenart · 2025-09-23T19:48:09Z


+/// IPv6 address prefix and optional scope ID for nat46 mapping.
+#[derive(Debug)]
+pub(crate) struct Nat46Address {


Instead of creating a new type and implementing by hand deserialization, why not using the following configuration form:

nat46: prefix: ... iface: ...

The prefix could be handled by Ip6Addr directly. The iface id might need a custom implementation, like what is done in deserialize_regex. But overall this would simplify things.

I find the proper scope id way much easier to understand and write as user. This is what other tools use as well on linux, therefor opted for the %iface way of writing it

YMMV. Not saying the syntax is bad in general, but the cost/benefit of having two fields here is nice.

I might have understood what was bothering me. There's no "nat46" concept in sniproxy, data can already flow between v4/v6. The added value here is the ability to select a translation prefix and to bind to a device. Those two are not linked (they can be used independently and/or in combination) and should come as separate features in separate patches. With that my proposal nat46 block doesn't make sense either.

The way forward is probably something like:

routes: - domains: - "example.net" backend: address: "[1111::1]:8080" translation_prefix: "64:ff9b:1::" device: eth0

There are kinda link. As it's part of the socketaddr to bind to a device for the backend used address

It depends. You might want to bind to a device for various reasons (e.g. VRF) without doing address translation or even using IPv6 at all. You could also do address translation without binding to a device. Or do both in the example you have in mind, which is very valid too.

But it's very much non trivial to implement. If we don't do this baring either way to already set a source address

But it's very much non trivial to implement.

I think that would look like (untested). That seems doable, unless I'm missing something.

let sock = socket2::Socket(...)?; if let Some(device) = &backend.device { sock.bind_device(Some(device))?; } if let Some(prefix) = &backend.translation_prefix { let prefix = compute_prefix(prefix, client); sock.bind(prefix)?; } sock.connect_timeout(...)?; let conn: TcpStream = sock.into();

It's slightly different from using sin6_scope_id; but for the use case here I think that should do, or is there an issue I'm not seeing?

This has some benefits, e.g. in allowing to bind to a device regardless of using a translation prefix and in making parsing easier.

I'm unsure if this will work with fe80 addresses. But will have to try once the time for it (hopefully Monday)

atenart · 2025-09-23T19:50:02Z

+                if let Some(len_str) = len_str
+                    && let len = len_str.parse::<u32>().map_err(|e| E::custom(e))?
+                    && len > 96
+                {
+                    return Err(E::custom(format!(
+                        "Prefix len {len} is to small as nat46 prefix"
+                    )));
+                }


Why allowing a CIDR as it's not used? I guess it should be removed.

Idea was to allow pasting it in and as validation that we need a /96 or bigger CIDR. So intendet as a aid for the user without causing much harm

Let's not add it if it's not used. We can still have checks in Config::from_str making sure the prefix looks good.

atenart · 2025-09-23T19:52:01Z

+                let mut scope_id = 0;
+                if let Some(scope_str) = scope_str {
+                    if let Ok(scope) = scope_str.parse::<u32>() {
+                        scope_id = scope;


Not sure a plain id should be allowed, as it's not a stable interface. Plus converting the name to its id is only done at startup so that won't impact performances.

atenart · 2025-09-23T19:54:50Z

+    time::Instant,
+};
+
+pub(crate) struct Socket6(OwnedFd);


I don't think there's a need to re-implement this manually. We're already using socket2 which provides Socket. My understanding is the following should do:

let sock = socket2::Socket::new(...)?; sock.bind(...)?; sock.connect_timeout(...)?; let conn: TcpStream = sock.into();

atenart · 2025-09-23T19:55:15Z

+    let peer_addr = match peer_addr {
+        SocketAddr::V4(v) => v,
+        SocketAddr::V6(v) if matches!(v.ip().to_ipv4_mapped(), Some(_ip)) => {
+            let ip = unsafe { v.ip().to_ipv4_mapped().unwrap_unchecked() };


Why would we get an IPv4-mapped IPv6 address here?

atenart · 2025-09-23T20:01:06Z

+    #[cfg(test)]
+    fn if_nametoindex(if_name: &str) -> std::io::Result<u32> {
+        match if_name {
+            "eth0" => Ok(2),
+            "eth1" => Ok(3),
+            _ => Err(std::io::Error::from_raw_os_error(libc::ENODEV)),
+        }
+    }


Could you instead not call the helper from the deserialization logic when testing and convert it there to a static known value ("0")?

Not sure what you mean. also this is not only returning 0 but depending on the iface name?

I was thinking something like the following to avoid a full function redefinition and still test most of it.

let scope_id = if !cfg!(test) { unsafe { libc::if_nametoindex(ifname_c.as_ptr().cast()) } } else { 1 };

kloenk · 2025-09-23T21:33:06Z

Thanks for the PR! I believe this will need some revisions but I also don't see a blocker about the feature itself.

A few global comments:
* All commits should have a proper Signed-off-by tag (some are missing and I'm not sure about the one in patch 2). Sorry, this should be in a contributing guide.

Furthermore there should be something telling which DoC I would sign with that. I did not find that, and therefore did not sign it off. would like that to be added before

* What are the advantages in using NAT46 instead of using a public v4 address + v6 backend + proxy protocol header? Is it only about conveying the client IP at a lower level?

proxy protocol header is something you have to do more consciously, with this you can just look into the log and see the original client v4 address

* No need to reference third party projects in commit logs, NAT46 is well defined in an RFC which can be referenced directly instead.

* Please update the documentation as well (README.md), this also helps while reviewing as it shows the intent.

* Please put the formatting patch before introducing new code.

atenart · 2025-09-24T17:49:28Z

Furthermore there should be something telling which DoC I would sign with that. I did not find that, and therefore did not sign it off. would like that to be added before

That's fair. I'll add something in the doc with a link to https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed%20off#developer-s-certificate-of-origin-1-1

proxy protocol header is something you have to do more consciously, with this you can just look into the log and see the original client v4 address

I was wondering if there was something else. This alone is nice though.

kloenk and others added 3 commits September 14, 2025 10:54

cargo: update edition

d018627

Update edition to 2024, this allows to use if let chains, and similar features, that got introduced with edition 2024.

Add nat46 prefix to Backend

61447c5

Add a `nat46_prefix` to the Backend struct. This allows to configure a nat46 prefix to use when encoding the v4 address in a v6 destination address. Co-authored-by: Schrottkatze <git@schrottkatze.de>

chore(fmt): run cargo fmt

0300772

Run cargo fmt to cleanup further commits when running fmt localy

kloenk mentioned this pull request Sep 22, 2025

cargo: update edition #71

Closed

feat(nat46): Add nat46 implementation

a761b1f

Add a nat46 feature similar to the one implemented in snid[0]. [0]: https://github.com/AGWA/snid/tree/main?tab=readme-ov-file#nat46-mode

kloenk force-pushed the nat46 branch from 955d725 to a761b1f Compare September 22, 2025 17:29

atenart requested changes Sep 23, 2025

View reviewed changes

		let mut ifname_c = [0u8; libc::IF_NAMESIZE];
		(&mut ifname_c[..if_name.len()]).copy_from_slice(if_name.as_bytes());

Conversation

kloenk commented Sep 22, 2025

Uh oh!

atenart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atenart Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kloenk commented Sep 23, 2025

Uh oh!

atenart commented Sep 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

atenart Sep 24, 2025 •

edited

Loading