libalias: Allow setting alias port ranges

[nc]

Summary

Allow setting alias port ranges in libalias and ipfw. Along with r357092, this will allow a system to be a true RFC 6598 NAT444 setup, where each network segment (e.g. user, subnet) can have their own dedicated port aliasing ranges.

Submitted by: Neel Chauhan <neel AT neelc DOT org>

Test Plan

In short: test a network with an internal IP address in the 100.64.0.0/10 with port_alias LOWER UPPER in IPFW, and see if NAT is performed.

Explained:

Compile a HEAD with this patch and reboot.

Add the following to /etc/rc.conf:

ifconfig_lan0="inet 100.64.0.1 netmask 255.255.255.0"
firewall_enable="YES"
firewall_nat_enable="YES"
firewall_script="/etc/ipfw.conf"

Add the following to /etc/ipfw.conf:

#!/bin/sh

ipfw -q flush

ipfw nat 1 config if wan0 unreg_cgn port_alias 2000-3000
ipfw add 100 nat 1 ip from any to me 2000-3000 in via wan0
ipfw add 200 nat 1 ip from 100.64.0.0/24 to any out via wan0
ipfw add allow ip from any to any

Replace 2000 and 3000 with your lower and upper port ranges. Keep in mind that both have to be greater than 1024, and UPPER (obviously) has to be greater than LOWER.

Replace wan0 with your WAN (outside) interface, and lan0 with your LAN (inside) interface.

Then run

kldload ipfw ipfw_nat

and

service netif restart

Then, add clients on the 100.64.0.0/24 subnet with the 100.64.0.1 gateway and 255.255.255.0 subnet mask.

You could also do DHCP, or NAT from a loopback interface, I won't mention that here.

[nc]

sbin/ipfw/ipfw.8

3290

Sure, fixed it.

tests/sys/netpfil/common/nat.sh

185–186

Fixed it.

187–188

I'm not sure if this would work, but here it is.

[donner]

tests/sys/netpfil/common/nat.sh

187–188

Line 187 and 188 have exactly the same match, so only 187 is invoked. 188 will never be used. Furthermore they are only outgoing (for aliasing), they do not match incoming packets (for dealiasing)

[nc]

In the test, incoming connections are now based on their port, each NAT host gets their own port range.

Somewhat unrelated (unrelated?), I believe we should commit D23448 before this.

[donner]

sbin/ipfw/nat.c

757–758

I'd prefer a function witch return the valid port number or 0 in the case of failure. Writable pointers in arguments open a special class of worms in debugging.

760

Did you try "12345six" as a port number?

761

Did try "7654321" as a port number?

956–957

Is this case already covered by the validation functions? If yes, remove those check.

959

Using a other type of function call this would be

(0 < (hp = nat_port_...(av[1])))

which is easier to audit.

sys/netinet/libalias/alias_db.c

589

So PKT_ALIAS_SAME_PORT is incompatible with port ranges?
Can we prevent setting both flags with an error message in parsing the config?

sys/netinet/libalias/alias_local.h

166–168

How about a stuct and a single enty in the record?

struct nat_port_range {
  u_short lower, upper;
};

and

struct nat_port_range portRange;

sys/netpfil/ipfw/ip_fw_nat.c

96–97

Here the struct can be reused.

533–534

Using a stuct the lines become

ptr->portRange = ucfg->portRange;

[donner]

In D23450#516086, @lutz_donnerhacke.de wrote:

In D23450#515858, @neel_neelc.org wrote:

Also, may I please have your patch for NAT table lookup? It would work very well for this.

Yep, I'v to adapt it to CURRECT, the structure of accessing the "nat" component in ipfw was changed (which make it much easier to apply a different access scheme)

Please have a look at D23586

[nc]

I made the requested changes.

UPDATE: I forgot the PKT_ALIAS_SAME_PORT change, I realized that after I posted this so I am working on that now.

Also Lutz, thank you so much for posting D23586.

[nc]

Made the change.

[donner]

You do not have to follow my comments.
I just express my feelings, which may be wrong or misleading.
If your idea is different, please feel free to refuse the advice.

sbin/ipfw/nat.c

761–763

The common idiom is to provide a pointer as the second argument ti strtol and inspect the (first unparsable) char it points to afterwards.

port = strtol(ptr, ptr2, 10);
if ( *ptr2 != '\0' || port < 1024 )
  error

764

u_short comparsion with MAX_USHORT is not recommended.

958

Given the fact, that the upper limit is one above the usable range, this can't be specified, because the port number to configure is out of range. So it might be advisable to redefine the upper limit to be part of the interval (2000 2999 instead of 2000 3000).

