Skip to content

nixos/wireless: reimplement secrets using ext_password_backend#180872

Merged
rnhmjoj merged 4 commits into
NixOS:masterfrom
rnhmjoj:pr-wpa-ext-secrets
Sep 11, 2024
Merged

nixos/wireless: reimplement secrets using ext_password_backend#180872
rnhmjoj merged 4 commits into
NixOS:masterfrom
rnhmjoj:pr-wpa-ext-secrets

Conversation

@rnhmjoj

@rnhmjoj rnhmjoj commented Jul 9, 2022

Copy link
Copy Markdown
Contributor
Description of changes

This replaces the current implementation (splicing the secrets into the
configuration file using environment variavles) with the new built-in
mechanism ext_password_backend.

With some minor syntax changes, it works exactly as before, except the
heavy lifting is done by wpa_supplicant and probably less error-prone.

Things done
  • Tested via nixosTests.wpa_supplicant
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

cc: @lopsided98

@github-actions github-actions Bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 9, 2022
@ofborg ofborg Bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jul 9, 2022

@pennae pennae left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while testing these changes we've run into problems. it seems like wpa_supplicant will provide the password from the external file only to the highest priority block for each entry, i.e. the default settings of fallbackToWPA2 is completely broken.

this is currently not usable at all, and it looks a lot like an upstream bug :(

@rnhmjoj
rnhmjoj marked this pull request as draft July 15, 2022 13:17
@rnhmjoj

rnhmjoj commented Jul 15, 2022

Copy link
Copy Markdown
Contributor Author

I made this a draft to prevent accidental merges.

@lopsided98

Copy link
Copy Markdown
Contributor

I found another upstream bug: the external password is not passed to the kernel if 4-way handshake offloading is enabled.

@lopsided98

Copy link
Copy Markdown
Contributor

It doesn't work with WPA3 SAE passwords either.

I have written patches to fix both of these issues, which I'll try to get feedback on upstream:

@lopsided98

Copy link
Copy Markdown
Contributor

My patches have been accepted upstream. I wonder if @pennae's issue is a different bug, or was caused by the SAE issue I fixed.

@pennae

pennae commented Dec 31, 2022

Copy link
Copy Markdown
Contributor

finally got around to this, and wpa_supplicant doesn't build from git:

wpa_supplicant> installing
wpa_supplicant> install flags: SHELL=/nix/store/nfgpn9av331q7zi1dl6d5qpir60y513s-bash-5.1-p16/bin/bash install
wpa_supplicant> install -D wpa_supplicant /nix/store/7gi8kpc4xjpnf1dq24m6rd3v4s32q0gq-wpa_supplicant-2.10/sbin/wpa_supplicant
wpa_supplicant> install -D wpa_cli /nix/store/7gi8kpc4xjpnf1dq24m6rd3v4s32q0gq-wpa_supplicant-2.10/sbin/wpa_cli
wpa_supplicant> make -C ../src install
wpa_supplicant> for d in ap common crypto drivers eapol_auth eapol_supp eap_common eap_peer eap_server l2_packet p2p pae pasn radius rsn_supp tls utils wps fst; do [ -d $d ] && make -C $d install; done
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> Nothing to be made.
wpa_supplicant> install -D wpa_passphrase //nix/store/7gi8kpc4xjpnf1dq24m6rd3v4s32q0gq-wpa_supplicant-2.10/sbin/wpa_passphrase
wpa_supplicant> if ls eap_*.so >/dev/null 2>&1; then \
wpa_supplicant> 	install -d /nix/store/7gi8kpc4xjpnf1dq24m6rd3v4s32q0gq-wpa_supplicant-2.10/lib/wpa_supplicant && \
wpa_supplicant> 	cp *.so /nix/store/7gi8kpc4xjpnf1dq24m6rd3v4s32q0gq-wpa_supplicant-2.10/lib/wpa_supplicant \
wpa_supplicant> ; fi
wpa_supplicant> cp: missing destination file operand after '/nix/store/7gi8kpc4xjpnf1dq24m6rd3v4s32q0gq-wpa_supplicant-2.10/share/man/man5/'
wpa_supplicant> Try 'cp --help' for more information.

builds fine from release 2.10, but that's almost a year old.

@rnhmjoj
rnhmjoj force-pushed the pr-wpa-ext-secrets branch from 4b5b352 to bb01a82 Compare July 10, 2023 16:17
@ofborg ofborg Bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 10, 2023
@rnhmjoj
rnhmjoj force-pushed the pr-wpa-ext-secrets branch from bb01a82 to 0cccd36 Compare July 10, 2023 19:06
@ofborg ofborg Bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 10, 2023
@rnhmjoj

rnhmjoj commented Jul 10, 2023

Copy link
Copy Markdown
Contributor Author

I rebased this branch to the latest master. I tried to patch the current wpa_supplicant, but the patches don't apply cleanly and I don't have time to do a backport. So, unless someone does it, this will have to wait for version 2.11.

The good news is that now we have tests checking both WPA2 and WPA3: the tests will pass once wpa_supplicant bugs are fixed and this PR can be merged.

@dkwo

dkwo commented Sep 24, 2023

Copy link
Copy Markdown

@lopsided98 I tried your two patches (on void linux, on top of wpa_supplicant 2.10): I do get the error

wpa_supplicant[1815]: Line 34: failed to parse sae_password '[KEY DATA REMOVED]'.
wpa_supplicant[1815]: Line 34: failed to parse sae_password 'ext:pwd_cia'.
wpa_supplicant[1815]: Line 36: failed to parse network block.
wpa_supplicant[1815]: Failed to read or parse configuration

my config file contains

network={
	ssid="some-ssid"
	key_mgmt=SAE
	ieee80211w=2
	sae_password=ext:pwd_cia

and the file has a line pwd_cia=the actual password without any quotations etc. Do you think I'm missing some quotations or similar, or maybe certain characters in the password do not work with this method, or else?

@lopsided98

Copy link
Copy Markdown
Contributor

I don't think it works with the sae_password option; you need to use the psk option even with WPA3.

@dkwo

dkwo commented Sep 25, 2023

Copy link
Copy Markdown

Thanks a lot, that works indeed.

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale Bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@rnhmjoj

rnhmjoj commented Apr 13, 2024

Copy link
Copy Markdown
Contributor Author

2.11 is taking forever, for... reasons.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
This replaces the current implementation (splicing the secrets into the
configuration file using environment variables) with the new built-in
mechanism ext_password_backend.

With some minor syntax changes, it works exactly as before, except the
heavy lifting is done by wpa_supplicant and probably less error-prone.
@rnhmjoj
rnhmjoj force-pushed the pr-wpa-ext-secrets branch from 0cccd36 to f951caf Compare August 18, 2024 07:12
@rnhmjoj

rnhmjoj commented Aug 18, 2024

Copy link
Copy Markdown
Contributor Author

2.11 is Nixpkgs, I rebased and... it doesn't work.

@stale stale Bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 18, 2024
@ofborg ofborg Bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 18, 2024
This links the generated configuration to /etc/wpa_supplicant.conf
unless `allowAuxiliaryImperativeNetworks`. In the latter case the
file in /etc should be writable and the generated one remains only
in the Nix store.
@rnhmjoj
rnhmjoj marked this pull request as ready for review August 22, 2024 19:48
@rnhmjoj

rnhmjoj commented Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

I should have sorted everything out. If you could re-test with the latest rebase, it would be great.

@rnhmjoj

rnhmjoj commented Aug 23, 2024

Copy link
Copy Markdown
Contributor Author

@GrahamcOfBorg test wpa_supplicant

@rnhmjoj
rnhmjoj requested a review from oddlama August 23, 2024 11:32
@rnhmjoj
rnhmjoj merged commit b94f259 into NixOS:master Sep 11, 2024
@nixos-discourse

Copy link
Copy Markdown

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/59

@trofi

trofi commented Sep 18, 2024

Copy link
Copy Markdown
Contributor

I had a simple setup on one of my NixOS machines that run from nixpkgs/master:

  1. configuration.nix contained networking.wireless.enable = true;
  2. /etc/wpa_supplcant.conf contained a single static network set up: network={ssid=...,psk=...}

Today after the reboot I found that my wifi is completely broken.

The contents of /etc/wpa_supplicant.conf is now just pmf=1 bgscan="simple:30:-70:3600" which fails to connect to any networks.

I suspect that commit 89eb93d nixos/wireless: link config to /etc by default now clobbers user's config.

From what I can see:

  • The contents of my original /etc/wpa_supplicant.conf just disappeared and booting to the previous generations would not get the wifi working. The file is not on dusk anymore.
  • nixos-rebuild switch did not complain about potential data loss and generation of guaranteed to be invalid config (yes? or there are cases when this new config could work?)

Did I get it right? Or something else clobbered my config? If it's indeed the commit 89eb93d is there a chance to get a failsafe assert to be added so that users would not lose their network setup after an upgrade?

@rnhmjoj

rnhmjoj commented Sep 18, 2024

Copy link
Copy Markdown
Contributor Author

Did I get it right? Or something else clobbered my config? If it's indeed the commit 89eb93d

I changed the module to link the generated configuration to /etc/wpa_supplicant.conf, because otherwise the file is not easily accessible (you'd need to look at the daemon argv, or inspect the systemd unit). That said, I have no idea what nixos-rebuild switch does if a file exists in place of a link it has to create. I just didn't think of this, sorry...

/etc/wpa_supplicant.conf contained a single static network set up: network={ssid=...,psk=...}

Did that ever work at all? Were you manually starting the wpa_supplicant daemon? AFAIK, without cfg.allowAuxiliaryImperativeNetworks NixOS does not look at /etc/wpa_supplicant.conf at all.

is there a chance to get a failsafe assert to be added so that users would not lose their network setup after an upgrade?

I don't think so... at evaluation time I can't inspect the filesystem to assert the link would overwrite an existing file.
I guess I could create the link from the wpa_supplicant service, instead of using environment.etc, but this is more of an issue with the activation script. (if that's really what happened in your case)

@trofi

trofi commented Sep 18, 2024

Copy link
Copy Markdown
Contributor

Did I get it right? Or something else clobbered my config? If it's indeed the commit 89eb93d

I changed the module to link the generated configuration to /etc/wpa_supplicant.conf, because otherwise the file is not easily accessible (you'd need to look at the daemon argv, or inspect the systemd unit). That said, I have no idea what nixos-rebuild switch does if a file exists in place of a link it has to create. I just didn't think of this, sorry...

/etc/wpa_supplicant.conf contained a single static network set up: network={ssid=...,psk=...}

Did that ever work at all? Were you manually starting the wpa_supplicant daemon? AFAIK, without cfg.allowAuxiliaryImperativeNetworks NixOS does not look at /etc/wpa_supplicant.conf at all.

No manual wpa_supplicant start. The setup worked fine for a few years.

I have another system that did not update yet. It looks like something is copying /etc/wpa_supplicant.conf to /run/wpa_supplicant/wpa_supplicant.conf:

$ systemctl status wpa_supplicant-wlp0s20f3.service
● wpa_supplicant-wlp0s20f3.service - WPA Supplicant instance for interface wlp0s20f3
     Loaded: loaded (/etc/systemd/system/wpa_supplicant-wlp0s20f3.service; enabled; preset: enabled)
     Active: active (running) since Wed 2024-08-07 23:47:17 BST; 1 month 11 days ago
   Main PID: 830 (wpa_supplicant)
        CPU: 8.382s
     CGroup: /system.slice/wpa_supplicant-wlp0s20f3.service
             └─830 wpa_supplicant -iwlp0s20f3 -s -u -Dnl80211,wext -c /run/wpa_supplicant/wpa_supplicant.conf
...

All network-relevant configuration.nix bits from this not yet updated machine:

  networking.wireless.enable = true;
  networking.wireless.interfaces = ["wlp0s20f3"];
  networking.useDHCP = false;
  networking.interfaces.wlp0s20f3.useDHCP = true;

/etc/wpa_supplicant.conf is a manually written local file with two networks defined.

@rnhmjoj

rnhmjoj commented Sep 18, 2024

Copy link
Copy Markdown
Contributor Author

Ah, you're right. The situation is quite messy because there are (were) three ways to configure wpa_supplicant: fully declaratively, mixed declarative/imperative and fully imperatively. The latter has been kind of unnecessary after networking.wireless.allowAuxiliaryImperativeNetworks was added, and that's why I forgot about it (also it was not being tested).

I think that commit should be reverted and this deprecated properly, not sure about how, exactly. About nixos overwriting your file, that's apparently how it works... yikes. This should be another issue, if there's not already one open.

@mrshmllow

This comment was marked as resolved.

@ritiek

ritiek commented Oct 23, 2024

Copy link
Copy Markdown
Member

This appears to of broken identity, in other words you can only use secrets for password. I cant put wireless stuff in my config without exposing details id rather not

Do we have a resolution for this? I have a similar concern. I'd like to not publish my SSID.

@mrshmllow

Copy link
Copy Markdown

This appears to of broken identity, in other words you can only use secrets for password. I cant put wireless stuff in my config without exposing details id rather not

Do we have a resolution for this? I have a similar concern. I'd like to not publish my SSID.

the example usage mislead me. you can use the substitution for stuff that isn't a Password. However, I'm not sure about ssid. you might have to mkAfter the string to add something custom, not sure

@rnhmjoj

rnhmjoj commented Oct 24, 2024

Copy link
Copy Markdown
Contributor Author

Sorry, but environmentFile was never meant to substitute stuff other than secrets, even though it could. See this if want to hide SSIDs and other parts of the configuration.

@DCsunset

Copy link
Copy Markdown
Member

I just discovered another issue using this new approach: when using psk=ext:password with sae_password_id, it failed to connect to the AP. But it works fine if I use sae_password with a string (without the external file).

This seems to be an upstream issue, but I just want to mention it here as the previous environmentFile approach works in such scenario (and it is a more general approach that is widely used in other modules too)

@rnhmjoj
rnhmjoj deleted the pr-wpa-ext-secrets branch November 29, 2024 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants