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

Determine QR security parameter im much simple way. #224

Merged
merged 5 commits into from
Jul 15, 2024

Conversation

derVedro
Copy link
Contributor

@derVedro derVedro commented Jun 28, 2024

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.

@derVedro derVedro changed the title Determine QR security parameter im much simpy way. Determine QR security parameter im much simply way. Jun 28, 2024
@derVedro derVedro changed the title Determine QR security parameter im much simply way. Determine QR security parameter im much simple way. Jun 30, 2024
@kbr
Copy link
Owner

kbr commented Jul 15, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

kbr commented Jul 15, 2024

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.

@kbr kbr merged commit d6def73 into kbr:master Jul 15, 2024
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.

None yet

2 participants