Page MenuHomeFreeBSD
Differential D46689

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

Authored by damjan.jov_gmail.com on Sep 17 2024, 6:44 PM.
Tags
Referenced Files
Unknown Object (File)
Fri, Nov 22, 10:40 AM
Unknown Object (File)
Fri, Nov 22, 10:35 AM
Unknown Object (File)
Sun, Nov 17, 8:27 PM
Unknown Object (File)
Sun, Nov 17, 7:12 PM
Unknown Object (File)
Sun, Nov 17, 7:07 PM
Unknown Object (File)
Sun, Nov 17, 2:27 PM
Unknown Object (File)
Sun, Nov 17, 2:08 PM
Unknown Object (File)
Sun, Nov 17, 1:21 PM
View All 31 Files
Subscribers
asomers
donner
emaste
glebius
imp
melifaro

Details

Reviewers
thj
donner
igoro
Group Reviewers
manpages
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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

damjan.jov_gmail.com created this revision.Sep 17 2024, 6:44 PM
damjan.jov_gmail.com created this object with edit policy "Subscribers".
Herald added subscribers: glebius, donner, melifaro and 2 others. · View Herald TranscriptSep 17 2024, 6:44 PM
damjan.jov_gmail.com requested review of this revision.Sep 17 2024, 6:44 PM
thj added reviewers: donner, glebius.Sep 23 2024, 12:25 PM

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 resigned from this revision.Sep 24 2024, 8:18 PM

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

emaste added a subscriber: emaste.Sep 27 2024, 2:41 PM
damjan.jov_gmail.com updated this revision to Diff 144862.Oct 15 2024, 2:09 AM

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 added a comment.Oct 18 2024, 1:01 PM

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 added a comment.Oct 18 2024, 1:33 PM

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 edited reviewers, added: igoro, manpages; removed: glebius.Oct 18 2024, 3:50 PM

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 added inline comments.Oct 18 2024, 4:35 PM
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 added a comment.Oct 18 2024, 4:36 PM

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 added inline comments.Oct 22 2024, 2:19 PM
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 added inline comments.Oct 22 2024, 4:36 PM
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 added inline comments.Tue, Oct 29, 7:04 PM
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 updated this revision to Diff 146063.Tue, Nov 5, 4:53 PM

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.
damjan.jov_gmail.com marked 9 inline comments as done.Tue, Nov 5, 4:57 PM
thj added a comment.Fri, Nov 8, 4:10 PM
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 ↗(On Diff #146063)

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 updated this revision to Diff 146179.Fri, Nov 8, 5:07 PM

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

damjan.jov_gmail.com updated this revision to Diff 146180.Fri, Nov 8, 5:08 PM

And this one adds more context to the patch.

damjan.jov_gmail.com marked an inline comment as done.Fri, Nov 8, 5:17 PM
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 accepted this revision.Thu, Nov 14, 4:14 PM

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

This revision is now accepted and ready to land.Thu, Nov 14, 4:14 PM
igoro accepted this revision.Fri, Nov 15, 1:29 PM

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.

Revision Contents
Changeset List

PathSize
sys/
netinet/
libalias/
24 lines
16 lines
65 lines
5 lines
20 lines
tests/
sys/
netinet/
libalias/
191 lines
2 lines
1 line

Diff 144862

View Options

sys/netinet/libalias/alias.h

Loading...
View Options

sys/netinet/libalias/alias_db.h

Loading...
View Options

sys/netinet/libalias/alias_db.c

Loading...
View Options

sys/netinet/libalias/alias_local.h

Loading...
View Options

sys/netinet/libalias/libalias.3

Loading...
View Options

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

Loading...
View Options

tests/sys/netinet/libalias/util.h

Loading...
View Options

tests/sys/netinet/libalias/util.c

Loading...