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

Feature request: bridge_isolate option in wireless interface configuration #7567

Open
JFroch opened this issue Jan 17, 2025 · 30 comments
Open

Comments

@JFroch
Copy link

JFroch commented Jan 17, 2025

What would you like to see in luci?

With this commit we got the bridge_isolate option which flags the wireless interface as isolated when connected to a bridge. This is useful to isolate the clients on multiple interfaces/frequencies on the same device, but at the moment only possible by adding the option in /etc/config/wireless manually.

I would like to see the bridge_isolate option right next to the isolate (Isolate Clients) option in the "Advanced Settings" tab of the wireless interface configuration. Perhaps "Isolate on Bridge" would be a fitting name?

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 19, 2025

I can make a PR for this if needed. Is it just a flag called bridge_isolate in the same section as the isolate option? i.e.

				o = ss.taboption('advanced', form.Flag, 'isolate', _('Isolate Clients on radio'), _('Prevents client-to-client communication on single radio'));
				o.depends('mode', 'ap');
				o.depends('mode', 'ap-wds');
				o = ss.taboption('advanced', form.Flag, 'isolate_bridge', _('Isolate Clients on bridge'), _('Prevents client-to-client communication over brige'));
				o.depends('mode', 'ap');
				o.depends('mode', 'ap-wds');

I edited the original flag description a bit as well.

@systemcrash
Copy link
Contributor

That'd be great @Ramon00 - that can go in for 24.

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 19, 2025

Ok @JFroch please have a test see PR: #7572

@JFroch
Copy link
Author

JFroch commented Jan 19, 2025

Thank you @Ramon00.
It looks like you got the order in the code switched, it should be bridge_isolate.

From the docs (https://openwrt.org/docs/guide-user/network/wifi/basic) I got this info:

Image

I guess we can drop the dependencies on the ap and ap-wds modes for bridge_isolate, as this option will just mark the interface as "isolated" on the bridge, regardless of the mode.
Perhaps we should change the description of the isolate option to 'Prevents wireless client-to-client communication on this interface' in order to avoid misinterpretation of the word "radio" and clarify that it affects just wireless clients?

I will try to test it tomorrow, if you have the time to update the PR.

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 20, 2025

@JFroch yes I swapped the words, but I actually already fixed that before you wrote your comment. :)

About dropping the dependencies. I am not sure. What would the point of this be if you have no AP mode? Either the dependency is for both or neither I would say.

About the use of the word "radio", im not sure that if you write "interface" it is more clear. It could be misinterpreted as e.g. the lan interface? Somehow interface makes it sounds more broad. I guess "wireless-interface" would already be better?

@JFroch
Copy link
Author

JFroch commented Jan 20, 2025

@Ramon00 Ahh sorry, I didn´t see you updated the PR. :)
I tested it on a 24.10.0-rc5 system and can confirm it works as expected.

While I can´t think of any useful case at the moment, this is more of a "we shouldn´t limit the feature just because we can´t think of a use case" opinion I have. This is also why I initially proposed a more generic name, since it doesn´t really only isolate wireless clients, but could be applied to a mesh or wds-client interface as well and restrict another bridge port from accessing devices over that connection if wanted for some reason.

Essentially it is the same as this option that can be set in Network->Interfaces->Devices just in a more convenient location, as you can just define it in the wireless interface settings.

Image

I agree on the last point, "wireless interface" would be a good fit and is already used in the menu.

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 20, 2025

Ok lets use "wireless interface".

How about the dependencies then? Remove it in both cases, i.e. for "isolate" and "bridge isolate"? Or would there be a reason to limit for one and not the other?

@JFroch
Copy link
Author

JFroch commented Jan 20, 2025

Remove them just from "bridge_isolate", the "isolate" option is just applicable in the two AP modes according to the documentation. The isolation happens in two different places, one is blocking the communication directly on the same wireless interface while the other one happens at the bridge port level.

Perhaps this illustrates it a bit more, isolate limits the communication between devices X and Y on wireless Interface A and bridge_isolate limits the traffic between Interfaces A and B over the bridge, if enabled on both Interfaces, which in turn leads to X or Y to not be able to communicate with Z.

Image

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 20, 2025

I understand the concept, but just feels wrong to "leave it open because you never know" for the one, and then decide for the other "there is never any reason". I like consistency, that helps with understanding how things work. In the end the GUI should also provide the user with useful things that is know to work, not overwhelm with things that have no effect or even have an adverse effect. What happens if you enable this on a WDS client?

I guess we need a 3rd person to vote on this :)

@JFroch
Copy link
Author

JFroch commented Jan 20, 2025

I get where you are coming from and would agree, but I suspect this to be more of a technical limitation rather than an arbitrary one with hostapd not being used for client modes and perhaps not supporting/ignoring the isolate option on modes other than AP and WDS AP, but that is speculation on my part. @systemcrash do you have any input on this?

