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
wifi: make it possible to have a psk and an eap password simultaneously #416
Conversation
a7d0719
to
b4acb08
Compare
Related to #415 |
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 Danilo for working out this generic solution inside the existing YAML schema!
Besides a single ABI-stability concern, which needs to be looked at, I think this is mostly good.
I think the extended logical checks can be a bit complex/confusing at times and should be supported by some more comments. Also, there might be further potential for simplification. See my other non-blocking inline comments.
src/parse-nm.c
Outdated
if (ap->auth.password) | ||
ap->has_auth = TRUE; | ||
} else { | ||
keyfile_handle_generic_str(kf, "wifi-security", "psk", &ap->auth.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.
question (non-blocking): I wonder if it'd make sense to parse the keyfile's wifi-security.psk
setting into Netplan's psk
field unconditionally, to make things less confusing? So Netplan's psk
field would always ever be used to store a PSK. In certain conditions that PSK might be duplicated into auth.password
for backwards compatibility.
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.
I did that to help differentiate when the PSK will go to "ap".password or "ap".auth.password so it wouldn't change behavior (and tests would not break :P). I'll see how I can always add the PSK to the new psk variable.
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.
yeah it cause a small conflict with our previous idea of always set "pmf" to optional if "ap".password is used. By always reading the PSK into ap->auth.psk
we'll end up always emitting "ap".password in the YAML and enabling the pmf setting even if it wasn't in the original keyfile we are loading, which is not a big deal I guess. That was the reason why I tried to differentiate the cases...
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.
I pushed this change in a new commit
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.
I did that to help differentiate when the PSK will go to "ap".password or "ap".auth.password so it wouldn't change behavior (and tests would not break :P). I'll see how I can always add the PSK to the new psk variable.
Understood. And I think that makes sense! Enabling the pmf
setting even if it wasn't in the original keyfile might actually be an issue, as it might break NetworkManager unit-tests at build time, when it's trying to re-generate the correct keyfile. I'm not sure if it has tests around that PMF setting, but it could have in the future.
So we should really try to not change behavior here. Do you think we could have a has_auth_psk_shortcut
helper variable, to indicate which case we're in? Then making use of this in the YAML emitter (netplan.c
), to write the correct password field, instead of changing the tests. And also no change keyfile behavior.
ba80152
to
e80696f
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.
I added some concerns about the recent changes in-line and marked some of the other comments as resolved.
src/parse-nm.c
Outdated
if (ap->auth.password) | ||
ap->has_auth = TRUE; | ||
} else { | ||
keyfile_handle_generic_str(kf, "wifi-security", "psk", &ap->auth.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.
I did that to help differentiate when the PSK will go to "ap".password or "ap".auth.password so it wouldn't change behavior (and tests would not break :P). I'll see how I can always add the PSK to the new psk variable.
Understood. And I think that makes sense! Enabling the pmf
setting even if it wasn't in the original keyfile might actually be an issue, as it might break NetworkManager unit-tests at build time, when it's trying to re-generate the correct keyfile. I'm not sure if it has tests around that PMF setting, but it could have in the future.
So we should really try to not change behavior here. Do you think we could have a has_auth_psk_shortcut
helper variable, to indicate which case we're in? Then making use of this in the YAML emitter (netplan.c
), to write the correct password field, instead of changing the tests. And also no change keyfile behavior.
c6d9bca
to
03fe185
Compare
I tried to restore the original behavior and still always read the psk in auth.psk. |
Originally, Netplan assumes the PSK and 802-1x identity password will not be used simultaneously. With this change, when both passwords are used, the PSK is expected to be placed in "ssid".password and the identity in "ssid".auth.password. "ssid".auth.password can also be used by the PSK if it's the only password. When 802-1x is used, the PMF setting will not be emitted if the PSK is also present. NM will error out if both settings are used simultaneously.
03fe185
to
eb16b51
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 for addressing my concerns, this is almost ready for merging!
I left a few tiny (one-line) comments inline, which I'd like to see fixed before merging.
This has the side effect of always emitting the PSK in "accesspoint".password. Which has the side effect of always enable the PMF setting when PSK is used.
eb16b51
to
108c999
Compare
Thank you! The autopkgtest "tunnels" failure is unrelated. |
Description
Originally, Netplan assumes the PSK and 802-1x identity password will
not be used simultaneously.
With this change, when both passwords are used, the PSK is expected to
be placed in "ssid".password and the identity in "ssid".auth.password.
"ssid".auth.password can also be used by the PSK if it's the only password.
When 802-1x is used, the PMF setting will not be emitted if the PSK is
also present. NM will error out if both settings are used simultaneously.
It also copes with unsupported EAP methods.
Here are some test cases that are currently leading to NM crashes:
Checklist
make check
successfully.make check-coverage
).