LibAlias: implement RFC 4787 REQ 1 and 3 (full cone NAT)

[damjan.jov_gmail.com]

Summary

Make libalias's NAT use endpoint-independent mapping ("full cone NAT") for UDP, conforming to RFC 4787 requirements 1 and 3. All UDP packets sent out from a particular internal address:port leave via the same NAT address:port, regardless of their destination.

Also add some libalias tests, and fix other tests that broke.

Closes: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219918

Test Plan

Tests are included in the patch.

[thj]

So far this generally looks ok to me and the tests seem to work.

I think this NAT behaviour should be optional in the same way we made it a configuration option for pf.

I would like someone more familiar with libalias to weigh in, in case there is something I am missing.

sys/netinet/libalias/alias_db.c

242

Can you split this so the line is shorter

tests/sys/netinet/libalias/2_natout.c

388

Can you split this so the line is shorter

[glebius]

I really don't remember much of libalias internals. Trusting your expertise, Tom! Thanks, Damjan!

[damjan.jov_gmail.com]

This new version adds a flag that can be used to turn this feature on and off (it is on by default for greater compatibility), adds documentation to the header file and man page, and wraps lines at 80 characters.

Also some new tests are added, including one for hairpinning (requirement 9 in RFC 4787)(and it works!), and other tests updated to use the flag.

[thj]

Two small things, I will start testing now

sys/netinet/libalias/alias.h

244

Please change "crippled" to "limited"

sys/netinet/libalias/libalias.3

275

Our man page style is quite strict, textproc/igor is a lint tool that will tell you how to correct stuff to match the style.

Just a heads up, I can do this once the code is ready it isn't a huge change

[thj]

Sorry one more thing, can you upload future diffs with full context. It makes reviewing on phabricator much easier.

Something like:
git diff --no-prefix -U1000

[thj]

Generally looks good, there are some small nits, but I'm happy for this to land once you've made the changes to the man page and the comment at the top.

I've asked Igoro to review as he has been looking at firewalls and I will add man pages now, they will say run the man page through textproc/igor too!

sys/netinet/libalias/alias_db.c

2199

Just a question this turns on EIM by default, but can still be over ridden by any application using libalias?

[damjan.jov_gmail.com]

sys/netinet/libalias/alias_db.c

2199

Yes it does, but it can be turned off by passing ~0 as the flags to LibAliasSetMode(), like the unit tests do.

[damjan.jov_gmail.com]

I also need to patch ipfw and natd with user-settable options for the new flag, before this patch is fully ready... I am busy so that will probably take a few more weeks.

[igoro]

sys/netinet/libalias/alias.h

252

Would it be better to mention UDP in the name? To make sure it's unambiguous and no one is thinking about TCP. Anyway, the code itself is read like if EIM && UDP.

sys/netinet/libalias/alias_db.c

2199

Thinking out loud.

From a glance it feels like we should follow POLA here. Practically all users of libalias (ipfw, ng_nat, natd, etc) will change their behavior after upgrade to 15.0-RELEASE or 14.x. It feels like UDP should still work, but logic of aliasing port assigning is changed, and we don't know whether it's okay for users, probably, they expect the today's behavior. Thus, it feels like it should be added as opt-in feature with the respective interface extension on libalias consumers' side, e.g. ipfw could have udp_eim option or something. And I do not count possibility of a regression in the existing setups.

Probably, the whole idea is to spread it like "to make all NATs in the world having UDP EIM turned on by default", when waiting for admins to do a configuration change yields smaller chances to reach the goal comparing to getting "infected" automatically after software upgrade.

[igoro]

sys/netinet/libalias/alias_db.c

275

Perhaps, it would be easier to read if it compares against the same boolean expression as above, i.e. if not (is EIM and is UDP). It's the same according to the DeMorgan's, but the intention would be got much quicker.

Also, an extra reading improvement could be to have a separate if (is EIM and is UDP) then continue; statement before the existing LIST_FOREACH loop left intact.

[igoro]

sys/netinet/libalias/alias_db.h

295

Another idea to think about is that probably adding direction to the name would help to follow the existing code. It could potentially help with code reading and understanding. Just sharing thoughts.

[damjan.jov_gmail.com]

This update:

• Makes EIM opt-in, off by default, although I still think this decision needs broader discussion, or even a vote.
• Adds "udp_eim" options to all the libalias uses: ipfw, natd, ng_nat, and ppp.
• Rewrites the man pages and header files so they are shorter and clearer.
• Man pages don't introduce new errors in igor.
• Uses PKT_ALIAS_UDP_EIM instead of PKT_ALIAS_ENDPOINT_INDEPENDENT.
• Renames the splay tree from "source" to "internal_endpoint" in the source code.
• Simplifies some conditions.

[thj]

In D46689#1082245, @damjan.jov_gmail.com wrote:

This update:

• Makes EIM opt-in, off by default, although I still think this decision needs broader discussion, or even a vote.

To be clear your position is that eim should be on by default?

I prefer to land the code and then we can have the discussion with the community about defaults.

From a quick read it looks good, I will do some testing on Monday, I don't think responses to my other comment will change the results of testing.

sys/netgraph/ng_nat.h

56

I'm not sure about skipping values here, I see it is to sync the value with libalias, but we already seem to have lost that with NG_NAT_UNREGISTERED.

I'd prefer you use the next value, or if you insist put place holders or a comment to indicate the missing values are available.

[damjan.jov_gmail.com]

This update also changes NG_NAT_UDP_EIM from 0x800 to 0x200.

[damjan.jov_gmail.com]

And this one adds more context to the patch.

[damjan.jov_gmail.com]

In D46689#1083011, @thj wrote:

In D46689#1082245, @damjan.jov_gmail.com wrote:

This update:

• Makes EIM opt-in, off by default, although I still think this decision needs broader discussion, or even a vote.

To be clear your position is that eim should be on by default?

I am not sure: EIM on by default would be better for small home users and SOHO setups, but large ISPs would probably also want LibAliasSetAddress() to support multiple public IP addresses for scalability. I think pf can already use a pool of public IP addresses when NATing out.

I prefer to land the code and then we can have the discussion with the community about defaults.

From a quick read it looks good, I will do some testing on Monday, I don't think responses to my other comment will change the results of testing.

What did you mean by "I'm not sure about making the rule so wordy"? Would you prefer PKT_ALIAS_ENDPOINT_INDEPENDENT instead of PKT_ALIAS_UDP_EIM?

[thj]

From my testing this seems good. From there I will break this down a bit so the commits are smaller and start adding stuff next week

[igoro]

Looks good to me too.

I would suggest to move TOK_UDP_EIM line in sbin/ipfw/ipfw2.h to its NAT friends, e.g. after the TOK_REDIR_PROTO line. It seems that enum values changing should not create troubles as long as this is ipfw binary internals. Also, you may think of adding yourself to the libalias.3 as the author of these RFC reqs implementation. As for me, these comments are not blockers and they can be done with separate patches if needed.

P.S. In the future it would be great to have integration tests on the consumers' side like ipfw udp_eim tests using jails & VNET, but it's out of scope here, just thinking out loud to spread the idea.