[pdm-devel] [PATCH datacenter-manager 0/3] token secret shadow config

[pdm-devel] [PATCH datacenter-manager 0/3] token secret shadow config

[Fabian Grünbichler]

this patch series splits out token secrets into their own file,
replacing them with a placeholder value in the regular remotes.cfg

Fabian Grünbichler (3):
  remote config: let save_config take ownership
  remote config: get token secret from shadow file if shadowed
  remote config: shadow token secrets when saving

 lib/pdm-api-types/src/remotes.rs | 50 +++++++++++++++++++++
 lib/pdm-config/src/remotes.rs    | 76 +++++++++++++++++++++++++++++---
 server/src/api/pve/lxc.rs        |  2 +-
 server/src/api/pve/qemu.rs       |  2 +-
 server/src/api/remotes.rs        |  6 +--
 server/src/connection.rs         |  8 +++-
 6 files changed, 130 insertions(+), 14 deletions(-)

-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

[pdm-devel] [PATCH datacenter-manager 1/3] remote config: let save_config take ownership

[Fabian Grünbichler]

as preparation for automatically modifying the saved config to split out token
secrets, if needed.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 lib/pdm-config/src/remotes.rs | 9 +++++----
 server/src/api/remotes.rs     | 6 +++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/lib/pdm-config/src/remotes.rs b/lib/pdm-config/src/remotes.rs
index 74c4671..fc78707 100644
--- a/lib/pdm-config/src/remotes.rs
+++ b/lib/pdm-config/src/remotes.rs
@@ -47,7 +47,7 @@ pub fn config() -> Result<(SectionConfigData<Remote>, ConfigDigest), Error> {
 /// Replace the currently persisted remotes config
 ///
 /// Will panic if the the remote config instance has not been set before.
-pub fn save_config(config: &SectionConfigData<Remote>) -> Result<(), Error> {
+pub fn save_config(config: SectionConfigData<Remote>) -> Result<(), Error> {
     instance().save_config(config)
 }
 
@@ -57,7 +57,7 @@ pub trait RemoteConfig {
     /// Lock the remotes config
     fn lock_config(&self) -> Result<ApiLockGuard, Error>;
     /// Replace the currently persisted remotes config
-    fn save_config(&self, remotes: &SectionConfigData<Remote>) -> Result<(), Error>;
+    fn save_config(&self, remotes: SectionConfigData<Remote>) -> Result<(), Error>;
 }
 
 /// Default, production implementation for reading/writing the `remotes.cfg`
@@ -75,11 +75,12 @@ impl RemoteConfig for DefaultRemoteConfig {
 
         let digest = openssl::sha::sha256(content.as_bytes());
         let data = Remote::parse_section_config(REMOTES_CFG_FILENAME, &content)?;
+
         Ok((data, digest.into()))
     }
 
-    fn save_config(&self, config: &SectionConfigData<Remote>) -> Result<(), Error> {
-        let raw = Remote::write_section_config(REMOTES_CFG_FILENAME, config)?;
+    fn save_config(&self, config: SectionConfigData<Remote>) -> Result<(), Error> {
+        let raw = Remote::write_section_config(REMOTES_CFG_FILENAME, &config)?;
         replace_config(REMOTES_CFG_FILENAME, raw.as_bytes())
     }
 }
diff --git a/server/src/api/remotes.rs b/server/src/api/remotes.rs
index 76b005d..a7463b9 100644
--- a/server/src/api/remotes.rs
+++ b/server/src/api/remotes.rs
@@ -184,7 +184,7 @@ pub async fn add_remote(mut entry: Remote, create_token: Option<String>) -> Resu
     let name = entry.id.clone();
     remotes.insert(entry.id.to_owned(), entry);
 
-    pdm_config::remotes::save_config(&remotes)?;
+    pdm_config::remotes::save_config(remotes)?;
 
     if let Err(e) = metric_collection::trigger_metric_collection(Some(name), false).await {
         log::error!("could not trigger metric collection after adding remote: {e}");
@@ -268,7 +268,7 @@ pub fn update_remote(
         entry.web_url = updater.web_url;
     }
 
-    pdm_config::remotes::save_config(&remotes)?;
+    pdm_config::remotes::save_config(remotes)?;
 
     Ok(())
 }
@@ -291,7 +291,7 @@ pub fn remove_remote(id: String) -> Result<(), Error> {
         http_bail!(NOT_FOUND, "no such entry {id:?}");
     }
 
-    pdm_config::remotes::save_config(&remotes)?;
+    pdm_config::remotes::save_config(remotes)?;
 
     Ok(())
 }
-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

[pdm-devel] [PATCH datacenter-manager 2/3] remote config: get token secret from shadow file if shadowed

[Fabian Grünbichler]

by making this an explicit call (as opposed to automatically reading and
merging the shadow config), accidental leaks are less likely.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
if we want to go down the other route, that's of course also possible..

 lib/pdm-api-types/src/remotes.rs | 50 ++++++++++++++++++++++++++++++++
 lib/pdm-config/src/remotes.rs    | 30 +++++++++++++++++--
 server/src/api/pve/lxc.rs        |  2 +-
 server/src/api/pve/qemu.rs       |  2 +-
 server/src/connection.rs         |  8 +++--
 5 files changed, 86 insertions(+), 6 deletions(-)

diff --git a/lib/pdm-api-types/src/remotes.rs b/lib/pdm-api-types/src/remotes.rs
index bd90ef1..f69609c 100644
--- a/lib/pdm-api-types/src/remotes.rs
+++ b/lib/pdm-api-types/src/remotes.rs
@@ -149,6 +149,56 @@ impl ApiSectionDataEntry for Remote {
     }
 }
 
