Conversation
|
CIDR is wrong approach for MAC address, you have to be able to extract 2 youngest bits of oldest byte. |
|
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). |
|
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. Example |
|
I get what you mean, I think. But if I limit it to bytes after the oui, we should be safe, right? |
|
Just use full mask, thats all I wanted to tell. There is no CIDR for mac addresses. |
a8d2c34 to
9dfde8b
Compare
|
Something wrong with parentheses ;-) |
|
Better? :) I dropped the CIDR approach and let it parse for ranges that are set or return as usual.
|
9dfde8b to
27ef07d
Compare
|
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) |
|
You dont have to check special case with 0 or -1 mask that makes no-op-ish rules |
root/usr/share/ucode/fw4.uc
Outdated
| icmp_codes: map(icmpcodes, ic => `${(family == 4) ? ic.type : ic.type6} . ${(family == 4) ? ic.code_min : ic.code6_min}`) | ||
| }; | ||
|
|
||
| if (!length(r.smacs_pos)) |
There was a problem hiding this comment.
You dont have to check special case with 0 or -1 mask that makes no-op-ish rules
Are you talking about these lines?
EDIT: The ./run_tests.sh was fine. All tests passed.
There was a problem hiding this comment.
If I understand you correctly now, you meant that I don't have to check for correctness because fw4 already handles that, am I right?
27ef07d to
52dfc8e
Compare
|
Does that work for you @brada4 ? |
d8b4117 to
2dd4c3e
Compare
|
Wait for @jow- to examine it. |
2dd4c3e to
176bb6f
Compare
This commit is a preparation to add the possibility to block or accept a range of MAC addresses int they syntax of: ''' 00:11:22:00:00:00-00:11:22:ff:ff:ff ''' The commit depends on the PR openwrt/firewall4#74 being merged first. Signed-off-by: Christian Korber <[email protected]>
176bb6f to
177a3db
Compare
This commit is a preparation to add the possibility to block or accept a range of MAC addresses int they syntax of: ''' 00:11:22:00:00:00-00:11:22:ff:ff:ff ''' The commit depends on the PR openwrt/firewall4#74 being merged first. Signed-off-by: Christian Korber <[email protected]>
This commit is a preparation to add the possibility to block or accept a range of MAC addresses int they syntax of: ''' 00:11:22:00:00:00-00:11:22:ff:ff:ff ''' The commit depends on the PR openwrt/firewall4#74 being merged first. Signed-off-by: Christian Korber <[email protected]>
|
ping @jow- |
|
Advise you reword commit message: |
|
But I meant what I wrote? I could change it to: |
|
It is double no in your sentence meaning YES in some languages... |
177a3db to
625340b
Compare
I rephrased it. Is it now clearer? |
|
Handle could be ambiguous depending on the action requested. The initial wording of ! == block (within the range) is clear and concrete. Less cognitive load. |
|
exclamation mark is called negation in iptables doc |
625340b to
6eaa214
Compare
|
One potential pitfall with ambiguity: it becomes difficult to differentiate between MAC and hyphen separator when the MACs themselves use hyphen - between octets. :)
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?
Thoughts @ckorber ? |
|
nftables takes all imaginable formats 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. |
|
Hate that I didn't see that 😄 Maybe Additionally after parsing it transforms to |
|
99% of users will supply valid values but we dont want others to break ruleset. |
Agreed. Just happier that we at least discussed the potential problem here. |
|
Don't want to push it, but there is no progress on this PR. Can someone review and merge it? |
|
Any maintainer can. |
|
@ckorber Recommend tweaking the commit message a bit to e.g. So this is now possible: In addition to the original: |
6eaa214 to
50ed665
Compare
|
I added that. Thanks!
|
|
Hi @robimarko could we get this in for 25? |
This commit is a preparation to add the possibility to block or accept a range of MAC addresses int they syntax of: ''' 00:11:22:00:00:00-00:11:22:ff:ff:ff ''' The commit depends on the PR openwrt/firewall4#74 being merged first. Signed-off-by: Christian Korber <[email protected]>
nft supports handling mac ranges and therefore this commit changes fw4 to support that feature. The src_mac is now allowed to be a range of two addresses. If no range is given, the old logic is applied. 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' ``` This is done by changing the regex to parse for an additional MAC address if '-' is matched after the first MAC address. Negative matching using exclamation mark is supported. Signed-off-by: Christian Korber <[email protected]>
50ed665 to
cc792d4
Compare
|
I updated the PR because as I tested it, it no longer supported inserting only one MAC. |
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:
Also negation with '!' at the beginning to block every MAC not in the range is possible.