fix(freedesktop-rs#508): redact credentials without breaking the public API

Author: cachebag

Cargo.lock

@@ -33,4 +33,3 @@ nmrs = { path = "nmrs", version = "3.0" }
 async-trait = "0.1.89"
 bitflags = "2.13.0"
 tokio = { version = "1.52.3", features = ["rt-multi-thread", "macros", "sync", "time", "process"] }
-zeroize = { version = "1.9.0", features = ["aarch64", "derive", "std"] }

Cargo.toml

@@ -4,15 +4,11 @@ All notable changes to the `nmrs` crate will be documented in this file.
 
 ## [Unreleased]
 
-### Added
-
-- `Passphrase` keeps Wi-Fi, OpenVPN, and WireGuard secrets redacted from debug
-  output and zeroizes their owned buffers when dropped. ([#508](https://github.com/freedesktop-rs/nmrs/pull/508))
-
 ### Changed
 
-- **Breaking:** Wi-Fi, OpenVPN, and WireGuard secret fields now use
-  `Passphrase` instead of `String`. ([#508](https://github.com/freedesktop-rs/nmrs/pull/508))
+- Credential-bearing Wi-Fi, EAP, OpenVPN, and WireGuard types now redact
+  passwords and private keys from debug output, and connection tracing no
+  longer logs raw settings, without changing public field types. ([#508](https://github.com/freedesktop-rs/nmrs/pull/508))
 
 ### Fixed
 

nmrs/CHANGELOG.md

@@ -25,7 +25,6 @@ base64.workspace = true
 tokio.workspace = true
 async-trait.workspace = true
 bitflags.workspace = true
-zeroize.workspace = true
 
 [package.metadata.docs.rs]
 all-features = true

nmrs/Cargo.toml

@@ -31,9 +31,7 @@ async fn main() -> nmrs::Result<()> {
         "MyNetwork",
         None,
         WifiSecurity::WpaPsk {
-            psk: std::env::var("WIFI_PASSWORD")
-                .unwrap_or_else(|_| "password".to_string())
-                .into(),
+            psk: std::env::var("WIFI_PASSWORD").unwrap_or_else(|_| "password".to_string()),
         },
     )
     .await?;

nmrs/examples/custom_timeouts.rs

@@ -46,13 +46,13 @@
 //!
 //! ```rust
 //! use nmrs::builders::{build_ethernet_connection, build_wifi_connection};
-//! use nmrs::{ConnectionOptions, Passphrase, WifiSecurity};
+//! use nmrs::{ConnectionOptions, WifiSecurity};
 //!
 //! let opts = ConnectionOptions::new(true).with_priority(10);
 //!
 //! let wifi = build_wifi_connection(
 //!     "MyNetwork",
-//!     &WifiSecurity::WpaPsk { psk: Passphrase::new("password".to_string()) },
+//!     &WifiSecurity::WpaPsk { psk: "password".into() },
 //!     &opts,
 //! );
 //! let eth = build_ethernet_connection("eth0", &opts);
@@ -62,12 +62,12 @@
 //!
 //! ```no_run
 //! use nmrs::builders::{WifiConnectionBuilder, WifiMode};
-//! use nmrs::{NetworkManager, Passphrase};
+//! use nmrs::NetworkManager;
 //!
 //! # async fn example() -> nmrs::Result<()> {
 //! let nm = NetworkManager::new().await?;
 //! let settings = WifiConnectionBuilder::new("Hotspot")
-//!     .wpa_psk(Passphrase::new("password".to_string()))
+//!     .wpa_psk("password")
 //!     .mode(WifiMode::Ap)
 //!     .ipv4_shared()
 //!     .build();
@@ -91,7 +91,7 @@
 //! ).with_persistent_keepalive(25);
 //!
 //! let settings = WireGuardBuilder::new("MyVPN")
-//!     .private_key("YBk6X3pP8KjKz7+HFWzVHNqL3qTZq8hX9VxFQJ4zVmM=".to_string())
+//!     .private_key("YBk6X3pP8KjKz7+HFWzVHNqL3qTZq8hX9VxFQJ4zVmM=")
 //!     .address("10.0.0.2/24")
 //!     .add_peer(peer)
 //!     .dns(vec!["1.1.1.1".into()])

nmrs/src/api/builders/mod.rs

@@ -9,14 +9,13 @@
 //! struct. Use [`super::vpn::build_openvpn_connection`] to convert it into
 //! NetworkManager connection settings.
 
-use std::path::Path;
+use std::{fmt, path::Path};
 
 use uuid::Uuid;
 
-use crate::Passphrase;
 use crate::api::models::{
     ConnectionError, OpenVpnAuthType, OpenVpnCompression, OpenVpnConfig, OpenVpnProxy, VpnRoute,
-    vpn_route_from_parser,
+    Redacted, redact_option, vpn_route_from_parser,
 };
 use crate::core::ovpn_parser::parser::{self, CertSource, OvpnFile};
 use crate::util::cert_store::store_inline_cert;
@@ -48,7 +47,6 @@ use crate::util::validation::validate_connection_name;
 ///     .expect("Failed to build OpenVPN config");
 /// ```
 #[non_exhaustive]
-#[derive(Debug)]
 pub struct OpenVpnBuilder {
     name: String,
     remote: Option<String>,
@@ -63,9 +61,9 @@ pub struct OpenVpnBuilder {
     ca_cert: Option<String>,
     client_cert: Option<String>,
     client_key: Option<String>,
-    key_password: Option<Passphrase>,
+    key_password: Option<String>,
     username: Option<String>,
-    password: Option<Passphrase>,
+    password: Option<String>,
     compression: Option<OpenVpnCompression>,
     proxy: Option<OpenVpnProxy>,
     tls_auth_key: Option<String>,
@@ -90,6 +88,52 @@ pub struct OpenVpnBuilder {
     ncp_disable: bool,
 }
 
+impl fmt::Debug for OpenVpnBuilder {
+    fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+        formatter
+            .debug_struct("OpenVpnBuilder")
+            .field("name", &self.name)
+            .field("remote", &self.remote)
+            .field("port", &self.port)
+            .field("tcp", &self.tcp)
+            .field("auth_type", &self.auth_type)
+            .field("auth", &self.auth)
+            .field("cipher", &self.cipher)
+            .field("dns", &self.dns)
+            .field("mtu", &self.mtu)
+            .field("uuid", &self.uuid)
+            .field("ca_cert", &self.ca_cert)
+            .field("client_cert", &self.client_cert)
+            .field("client_key", &self.client_key)
+            .field("key_password", &redact_option(&self.key_password))
+            .field("username", &self.username)
+            .field("password", &redact_option(&self.password))
+            .field("compression", &self.compression)
+            .field("proxy", &self.proxy)
+            .field("tls_auth_key", &self.tls_auth_key)
+            .field("tls_auth_direction", &self.tls_auth_direction)
+            .field("tls_crypt", &self.tls_crypt)
+            .field("tls_crypt_v2", &self.tls_crypt_v2)
+            .field("tls_version_min", &self.tls_version_min)
+            .field("tls_version_max", &self.tls_version_max)
+            .field("tls_cipher", &self.tls_cipher)
+            .field("remote_cert_tls", &self.remote_cert_tls)
+            .field("verify_x509_name", &self.verify_x509_name)
+            .field("crl_verify", &self.crl_verify)
+            .field("redirect_gateway", &self.redirect_gateway)
+            .field("routes", &self.routes)
+            .field("ping", &self.ping)
+            .field("ping_exit", &self.ping_exit)
+            .field("ping_restart", &self.ping_restart)
+            .field("reneg_seconds", &self.reneg_seconds)
+            .field("connect_timeout", &self.connect_timeout)
+            .field("data_ciphers", &self.data_ciphers)
+            .field("data_ciphers_fallback", &self.data_ciphers_fallback)
+            .field("ncp_disable", &self.ncp_disable)
+            .finish()
+    }
+}
+
 impl OpenVpnBuilder {
     /// Creates a new OpenVPN connection builder.
     #[must_use]
@@ -390,7 +434,7 @@ impl OpenVpnBuilder {
 
     /// Sets the password for an encrypted private key.
     #[must_use]
-    pub fn key_password(mut self, password: impl Into<Passphrase>) -> Self {
+    pub fn key_password(mut self, password: impl Into<String>) -> Self {
         self.key_password = Some(password.into());
         self
     }
@@ -404,7 +448,7 @@ impl OpenVpnBuilder {
 
     /// Sets the password for password authentication.
     #[must_use]
-    pub fn password(mut self, password: impl Into<Passphrase>) -> Self {
+    pub fn password(mut self, password: impl Into<String>) -> Self {
         self.password = Some(password.into());
         self
     }
@@ -1187,4 +1231,32 @@ key /etc/openvpn/client.key
 
         let _ = std::fs::remove_dir_all(&dir);
     }
+
+    #[test]
+    fn debug_output_redacts_passwords() {
+        let builder = OpenVpnBuilder::new("vpn")
+            .key_password("private-key-password")
+            .password("vpn-password")
+            .proxy(OpenVpnProxy::Http {
+                server: "proxy.example.com".into(),
+                port: 8080,
+                username: Some("proxy-user".into()),
+                password: Some("proxy-password".into()),
+                retry: true,
+            });
+
+        let output = format!("{builder:?}");
+
+        assert!(output.contains("[REDACTED]"));
+        for secret in [
+            "private-key-password",
+            "vpn-password",
+            "proxy-password",
+        ] {
+            assert!(
+                !output.contains(secret),
+                "debug output exposed secret {secret:?}: {output}"
+            );
+        }
+    }
 }

nmrs/src/api/builders/openvpn_builder.rs

@@ -27,7 +27,7 @@
 //! ).with_persistent_keepalive(25);
 //!
 //! let settings = WireGuardBuilder::new("MyVPN")
-//!     .private_key("YBk6X3pP8KjKz7+HFWzVHNqL3qTZq8hX9VxFQJ4zVmM=".to_string())
+//!     .private_key("YBk6X3pP8KjKz7+HFWzVHNqL3qTZq8hX9VxFQJ4zVmM=")
 //!     .address("10.0.0.2/24")
 //!     .add_peer(peer)
 //!     .dns(vec!["1.1.1.1".into()])
@@ -53,7 +53,7 @@
 //!     VpnKind::WireGuard,
 //!     "MyVPN",
 //!     "vpn.example.com:51820",
-//!     "YBk6X3pP8KjKz7+HFWzVHNqL3qTZq8hX9VxFQJ4zVmM=".to_string(),
+//!     "YBk6X3pP8KjKz7+HFWzVHNqL3qTZq8hX9VxFQJ4zVmM=",
 //!     "10.0.0.2/24",
 //!     vec![peer],
 //! ).with_dns(vec!["1.1.1.1".into()]);
@@ -71,7 +71,7 @@ use zvariant::{Dict, Value, signature};
 use super::wireguard_builder::WireGuardBuilder;
 use crate::api::models::{
     ConnectionError, ConnectionOptions, OpenVpnAuthType, OpenVpnCompression, OpenVpnConfig,
-    OpenVpnProxy, Passphrase, VpnCredentials,
+    OpenVpnProxy, VpnCredentials,
 };
 
 /// Builds WireGuard VPN connection settings.
@@ -94,7 +94,7 @@ pub fn build_wireguard_connection(
     opts: &ConnectionOptions,
 ) -> Result<HashMap<&'static str, HashMap<&'static str, Value<'static>>>, ConnectionError> {
     let mut builder = WireGuardBuilder::new(&creds.name)
-        .private_key(creds.private_key.clone())
+        .private_key(&creds.private_key)
         .address(&creds.address)
         .add_peers(creds.peers.iter().cloned())
         .options(opts);
@@ -141,16 +141,6 @@ fn push_opt_str(out: &mut Vec<(String, String)>, key: &str, value: Option<&Strin
     }
 }
 
-/// Pushes a plaintext copy of a secret onto `out` if `value` is `Some`.
-///
-/// NetworkManager's VPN settings dictionary owns its strings, so the copy
-/// cannot retain the zeroization behavior of [`Passphrase`].
-fn push_opt_secret(out: &mut Vec<(String, String)>, key: &str, value: Option<&Passphrase>) {
-    if let Some(value) = value {
-        out.push((key.to_string(), value.expose_secret().to_owned()));
-    }
-}
-
 /// Pushes `(key, value.to_string())` onto `out` if `value` is `Some`.
 ///
 /// Used for numeric/boolean OpenVPN options that NetworkManager stores as
@@ -311,7 +301,7 @@ pub fn build_openvpn_connection(
                     vpn_data.push(("http-proxy-username".into(), u.clone()));
                 }
                 if let Some(p) = password {
-                    vpn_data.push(("http-proxy-password".into(), p.expose_secret().to_owned()));
+                    vpn_data.push(("http-proxy-password".into(), p.clone()));
                 }
             }
             OpenVpnProxy::Socks {
@@ -338,8 +328,8 @@ pub fn build_openvpn_connection(
     let data_dict = string_pairs_to_dict(vpn_data)?;
 
     let mut vpn_secrets: Vec<(String, String)> = Vec::new();
-    push_opt_secret(&mut vpn_secrets, "password", config.password.as_ref());
-    push_opt_secret(&mut vpn_secrets, "cert-pass", config.key_password.as_ref());
+    push_opt_str(&mut vpn_secrets, "password", config.password.as_ref());
+    push_opt_str(&mut vpn_secrets, "cert-pass", config.key_password.as_ref());
 
     let mut vpn: HashMap<&'static str, Value<'static>> = HashMap::new();
     vpn.insert(
@@ -390,12 +380,11 @@ pub fn build_openvpn_connection(
 
     Ok(settings)
 }
-
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::{
-        OpenVpnCompression, OpenVpnConfig, OpenVpnProxy, Passphrase, VpnKind, WireGuardPeer,
+    use crate::api::models::{
+        OpenVpnCompression, OpenVpnConfig, OpenVpnProxy, VpnKind, WireGuardPeer,
     };
 
     fn create_test_credentials() -> VpnCredentials {
@@ -410,7 +399,7 @@ mod tests {
             VpnKind::WireGuard,
             "TestVPN",
             "vpn.example.com:51820",
-            "YBk6X3pP8KjKz7+HFWzVHNqL3qTZq8hX9VxFQJ4zVmM=".to_string(),
+            "YBk6X3pP8KjKz7+HFWzVHNqL3qTZq8hX9VxFQJ4zVmM=",
             "10.0.0.2/24",
             vec![peer],
         )
@@ -532,7 +521,7 @@ mod tests {
             "peer2.example.com:51821",
             vec!["192.168.0.0/16".into()],
         )
-        .with_preshared_key("PSKABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklm=".to_string());
+        .with_preshared_key("PSKABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklm=");
 
         creds.peers.push(extra_peer);
         let opts = create_test_options();
@@ -726,9 +715,7 @@ mod tests {
     #[test]
     fn peer_with_preshared_key() {
         let mut creds = create_test_credentials();
-        creds.peers[0].preshared_key = Some(Passphrase::new(
-            "PSKABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklm=".to_string(),
-        ));
+        creds.peers[0].preshared_key = Some("PSKABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklm=".into());
         let opts = create_test_options();
 
         let settings = build_wireguard_connection(&creds, &opts).unwrap();
@@ -1133,7 +1120,7 @@ mod tests {
         let config = create_openvpn_config()
             .with_auth_type(OpenVpnAuthType::Password)
             .with_username("user")
-            .with_password(Passphrase::new("secret".to_string()));
+            .with_password("secret");
         let opts = create_test_options();
         let settings = build_openvpn_connection(&config, &opts).unwrap();
         let vpn = settings.get("vpn").unwrap();