As for enabling bridge_isolate on a WDS client interface: It depends on the bridge config.
Let´s say we have br-test with lan1, lan2 and phy0-sta0 connected to a WDS AP. If we enable bridge_isolate just on phy0-sta0 nothing happens. Now if we enable it on phy0-sta0 and lan2 (where it is just isolate) from my understanding we have the case that the device(s) on lan2 can send packets just to lan1 but not phy0-sta0 and whatever is connected behind that, while anything on lan1 has access to lan2 and anything connected to phy0-sta0, while anything behind phy0-sta0 can only send packets to lan1, not being able to reach anything connected to lan2.

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 21, 2025

@systemcrash what do you think, remove the depends or not?

@systemcrash
Copy link
Contributor

systemcrash commented Jan 21, 2025

I'm not familiar with the setting yet. Best is to trace it back to its introduction and ask the implementer its intended usage if it isn't documented in the wiki.

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 21, 2025

@nbd168 Can you enlighten us? When is bridge_isolate option supposed to be function? Same depends as isolate or is it also useful in other situations?

@systemcrash
Copy link
Contributor

Might be worthwhile to dig in the code to figure out its usage. Felix is not frequently available if that's who you tagged.

@systemcrash
Copy link
Contributor

So the commit message says openwrt/netifd@f3e06e8

This enables the device bridge port isolate flag

So it would seem that this option is indifferent to wifi modes, since the option affects bridge behaviour.

I get where you are coming from and would agree, but I suspect this to be more of a technical limitation rather than an arbitrary one with hostapd not being used for client modes and perhaps not supporting/ignoring the isolate option on modes other than AP and WDS AP, but that is speculation on my part. @systemcrash do you have any input on this?

The depends option affects whether the option is shown in the GUI or not (implying suitability or dependency), and has no bearing on whether the option, when enabled, takes effect (unless special unset handling takes place).

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 22, 2025

So it would seem that this option is indifferent to wifi modes, since the option affects bridge behaviour.

If this is affecting bridge behavior only, i.e. independent of wifi, then this limits the packets to only go to the CPU where the bridge is located? Or can it go out a LAN port?

If the affect is only limited to wifi device, and you have a WDS client which does outside world connection does it then stop this as well?

@systemcrash
Copy link
Contributor

I think it’s like that old internet cafe client isolation option. Access the world except for each other locally. It requires understanding the corresponding kernel option..

@JFroch
Copy link
Author

JFroch commented Jan 22, 2025

If this is affecting bridge behavior only, i.e. independent of wifi, then this limits the packets to only go to the CPU where the bridge is located? Or can it go out a LAN port?

As I mentioned in a previous comment it does the same as the "Port Isolation" option for network interfaces. Quoting the description: "Only allow communication with non-isolated bridge ports when enabled"

I think it’s like that old internet cafe client isolation option. Access the world except for each other locally.

That pretty much sums it up.

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 22, 2025

As I mentioned in a previous comment it does the same as the "Port Isolation" option for network interfaces. Quoting the description: "Only allow communication with non-isolated bridge ports when enabled"

Ok so, in 24 you can select for each bridge port and each bridged interface if it is isolated or not?

This also means that "Prevents client-to-client communication over brige" is not (fully) describing what it is doing. "Prevents clients from contacting other wired or wireless clients on other isolated wired or wireless interfaces."

@JFroch
Copy link
Author

JFroch commented Jan 22, 2025

Ok so, in 24 you can select for each bridge port and each bridged interface if it is isolated or not?

Exactly, from what i´ve found the ability to configure the option in the Network tab in LUCI was added in 2020, while the 'bridge_isolate' option was added in 2023 and does work on 23.05.5, so it isn´t a 24 feature.

This also means that "Prevents client-to-client communication over brige" is not (fully) describing what it is doing. "Prevents clients from contacting other wired or wireless clients on other isolated wired or wireless interfaces."

What about using the already existing description from the Network tab, or changing it to "Prevent communication with other isolated bridge ports."? Could that be too technical/confusing?

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 23, 2025

Just say what you want as help text

@systemcrash
Copy link
Contributor

systemcrash commented Jan 23, 2025

My reading of this setting from the original commit:

bridge: add support for port isolation. Isolated ports cannot communicate between each other, but they can still communicate with non-isolated ports commit.

I think this is the best we can do - since the intention of this setting seems to have been to turn this behaviour on, for a bridge port which connects to an AP. Other devices which have the same setting cannot directly communicate.

So something like:

Isolate Bridge Port and Isolated ports cannot communicate between each other, but can with non-isolated ports.

This avoids the baggage of wifi, or AP, or any other second-order implications of how it's all connected. I think @Ramon00's first suggestion is quite close to this, but @Ramon00's suggestion of client-to-client might not always hold true.

What would you be satisfied with @JFroch ? If you can indeed confirm whether a client on an isolated port is no longer able to communicate with another client on the same isolated port, then @Ramon00's description is very fitting.

( The original commit mentions a source and destination port test, which leads me to believe that traffic originating and destined to the same port, which is 'isolated' would not be possible )

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 23, 2025