sys/netpfil/ipfw/ip_fw_nat.c

538

Why not take the (pointer to the) struct as argument?

[nc]

I'm going to revert to the code to the one without the struct nat_port_range since it caused more problems than it's worth, especially with #include statements.

Your other requested changes, including those to the port parsing and options checking will be merged in the new patch.

[nc]

Here's my updated patch.

[donner]

sbin/ipfw/nat.c

762

The pointer end will always be != NULL, so you may even allow port numbers below 1024.

tests/sys/netpfil/common/nat.sh

185–186

No no. The concern is only valid as long as the documentation about the half open interval was missing. The configuration you need (using half open intervals) is 2000 3000 and 3000 4000 in order to match the port ranges in lines 187-188 below.

[nc]

(Hopefully) fixed the interval and port parsing.

[jilles]

sbin/ipfw/nat.c

761

It looks like that will still get through, since the cast will convert it to 52145 (the C standard defines this conversion, although it is not what is wanted here).

I suggest storing the result from strtol() in a variable of type long and doing range checks on that.

[nc]

Good catch. Fixed it.

[donner]

I wonder how it is possible to configure the whole range 65000 to 65535 as usable ports,

sbin/ipfw/nat.c

761

long is indeed better than any unsigned type, because it would catch negative numbers easily.

[nc]

Using long for the port parsing sounds good. Port numbers obviously can't be negative, neither than overflowed ints.

New patch does exactly this.

[donner]

Can you please mark all the comments as "Done", which are solved. Only the author of the patch can do this.

[nc]

Sure, done that.

[donner]

I'm still not satisfied with the "upper bound", which is inconsistent between "config port range" and "matching port range" in the ipfw rule set. It does not allow to specify the highest port (but this is a minor issue).

Furthermore I'm not familiar with the test framework, so somebody else should have a look at this part. Especially, because the is no success report here (but a failure). I don't want to stumble over an erroneous test ...

So most of my concerns are handled, I can use this patch to extend it to an dynamic port allocator (for less extensive lawful logging).

But I'm not in the position to give the final go.

[nc]

Thanks for your feedback.

I'm thinking about switching the NAT port range to something like 2000-2999 instead of 2000 3000 for consistency with the rest of IPFW. Would this be okay?

[donner]

In D23450#518558, @neel_neelc.org wrote:

Thanks for your feedback.

I'm thinking about switching the NAT port range to something like 2000-2999 instead of 2000 3000 for consistency with the rest of IPFW. Would this be okay?

I'd feel better with this, yes. It solves some corner cases, like storing the upper bound in ushort.

[melifaro]

sbin/ipfw/ipfw.8

3288

Given we're defining port range here, wouldn't the port_range be a bit more relevant name here?

sys/netinet/libalias/alias_db.c

598

Why do we need to check for both lower and upper in fast path?

[nc]

Here, I switch to the range separated by a - (e.g. 2000-2999 instead of 2000 3000), where the upper number is also included. On lines 611 and 668 in alias_db.c I added a " + 1" in order to account for the new range allocation mechanism.

I also switched the argument to port_range from port_alias.

[donner]

In general, I'm pleased with the renaming from the generic "alias" to "range".

sbin/ipfw/ipfw.8

3290

That's not true any longer. The interval is closed now, it includes the end points.

sbin/ipfw/nat.c

770

Why parsing from again from the very beginning of the string?

sys/netinet/libalias/alias_db.c

601

In terms of performance, it would be interesting to replace "Upper" with a precomputed "Range" or "Length". This part is used for ever new flow (very often).

For printing the configuration, the "Upper" value an be synthesized on demand.

[bcr]

Minor man page change. You can run checks on man pages with "mandoc -Tlint" and textproc/igor.

sbin/ipfw/ipfw.8

3290

You also need to have a line break after a sentence stop here.

[nc]

Here, I made changes to the:

• man page
• argument parsing
• switch to a range, per Lutz's suggestion

[gbe]

LGTM.

[kp]

Couple trivial cleanups and then I think this is good.

sbin/ipfw/nat.c

761

There's some extraneous whitespace here.

768

and here.

957

'You need' (missing 'e'), and line length.

959

Line length, and small inconsistency in that some errors have their first word capitalised and some don't.

970

Line length.

[nc]

Here is an updated version with some of the fixed typos/formatting.

[0mp]

I approve as a mentor assuming that you get an approval from a src committer, @nc

[kp]

Approved by: kp

Lutz and Neel will have to work out who actually commits it :)

[donner]

Congrats @nc for the src bit.
Your turn!