fw4: enable mac ranges for rule

[ckorber]

nft supports handling mac ranges and therefore this commit changes fw4 to support that feature. The src_mac is now allowed to be a address with CIDR notation. If no CIDR notation is configured, the old logic is applied.

So this is now possible:

option src_mac '00:11:AA:BB:CC:DD-00:11:AA:FF:FF:FF'

Also negation with '!' at the beginning to block every MAC not in the range is possible.

[brada4]

CIDR is wrong approach for MAC address, you have to be able to extract 2 youngest bits of oldest byte.

[ckorber]

So if I understand you correctly you mean that it is not useful to enable the whole range, because the first byte contains important information that should not be manipulated, am I right?

I can modify it to only allow ranges in the last three youngest bytes (so not the oui). Is that also your intention?

[brada4]

The grouping of MAC addresses with mask is very welcome, just that presuming CIDR is wrong. Yo uare on the right path, just that took a bit odd turn. DOC: https://en.wikipedia.org/wiki/MAC_address#Ranges_of_group_and_locally_administered_addresses

Example src 00:00:00:00:00:00/01:00:00:00:00:00 drop would drop unrealistic packets sourcing from multicast addresses.

[ckorber]

I get what you mean, I think. The first byte holds valuable information that shouldn't be touched.

But if I limit it to bytes after the oui, we should be safe, right?

[brada4]

Just use full mask, thats all I wanted to tell. There is no CIDR for mac addresses.

[brada4]

Something wrong with parentheses ;-)

[ckorber]

Better? :)

I dropped the CIDR approach and let it parse for ranges that are set or return as usual. If the range is wrong (that is if the lower is on the right) it returns nothing.

~~EDIT: Oh I didn't push again.~~

[brada4]

You can compile ucode and ./run_tests.sh (it is a bit tricky first time, you have to disable all or most extensions via cmake-gui or parameters)

[brada4]

You dont have to check special case with 0 or -1 mask that makes no-op-ish rules

[ckorber]

Does that work for you @brada4 ?

[brada4]

Wait for @jow- to examine it.

[ckorber]

ping @jow-

[systemcrash]

Advise you reword commit message: Also negation with '!' at the beginning to block every MAC not in the range is possible. to Also negation with '!' at the beginning to block every MAC in the range is possible.

[ckorber]

But I meant what I wrote? AFAIK the '!' negates the range and applies to the opposite, so it handles every MAC not in the range.

I could change it to: Also negation with '!' at the beginning to handle every MAC not in the range is possible.

[brada4]

It is double no in your sentence meaning YES in some languages...

[ckorber]

If a range is specified with '!', like '!00:11:AA:00:00:00-00:11:AA:FF:FF:FF'
it handles every MAC that is not within the range.

I rephrased it. Is it now clearer?

[systemcrash]

Handle could be ambiguous depending on the action requested. The initial wording of ! == block (within the range) is clear and concrete. Less cognitive load.

[brada4]

exclamation mark is called negation in iptables doc eg "negative matching using exclamation mark is supported"

[ckorber]

@brada4 : I used your suggestion for the commit message.

Any other way to progress this PR? I think @jow- is not available at the moment.

[systemcrash]

One potential pitfall with ambiguity: it becomes difficult to differentiate between MAC and hyphen separator when the MACs themselves use hyphen - between octets. :)

00-11-AA-00-00-00-00-11-AA-FF-FF-FF.

Although I can't think of a better separator. Hyphen is typically used to denote ranges. If this is deemed problematic, how about underscore? No, that's a binder. Tilde?

00-11-AA-00-00-00~00-11-AA-FF-FF-FF.

Thoughts @ckorber ?

[brada4]

nftables takes all imaginable formats in

table inet t {
 chain c {
 ether saddr & 01:00:00:00:00:00 == 0x0
  ether saddr "02-00-00-00-00-00"-"04.ff.ff.ff.ff.ff"
 }
}

out

table inet t {
        chain c {
                ether saddr & 01:00:00:00:00:00 == 00:00:00:00:00:00
                ether saddr 02:00:00:00:00:00-04:ff:ff:ff:ff:ff
        }
}

0x0 in first line overflows if exceeding uint32 so it is not really useful for anything. standard format is with colon, other formats needs to be escaped.

[ckorber]

Hate that I didn't see that :smile: It wouldn't be a problem in luci (as it is with my depending PR openwrt/luci#8017), because it relies on : as separator.

Maybe 00-01-AA-00-00-00..00-01-AA-FF-FF-FF gives the right idea? But I don't know if this is the best approach? Maybe changing this adds an additional cognitive load to remember that ranges are noted with .. as range symbol in fw4. I think - is more intuitive.

Additionally after parsing it transforms to 00:01:AA:00:00:00-00:01:AA:FF:FF:FF again in fw4.

[brada4]

99% of users will supply valid values but we dont want others to break ruleset.

[systemcrash]

I think - is more intuitive.

Agreed. Just happier that we at least discussed the potential problem here.

[ckorber]

Don't want to push it, but there is no progress on this PR. Can someone review and merge it? Or is it only possible by @jow-?

[systemcrash]

Any maintainer can.

[systemcrash]

@ckorber Recommend tweaking the commit message a bit to e.g.

So this is now possible:

option src_mac '00:11:AA:00:00:00-00:11:AA:FF:FF:FF'

In addition to the original:

option src_mac '00:11:AA:00:00:00'

[ckorber]

I added that. Thanks!

@ckorber Recommend tweaking the commit message a bit to e.g.

So this is now possible:

option src_mac '00:11:AA:00:00:00-00:11:AA:FF:FF:FF'

In addition to the original:

option src_mac '00:11:AA:00:00:00'

[ckorber]

@nbd168 or @stintel : Do one of you want to review, because you committed to the project in the past? @jow- seems to be unavailable for review. Thanks in advance!

[systemcrash]

Hi @robimarko could we get this in for 25?