Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions nmrs/src/api/models/error.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use thiserror::Error;

use crate::core::ovpn_parser::error::OvpnParseError;
use crate::models::IpAddressParseError;

use super::connection_state::ConnectionStateReason;
use super::state_reason::StateReason;
Expand Down Expand Up @@ -280,4 +281,7 @@ pub enum ConnectionError {
/// No interface was found with the given name
#[error("no interface named '{0}'")]
InterfaceNotFound(String),

#[error("invalid IP address: {0}")]
AddressParse(IpAddressParseError),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Hmmm I don't think I understand what the point of this is, are you planning to use it in a follow up PR for this issue?ConnectionError is #[non_exhaustive], so we can add it back if a fallible path ever appears.

}
92 changes: 92 additions & 0 deletions nmrs/src/api/models/ip_address.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
use std::{
fmt::{self, Display},
net::{AddrParseError, Ipv4Addr, Ipv6Addr},
num::ParseIntError,
str::FromStr,
};

use thiserror::Error;

use crate::ConnectionError;

/// An IP address with its prefix.
#[derive(Copy, Clone, Eq, PartialEq)]
pub struct IpAddress<A> {
pub address: A,
pub prefix: u8,
}

impl<A> IpAddress<A> {
/// Create the IP address from the address and prefix.
pub fn new(address: A, prefix: u8) -> Self {
Self { address, prefix }
}
}

impl<A> Display for IpAddress<A>
where
A: Display,
{
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}/{}", self.address, self.prefix)
}
}

impl<A> fmt::Debug for IpAddress<A>
where
A: fmt::Debug,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{:?}/{}", self.address, self.prefix)
}
}

impl FromStr for IpAddress<Ipv4Addr> {
type Err = IpAddressParseError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let (address, prefix) = s.rsplit_once('/').ok_or(IpAddressParseError::Split)?;
let address = address.parse()?;
let prefix = prefix.parse()?;
Ok(Self { address, prefix })
}
}

impl FromStr for IpAddress<Ipv6Addr> {
type Err = IpAddressParseError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let (address, prefix) = s.rsplit_once('/').ok_or(IpAddressParseError::Split)?;
let address = address.parse()?;
let prefix = prefix.parse()?;
Ok(Self { address, prefix })
}
}

impl From<IpAddress<Ipv4Addr>> for Ipv4Addr {
fn from(value: IpAddress<Ipv4Addr>) -> Self {
value.address
}
}

impl From<IpAddress<Ipv6Addr>> for Ipv6Addr {
fn from(value: IpAddress<Ipv6Addr>) -> Self {
value.address
}
}

