Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fw4: when ipset matches MAC, allow set family to 'any' #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zsien
Copy link

@zsien zsien commented Jul 27, 2024

When filtering by MAC address, it is usually necessary to filter both IPv4 and IPv6.
If it is not allowed to set the family of ipset to any, it will be necessary to create a separate, identical ipset for both IPv4 and IPv6.

Fixes: #16

When filtering by MAC address, it is usually necessary to
filter both IPv4 and IPv6.
If it is not allowed to set the family of ipset to any,
it will be necessary to create a separate, identical ipset
for both IPv4 and IPv6.

Fixes: openwrt#16
Copy link

@brada4 brada4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does what it says.

@f00b4r0
Copy link

f00b4r0 commented Feb 6, 2025

So I just stumbled upon this PR while looking again at #16 and after testing it I don't see how it "fixes" anything.

The meta nfproto ipv4 match is still emitted by fw4 for every rule that refers to the set. Sure you can add an option family 'any' to the ipset configuration, it will not emit a warning but it changes nothing.

@zsien
Copy link
Author

zsien commented Feb 15, 2025

So I just stumbled upon this PR while looking again at #16* and after testing it I don't see how it "fixes" anything.

The meta nfproto ipv4 match is still emitted by fw4 for every rule that refers to the set. Sure you can add an option family 'any' to the ipset configuration, it will not emit a warning but it changes nothing.

@f00b4r0 To maintain compatibility with the configuration, the default value for the ipset family is still ipv4.

I tested the configuration from #16 as follows:

config rule
  option name 'Forward-auth-captive'
  option src 'lan'
  option dest 'wan'
  option proto 'any'
  option target 'ACCEPT'
  option ipset 'captive'

config ipset
  option name 'captive'
  option family 'any'
  list match 'src_mac'

The generated nftables rules are:

set captive {
  type ether_addr
}
...
chain forward_lan {
  ...
  ether saddr @captive counter packets 0 bytes 0 jump accept_to_wan comment "!fw4: Forward-auth-captive"
  ...
}

It can be seen that this modification is effective.

@brada4
Copy link

brada4 commented Feb 15, 2025

I know, it duly prints warning when that is employed.

@f00b4r0
Copy link

f00b4r0 commented Feb 16, 2025

@zsien unfortunately while the test you performed works, this one doesn't:

config redirect
        option name 'Redirect-unauth-captive'
        option src 'lan'
        option src_dport '80'
        option proto 'tcp'    
        option target 'DNAT'   
        option reflection '0'
        option ipset '!captive'

config ipset
        option name 'captive'
        option family 'any'
        list match 'src_mac'

It fails to load the firewall with:

/dev/stdin:236:35-36: Error: syntax error, unexpected !=, expecting newline or semicolon
		meta nfproto ipv4 tcp dport 80  != @captive counter redirect to 80 comment "!fw4: Redirect-unauth-captive"
		                                ^^

meta nftproto ipv4 is still emitted and ether saddr is missing.
It's likely that other corner cases are thus broken by your change.

I also don't think that src_mac ipsets should still default to ipv4, but that's left to the maintainer's appreciation.

HTH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fw4 assumes ether_addr sets are ipv4 only
3 participants