+#[api(
+    properties: {
+        "id": { schema: REMOTE_ID_SCHEMA },
+        "type": { type: RemoteType },
+    },
+)]
+/// The information required to connect to a remote instance.
+#[derive(Clone, Debug, Deserialize, Serialize, PartialEq)]
+#[serde(rename_all = "kebab-case")]
+pub struct RemoteShadow {
+    #[serde(rename = "type")]
+    pub ty: RemoteType,
+
+    /// An id for this entry.
+    pub id: String,
+
+    /// The access token's secret.
+    pub token: String,
+}
+
+impl ApiSectionDataEntry for RemoteShadow {
+    const INTERNALLY_TAGGED: Option<&'static str> = Some("type");
+    const SECION_CONFIG_USES_TYPE_KEY: bool = true;
+
+    /// Get the `SectionConfig` configuration for this enum.
+    fn section_config() -> &'static SectionConfig {
+        static CONFIG: OnceLock<SectionConfig> = OnceLock::new();
+
+        CONFIG.get_or_init(|| {
+            let mut this = SectionConfig::new(&REMOTE_ID_SCHEMA).with_type_key("type");
+            for ty in ["pve", "pbs"] {
+                this.register_plugin(SectionConfigPlugin::new(
+                    ty.to_string(),
+                    Some("id".to_string()),
+                    RemoteShadow::API_SCHEMA.unwrap_object_schema(),
+                ));
+            }
+            this
+        })
+    }
+
+    /// Maps an enum value to its type name.
+    fn section_type(&self) -> &'static str {
+        match self.ty {
+            RemoteType::Pve => "pve",
+            RemoteType::Pbs => "pbs",
+        }
+    }
+}
+
 /// Since `Uri` does not directly support `serde`, we turn this into using FromStr/Display.
 ///
 /// If we want to turn this into a property string, we can use a default_key, but may need to work
diff --git a/lib/pdm-config/src/remotes.rs b/lib/pdm-config/src/remotes.rs
index fc78707..93f80eb 100644
--- a/lib/pdm-config/src/remotes.rs
+++ b/lib/pdm-config/src/remotes.rs
@@ -5,17 +5,18 @@
 
 use std::sync::OnceLock;
 
-use anyhow::Error;
+use anyhow::{bail, Error};
 
 use proxmox_config_digest::ConfigDigest;
 use proxmox_product_config::{open_api_lockfile, replace_config, ApiLockGuard};
 use proxmox_section_config::typed::{ApiSectionDataEntry, SectionConfigData};
 
-use pdm_api_types::remotes::Remote;
+use pdm_api_types::remotes::{Remote, RemoteShadow};
 
 use pdm_buildcfg::configdir;
 
 pub const REMOTES_CFG_FILENAME: &str = configdir!("/remotes.cfg");
+const REMOTES_SHADOW_FILENAME: &str = configdir!("/remotes.shadow");
 pub const REMOTES_CFG_LOCKFILE: &str = configdir!("/.remotes.lock");
 
 static INSTANCE: OnceLock<Box<dyn RemoteConfig + Send + Sync>> = OnceLock::new();
@@ -44,6 +45,10 @@ pub fn config() -> Result<(SectionConfigData<Remote>, ConfigDigest), Error> {
     instance().config()
 }
 
