[PATCH proxmox-backup 0/5] report: cover the s3 configuration

[PATCH proxmox-backup 0/5] report: cover the s3 configuration

[Erik Fastermann]

NOTE: This patch series depends on an unapplied series [0].

Include the ability to list the s3 endpoints and corresponding buckets,
which can help when debugging a system. See the individual commit
messages for the rational.

The first two patches are independent of that. The second one stops the
report from exposing the password or API token of every configured
remote, which remote.cfg stores base64 encoded. Reports are regularly
attached to public forum threads, so this seems worth fixing on its
own. There are other places where credentials can be exposed. I created
an entry on the bug tracker for this [1].


[0] https://lore.proxmox.com/pbs-devel/20260811114332.283776-1-e.fastermann@proxmox.com/
[1] https://bugzilla.proxmox.com/show_bug.cgi?id=8023


Erik Fastermann (5):
  report: rename mislabeled datastore file group
  report: do not include remote passwords
  report: include the s3 endpoint configuration
  api: s3: drop unused rpcenv parameter from bucket listing
  report: list the accessible s3 buckets

 src/api2/config/s3.rs                |  5 +-
 src/bin/proxmox_backup_manager/s3.rs |  7 +--
 src/server/report.rs                 | 92 +++++++++++++++++++++++-----
 3 files changed, 79 insertions(+), 25 deletions(-)

-- 
2.47.3

[PATCH proxmox-backup 1/5] report: rename mislabeled datastore file group

[Erik Fastermann]