#[derive(Debug, Clone, Error)]
pub enum IpAddressParseError {
#[error("address parsing failed: {0}")]
Addr(#[from] AddrParseError),
#[error("prefix parsing failed: {0}")]
Prefix(#[from] ParseIntError),
#[error("could not split into address and prefix")]
Split,
}

impl From<IpAddressParseError> for ConnectionError {
fn from(value: IpAddressParseError) -> Self {
Self::AddressParse(value)
}
}
2 changes: 2 additions & 0 deletions nmrs/src/api/models/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ mod connection_state;
mod connectivity;
mod device;
mod error;
mod ip_address;
mod monitor;
mod network_event;
mod openvpn;
Expand Down Expand Up @@ -44,6 +45,7 @@ pub use connection_state::*;
pub use connectivity::*;
pub use device::*;
pub use error::*;
pub use ip_address::*;
pub use monitor::*;
pub use network_event::*;
pub use openvpn::*;
Expand Down
64 changes: 63 additions & 1 deletion nmrs/src/api/models/saved_connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,15 @@
//! [`GetSecrets`](https://networkmanager.dev/docs/api/latest/gdbus-org.freedesktop.NetworkManager.Settings.Connection.html#gdbus-method-org-freedesktop-NetworkManager-Settings-Connection.GetSecrets)
//! when a [secret agent](crate::agent) is registered. See feature `01-secret-agent`.

use std::collections::HashMap;
use std::{
collections::HashMap,
net::{Ipv4Addr, Ipv6Addr},
};

use zvariant::{OwnedObjectPath, OwnedValue};

use crate::{builders::Route, models::IpAddress};

/// Full saved profile with a structured [`SettingsSummary`].
#[non_exhaustive]
#[derive(Debug, Clone)]
Expand Down Expand Up @@ -39,6 +44,10 @@ pub struct SavedConnection {
pub filename: Option<String>,
/// Decoded type-specific fields (no secrets).
pub summary: SettingsSummary,
/// IPv4 specific settings.
pub ipv4: Option<IpSettings<Ipv4Addr>>,
/// IPv6 specific settings.
pub ipv6: Option<IpSettings<Ipv6Addr>>,
}

/// Cheap listing: path plus `connection` identity fields only (still one `GetSettings` per profile).
Expand Down Expand Up @@ -214,3 +223,56 @@ pub enum SettingsSummary {
sections: Vec<String>,
},
}

/// Settings from the ipv4/6 section.
#[derive(Debug, Clone)]
#[non_exhaustive]
pub struct IpSettings<A> {
/// The method by which the connection obtains its IP address for this protocol.
pub method: IpMethod,
/// The list of IP addresses.
pub address_data: Vec<IpAddress<A>>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
pub address_data: Vec<IpAddress<A>>,
pub addresses: Vec<IpAddress<A>>,

? The current name leaks NM's wire keys into our public API. wdyt?

/// The gateway associated with this protocol configuration.
pub gateway: Option<A>,
/// The list of DNS search domains.
pub dns_search: Vec<String>,
/// The list of IP routes.
pub route_data: Vec<Route>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's make this typed. We ruled out sharing structs between builders and reads in #524: builders hold only what the caller set, reads arrive fully populated with NM's defaults.

IpSettings<A> is generic and Route isn't, so an IpSettings<Ipv6Addr> can hold IPv4 route strings. And next_hop() / metric() are #[must_use] builder setters that mean nothing on decoded output.dest + prefix is already your IpAddress<A>:

IpRoute<A> { dest: IpAddress<A>, next_hop: Option<A>, metric: Option<u32> }

@cachebag cachebag Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
pub route_data: Vec<Route>,
pub routes: Vec<IpRoute<A>>,

Same thing here

/// If true, this connection will never be assigned the default route.
pub never_default: bool,
/// Ignore the automatically configured DNS servers, and only use the saved configuration.
pub ignore_auto_dns: bool,
}
Comment on lines +243 to +245

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

dns is missing from here? without it the struct is incoherent

also prefer dns-data
https://networkmanager.dev/docs/api/latest/settings-ipv4.html#:~:text=dns%2Ddata


/// How the connection obtains its IP address.
#[derive(Debug, Clone)]
pub enum IpMethod {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
pub enum IpMethod {
#[non_exhaustive]
pub enum IpMethod {

/// IP configuration is automatically defined according to the hardware interface.
Auto,
/// IP configuration is manually specified in the connection settings.
Manual,
/// The connection does not use or require an IP address of this type.
Disabled,
/// The connection should only be configured for link-local operation.
LinkLocal,
/// Allows other devices to connect through this device to the default network.
Shared,
/// IP configuration is ignored for this protocol
Ignore,
/// Unknown method
Other(String),
}

impl From<String> for IpMethod {
fn from(value: String) -> Self {
match value.as_str() {
"auto" => Self::Auto,
"manual" => Self::Manual,
"disabled" => Self::Disabled,
"link-local" => Self::LinkLocal,
"shared" => Self::Shared,
"ignore" => Self::Ignore,
_ => Self::Other(value),
}
}
}
8 changes: 8 additions & 0 deletions nmrs/src/api/models/snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,8 @@ mod tests {
summary: SettingsSummary::Other {
sections: vec!["connection".into()],
},
ipv4: None,
ipv6: None,
}
}

Expand Down Expand Up @@ -463,6 +465,8 @@ mod tests {
hidden: false,
mac_randomization: None,
},
ipv4: None,
ipv6: None,
}
}

Expand All @@ -486,6 +490,8 @@ mod tests {
data_keys: Vec::new(),
persistent: false,
},
ipv4: None,
ipv6: None,
}
}

Expand All @@ -509,6 +515,8 @@ mod tests {
peer_count: 1,
first_peer_endpoint: None,
},
ipv4: None,
ipv6: None,
}
}

Expand Down
72 changes: 71 additions & 1 deletion nmrs/src/core/saved_connection.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
//! Decode and manage NetworkManager saved connection settings.

use std::collections::HashMap;
use std::str::FromStr;

use futures::stream::{self, StreamExt};
use log::warn;
use zbus::Connection;
use zvariant::{OwnedObjectPath, OwnedValue, Str};
use zvariant::{Array, OwnedObjectPath, OwnedValue, Str, Value};

use crate::Result;
use crate::api::models::{
ConnectionError, SavedConnection, SavedConnectionBrief, SettingsPatch, SettingsSummary,
VpnSecretFlags, WifiKeyMgmt, WifiSecuritySummary,
};
use crate::builders::Route;
use crate::dbus::{NMSettingsConnectionProxy, NMSettingsProxy};
use crate::models::{IpAddress, IpMethod, IpSettings};
use crate::util::utils::decode_ssid_or_empty;