I would not call a wireless interface a port. Anyway, for me the documentation is unclear, so for me not clear what this setting is exactly doing. Maybe its best if I just close my PR then somebody that actually knows what this does makes a PR.

@systemcrash
Copy link
Contributor

I would not call a wireless interface a port.

Nor would I - but that AP somehow connects to the bridge, and are thus a port on it.

Anyway, for me the documentation is unclear, so for me not clear what this setting is exactly doing.

Fair. It's quite terse, so I figure it does exactly as it says, but where it is implemented makes it perhaps slightly confusing.

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 23, 2025

but if i bridged two radios to a switch, and isolate both radio interfaces, then there is no isolation towards the bridge port. Only the one wireless interface to the other?

@JFroch
Copy link
Author

JFroch commented Jan 24, 2025

Alright, I have done some reading and testing, the short answer is:

If you can indeed confirm whether a client on an isolated port is no longer able to communicate with another client on the same isolated port, then @Ramon00's description is very fitting.

I can confirm that.

The long answer/what I have found

In OpenWRT the 'ap_isolate' hostapd option is always set, regardless of the 'isolate' wireless option. This leads to the traffic always being forwarded to the bridge, instead of being directly forwarded by hostapd to the destination client on the same wireless interface, never reaching the bridge, which I assumed to be the case when the 'isolated' wireless option was disabled. So I was wrong about 'bridge_isolate' not affecting clients on a single wireless interface in AP mode with 'isolate' disabled.
What the 'isolate' wireless option does is affect (disable) the 'hairpin_mode' kernel setting (/sys/class/net/*/brport/hairpin_mode) which is normally enabled for wireless interfaces. What this setting does: "Controls whether traffic may be send back out of the port on which it was received."
Now the 'isolated' kernel option (/sys/class/net/*/brport/isolated) which is affected by the 'bridge_isolate' option isolates it from other isolated bridge ports as well as itself, as it it also isolated.

So 'bridge_isolate' "overrides" 'isolate' being disabled, leading to the clients on the interface not being able to communicate among themselves.

Some more details can be found here

As for the description I guess with this new info a short/simple description wouldn´t be up to the task.

I suggest we use Isolate Bridge Port for the name and Prevents communication with other isolated bridge ports while allowing it with non-isolated ones. In AP modes this also prevents client-to-client communication on this interface. for the description.
@systemcrash what do you think?

Sorry for the back and forth on this.

@systemcrash
Copy link
Contributor

I suggest we use Isolate Bridge Port for the name and Prevents communication with other isolated bridge ports while allowing it with non-isolated ones. In AP modes this also prevents client-to-client communication on this interface. for the description. @systemcrash what do you think?

This is good detective work. In this case, it's not a race, so we had time to get it right. I might suggest this:

Prevents communication only with targets on isolated bridge ports (while allowing it with targets on non-isolated ones).

I think that makes this part redundant: In AP modes this also prevents client-to-client communication on this interface.

Here, it's a bit ambiguous what AP modes refers to.

Maybe: This also prevents client-to-client communication on the same interface when the WiFi device is in AP mode?

@JFroch
Copy link
Author

JFroch commented Jan 24, 2025

Sounds good to me.

@Ramon00
Copy link
Contributor

Ramon00 commented Jan 25, 2025

`Isolate Bridge Port.'

A wireless interface is not generally considered a port

Prevents communication only with targets on isolated bridge ports (while allowing it with targets on non-isolated ones).

Remove the word only.

This also prevents client-to-client communication on the same interface when the WiFi device is in AP mode

Are you sure it prevents all client to client communication? No driver shortcuts?
How about if the other isolated port is on another AP? Is this only local?

@JFroch
Copy link
Author

JFroch commented Jan 27, 2025

A wireless interface is not generally considered a port

I agree, but as @systemcrash already said, it connects to the bridge as a port on it. The path for the kernel option (/sys/class/net/phy0-ap0/brport/isolated) also uses that naming convention.

No driver shortcuts?

Not as far as my testing showed (with hardware using the mt76 driver). Also in the case there were driver shortcuts (and the ap_isolate hostapd option wouldn´t disable that behaviour) the "normal" isolate option wouldn´t work as well, so we would have got bug reports for devices using a driver featuring this behaviour.

Are you sure it prevents all client to client communication?

On that specific interface: yes.

How about if the other isolated port is on another AP?

Can you clarify that question? If you mean we have two APs "A" and "B" both with bridge "br-guest" consisting of [phy0-ap0, phy1-ap0, lan1] with bridge_isolate enabled for phy0-ap0 and phy1-ap0, with lan1 connected to the same switch the answer is:

A wireless client of AP "A" could communicate with clients on AP "B" but not with other clients on AP "A". To isolate the clients in that scenario we would need the switch to have the same kind of port isolation feature or add nftables bridge rules that deny unwanted traffic.

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

No branches or pull requests

3 participants