The group only ever contained the datastore configuration, remote.cfg
is listed in its own "Remotes" group right below.

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 src/server/report.rs | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/server/report.rs b/src/server/report.rs
index 79a5a8eb5..d293aa6dd 100644
--- a/src/server/report.rs
+++ b/src/server/report.rs
@@ -2,10 +2,7 @@ use proxmox_system_report::{FileGroup, FunctionMapping, StaticArgsCommandSpec};
 
 fn project_files() -> Vec<FileGroup> {
     vec![
-        (
-            "Datastores & Remotes",
-            vec!["/etc/proxmox-backup/datastore.cfg"],
-        ),
+        ("Datastores", vec!["/etc/proxmox-backup/datastore.cfg"]),
         (
             "User & Access",
             vec![
-- 
2.47.3

[PATCH proxmox-backup 2/5] report: do not include remote passwords

[Erik Fastermann]

remote.cfg stores the password or API token of the remote user as a
base64 string, which is trivially reversible. As reports are commonly
shared with third parties, for example in public forum threads, list
the remotes through the API instead, which returns every property but
the password. Proxmox Datacenter Manager handles its own remote
configuration the same way.

The JSON output format is used for two reasons: the text table only
covers a subset of the configured properties, and the report runs all
commands with PROXMOX_OUTPUT_NO_BORDER set, which also suppresses the
table header and thus leaves the columns unlabeled. The latter could
alternatively be solved with a custom text formatter, as done for the
remote listing in Proxmox Datacenter Manager.

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 src/server/report.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/server/report.rs b/src/server/report.rs
index d293aa6dd..c22141810 100644
--- a/src/server/report.rs
+++ b/src/server/report.rs
@@ -10,7 +10,6 @@ fn project_files() -> Vec<FileGroup> {
                 "/etc/proxmox-backup/acl.cfg",
             ],
         ),
-        ("Remotes", vec!["/etc/proxmox-backup/remote.cfg"]),
         (
             "Jobs",
             vec![
@@ -41,6 +40,10 @@ fn project_commands() -> Vec<StaticArgsCommandSpec> {
         ("date", vec!["-R"]),
         ("proxmox-backup-manager", vec!["versions", "--verbose"]),
         ("proxmox-backup-manager", vec!["subscription", "get"]),
+        (
+            "proxmox-backup-manager",
+            vec!["remote", "list", "--output-format", "json-pretty"],
+        ),
         ("proxmox-backup-manager", vec!["ldap", "list"]),
         ("proxmox-backup-manager", vec!["openid", "list"]),
     ]
-- 
2.47.3

[PATCH proxmox-backup 3/5] report: include the s3 endpoint configuration

[Erik Fastermann]

s3.cfg is not included as a file, since it also holds the secret key of
each endpoint. Listing the endpoints through the API keeps that
distinction where it is already maintained, as the API returns the
configuration without the secret. The access key is part of the
returned configuration, matching what the API and the web interface
expose to any user with Sys.Audit privileges.

The JSON output format is used because the text table only covers a
subset of the configured properties and because the report suppresses
the table header.

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 src/server/report.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/server/report.rs b/src/server/report.rs
index c22141810..b123c1fa9 100644
--- a/src/server/report.rs
+++ b/src/server/report.rs
@@ -46,6 +46,10 @@ fn project_commands() -> Vec<StaticArgsCommandSpec> {
         ),
         ("proxmox-backup-manager", vec!["ldap", "list"]),
         ("proxmox-backup-manager", vec!["openid", "list"]),
+        (
+            "proxmox-backup-manager",
+            vec!["s3", "endpoint", "list", "--output-format", "json-pretty"],
+        ),
     ]
 }
 
-- 
2.47.3

[PATCH proxmox-backup 4/5] api: s3: drop unused rpcenv parameter from bucket listing

[Erik Fastermann]

The bucket listing never touched the RPC environment, and the API
macro does not require handlers to accept one. Dropping it allows
callers without an RPC environment to use the function directly.

No functional change intended.

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 src/api2/config/s3.rs                | 5 +----
 src/bin/proxmox_backup_manager/s3.rs | 7 ++-----
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/api2/config/s3.rs b/src/api2/config/s3.rs
index 9cf61d23d..3e6599f53 100644
--- a/src/api2/config/s3.rs
+++ b/src/api2/config/s3.rs
@@ -358,10 +358,7 @@ pub fn delete_s3_client_config(
     },
 )]
 /// List buckets accessible by given s3 client configuration
-pub async fn list_buckets(
-    id: String,
-    _rpcenv: &mut dyn RpcEnvironment,
-) -> Result<Vec<S3BucketListItem>, Error> {
+pub async fn list_buckets(id: String) -> Result<Vec<S3BucketListItem>, Error> {
     let (config, _digest) = pbs_config::s3::config()?;
     let config: S3ClientConf = config
         .lookup(S3_CFG_TYPE_ID, &id)
diff --git a/src/bin/proxmox_backup_manager/s3.rs b/src/bin/proxmox_backup_manager/s3.rs
index 046b12dd0..b86f5d625 100644
--- a/src/bin/proxmox_backup_manager/s3.rs
+++ b/src/bin/proxmox_backup_manager/s3.rs
@@ -45,11 +45,8 @@ async fn check(
     },
 )]
 /// List buckets accessible by the given S3 client configuration
-async fn list_buckets(
-    s3_endpoint_id: String,
-    rpcenv: &mut dyn RpcEnvironment,
-) -> Result<Vec<S3BucketListItem>, Error> {
-    api2::config::s3::list_buckets(s3_endpoint_id, rpcenv).await
+async fn list_buckets(s3_endpoint_id: String) -> Result<Vec<S3BucketListItem>, Error> {
+    api2::config::s3::list_buckets(s3_endpoint_id).await
 }
 
 #[api(
-- 
2.47.3

[PATCH proxmox-backup 5/5] report: list the accessible s3 buckets

[Erik Fastermann]

This shows whether the configured credentials still work, whether the
endpoint is reachable and which buckets are reported.

Implemented as a report function rather than as a command to set a
timeout and query the endpoints concurrently.

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 src/server/report.rs | 78 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 67 insertions(+), 11 deletions(-)

diff --git a/src/server/report.rs b/src/server/report.rs
index b123c1fa9..a454b52bc 100644
--- a/src/server/report.rs
+++ b/src/server/report.rs
@@ -1,5 +1,14 @@
+use std::fmt::Write;
+use std::time::Duration;
+
+use futures::future::join_all;
+
 use proxmox_system_report::{FileGroup, FunctionMapping, StaticArgsCommandSpec};
 
+use crate::api2::config::s3::list_buckets;
+
+const S3_LIST_BUCKETS_TIMEOUT: Duration = Duration::from_secs(5);
+
 fn project_files() -> Vec<FileGroup> {
     vec![
         ("Datastores", vec!["/etc/proxmox-backup/datastore.cfg"]),
@@ -53,19 +62,66 @@ fn project_commands() -> Vec<StaticArgsCommandSpec> {
     ]
 }
 
-fn project_function_calls() -> Vec<FunctionMapping> {
-    vec![("Datastores", || {
-        let config = match pbs_config::datastore::config() {
-            Ok((config, _digest)) => config,
-            _ => return String::from("could not read datastore config"),
+fn s3_buckets() -> String {
+    let config = match pbs_config::s3::config() {
+        Ok((config, _digest)) => config,
+        Err(err) => return format!("*error*: could not read s3 config: {err:#}"),
+    };
+    if config.order.is_empty() {
+        return String::from("*note*: no s3 endpoints configured");
+    }
+
+    let listings = proxmox_async::runtime::block_on(async {
+        join_all(
+            config
+                .order
+                .iter()
+                .map(|id| tokio::time::timeout(S3_LIST_BUCKETS_TIMEOUT, list_buckets(id.clone()))),
+        )
+        .await
+    });
+
+    let mut out = String::new();
+
+    for (id, listing) in config.order.iter().zip(listings) {
+        writeln!(out, "\n###### {id}").unwrap();
+
+        match listing {
+            Ok(Ok(buckets)) if buckets.is_empty() => {
+                writeln!(out, "*note*: endpoint reported no buckets").unwrap()
+            }
+            Ok(Ok(buckets)) => buckets
+                .into_iter()
+                .for_each(|bucket| writeln!(out, "- {}", bucket.name).unwrap()),
+            Ok(Err(err)) => writeln!(out, "*error*: {err:#}").unwrap(),
+            Err(_timeout) => writeln!(
+                out,
+                "*error*: timed out after {} seconds",
+                S3_LIST_BUCKETS_TIMEOUT.as_secs(),
+            )
+            .unwrap(),
         };
+    }
+
+    out
+}
 
-        let mut list = Vec::new();
-        for store in config.sections.keys() {
-            list.push(store.as_str());
-        }
-        format!("```\n{}\n```", list.join(", "))
-    })]
+fn project_function_calls() -> Vec<FunctionMapping> {
+    vec![
+        ("Datastores", || {
+            let config = match pbs_config::datastore::config() {
+                Ok((config, _digest)) => config,
+                _ => return String::from("could not read datastore config"),
+            };
+
+            let mut list = Vec::new();
+            for store in config.sections.keys() {
+                list.push(store.as_str());
+            }
+            format!("```\n{}\n```", list.join(", "))
+        }),
+        ("S3 Buckets", s3_buckets),
+    ]
 }
 
 pub fn generate_report() -> String {
-- 
2.47.3