[openwrt] Add support for WPA3-Personal #200
Conversation
|
Hello ! I am new to this project ! |
|
This is a first step for issue #194. |
|
I revised this PR to make IEEE802.11w configurable. |
masap
force-pushed
the
add-wpa3-psk
branch
2 times, most recently
from
226da68
to
c9f714c
Compare
|
I added support for WPA2/WPA3-Personal-mixed also. |
There was a problem hiding this comment.
Thanks @masap for your contribution, I will need some time before I am able to dedicate the proper time to test this, but I will get to it.
Ok. Now I added support for WPA3-Enterprise and WPA2/WPA3-Enterprise-mixed also. |
nemesifier
added this to In progress
in OpenWISP Priorities for next releases
via automation
|
PS, I think this PR would solve these issues, do you confirm? |
Yes, this PR fixes both of them. |
|
I added some tests to fix test coverage. |
| or protocol == 'wpa2_personal_mixed' | ||
| or protocol == 'wpa3_enterprise' | ||
| ): | ||
| cipher = 'auto' |
There was a problem hiding this comment.
According to this table on Encryption Modes in OpenWrt Wiki, WPA3 is compatible with only CCMP cipher.
Why did you use auto here @masap?
There was a problem hiding this comment.
I think we can remove this part. The following code must be able to handle it:
There was a problem hiding this comment.
Why did you use auto here
Because it is converted to CCMP in the OpenWrt when WPA3 is used.
But indeed it is not clear, I fixed it.
I think we can remove this part. The following code must be able to handle it:
We need this code when TKIP is specified.
| if (protocol == 'wpa3_personal' or protocol == 'wpa3_enterprise') and ( | ||
| 'ieee80211w' not in uci or uci['ieee80211w'] == '0' | ||
| ): | ||
| uci['ieee80211w'] = '1' |
There was a problem hiding this comment.
I believe this can be simplified as following
| if (protocol == 'wpa3_personal' or protocol == 'wpa3_enterprise') and ( | |
| 'ieee80211w' not in uci or uci['ieee80211w'] == '0' | |
| ): | |
| uci['ieee80211w'] = '1' | |
| if if protocol.startswith('wep') and 'ieee80211w' not in uci: | |
| uci['ieee80211w'] = '1' |
I don't see that it will be wise to overwrite user's configuration (uci['ieee80211w'] == '0'), since we won't be able to reflect this to use in configuration editor.
There was a problem hiding this comment.
I believe this can be simplified as following
Ok, I fixed to use if 'wpa3' in protocol: to include mixed mode.
I don't see that it will be wise to overwrite user's configuration (uci['ieee80211w'] == '0'), since we won't be able to reflect this to use in configuration editor.
Ok, removed.
Nice catch ! Indeed WPA3 spec has I would like to implement it at another PR after this PR is merged, is it OK? |
There was a problem hiding this comment.
@masap Thanks a lot for following up!
I have some concerns which I explained below, let us know if you can handle these changes or we will merge your patch in a branch and continue to improve it before merging to master.
| "propertyOrder": 4, | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
I think we will need a new encryption_mfp_property section which would be used only by WPA3 enterprise, and in this case it should only allow to be required, so something like encryption_mfp_property_required, eg:
"encryption_mfp_property_required": {
"properties": {
"ieee80211w": {
"type": "string",
"title": "management frame protection",
"enum": ["2"],
"options": {"enum_titles": ["required"]},
"propertyOrder": 4,
}
}
},There was a problem hiding this comment.
For that reason I think we should keep this value as is and just introduce
the new WPA personal mixed mode, maybe we can call the new option wpa_personal_sae_mixed?
Ok, I will leave the old options as they are. How about like this ?
| Description | value | old/new |
|---|---|---|
| WPA Personal | wpa_personal | |
| WPA2 Personal | wpa2_personal | |
| WPA3 Personal | wpa3_personal | new |
| WPA Enterprise | wpa_enterprise | |
| WPA2 Enterprise | wpa2_enterprise | |
| WPA3 Enterprise | wpa3_enterprise | new |
| WPA/WPA2 Personal Mixed Mode | wpa_personal_mixed | |
| WPA2/WPA3 Personal Mixed Mode | wpa2_personal_mixed | new |
| WPA/WPA2 Enterprise Mixed Mode | wpa_enterprise_mixed | |
| WPA2/WPA3 Enterprise Mixed Mode | wpa2_enterprise_mixed | new |
I think we will need a new encryption_mfp_property section which would be used only by WPA3 enterprise
Since 802.11w has existed before WPA3 and has been used together with WPA/WPA2, I think it should be able to be used for other than WPA3.
There was a problem hiding this comment.
@masap check the comment below (#200 (comment)). The current schema for encryption_mfp_property will stay and it will be used with WPA2/WPA (allowing users . For WPA3, we will use encryption_mfp_property_required which will set MFP to required by default.
There was a problem hiding this comment.
Sorry, I mis-understood. Ok, I will separate WPA3 only mode and use encryption_mfp_property_required.
There was a problem hiding this comment.
@masap sounds great, I think we could show WPA3 first in the UI, then WPA2 and WPA, what do you think?
There was a problem hiding this comment.
I think it is nice. I will fix it like this.
| title |
|---|
| No encryption |
| WPA3 Personal (new) |
| WPA3 Enterprise (new) |
| WPA2/WPA Personal |
| WPA2/WPA Enterprise |
| WPS |
| WEP |
There was a problem hiding this comment.
I fixed it like this. Because WPA3/WPA2/WPA Personal/Enterprise includes WPA3/WPA2 Personal/Enterprise Mixed Mode.
| title |
|---|
| No encryption |
| WPA3 Personal (new) |
| WPA3 Enterprise (new) |
| WPA3/WPA2/WPA Personal (modified) |
| WPA3/WPA2/WPA Enterprise (modified) |
| WPS |
| WEP |
netjsonconfig/schema.py
Outdated
| "encryption_wpa_personal": { | ||
| "title": "WPA2/WPA Personal", | ||
| "title": "WPA3/WPA2/WPA Personal", |
There was a problem hiding this comment.
I think we have to leave this as is and add a new section dedicated to WPA3 which would make use of encryption_mfp_property_required so that we ensure that when a user selects WPA3, the 802.11w option is automatically set as required without user intervention, otherwise we can all bet that most users will not figure out they have to change it and will be mad at us for not making it easy for them and that's something we should avoid.
netjsonconfig/schema.py
Outdated
| @@ -491,10 +511,11 @@ | |||
| } | |||
| }, | |||
| "encryption_wpa_enterprise_ap": { | |||
| "title": "WPA2/WPA Enterprise (access point)", | |||
| "title": "WPA3/WPA2/WPA Enterprise (access point)", | |||
There was a problem hiding this comment.
similar concept here as above, that is, I think we have to create a new dedicated section for WPA3 enterprise which makes use of encryption_mfp_property_required
netjsonconfig/schema.py
Outdated
| @@ -532,10 +553,11 @@ | |||
| ], | |||
| }, | |||
| "encryption_wpa_enterprise_sta": { | |||
| "title": "WPA2/WPA Enterprise (client)", | |||
| "title": "WPA3/WPA2/WPA Enterprise (client)", | |||
|
I have already fixed all the pointed out items 2 weeks ago. Is there any comment ? |
|
Hey @masap, I got occupied with other things in OpenWISP (we are pushing the next release). It worked fine the last time I tested it after you made requested changes. It will take some time for us to review the code changes more closely. I appreciate your patience. |
|
Ok, Can I help you guys with your review by attending this event? |
Sure, you are welcome to participate. 😄 |
|
Could I discuss this PR at the event ? Or is this an event for brand new contributor ? |
|
@masap it is open for everyone! If you have any doubts regarding the event, please ask them on our development chat channel: https://gitter.im/openwisp/development |
|
I had to cancel my attendance at that event due to scheduling conflicts. |
There was a problem hiding this comment.
Thank you for following up!
Me and @pandafy have been testing this in the last 2 hours and it's really good.
The only remaining issue with the current changes that we see is with the mixed modes which have the management frame protection disabled by default and that could be a problem: doesn't WPA3 require that to be at least "optional"?
In our understanding, WPA3/WPA2 mixed mode (transition mode?) requires "management frame protection" to be "optional", while now it's disabled by default.
This is an issue because most users will not know this and will expect OpenWISP to select the right default for them, so we would be selecting a wrong
default and they will 100% complain to us, which is very undesirable and we have to avoid it at all costs.
I think we could add the following options:
- a dedicated WPA3/WPA2 Personal mixed mode with management frame protection set to optional
- a dedicated WPA3/WPA2 Enterprise mixed mode with management frame protection set to optional
- leave the rest (WPA2 mixed mode) mostly untouched apart from adding the management frame protection option (which you did) set to disabled by default
All the rest looks good! Apart from the fact that we'll have to update the docs and provide at least an example of WPA3 access point.
Let us know what you think.
Fixed.
I want to finish this PR first. Could you please make an issue about this ? |
This patch is tested on these. - OpenWrt: latest (4b587f25614f3f7215360f96807ce760fa4ef3aa) - hardware: TP-Link Archer C6 v2 Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
This patch is tested on these. - OpenWrt: latest (4b587f25614f3f7215360f96807ce760fa4ef3aa) - hardware: TP-Link Archer C6 v2 Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
This patch is tested on these. - OpenWrt: latest (4b587f25614f3f7215360f96807ce760fa4ef3aa) - hardware: TP-Link Archer C6 v2 Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
This patch is tested on these. - OpenWrt: latest (4b587f25614f3f7215360f96807ce760fa4ef3aa) - hardware: TP-Link Archer C6 v2 Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
This patch is tested on these. - RADIUS authentication server: FreeRadius 3.0.25 - OpenWrt: latest (4b587f25614f3f7215360f96807ce760fa4ef3aa) - hardware: TP-Link Archer C6 v2 Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
Please add the documentation in this PR. |
|
Thank you very much for following up @masap! While testing with openwisp-controller, I found some small issues (mostly related to make this more user friendly). |
| "ieee80211w": { | ||
| "type": "string", | ||
| "title": "management frame protection", | ||
| "enum": ["2"], |
There was a problem hiding this comment.
We can also add "readOnly": True, to make this option disabled in Configuration Editor of openwisp-controller.
Same applies for encryption_mfp_property_optional.
There was a problem hiding this comment.
We convened with @pandafy that this PR is almost ready and we'll handle the last adjustments.
|
Thank you very much for contributing @masap! 🚀 |
This patch is tested on these.
OpenWrt: latest (4b587f25614f3f7215360f96807ce760fa4ef3aa)Signed-off-by: Masashi Honma masashi.honma@gmail.com