From 323236719db45e2a53808359b667447a1745573e Mon Sep 17 00:00:00 2001 From: Pierre Rouanet Date: Wed, 5 Aug 2026 19:13:33 +0200 Subject: [PATCH] configd: report a rejected passphrase as BadKey, not other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A wrong key against a real network answered `{"reason":"other","detail":"state 120, reason 0"}`. The outcome was right — failed, not a false success — but the reason is the part a client acts on: `BadKey` means "ask for the password again", and `other` means nothing at all. Reason 0 because the `StateReason` *property* was read after the activation had already failed. By then NM has moved the device on, usually back to autoconnecting the previous profile, and the reason for the failure is gone. The mapping to `BadKey` was correct all along; it was being fed a zero. Now the device's `StateChanged` signal is subscribed *before* the activation starts and consumed alongside the poll, recording the reason from any transition into `FAILED` or `NEED_AUTH`. `NEED_AUTH` matters: on a rejected key NM passes through it carrying `NO_SECRETS` and only later reaches `FAILED`, sometimes with the reason already cleared. The property read stays as a fallback. The signal is declared as `device_state_changed` because the `state` property already generates a `receive_state_changed` for property changes, and the two names would collide. `futures` joins configd's Linux-only dependencies for `StreamExt`: zbus 5 re-exports `futures_core`, the trait, but not `futures_util`, the combinators. It is already in the graph via btd. Assisted-by: Claude:claude-opus-5 --- Cargo.lock | 1 + configd/Cargo.toml | 4 +++ configd/src/nm.rs | 69 ++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 66 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d27c8a7..74153aa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -603,6 +603,7 @@ dependencies = [ "async-trait", "clap", "duck-ipc-proto", + "futures", "libc", "serde", "serde_json", diff --git a/configd/Cargo.toml b/configd/Cargo.toml index 5ac6e4b..218952f 100644 --- a/configd/Cargo.toml +++ b/configd/Cargo.toml @@ -32,6 +32,10 @@ libc = "0.2" # revisiting if bluer ever grows a zbus backend. [target.'cfg(target_os = "linux")'.dependencies] zbus = "5" +# For `StreamExt` over zbus's signal streams. zbus 5 re-exports `futures_core` (the `Stream` trait) +# but not `futures_util` (the combinators), and the connect path has to consume NM's `StateChanged` +# signal to learn *why* an activation failed. Already in the graph via btd. +futures = "0.3" [dev-dependencies] tempfile = "3.27.0" diff --git a/configd/src/nm.rs b/configd/src/nm.rs index 2b78338..6263d86 100644 --- a/configd/src/nm.rs +++ b/configd/src/nm.rs @@ -14,6 +14,7 @@ use std::time::{Duration, Instant}; use async_trait::async_trait; use duck_ipc_proto as proto; +use futures::StreamExt; use zbus::zvariant::{OwnedObjectPath, OwnedValue, Value}; use crate::net::{Net, NetResult}; @@ -38,6 +39,9 @@ mod ids { pub const ACTIVE_DEACTIVATING: u32 = 3; pub const ACTIVE_DEACTIVATED: u32 = 4; + /// The device is asking for a key. NM passes through this on a rejected passphrase before it + /// gives up, and the transition carries the reason the property no longer holds. + pub const STATE_NEED_AUTH: u32 = 60; pub const REASON_NO_SECRETS: u32 = 7; pub const REASON_SUPPLICANT_DISCONNECT: u32 = 8; pub const REASON_SUPPLICANT_TIMEOUT: u32 = 11; @@ -98,6 +102,19 @@ trait Device { fn state(&self) -> zbus::Result; #[zbus(property)] fn state_reason(&self) -> zbus::Result<(u32, u32)>; + /// Every state transition, with the reason for it. + /// + /// The only reliable source of "the passphrase was rejected". Reading the `StateReason` + /// *property* after an activation fails gives reason 0: by then NM has moved the device on — + /// usually back to autoconnecting the previous profile — and the reason for the failure is gone. + /// A wrong key reported as `other` tells a phone nothing, when it is the one failure the user + /// can actually fix. + /// + /// Named `device_state_changed` because the `state` property already generates a + /// `receive_state_changed` for property changes, and the two would collide. + #[zbus(signal, name = "StateChanged")] + fn device_state_changed(&self, new_state: u32, old_state: u32, reason: u32) + -> zbus::Result<()>; #[zbus(property, name = "Ip4Config")] fn ip4_config(&self) -> zbus::Result; #[zbus(property, name = "Ip6Config")] @@ -643,6 +660,19 @@ impl Net for NetworkManager { let root = zbus::zvariant::ObjectPath::try_from("/").map_err(bus_err)?; let device = zbus::zvariant::ObjectPath::try_from(device_path.as_str()).map_err(bus_err)?; + // Subscribed *before* the activation starts, or the transition that carries the reason has + // already happened by the time anyone is listening. + let device_proxy = DeviceProxy::builder(&self.bus) + .path(&device_path) + .map_err(bus_err)? + .build() + .await + .map_err(bus_err)?; + let mut transitions = device_proxy + .receive_device_state_changed() + .await + .map_err(bus_err)?; + let (added, activation) = match manager .add_and_activate_connection(settings, &device, &root) .await @@ -666,12 +696,6 @@ impl Net for NetworkManager { // the robot had been on all along. Reporting success for a join that never happened is the // worst answer available: a phone concludes it has provisioned the robot. let deadline = tokio::time::Instant::now() + CONNECT_TIMEOUT; - let device_proxy = DeviceProxy::builder(&self.bus) - .path(&device_path) - .map_err(bus_err)? - .build() - .await - .map_err(bus_err)?; let active = ActiveConnectionProxy::builder(&self.bus) .path(&activation) .map_err(bus_err)? @@ -679,6 +703,8 @@ impl Net for NetworkManager { .await .map_err(bus_err)?; + let mut observed_reason: Option = None; + loop { // An unreadable state means the object is gone, which NM does when an activation is // torn down — a failure, not a reason to keep waiting. @@ -697,7 +723,15 @@ impl Net for NetworkManager { let timed_out = tokio::time::Instant::now() >= deadline; if state == ids::ACTIVE_DEACTIVATED || state == ids::ACTIVE_DEACTIVATING || timed_out { - let (_, reason) = device_proxy.state_reason().await.unwrap_or((0, 0)); + // The reason seen as it happened, falling back to the property. The property is + // usually 0 here, which is what made a rejected passphrase report `other`. + let reason = match observed_reason { + Some(reason) => reason, + None => { + let (_, reason) = device_proxy.state_reason().await.unwrap_or((0, 0)); + reason + } + }; // NM's reason survives a timeout too: "still authenticating after 45s" and // "waiting for DHCP" are different problems. let (failure, detail) = failure_of(ids::STATE_FAILED, reason); @@ -725,7 +759,26 @@ impl Net for NetworkManager { }); } - tokio::time::sleep(Duration::from_millis(500)).await; + // Wait for either a transition or the next poll tick, so a reason that appears between + // ticks is still recorded. `NEED_AUTH` counts: on a rejected key NM passes through it + // with `NO_SECRETS` and only later reaches `FAILED`, sometimes with the reason cleared. + tokio::select! { + () = tokio::time::sleep(Duration::from_millis(500)) => {} + Some(transition) = transitions.next() => { + if let Ok(args) = transition.args() + && (*args.new_state() == ids::STATE_FAILED + || *args.new_state() == ids::STATE_NEED_AUTH) + && *args.reason() != 0 + { + tracing::debug!( + new_state = *args.new_state(), + reason = *args.reason(), + "device transition" + ); + observed_reason = Some(*args.reason()); + } + } + } } }