From 7287257ec9f2ca301642bd4800f391ad9079d3e9 Mon Sep 17 00:00:00 2001 From: akrm al-hakimi Date: Tue, 28 Jul 2026 18:29:19 -0400 Subject: [PATCH] fix(networking): stop storing the VPN username as a secret (#2116) This PRs diff fixes #2109 such that a VPN username now gets written to `vpn.data`, where NetworkManager and the VPN plugins actually read it from. The existing `vpn.data` dict is read and merged rather than replaced, so unrelated keys like remote and ca survive the update. Profiles saved by earlier versions already have the username sitting in the keyring, so the secret agent filters it out on read as well. Without that, the stale entry keeps getting replayed to NetworkManager and affected connections stay broken after upgrading. --- Cargo.lock | 4 +- cosmic-settings/Cargo.toml | 2 +- .../src/pages/networking/backend.rs | 83 +++++++++++++++++-- .../src/pages/networking/vpn/mod.rs | 40 ++++++--- 4 files changed, 109 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ce47a4a..6c11fe1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5042,9 +5042,9 @@ checksum = "650eef8c711430f1a879fdd01d4745a7deea475becfb90269c06775983bbf086" [[package]] name = "nmrs" -version = "3.4.1" +version = "3.4.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8c58e7c48ffdccfa5164000de751cbdf5c9cb2d79a33e75b9e2618ab72ea86f7" +checksum = "33e42c05118e18f29f55eefa2fbfd98a06b0c7a1a829bdb0f328137d358c6013" dependencies = [ "async-trait", "base64", diff --git a/cosmic-settings/Cargo.toml b/cosmic-settings/Cargo.toml index 93683e2..226cbf0 100644 --- a/cosmic-settings/Cargo.toml +++ b/cosmic-settings/Cargo.toml @@ -54,7 +54,7 @@ locale1 = { git = "https://github.com/pop-os/dbus-settings-bindings", optional = sysinfo = { version = "=0.38.0", optional = true } mime-apps = { package = "cosmic-mime-apps", git = "https://github.com/pop-os/cosmic-mime-apps", features = ["tokio"], optional = true } notify = "8.2.0" -nmrs = { version = "3.4.1", optional = true } +nmrs = { version = "3.4.2", optional = true } regex = "1.13.1" ron = "0.12" rust-embed = "8.12.0" diff --git a/cosmic-settings/src/pages/networking/backend.rs b/cosmic-settings/src/pages/networking/backend.rs index eec2e4e..db0e3be 100644 --- a/cosmic-settings/src/pages/networking/backend.rs +++ b/cosmic-settings/src/pages/networking/backend.rs @@ -12,7 +12,7 @@ use futures::channel::mpsc::{UnboundedReceiver, UnboundedSender, unbounded}; use futures::{FutureExt, SinkExt, StreamExt}; use nmrs::agent::{SecretAgent, SecretAgentFlags, SecretRequest, SecretResponder, SecretSetting}; use nmrs::raw::zbus; -use nmrs::raw::zvariant::{OwnedObjectPath, OwnedValue, Str, Value}; +use nmrs::raw::zvariant::{Dict, OwnedObjectPath, OwnedValue, Str, Value}; use nmrs::{ ActiveConnection, ConnectType, ConnectionOptions, EapOptions, NetworkEvent, NetworkManager, SavedConnection, SettingsSummary, WifiKeyMgmt, WifiSecurity, @@ -514,6 +514,10 @@ pub enum Request { Option, ), SetAirplaneMode(bool), + SetVpnUsername { + uuid: UUID, + username: String, + }, SetWiFi(bool), } @@ -720,12 +724,64 @@ async fn handle_request(nm: &NetworkManager, req: Request) -> Event { result.is_ok() } Request::SetAirplaneMode(enabled) => nm.set_airplane_mode(*enabled).await.is_ok(), + Request::SetVpnUsername { uuid, username } => { + let result = set_vpn_username(nm, uuid, username).await; + if let Err(err) = &result { + tracing::error!(%err, %uuid, "failed to store VPN username"); + } + result.is_ok() + } Request::SetWiFi(enabled) => nm.set_wireless_enabled(*enabled).await.is_ok(), }; request_response(nm, req, success).await } +/// Stores a VPN username in `vpn.data`, which is where NetworkManager and the VPN +/// plugins read it from. +/// +/// The username must not be sent through the secret agent: openvpn fails to +/// reconnect when it receives a `username` key in the VPN secrets dictionary. +async fn set_vpn_username( + nm: &NetworkManager, + uuid: &str, + username: &str, +) -> Result<(), nmrs::ConnectionError> { + let settings = nm.get_saved_connection_raw(uuid).await?; + + // The whole dict is rewritten, so the keys already in it have to be carried over. + let mut data = HashMap::::new(); + if let Some(stored) = settings.get("vpn").and_then(|vpn| vpn.get("data")) + && let Ok(dict) = Dict::try_from(stored.clone()) + { + for (key, value) in dict.iter() { + if let (Ok(key), Ok(value)) = (Str::try_from(key.clone()), Str::try_from(value.clone())) + { + data.insert(key.to_string(), value.to_string()); + } + } + } + + if data + .get("username") + .is_some_and(|stored| stored == username) + { + return Ok(()); + } + + data.insert("username".to_string(), username.to_string()); + let data = OwnedValue::try_from(Value::from(data)) + .map_err(|err| nmrs::ConnectionError::VpnFailed(err.to_string()))?; + + let mut patch = nmrs::SettingsPatch::default(); + patch.raw_overlay = Some(HashMap::from([( + "vpn".to_string(), + HashMap::from([("data".to_string(), data)]), + )])); + + nm.update_saved_connection(uuid, patch).await +} + async fn request_response(nm: &NetworkManager, req: Request, success: bool) -> Event { Event::RequestResponse { req, @@ -1649,15 +1705,28 @@ pub mod nm_secret_agent { } } SecretSetting::Vpn { .. } => { - let secrets = stored + // Earlier versions stored the username next to the password. openvpn + // fails to reconnect when one is present in the secrets dictionary, so + // profiles saved by those versions have to be filtered on read too. + let secrets: HashMap = stored .into_iter() + .filter(|(key, _)| key != "username") .map(|(key, value)| (key, value.unsecure().to_string())) .collect(); - request - .responder - .vpn_secrets(secrets) - .await - .map_err(|err| Error(err.to_string()))?; + + if secrets.is_empty() { + request + .responder + .no_secrets() + .await + .map_err(|err| Error(err.to_string()))?; + } else { + request + .responder + .vpn_secrets(secrets) + .await + .map_err(|err| Error(err.to_string()))?; + } } _ => { let setting_name = setting_name(&request.setting).to_string(); diff --git a/cosmic-settings/src/pages/networking/vpn/mod.rs b/cosmic-settings/src/pages/networking/vpn/mod.rs index 6c2c9e7..7323d84 100644 --- a/cosmic-settings/src/pages/networking/vpn/mod.rs +++ b/cosmic-settings/src/pages/networking/vpn/mod.rs @@ -612,7 +612,7 @@ impl Page { } = dialog && let Some(NmState { ref sender, .. }) = self.nm_state { - let username_unwrapped = username.clone().unwrap_or_default(); + let username = username.clone().unwrap_or_default(); let sec_tx = self.secret_tx.clone(); let nm_sender = sender.clone(); return Task::future(async move { @@ -626,10 +626,10 @@ impl Page { .send(nm_secret_agent::Request::SetSecrets { setting_name: "vpn".to_string(), uuid: uuid.to_string(), - secrets: HashMap::from_iter([ - ("username".to_string(), username_unwrapped.into()), - ("password".to_string(), password), - ]), + secrets: HashMap::from_iter([( + "password".to_string(), + password, + )]), applied_tx, }) .await @@ -645,6 +645,16 @@ impl Page { tracing::error!(%err, "failed to apply secret"); } } + + if !username.is_empty() { + _ = nm_sender.unbounded_send( + network_manager::Request::SetVpnUsername { + uuid: uuid.clone(), + username, + }, + ); + } + _ = nm_sender .unbounded_send(network_manager::Request::ActivateVpn(uuid)); } @@ -668,7 +678,7 @@ impl Page { } = dialog && let Some(NmState { ref sender, .. }) = self.nm_state { - let username_unwrapped = username.unwrap_or_default(); + let username = username.unwrap_or_default(); let sec_tx = self.secret_tx.clone(); let nm_sender = sender.clone(); return Task::future(async move { @@ -678,10 +688,10 @@ impl Page { .send(nm_secret_agent::Request::SetSecrets { setting_name: "vpn".to_string(), uuid: uuid.to_string(), - secrets: HashMap::from_iter([ - ("username".to_string(), username_unwrapped.into()), - ("password".to_string(), password), - ]), + secrets: HashMap::from_iter([( + "password".to_string(), + password, + )]), applied_tx, }) .await; @@ -689,6 +699,16 @@ impl Page { tokio::time::timeout(std::time::Duration::from_secs(1), applied_rx) .await; } + + if !username.is_empty() { + _ = nm_sender.unbounded_send( + network_manager::Request::SetVpnUsername { + uuid: uuid.clone(), + username, + }, + ); + } + _ = nm_sender.unbounded_send(network_manager::Request::ActivateVpn(uuid)); Message::Refresh })