-
Notifications
You must be signed in to change notification settings - Fork 206
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
netplan key-management support wpa-psk-sha256 (LP# 2085320) #531
Conversation
52fd4b1
to
9fc1e78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Isaac,
Thank you so much for your PR, it looks mostly good to me.
I just left a few comments that we should address before merging.
Apart from that, can you also rebase it into a single commit please?
In the first line of the commit message use something like: wifi: add support for wpa-psk-sha256
. And add a short description of the change with a link to the Launchpad bug.
Did you test the configuration against a real setup that only supports psk-sha256? I'll also try that later.
Thanks again!
@slyon feel free to also take a look 🌚
src/abi.h
Outdated
@@ -140,6 +140,7 @@ typedef enum { | |||
typedef enum { | |||
NETPLAN_AUTH_KEY_MANAGEMENT_NONE, | |||
NETPLAN_AUTH_KEY_MANAGEMENT_WPA_PSK, | |||
NETPLAN_AUTH_KEY_MANAGEMENT_WPA_PSKSHA256, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: please move the new option to the end of the enum (before _MAX) to avoid renumbering the other options. It might not cause any harm but the generate
binary (sadly) import internal files so we just want to avoid any ABI changes that might break it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part made me hard to decide where should I put when I did this PR.
Thanks for your great explanation
@@ -1019,6 +1019,8 @@ netplan_parser_load_keyfile(NetplanParser* npp, const char* filename, GError** e | |||
*/ | |||
if (ap->auth.key_management == NETPLAN_AUTH_KEY_MANAGEMENT_WPA_EAP) | |||
ap->auth.key_management = NETPLAN_AUTH_KEY_MANAGEMENT_WPA_EAPSHA256; | |||
else if (ap->auth.key_management == NETPLAN_AUTH_KEY_MANAGEMENT_WPA_PSK) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: the comment above needs to be updated now. Maybe just append this to the comment: The same logic is used for WPA-PSK
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree!! It will be clearer for other when reading this part~
9fc1e78
to
1e948a9
Compare
Hello @daniloegea, thanks for your quickly response and give the great feedback! Using this machine https://certification.canonical.com/hardware/202407-34231/ to connect the cert-wpa-tel-l4 which is only support the wpa-psk-sha256
|
46cfbfb
to
1e948a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! lgtm!
I tested it with the following hostapd.conf:
hw_mode=g
channel=1
ssid=fake net
wpa=2
wpa_key_mgmt=WPA-PSK-SHA256
wpa_pairwise=CCMP
ieee80211w=2
wpa_passphrase=12345678
It will connect only when using psk-sha256
. I'll prepare an autopkgtest for it later.
Description
Make the netplan key-management support wpa-psk-sha256.
If the AP only support the WPA-PSK-SHA256 key management, the original netplan code will only add the
wpa-psk
in wpa_supplicant confgAnd the networkctl status wlp1s0 will alway show
configuration
This change is add the
psk-sha256
once the use want to connect the ap which only support thewpa-psk-sha256
.The change is like
eap
andeap-sha256
.Checklist
make check
successfully.make check-coverage
).