+pub fn get_secret_token(remote: &Remote) -> Result<String, Error> {
+    instance().get_secret_token(remote)
+}
+
 /// Replace the currently persisted remotes config
 ///
 /// Will panic if the the remote config instance has not been set before.
@@ -54,6 +59,8 @@ pub fn save_config(config: SectionConfigData<Remote>) -> Result<(), Error> {
 pub trait RemoteConfig {
     /// Return contents of the remotes config
     fn config(&self) -> Result<(SectionConfigData<Remote>, ConfigDigest), Error>;
+    /// Return contents of the remotes shadow config
+    fn get_secret_token(&self, remote: &Remote) -> Result<String, Error>;
     /// Lock the remotes config
     fn lock_config(&self) -> Result<ApiLockGuard, Error>;
     /// Replace the currently persisted remotes config
@@ -83,6 +90,25 @@ impl RemoteConfig for DefaultRemoteConfig {
         let raw = Remote::write_section_config(REMOTES_CFG_FILENAME, &config)?;
         replace_config(REMOTES_CFG_FILENAME, raw.as_bytes())
     }
+
+    fn get_secret_token(&self, remote: &Remote) -> Result<String, Error> {
+        // not yet rewritten into shadow config
+        if remote.token != "-" {
+            return Ok(remote.token.clone());
+        }
+
+        let shadow_content = proxmox_sys::fs::file_read_optional_string(REMOTES_SHADOW_FILENAME)?
+            .unwrap_or_default();
+
+        let shadow_config =
+            RemoteShadow::parse_section_config(REMOTES_SHADOW_FILENAME, &shadow_content)?;
+
+        if let Some(shadow_entry) = shadow_config.get(&remote.id) {
+            Ok(shadow_entry.token.clone())
+        } else {
+            bail!("No shadow entry found for remote {id}", id = remote.id);
+        }
+    }
 }
 
 /// Initialize the [`RemoteConfig`] instance.
diff --git a/server/src/api/pve/lxc.rs b/server/src/api/pve/lxc.rs
index dc95783..198137b 100644
--- a/server/src/api/pve/lxc.rs
+++ b/server/src/api/pve/lxc.rs
@@ -545,7 +545,7 @@ pub async fn lxc_remote_migrate(
         "host={host},port={port},apitoken=PVEAPIToken={authid}={secret}",
         host = target_host_port.host(),
         authid = target.authid,
-        secret = target.token,
+        secret = pdm_config::remotes::get_secret_token(target)?,
         port = target_host_port.port_u16().unwrap_or(8006),
     );
     if let Some(fp) = target_node.fingerprint.as_deref() {
diff --git a/server/src/api/pve/qemu.rs b/server/src/api/pve/qemu.rs
index efab228..4a25ecb 100644
--- a/server/src/api/pve/qemu.rs
+++ b/server/src/api/pve/qemu.rs
@@ -593,7 +593,7 @@ pub async fn qemu_remote_migrate(
         "host={host},port={port},apitoken=PVEAPIToken={authid}={secret}",
         host = target_host_port.host(),
         authid = target.authid,
-        secret = target.token,
+        secret = pdm_config::remotes::get_secret_token(target)?,
         port = target_host_port.port_u16().unwrap_or(8006),
     );
     if let Some(fp) = target_node.fingerprint.as_deref() {
diff --git a/server/src/connection.rs b/server/src/connection.rs
index 1c0069e..7e36671 100644
--- a/server/src/connection.rs
+++ b/server/src/connection.rs
@@ -110,9 +110,11 @@ fn prepare_connect_client(
 /// authentication information and settings for the [`RemoteType`]
 fn connect(remote: &Remote, target_endpoint: Option<&str>) -> Result<Client, anyhow::Error> {
     let (client, info) = prepare_connect_client(remote, target_endpoint)?;
+    let token = pdm_config::remotes::get_secret_token(remote)?;
+
     client.set_authentication(proxmox_client::Token {
         userid: remote.authid.to_string(),
-        value: remote.token.to_string(),
+        value: token,
         prefix: info.prefix,
         perl_compat: info.perl_compat,
     });
@@ -148,10 +150,12 @@ fn prepare_connect_multi_client(remote: &Remote) -> Result<(MultiClient, Connect
 fn multi_connect(remote: &Remote) -> Result<MultiClient, anyhow::Error> {
     let (client, info) = prepare_connect_multi_client(remote)?;
 
+    let token = pdm_config::remotes::get_secret_token(remote)?;
+
     client.for_each_client(|client| {
         client.set_authentication(proxmox_client::Token {
             userid: remote.authid.to_string(),
-            value: remote.token.to_string(),
+            value: token.clone(),
             prefix: info.prefix.clone(),
             perl_compat: info.perl_compat,
         });
-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

[pdm-devel] [PATCH datacenter-manager 3/3] remote config: shadow token secrets when saving

[Fabian Grünbichler]

but only if the to-be-saved config contains at least one non-shadowed entry, to
avoid unnecessary work.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 lib/pdm-config/src/remotes.rs | 39 +++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/lib/pdm-config/src/remotes.rs b/lib/pdm-config/src/remotes.rs
index 93f80eb..677981f 100644
--- a/lib/pdm-config/src/remotes.rs
+++ b/lib/pdm-config/src/remotes.rs
@@ -3,7 +3,7 @@
 //! Make sure to call [`init`] to inject a concrete `RemoteConfig` instance
 //! before calling the [`lock_config`], [`config`] or [`save_config`] functions.
 
-use std::sync::OnceLock;
+use std::{collections::HashMap, sync::OnceLock};
 
 use anyhow::{bail, Error};
 
@@ -86,7 +86,42 @@ impl RemoteConfig for DefaultRemoteConfig {
         Ok((data, digest.into()))
     }
 
-    fn save_config(&self, config: SectionConfigData<Remote>) -> Result<(), Error> {
+    fn save_config(&self, mut config: SectionConfigData<Remote>) -> Result<(), Error> {
+        let mut new_shadow_entries = HashMap::new();
+        for (id, remote) in config.iter() {
+            if remote.token != "-" {
+                new_shadow_entries.insert(
+                    id.to_string(),
+                    RemoteShadow {
+                        ty: remote.ty,
+                        id: remote.id.clone(),
+                        token: remote.token.clone(),
+                    },
+                );
+            }
+        }
+
+        // only read and modify shadow config if needed
+        if !new_shadow_entries.is_empty() {
+            let shadow_content =
+                proxmox_sys::fs::file_read_optional_string(REMOTES_SHADOW_FILENAME)?
+                    .unwrap_or_default();
+
+            let mut shadow_config =
+                RemoteShadow::parse_section_config(REMOTES_SHADOW_FILENAME, &shadow_content)?;
+
+            for (id, shadow_entry) in new_shadow_entries.into_iter() {
+                if let Some(remote) = config.get_mut(&id) {
+                    remote.token = '-'.to_string();
+                }
+                shadow_config.insert(id, shadow_entry);
+            }
+            let raw = RemoteShadow::write_section_config(REMOTES_SHADOW_FILENAME, &shadow_config)?;
+            replace_config(REMOTES_SHADOW_FILENAME, raw.as_bytes())?;
+        }
+
+        // only write out remote.cfg with potentially new shadowed entries
+        // if shadow file was successfully written!
         let raw = Remote::write_section_config(REMOTES_CFG_FILENAME, &config)?;
         replace_config(REMOTES_CFG_FILENAME, raw.as_bytes())
     }
-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

Re: [pdm-devel] [PATCH datacenter-manager 3/3] remote config: shadow token secrets when saving

[Lukas Wagner]

On Mon Dec 1, 2025 at 10:29 AM CET, Fabian Grünbichler wrote:
> but only if the to-be-saved config contains at least one non-shadowed entry, to
> avoid unnecessary work.
>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  lib/pdm-config/src/remotes.rs | 39 +++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/lib/pdm-config/src/remotes.rs b/lib/pdm-config/src/remotes.rs
> index 93f80eb..677981f 100644
> --- a/lib/pdm-config/src/remotes.rs
> +++ b/lib/pdm-config/src/remotes.rs
> @@ -3,7 +3,7 @@
>  //! Make sure to call [`init`] to inject a concrete `RemoteConfig` instance
>  //! before calling the [`lock_config`], [`config`] or [`save_config`] functions.
>  
> -use std::sync::OnceLock;
> +use std::{collections::HashMap, sync::OnceLock};
>  
>  use anyhow::{bail, Error};
>  
> @@ -86,7 +86,42 @@ impl RemoteConfig for DefaultRemoteConfig {
>          Ok((data, digest.into()))
>      }
>  
> -    fn save_config(&self, config: SectionConfigData<Remote>) -> Result<(), Error> {
> +    fn save_config(&self, mut config: SectionConfigData<Remote>) -> Result<(), Error> {
> +        let mut new_shadow_entries = HashMap::new();
> +        for (id, remote) in config.iter() {
> +            if remote.token != "-" {
> +                new_shadow_entries.insert(
> +                    id.to_string(),
> +                    RemoteShadow {
> +                        ty: remote.ty,
> +                        id: remote.id.clone(),
> +                        token: remote.token.clone(),
> +                    },
> +                );
> +            }
> +        }
> +
> +        // only read and modify shadow config if needed
> +        if !new_shadow_entries.is_empty() {
> +            let shadow_content =
> +                proxmox_sys::fs::file_read_optional_string(REMOTES_SHADOW_FILENAME)?
> +                    .unwrap_or_default();
> +
> +            let mut shadow_config =
> +                RemoteShadow::parse_section_config(REMOTES_SHADOW_FILENAME, &shadow_content)?;
> +
> +            for (id, shadow_entry) in new_shadow_entries.into_iter() {
> +                if let Some(remote) = config.get_mut(&id) {
> +                    remote.token = '-'.to_string();
> +                }
> +                shadow_config.insert(id, shadow_entry);
> +            }
> +            let raw = RemoteShadow::write_section_config(REMOTES_SHADOW_FILENAME, &shadow_config)?;
> +            replace_config(REMOTES_SHADOW_FILENAME, raw.as_bytes())?;
> +        }
> +
> +        // only write out remote.cfg with potentially new shadowed entries
> +        // if shadow file was successfully written!
>          let raw = Remote::write_section_config(REMOTES_CFG_FILENAME, &config)?;
>          replace_config(REMOTES_CFG_FILENAME, raw.as_bytes())
>      }


Right we don't clean up tokens secrets for remotes that have been
removed. I guess we could 'garbage-collect' the shadow file when saving,
removing all entries that don't have a matching section in remotes.cfg?


_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

Re: [pdm-devel] [PATCH datacenter-manager 0/3] token secret shadow config

[Lukas Wagner]

On Mon Dec 1, 2025 at 10:29 AM CET, Fabian Grünbichler wrote:
> this patch series splits out token secrets into their own file,
> replacing them with a placeholder value in the regular remotes.cfg
>
> Fabian Grünbichler (3):
>   remote config: let save_config take ownership
>   remote config: get token secret from shadow file if shadowed
>   remote config: shadow token secrets when saving
>
>  lib/pdm-api-types/src/remotes.rs | 50 +++++++++++++++++++++
>  lib/pdm-config/src/remotes.rs    | 76 +++++++++++++++++++++++++++++---
>  server/src/api/pve/lxc.rs        |  2 +-
>  server/src/api/pve/qemu.rs       |  2 +-
>  server/src/api/remotes.rs        |  6 +--
>  server/src/connection.rs         |  8 +++-
>  6 files changed, 130 insertions(+), 14 deletions(-)

In general, this looks good to me.

Two things came to mind when testing/reviewing this:
  - as noted in the individual patch, the token should be removed when
    deleting a remote

  - I'm not sure it's super elegant to have `token -` even for *new*
    remotes. I think it would be much cleaner to make the token field
    optional, but I can see how that would be quite a bit more work
    (could entail having to use distinct types for the API, the config
    file and internal usage) so I guess it can stay as is for now. Maybe
    we can see how we could achieve this in a follow-up. Worst case, we
    drop the token parameter in the next major release.

With this in mind,

Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Lukas Wagner <l.wagner@proxmox.com>


_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

[pdm-devel] applied: [PATCH datacenter-manager 0/3] token secret shadow config

[Thomas Lamprecht]

On Mon, 01 Dec 2025 10:29:12 +0100, Fabian Grünbichler wrote:
> this patch series splits out token secrets into their own file,
> replacing them with a placeholder value in the regular remotes.cfg
> 
> Fabian Grünbichler (3):
>   remote config: let save_config take ownership
>   remote config: get token secret from shadow file if shadowed
>   remote config: shadow token secrets when saving
> 
> [...]

Applied as is, but would be nice to check out deleting tokens when deleting
remotes as mentioned by Lukas, thanks!

[1/3] remote config: let save_config take ownership
      commit: f757bbfbf4137b438e73b617f5cd09c3b887fb07
[2/3] remote config: get token secret from shadow file if shadowed
      commit: 4d2f06792aeab2c57b568c8c4332e5f5818b9d70
[3/3] remote config: shadow token secrets when saving
      commit: d5e4eeac864a89641d59b016e0a04fca6baabc16


_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel