search

Koenkk / zigbee-herdsman

A Node.js Zigbee library
MIT License
477 stars 297 forks source link

[Proposal] Removal of "forever" Permit Join #940

Open Nerivec opened 7 months ago

Nerivec commented 7 months ago

After observing behaviors while testing ember and seeing reports here and there about this, I think removing the possibility to open the network "forever" via internal workarounds would make sense.

1) It doesn't seem to work well in general from Discord reports (compared to triggering as needed with the Permit Join button). 2) Some stacks now enforce no "forever", which messes with the interval refresh triggered by Z2M, basically breaking the "assumed state". 3) It prevents state validation, because of the interval refresh that won't actually do anything if the network is already open (at least in EmberZNet) 4) It's bad for security...

The TODOs on this would appear to be:

Related code (probably not all...):

https://github.com/Koenkk/zigbee-herdsman/blob/b004cf827da04eb78e7658776bd2726289797b16/src/controller/controller.ts#L237

https://github.com/Koenkk/zigbee2mqtt/blob/f04ade6d764518bd8228eb32a15e96cd4e3fbde6/data/configuration.yaml#L5 https://github.com/Koenkk/zigbee2mqtt/blob/f04ade6d764518bd8228eb32a15e96cd4e3fbde6/lib/types/types.d.ts#L122 https://github.com/Koenkk/zigbee2mqtt/blob/f04ade6d764518bd8228eb32a15e96cd4e3fbde6/lib/util/settings.schema.json#L48 https://github.com/Koenkk/zigbee2mqtt/blob/f04ade6d764518bd8228eb32a15e96cd4e3fbde6/lib/util/settings.ts#L39 https://github.com/Koenkk/zigbee2mqtt/blob/f04ade6d764518bd8228eb32a15e96cd4e3fbde6/lib/controller.ts#L139 https://github.com/Koenkk/zigbee2mqtt/blob/f04ade6d764518bd8228eb32a15e96cd4e3fbde6/lib/extension/bridge.ts#L161

Koenkk commented 7 months ago

There are some special cases where permit join is required to be permanently enabled, if I remember correctly for some green power devices. As a simple first step, let's just remove the permit_join option from the Z2M configuration.yaml. Then we can see if any setups break.

Nerivec commented 7 months ago

Just putting false as override would be a good first step for sure (just ignore the config at first). Better off waiting until after the 1st to implement any of this; just thought I'd get the ball rolling on the topic 😉

There are some special cases where permit join is required to be permanently enabled, if I remember correctly for some green power devices.

Is it the network that needs to be open, or the commissioning mode that needs refreshing? I guess that refresh logic could be moved to GP if it's the second case (I suppose a separate button would be ideal in that scenario, kind of like Touchlink... make it clear what's what). @dduransseau @chris-1243 Sorry for the ping, again, but I guess you've gained the GP-go-to status 😁

chris-1243 commented 7 months ago

For the PTM215Z/216Z, there is no need to have the permit join always on, on my side at least. I do open the network for the pairing and close it when done.

In configuration.yaml permit join is set to false for me.

dduransseau commented 7 months ago

That's an interesting case, I have a big doubt that permit_joint status (true/false) make any difference to allow GP device to join or not. Sometimes I figure out that GP device joined the "wrong" instance of Z2M when I performed some test. For example during ember test I figure out that GP switch joined the "main" instance and not the "lab" even if "main" was not configured to allow join. I'll perform some tests with only one instance up and running to confirm this.

Nerivec commented 7 months ago

@chris-1243 Can you confirm this is the case in zStack too? I believe you have access to a network using it.

@dduransseau If that's confirmed, my guess would be that the coordinator is always in commissioning mode for GP (no matter what Z2M says), but that would likely be EmberZNet-specific, and coordinator-only.

dduransseau commented 7 months ago

Nope, I figure out this behavior on deconz with conbee II for the first time. I'm not an expert of herdsman, since GP provisioning do not use same type of frame, would it be possible that GP frame bypass the allow_join filter ?

EDIT: I just performed the test to remove a GP device from the test instance (ember) and the main one (that use conbee II), when I execute the the commissioning sequence on the GP switch, it appear on both instance and actions are correctly handled by both instance.

chris-1243 commented 7 months ago

@Nerivec I need to check if this behaviour occurs between my main network (zstack) and test (ember).

As I have both PTM215/216, I will try with both of them. As far as I remember, I have not seen such behaviour.

My two networks are not on the same channel and do not share the same network keys, pan id and extended id. All those values are different.

As you have to choose your network channel when pairing, it sounds strange for me to see the same device connected to two differents networks.

EDIT: If adapters are not on the same channel, nothing happens. That make sense. If I pair a ZGP device in zstack (channel 20), nothing is shown in ember (channel 25). The other way around is the same.

Then if both adapters are on the same channel, zstack is not impacted as it needs an open router (Hue for PTM216Z, Hue or any brand able to translate like IKEA, Sunricher for PTM215Z). ember, the device pairs even if permit join is not active. Then, if you completely reset the device (all buttons pressed for ~10s) and you try to pair it again:

It seems ember always reacts to the commissioning frame sent by the PTM215Z/216Z even if the permit join option is turned off

dduransseau commented 7 months ago

In my scenario both network are on the same channel but different network and pan key.

Nerivec commented 7 months ago

Since the current "Permit join" executes 2 actions (1- open the network, 2- turn on GP commissioning mode), the steps are a bit blurred as to what GP actually needs to pair/pair after reset. I think there is a need for separation here. If only to ensure proper logic, because it seems devices are currently not following open/close as directed/expected by Z2M for GP stuff.

dduransseau commented 7 months ago

I just found this issue that seems related

MattWestb commented 6 months ago

In my scenario both network are on the same channel but different network and pan key.

Make sure also have different Extended Pan-ID or you can getting strange problems then paring and network managements commands in both networks.

github-actions[bot] commented 4 weeks ago

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 30 days

Hedda commented 2 weeks ago

After observing behaviors while testing ember and seeing reports here and there about this, I think removing the possibility to open the network "forever" via internal workarounds would make sense.

  1. It doesn't seem to work well in general from Discord reports (compared to triggering as needed with the Permit Join button).
  2. Some stacks now enforce no "forever", which messes with the interval refresh triggered by Z2M, basically breaking the "assumed state".
  3. It prevents state validation, because of the interval refresh that won't actually do anything if the network is already open (at least in EmberZNet)
  4. It's bad for security...

Still makes sense to me.