search

kbr / fritzconnection

Python-Tool to communicate with the AVM Fritz!Box by the TR-064 protocol and the AHA-HTTP-Interface
MIT License
314 stars 59 forks source link

Determine QR security parameter im much simple way. #224

Closed derVedro closed 2 months ago

derVedro commented 3 months ago

According to the specification, there are only four possible values for security parameter: nopass, WEP, WPA and WPA-EAP. The WPA-EAP is no relevant for the FritzBox, so we had to deal only with three values.

The way security parameter is created for now is a bit overkill. There is no particular need for checking if beacon value match something in NewX_AVM-DE_PossibleBeaconTypes call result. Sure, some one can put a garbage value in security variable an call _get_beacon_security(..., security), but why some one should? The function highly depends on FritzWLAN instance and is only usable in one context. So I think there is no need for a _get_beacon_security() function at all, all it do can be achieved much simpler way with a dict and a default value. The default value can still be discussed, but for now I decided to go with nopass.

My guess is that most devices that get wi-fi credentials via QR code don't care much about security value if it's not nopass. They just determine connection for yourself and use the secret given in QR code. By the way, there is currently a security issue in code.

kbr commented 2 months ago

Thanks for the work on this. The original idea was to avoid hardcoding a lookup-dictionary with fixed keys but take everything provided "as is" from the router. This worked fine until #191 popped up. Then the code got more complicated with the guessing for WPA on older models.

I will merge this as it seems to also cover #191, assuming that even outdated FritzOS versions should return a beacontype covered by the _BEACONTYPE_TO_QR_SECURITY dictionary. I'm unable to test this because of missing older hardware, but suppose that is a reasonable assumption. We will see.

Beside this I would suggest to rename _QR_SECURITY to _WLAN_SECURITY. Well, it is used to create qr-codes, but in the end it is describing the latter. However, for the functionality of the code this does not matter.

derVedro commented 2 months ago

Thanks for the work on this. The original idea was to avoid hardcoding a lookup-dictionary with fixed keys but take everything provided "as is" from the router. This worked fine until #191 popped up. Then the code got more complicated with the guessing for WPA on older models.

Yes, NewX_AVM-DE_PossibleBeaconTypes would be a good idea for itself, but it gives back all the types, that must be separated into three groups. Unfortunately, you can't get around this without some kind of hard-coded values.

Beside this I would suggest to rename _QR_SECURITY to _WLAN_SECURITY. Well, it is used to create qr-codes, but in the end it is describing the latter. However, for the functionality of the code this does not matter.

I don't think the proposed renaming is such a good idea. These strings are really only relevant in the context of QR codes. A network that is encrypted with WPA2 would therefore only be listed as "WPA". However, this is not the same in terms of encryption, but this is the case for QR codes. Users who use the library could be misled into thinking that this is the actual encryption type of the network. They would therefore have to read deeper into the code in order to understand it.

kbr commented 2 months ago

These strings are really only relevant in the context of QR codes. A network that is encrypted with WPA2 would therefore only be listed as "WPA". However, this is not the same in terms of encryption, but this is the case for QR codes.

Ok, this is also an argument and all after all the constants are just used in the context of creating QR codes.