Page MenuHomeFreeBSD

ipfw: Support [w:x:y::z]:port (bracketed) IPv6 addresses in the fwd command
ClosedPublic

Authored by neel_neelc.org on Mar 10 2020, 3:57 AM.

Details

Summary

ipfw: Support {w:x:y::z}:port (bracketed) IPv6 addresses in the fwd command.

While we're doing this, optimize by not attempting to parse addresses if we're using tableargs.

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

Test Plan

Try compiling HEAD with this patch (or at least /usr/src/sbin/ipfw) and then run the following command:

ipfw add fwd "[::1]:3128" tcp from any to any

It should work

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

neel_neelc.org created this revision.Mar 10 2020, 3:57 AM

How about detecting the port separator first? (i.e. repeatly call strpbrk)
Then you can easily distinguish between the cases

  • starts with '[' -> numeric IPb6
  • contains ':' -> numeric IPv6
  • contains no letters -> numeric IPv4
  • use gai()

How about detecting the port separator first? (i.e. repeatly call strpbrk)

getaddrinfo let you simplify this even more:

  • strchr(',') and if found, you have the addr/port separated.
  • otherwise strrchr(':') and if found, separation done
  • if first char '[', check and remove last char ']', set numeric flag
  • call getaddrinfo with both pieces

Do not try to parse things, which are already handled by gai.

Your IPv4 suggestion is wrong. There can be IPv6 addresses without letters, like 2001:470:20::2, or 2600::1.

Also, your suggestions would require rewriting of the IP parsing code I modify, so I'd prefer not to do this unless necessary.

However, it's possible with my code that you can do IPv4 in brackets, like [1.2.3.4]:56789. I don't believe this causes any harm, but if it does I'm open to modifications or even the "rewrite" I mentioned earlier.

That would be my approach https://reviews.freebsd.org/differential/diff/69565/
I'd further eliminate the temporary storage "struct sockaddr_storage result", and copy directly from the gai result into the action (with memcpy).

Your approach looks good, reducing memory consumption.

rgrimes added a reviewer: bz.Apr 28 2020, 2:00 AM

Adding bz, as it is his XXX-BZ comment being removed about implementing this.

rgrimes added inline comments.Apr 28 2020, 2:10 AM
sbin/ipfw/ipfw2.c
4042 ↗(On Diff #69346)

So technically I can not have a hostnamed tablearg?

4046 ↗(On Diff #69346)

I would think all of the above attempt at parsing an IP address belongs inside this else block as none of it is needed in the first half of this if.

4051 ↗(On Diff #69346)

Magic pointer arithmetic? v6brac is 0 or 1 when we get here, so probably some place an *av++ should of happened to indicate we have swallowed a token?

neel_neelc.org updated this revision to Diff 71094.EditedApr 28 2020, 4:16 AM
neel_neelc.org retitled this revision from ipfw: Support {w:x:y::z}:port (bracketed) IPv6 addresses in the fwd command to ipfw: Support [w:x:y::z]:port (bracketed) IPv6 addresses in the fwd command.
neel_neelc.org edited the summary of this revision. (Show Details)

Here, I resolve two of three of rgrimes' suggestions.

About the hostnamed tablearg, I believe it should stay the same (CLARIFICATION: as it is today) but did not test this.

rgrimes accepted this revision.Apr 28 2020, 4:23 AM

That reads much better, cleaned it up a fair bit more than I thought it would.

This revision is now accepted and ready to land.Apr 28 2020, 4:23 AM

I'm not a committer (I'm also not neel@), could someone please commit this?

markj added inline comments.Jun 24 2020, 5:13 PM
ipfw2.c
3992 ↗(On Diff #71094)

Hmm, is this standard syntax for IPv4 addresses? I have never seen it.

4040 ↗(On Diff #71094)

Per style(9), casts are not followed by a space.

Made the corrections.

This revision now requires review to proceed.Jun 24 2020, 5:54 PM
neel_neelc.org marked 3 inline comments as done.Jun 24 2020, 5:54 PM
ipfw2.c
3992 ↗(On Diff #71094)

No, but a canonical extension.

markj accepted this revision.Jun 25 2020, 3:48 PM

This looks ok to me modulo the nits I pointed out.

sbin/ipfw/ipfw2.c
3999 ↗(On Diff #73587)

style(9) discourages combined declarations and initializations.

4019 ↗(On Diff #73587)

Similarly here. ptr also doesn't seem like a great name to me. I think you could simply do without this variable:

s = strchr(*av, ']');
if (s != NULL) {
    *(s++) = '\0';
} else {
    s = *av;
}
s = strchr(s, ':');
...
This revision is now accepted and ready to land.Jun 25 2020, 3:48 PM
neel_neelc.org marked 2 inline comments as done.

Good catch on the tidbits, here's an revised patch.

This revision now requires review to proceed.Jun 25 2020, 4:11 PM
markj accepted this revision.Jun 25 2020, 6:02 PM
This revision is now accepted and ready to land.Jun 25 2020, 6:02 PM
This revision was automatically updated to reflect the committed changes.