wifi: make it possible to have a psk and an eap password simultaneously by daniloegea · Pull Request #416 · canonical/netplan

daniloegea · 2023-10-13T16:37:24Z

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:

nmcli con add type wifi ifname wlan0 ssid asdasd wifi-sec.key-mgmt wpa-psk wifi-sec.psk asdasdasd 802-1x.eap md5 802-1x.identity username 802-1x.password aaaaaaaa

nmcli con add type wifi ifname wlan0 ssid asdasd wifi-sec.key-mgmt wpa-psk wifi-sec.psk asdasdasd 802-1x.eap fast 802-1x.identity username 802-1x.password aaaaaaaa

nmcli con add type wifi ifname wlan0 ssid asdasd wifi-sec.key-mgmt ieee8021x wifi-sec.psk bbbbbbbb 802-1x.eap leap 802-1x.identity username 802-1x.password aaaaaaaa

nmcli con add type wifi ifname wlan0 ssid asdasd wifi-sec.key-mgmt wpa-eap 802-1x.eap md5 802-1x.identity username 802-1x.password aaaaaaaa

Checklist

Runs make check successfully.
Retains 100% code coverage (make check-coverage).
New/changed keys in YAML format are documented.
(Optional) Adds example YAML for new feature.
(Optional) Closes an open bug in Launchpad.

slyon · 2023-10-16T15:32:56Z

Related to #415

slyon

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/abi.h

src/parse-nm.c

slyon · 2023-10-18T09:47:02Z

src/parse-nm.c

+            if (ap->auth.password)
+                ap->has_auth = TRUE;
+        } else {
+            keyfile_handle_generic_str(kf, "wifi-security", "psk", &ap->auth.psk);


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.

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.

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...

I pushed this change in a new commit

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.

src/parse-nm.c

src/nm.c

src/networkd.c

slyon

I added some concerns about the recent changes in-line and marked some of the other comments as resolved.

src/util-internal.h

src/abi.h

slyon · 2023-10-19T12:38:39Z

src/parse-nm.c

+            if (ap->auth.password)
+                ap->has_auth = TRUE;
+        } else {
+            keyfile_handle_generic_str(kf, "wifi-security", "psk", &ap->auth.psk);


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.

src/nm.c

daniloegea · 2023-10-23T08:55:26Z

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.

slyon

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.

src/util-internal.h

src/networkd.c

src/nm.c

tests/parser/test_keyfile.py

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.

slyon · 2023-11-27T09:30:04Z

Thank you! The autopkgtest "tunnels" failure is unrelated.
Let's merge this.

daniloegea force-pushed the key_mgmt_ieee8021x_part2 branch from a7d0719 to b4acb08 Compare October 16, 2023 10:47

daniloegea marked this pull request as ready for review October 16, 2023 13:03

daniloegea requested a review from slyon October 16, 2023 13:03

slyon approved these changes Oct 18, 2023

View reviewed changes

daniloegea force-pushed the key_mgmt_ieee8021x_part2 branch 2 times, most recently from ba80152 to e80696f Compare October 18, 2023 14:59

daniloegea requested a review from slyon October 19, 2023 08:48

slyon reviewed Oct 19, 2023

View reviewed changes

daniloegea force-pushed the key_mgmt_ieee8021x_part2 branch from c6d9bca to 03fe185 Compare October 20, 2023 15:38

daniloegea force-pushed the key_mgmt_ieee8021x_part2 branch from 03fe185 to eb16b51 Compare October 31, 2023 17:09

slyon approved these changes Nov 22, 2023

View reviewed changes

src/util-internal.h Outdated Show resolved Hide resolved

src/networkd.c Outdated Show resolved Hide resolved

src/nm.c Outdated Show resolved Hide resolved

tests/parser/test_keyfile.py Outdated Show resolved Hide resolved

tests/parser/test_keyfile.py Show resolved Hide resolved

nm-parse: always read the PSK into the new psk variable

108c999

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.

daniloegea force-pushed the key_mgmt_ieee8021x_part2 branch from eb16b51 to 108c999 Compare November 24, 2023 12:55

slyon merged commit 76dec54 into canonical:main Nov 27, 2023

Conversation

daniloegea commented Oct 13, 2023 • edited by slyon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

slyon commented Oct 16, 2023

Uh oh!

slyon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

slyon Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

daniloegea Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

daniloegea Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

daniloegea Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

slyon Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

slyon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

slyon Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daniloegea commented Oct 23, 2023

Uh oh!

slyon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

slyon commented Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daniloegea commented Oct 13, 2023 •

edited by slyon

Loading