/// Builds the `a{sa{sv}}` delta for [`SettingsPatch`] (unit-tested).
Expand Down Expand Up @@ -111,6 +114,10 @@ fn take_str(m: &HashMap<String, OwnedValue>, key: &str) -> Option<String> {
m.get(key).and_then(owned_to_str)
}

fn take_str_ref<'a>(m: &'a HashMap<String, OwnedValue>, key: &str) -> Option<&'a str> {
m.get(key).and_then(|s| s.try_into().ok())
}

fn take_bool(m: &HashMap<String, OwnedValue>, key: &str) -> Option<bool> {
m.get(key).and_then(owned_to_bool)
}
Expand Down Expand Up @@ -172,6 +179,8 @@ pub(crate) fn decode_saved(
let permissions = take_str_vec(conn, "permissions");

let summary = decode_summary(&connection_type, &settings);
let ipv4 = settings.get("ipv4").map(decode_ip);
let ipv6 = settings.get("ipv6").map(decode_ip);

Ok(SavedConnection {
path,
Expand All @@ -186,6 +195,8 @@ pub(crate) fn decode_saved(
unsaved,
filename,
summary,
ipv4,
ipv6,
})
}

Expand Down Expand Up @@ -427,6 +438,65 @@ fn decode_bluetooth(settings: &HashMap<String, HashMap<String, OwnedValue>>) ->
SettingsSummary::Bluetooth { bdaddr, bt_type }
}

fn decode_ip<A>(settings: &HashMap<String, OwnedValue>) -> IpSettings<A>
where
A: FromStr,
{
let method = take_str(settings, "method");
let method = match method {
Some(method) => method.into(),
None => IpMethod::Auto,
};
let mut address_data = Vec::new();
if let Some(value) = settings.get("address-data")
&& let Ok(array) = TryInto::<&Array>::try_into(value)
{
for entry in array.iter() {
if let Value::Dict(dict) = entry
&& let Ok(Some(address)) = dict.get::<_, &str>(&"address")
&& let Ok(Some(prefix)) = dict.get::<_, u32>(&"prefix")
{
if let Ok(address) = address.parse() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Malformed addresses and gateways are lost here. I believe we settled on dropping them with a warn! in #524

address_data.push(IpAddress::new(address, prefix as u8));
}
}
}
};
let gateway = take_str_ref(settings, "gateway").and_then(|gateway| gateway.parse().ok());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let dns_search = take_str_vec(settings, "dns-search");
let mut route_data = Vec::new();
if let Some(value) = settings.get("route-data")
&& let Ok(array) = TryInto::<&Array>::try_into(value)
{
for entry in array.iter() {
if let Value::Dict(dict) = entry
&& let Ok(Some(dest)) = dict.get::<_, String>(&"dest")
&& let Ok(Some(prefix)) = dict.get(&"prefix")
{
let mut route = Route::new(dest, prefix);
if let Ok(Some(next_hop)) = dict.get::<_, String>(&"next_hop") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if let Ok(Some(next_hop)) = dict.get::<_, String>(&"next_hop") {
if let Ok(Some(next_hop)) = dict.get::<_, String>(&"next-hop") {

https://networkmanager.dev/docs/api/latest/settings-ipv4.html#:~:text=%27next%2Dhop

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A regression test for this would be good as well

route = route.next_hop(next_hop);
}
if let Ok(Some(metric)) = dict.get(&"metric") {
route = route.metric(metric)
}
route_data.push(route);
}
}
};
let never_default = take_bool(settings, "never-default").unwrap_or(false);
let ignore_auto_dns = take_bool(settings, "ignore-auto-dns").unwrap_or(false);
IpSettings {
method,
address_data,
gateway,
dns_search,
route_data,
never_default,
ignore_auto_dns,
}
}

async fn fetch_one_full(
conn: &Connection,
path: OwnedObjectPath,
Expand Down
8 changes: 8 additions & 0 deletions nmrs/tests/integration_test.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::collections::HashMap;
use std::future::Future;
use std::net::Ipv4Addr;
use std::panic::{AssertUnwindSafe, resume_unwind};
use std::sync::Arc;
use std::sync::atomic::{AtomicUsize, Ordering};
Expand Down Expand Up @@ -402,6 +403,13 @@ async fn networkmanager_profile_crud_and_settings_events() {
assert_eq!(profile.id, id);
assert_eq!(profile.connection_type, "wireguard");
assert!(!profile.autoconnect);
match profile.ipv4 {
Some(ipv4) => {
assert_eq!(ipv4.address_data[0].address, Ipv4Addr::new(10, 203, 0, 2));
assert_eq!(ipv4.address_data[0].prefix, 24);
}
None => panic!("expected an ipv4 section")
}
match profile.summary {
SettingsSummary::WireGuard {
mtu,
Expand Down
Loading