[pbs-devel] [PATCH offline-mirror/proxmox/backup-server 0/6] improve pom multi-key handling and pbs key check

[pbs-devel] [PATCH offline-mirror/proxmox/backup-server 0/6] improve pom multi-key handling and pbs key check

[Stefan Sterz]

this patch series intends to improve the usability of the
`proxmox-offline-mirror-helper`. it also refactors the handling of the
`ProductType` type. by moving it into the `proxmox-subscription` crate, it is
possible to check the product type consistently accross products. this enables
us to also add a check for the product type to the `subscription
set-offline-key` command in pbs.

the first and third patches move the `ProductType` trait from pom into the
`proxmox-subscription`. the second patch also exposes a `get_next_due_date()`
function for the `SubscriptionInfo` type. these patches need to be applied for
the other patches to work.

the third patch adapts pom's handling of multi-key key-media. previously there
was a bug where pom did not check if the subscription's server id matche the
current system's. this commit fixes that and also adds some more convenience
for systems that need keys for multiple products (e. g. a pve+pbs host). it
also filters the keys by server id that are shown to cli users when using the
interactive `setup` command.

the fifth commit is just a rustfmt clean up for pom. the sixth commit adds a
check to pbs that makes sure an offline subscription that is applied throuh the
manager's `subscription set-offline-key` command is actually a pbs
subscription.

Stefan Sterz (2) @ proxmox:
  type: move `ProductType` type to `proxmox-subscription` from pom
  subscription: expose the `next_due_date` as an `i64`

 proxmox-subscription/src/lib.rs               |  4 +-
 proxmox-subscription/src/subscription_info.rs | 58 ++++++++++++++++++-
 2 files changed, 60 insertions(+), 2 deletions(-)

Stefan Sterz (3) @ proxmox-offline-mirror:
  type: move `ProductType` enum to `proxmox-subscription`
  helper: improve handling of multiple keys when activating them
  offline mirror binary: rustfmt clean up

 src/bin/proxmox-offline-mirror-helper.rs      | 101 +++++++++++-------
 src/bin/proxmox-offline-mirror.rs             |  15 ++-
 src/bin/proxmox_offline_mirror_cmds/medium.rs |   4 +-
 .../subscription.rs                           |   4 +-
 src/config.rs                                 |   4 +-
 src/subscription.rs                           |   4 +-
 src/types.rs                                  |  44 +-------
 7 files changed, 77 insertions(+), 99 deletions(-)

Stefan Sterz (1) @ proxmox-backup:
  manager: check if offline subscription is for the correct product

 src/bin/proxmox_backup_manager/subscription.rs | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--
2.39.2

[pbs-devel] [PATCH proxmox 1/6] type: move `ProductType` type to `proxmox-subscription` from pom

[Stefan Sterz]

previously this type lived inside of pom. this made it harder to
access the product type from a `SubscriptionInfo` trait in other
products. move the type here so we can check product types more
consistently across products (e. g. in pom and pbs)

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 proxmox-subscription/src/lib.rs               |  4 +-
 proxmox-subscription/src/subscription_info.rs | 51 ++++++++++++++++++-
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/proxmox-subscription/src/lib.rs b/proxmox-subscription/src/lib.rs
index 6c30a91..8a298f2 100644
--- a/proxmox-subscription/src/lib.rs
+++ b/proxmox-subscription/src/lib.rs
@@ -1,5 +1,7 @@
 mod subscription_info;
-pub use subscription_info::{get_hardware_address, SubscriptionInfo, SubscriptionStatus};
+pub use subscription_info::{
+    get_hardware_address, ProductType, SubscriptionInfo, SubscriptionStatus,
+};

 pub mod check;
 pub mod files;
diff --git a/proxmox-subscription/src/subscription_info.rs b/proxmox-subscription/src/subscription_info.rs
index f455392..dc10a2a 100644
--- a/proxmox-subscription/src/subscription_info.rs
+++ b/proxmox-subscription/src/subscription_info.rs
@@ -1,4 +1,4 @@
-use std::path::Path;
+use std::{fmt::Display, path::Path, str::FromStr};

 use anyhow::{bail, format_err, Error};
 use openssl::hash::{hash, DigestBytes, MessageDigest};
@@ -59,6 +59,48 @@ impl std::fmt::Display for SubscriptionStatus {
     }
 }

+#[cfg_attr(feature = "api-types", api())]
+#[cfg_attr(feature = "api-types", derive(Updater))]
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
+#[serde(rename_all = "lowercase")]
+/// Product type
+pub enum ProductType {
+    /// Proxmox Virtual Environment
+    Pve,
+    /// Proxmox Backup Server
+    Pbs,
+    /// Proxmox Mail Gateway
+    Pmg,
+    /// Proxmox Offline Mirror
+    Pom,
+}
+
+impl Display for ProductType {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let txt = match self {
+            ProductType::Pve => "pve",
+            ProductType::Pbs => "pbs",
+            ProductType::Pmg => "pmg",
+            ProductType::Pom => "pom",
+        };
+        f.write_str(txt)
+    }
+}
+
+impl FromStr for ProductType {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s {
+            "pve" => Ok(ProductType::Pve),
+            "pmg" => Ok(ProductType::Pmg),
+            "pbs" => Ok(ProductType::Pbs),
+            "pom" => Ok(ProductType::Pom),
+            _ => bail!("unknown product type '{s}'"),
+        }
+    }
+}
+
 #[cfg_attr(feature = "api-types", api(
     properties: {
         status: {
@@ -237,6 +279,13 @@ impl SubscriptionInfo {
             }
         }
     }
+
+    pub fn get_product_type(&self) -> Result<ProductType, Error> {
+        self.key
+            .as_ref()
+            .ok_or_else(|| format_err!("no product key set"))
+            .map(|key| key[..3].parse::<ProductType>())?
+    }
 }

 /// Shortcut for md5 sums.
--
2.39.2

[pbs-devel] applied: [PATCH proxmox 1/6] type: move `ProductType` type to `proxmox-subscription` from pom

[Fabian Grünbichler]

+ a follow-up to add `Hash` as well, see my reply to the POM patch

On November 9, 2023 4:33 pm, Stefan Sterz wrote:
> previously this type lived inside of pom. this made it harder to
> access the product type from a `SubscriptionInfo` trait in other
> products. move the type here so we can check product types more
> consistently across products (e. g. in pom and pbs)
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
>  proxmox-subscription/src/lib.rs               |  4 +-
>  proxmox-subscription/src/subscription_info.rs | 51 ++++++++++++++++++-
>  2 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-subscription/src/lib.rs b/proxmox-subscription/src/lib.rs
> index 6c30a91..8a298f2 100644
> --- a/proxmox-subscription/src/lib.rs
> +++ b/proxmox-subscription/src/lib.rs
> @@ -1,5 +1,7 @@
>  mod subscription_info;
> -pub use subscription_info::{get_hardware_address, SubscriptionInfo, SubscriptionStatus};
> +pub use subscription_info::{
> +    get_hardware_address, ProductType, SubscriptionInfo, SubscriptionStatus,
> +};
> 
>  pub mod check;
>  pub mod files;
> diff --git a/proxmox-subscription/src/subscription_info.rs b/proxmox-subscription/src/subscription_info.rs
> index f455392..dc10a2a 100644
> --- a/proxmox-subscription/src/subscription_info.rs
> +++ b/proxmox-subscription/src/subscription_info.rs
> @@ -1,4 +1,4 @@
> -use std::path::Path;
> +use std::{fmt::Display, path::Path, str::FromStr};
> 
>  use anyhow::{bail, format_err, Error};
>  use openssl::hash::{hash, DigestBytes, MessageDigest};
> @@ -59,6 +59,48 @@ impl std::fmt::Display for SubscriptionStatus {
>      }
>  }
> 
> +#[cfg_attr(feature = "api-types", api())]
> +#[cfg_attr(feature = "api-types", derive(Updater))]
> +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
> +#[serde(rename_all = "lowercase")]
> +/// Product type
> +pub enum ProductType {
> +    /// Proxmox Virtual Environment
> +    Pve,
> +    /// Proxmox Backup Server
> +    Pbs,
> +    /// Proxmox Mail Gateway
> +    Pmg,
> +    /// Proxmox Offline Mirror
> +    Pom,
> +}
> +
> +impl Display for ProductType {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        let txt = match self {
> +            ProductType::Pve => "pve",
> +            ProductType::Pbs => "pbs",
> +            ProductType::Pmg => "pmg",
> +            ProductType::Pom => "pom",
> +        };
> +        f.write_str(txt)
> +    }
> +}
> +
> +impl FromStr for ProductType {
> +    type Err = Error;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        match s {
> +            "pve" => Ok(ProductType::Pve),
> +            "pmg" => Ok(ProductType::Pmg),
> +            "pbs" => Ok(ProductType::Pbs),
> +            "pom" => Ok(ProductType::Pom),
> +            _ => bail!("unknown product type '{s}'"),
> +        }
> +    }
> +}
> +
>  #[cfg_attr(feature = "api-types", api(
>      properties: {
>          status: {
> @@ -237,6 +279,13 @@ impl SubscriptionInfo {
>              }
>          }
>      }
> +
> +    pub fn get_product_type(&self) -> Result<ProductType, Error> {
> +        self.key
> +            .as_ref()
> +            .ok_or_else(|| format_err!("no product key set"))
> +            .map(|key| key[..3].parse::<ProductType>())?
> +    }
>  }
> 
>  /// Shortcut for md5 sums.
> --
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

[pbs-devel] [PATCH proxmox 2/6] subscription: expose the `next_due_date` as an `i64`

[Stefan Sterz]

internally `SubscriptionInfo` already uses the `parse_next_due` helper
to parse the next due date to an epoch. this exposes a function that
allows us to use the epoch outside of this crate too. for example, a
user of pom may have multiple subscription for the same system. in
that case we want to apply the one with the due date that is furthest
in the future.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 proxmox-subscription/src/subscription_info.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/proxmox-subscription/src/subscription_info.rs b/proxmox-subscription/src/subscription_info.rs
index dc10a2a..45f1796 100644
--- a/proxmox-subscription/src/subscription_info.rs
+++ b/proxmox-subscription/src/subscription_info.rs
@@ -286,6 +286,13 @@ impl SubscriptionInfo {
             .ok_or_else(|| format_err!("no product key set"))
             .map(|key| key[..3].parse::<ProductType>())?
     }
+
+    pub fn get_next_due_date(&self) -> Result<i64, Error> {
+        self.nextduedate
+            .as_ref()
+            .ok_or_else(|| format_err!("no next due date set"))
+            .map(|e| parse_next_due(e))?
+    }
 }

 /// Shortcut for md5 sums.
--
2.39.2

[pbs-devel] applied: [PATCH proxmox 2/6] subscription: expose the `next_due_date` as an `i64`

[Fabian Grünbichler]

On November 9, 2023 4:33 pm, Stefan Sterz wrote:
> internally `SubscriptionInfo` already uses the `parse_next_due` helper
> to parse the next due date to an epoch. this exposes a function that
> allows us to use the epoch outside of this crate too. for example, a
> user of pom may have multiple subscription for the same system. in
> that case we want to apply the one with the due date that is furthest
> in the future.
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
>  proxmox-subscription/src/subscription_info.rs | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/proxmox-subscription/src/subscription_info.rs b/proxmox-subscription/src/subscription_info.rs
> index dc10a2a..45f1796 100644
> --- a/proxmox-subscription/src/subscription_info.rs
> +++ b/proxmox-subscription/src/subscription_info.rs
> @@ -286,6 +286,13 @@ impl SubscriptionInfo {
>              .ok_or_else(|| format_err!("no product key set"))
>              .map(|key| key[..3].parse::<ProductType>())?
>      }
> +
> +    pub fn get_next_due_date(&self) -> Result<i64, Error> {
> +        self.nextduedate
> +            .as_ref()
> +            .ok_or_else(|| format_err!("no next due date set"))
> +            .map(|e| parse_next_due(e))?
> +    }
>  }
> 
>  /// Shortcut for md5 sums.
> --
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

[pbs-devel] [PATCH proxmox-offline-mirror 3/6] type: move `ProductType` enum to `proxmox-subscription`

[Stefan Sterz]

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/bin/proxmox-offline-mirror-helper.rs      |  4 +-
 src/bin/proxmox-offline-mirror.rs             |  3 +-
 src/bin/proxmox_offline_mirror_cmds/medium.rs |  4 +-
 .../subscription.rs                           |  4 +-
 src/config.rs                                 |  4 +-
 src/subscription.rs                           |  4 +-
 src/types.rs                                  | 44 +------------------
 7 files changed, 13 insertions(+), 54 deletions(-)

diff --git a/src/bin/proxmox-offline-mirror-helper.rs b/src/bin/proxmox-offline-mirror-helper.rs
index e62206c..f1b4bd7 100644
--- a/src/bin/proxmox-offline-mirror-helper.rs
+++ b/src/bin/proxmox-offline-mirror-helper.rs
@@ -4,8 +4,8 @@ use std::{collections::HashMap, path::Path};

 use anyhow::{bail, format_err, Error};

-use proxmox_offline_mirror::types::{ProductType, Snapshot};
-use proxmox_subscription::SubscriptionInfo;
+use proxmox_offline_mirror::types::Snapshot;
+use proxmox_subscription::{ProductType, SubscriptionInfo};
 use proxmox_sys::command::run_command;
 use proxmox_sys::fs::{replace_file, CreateOptions};
 use proxmox_sys::{fs::file_get_contents, linux::tty};
diff --git a/src/bin/proxmox-offline-mirror.rs b/src/bin/proxmox-offline-mirror.rs
index 9d37ea6..383f1c3 100644
--- a/src/bin/proxmox-offline-mirror.rs
+++ b/src/bin/proxmox-offline-mirror.rs
@@ -10,6 +10,7 @@ use serde_json::Value;
 use proxmox_router::cli::{run_cli_command, CliCommand, CliCommandMap, CliEnvironment};
 use proxmox_schema::api;
 use proxmox_section_config::SectionConfigData;
+use proxmox_subscription::ProductType;
 use proxmox_sys::linux::tty;

 use proxmox_offline_mirror::helpers::tty::{
@@ -18,7 +19,7 @@ use proxmox_offline_mirror::helpers::tty::{
 use proxmox_offline_mirror::{
     config::{save_config, MediaConfig, MirrorConfig, SkipConfig},
     mirror,
-    types::{ProductType, MEDIA_ID_SCHEMA, MIRROR_ID_SCHEMA},
+    types::{MEDIA_ID_SCHEMA, MIRROR_ID_SCHEMA},
 };

 mod proxmox_offline_mirror_cmds;
diff --git a/src/bin/proxmox_offline_mirror_cmds/medium.rs b/src/bin/proxmox_offline_mirror_cmds/medium.rs
index 574f748..3f05d43 100644
--- a/src/bin/proxmox_offline_mirror_cmds/medium.rs
+++ b/src/bin/proxmox_offline_mirror_cmds/medium.rs
@@ -6,7 +6,7 @@ use serde_json::Value;
 use proxmox_router::cli::{CliCommand, CliCommandMap, CommandLineInterface, OUTPUT_FORMAT};
 use proxmox_schema::api;
 use proxmox_section_config::SectionConfigData;
-use proxmox_subscription::SubscriptionInfo;
+use proxmox_subscription::{ProductType, SubscriptionInfo};
 use proxmox_time::epoch_to_rfc3339_utc;

 use proxmox_offline_mirror::{
@@ -14,7 +14,7 @@ use proxmox_offline_mirror::{
     generate_repo_file_line,
     medium::{self},
     mirror,
-    types::{ProductType, Snapshot, MEDIA_ID_SCHEMA},
+    types::{Snapshot, MEDIA_ID_SCHEMA},
 };

 use super::get_config_path;
diff --git a/src/bin/proxmox_offline_mirror_cmds/subscription.rs b/src/bin/proxmox_offline_mirror_cmds/subscription.rs
index e58b049..c5b8f3d 100644
--- a/src/bin/proxmox_offline_mirror_cmds/subscription.rs
+++ b/src/bin/proxmox_offline_mirror_cmds/subscription.rs
@@ -7,9 +7,9 @@ use std::convert::TryFrom;
 use proxmox_offline_mirror::{
     config::{SubscriptionKey, SubscriptionKeyUpdater},
     subscription::{extract_mirror_key, refresh_mirror_key, refresh_offline_keys},
-    types::{ProductType, PROXMOX_SUBSCRIPTION_KEY_SCHEMA},
+    types::PROXMOX_SUBSCRIPTION_KEY_SCHEMA,
 };
-use proxmox_subscription::{files::DEFAULT_SIGNING_KEY, SubscriptionStatus};
+use proxmox_subscription::{files::DEFAULT_SIGNING_KEY, ProductType, SubscriptionStatus};
 use proxmox_sys::fs::file_get_contents;
 use proxmox_time::epoch_to_rfc3339_utc;

diff --git a/src/config.rs b/src/config.rs
index d0bc14e..9c75717 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -7,11 +7,11 @@ use serde::{Deserialize, Serialize};

 use proxmox_schema::{api, ApiStringFormat, ApiType, Schema, Updater};
 use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
+use proxmox_subscription::ProductType;
 use proxmox_sys::fs::{replace_file, CreateOptions};

 use crate::types::{
-    ProductType, MEDIA_ID_SCHEMA, MIRROR_ID_SCHEMA, PROXMOX_SERVER_ID_SCHEMA,
-    PROXMOX_SUBSCRIPTION_KEY_SCHEMA,
+    MEDIA_ID_SCHEMA, MIRROR_ID_SCHEMA, PROXMOX_SERVER_ID_SCHEMA, PROXMOX_SUBSCRIPTION_KEY_SCHEMA,
 };

 /// Skip Configuration
diff --git a/src/subscription.rs b/src/subscription.rs
index 91b7bdb..3f0c776 100644
--- a/src/subscription.rs
+++ b/src/subscription.rs
@@ -5,10 +5,10 @@ use proxmox_http::{HttpClient, HttpOptions, ProxyConfig};
 use proxmox_subscription::SubscriptionStatus;
 use proxmox_subscription::{
     sign::{SignRequest, SignedResponse},
-    SubscriptionInfo,
+    ProductType, SubscriptionInfo,
 };

-use crate::{config::SubscriptionKey, types::ProductType};
+use crate::config::SubscriptionKey;

 // TODO: Update with final, public URL
 const PRODUCT_URL: &str = "-";
diff --git a/src/types.rs b/src/types.rs
index 3098a8d..7544d5e 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -1,10 +1,9 @@
 use std::{fmt::Display, path::PathBuf, str::FromStr};

 use anyhow::Error;
-use proxmox_schema::{api, const_regex, ApiStringFormat, Schema, StringSchema, Updater};
+use proxmox_schema::{api, const_regex, ApiStringFormat, Schema, StringSchema};
 use proxmox_serde::{forward_deserialize_to_from_str, forward_serialize_to_display};
 use proxmox_time::{epoch_i64, epoch_to_rfc3339_utc, parse_rfc3339};
-use serde::{Deserialize, Serialize};

 #[rustfmt::skip]
 #[macro_export]
@@ -100,47 +99,6 @@ impl FromStr for Snapshot {
     }
 }

-#[api()]
-#[derive(Debug, Clone, Serialize, Deserialize, Updater, PartialEq, Eq)]
-#[serde(rename_all = "lowercase")]
-/// Product type
-pub enum ProductType {
-    /// Proxmox Virtual Environment
-    Pve,
-    /// Proxmox Backup Server
-    Pbs,
-    /// Proxmox Mail Gateway
-    Pmg,
-    /// Proxmox Offline Mirror
-    Pom,
-}
-
-impl Display for ProductType {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        let txt = match self {
-            ProductType::Pve => "pve",
-            ProductType::Pbs => "pbs",
-            ProductType::Pmg => "pmg",
-            ProductType::Pom => "pom",
-        };
-        f.write_str(txt)
-    }
-}
-
-impl FromStr for ProductType {
-    type Err = Error;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        match s {
-            "pve" => Ok(ProductType::Pve),
-            "pmg" => Ok(ProductType::Pmg),
-            "pbs" => Ok(ProductType::Pbs),
-            "pom" => Ok(ProductType::Pom),
-            _ => unimplemented!(),
-        }
-    }
-}
-
 /// Entries of Diff
 #[derive(Default)]
 pub struct DiffMember {
--
2.39.2

[pbs-devel] [PATCH proxmox-offline-mirror 4/6] helper: improve handling of multiple keys when activating them

[Stefan Sterz]

this commit fixes a behavior where pom would applied any subscription
key that matched the provided product. it did not check whether the
server id of the activated subscription matched the current system.
this commit fixes that and only allows applying subscriptions for the
current system.

it also adds a couple of ux improvements:

- the `offline-key` sub-command now does not require the `--product`
  parameter anymore. if there are multiple keys with different
  products for the same server we will try to activate them all. the
  assumption is that the user added all keys intentionally (e.g. a
  combo pbs+pve system) and would like to activate them all at once.
  since this only makes the api more permissive this shouldn't be a
  breaking change.
- if the `offline-key` sub-command encounters multiple subscription
  keys with the same product and server id, it only activates the one
  with the due date furthest in the future. this makes sense in a
  scenario where a user simply adds new subscription keys to their
  key medium without removing older ones (perhaps older subscriptions
  haven't even expired just yet).
- the interactive `setup` sub-command now only offers keys that have a
  matching server id. it also orders them in such a way that the top
  most key for a given product has the next due date furthest in the
  future.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/bin/proxmox-offline-mirror-helper.rs | 97 ++++++++++++++----------
 1 file changed, 59 insertions(+), 38 deletions(-)

diff --git a/src/bin/proxmox-offline-mirror-helper.rs b/src/bin/proxmox-offline-mirror-helper.rs
index f1b4bd7..a231e84 100644
--- a/src/bin/proxmox-offline-mirror-helper.rs
+++ b/src/bin/proxmox-offline-mirror-helper.rs
@@ -24,7 +24,7 @@ use proxmox_offline_mirror::helpers::tty::{
 use proxmox_offline_mirror::medium::{self, generate_repo_snippet, MediumState};

 fn set_subscription_key(
-    product: ProductType,
+    product: &ProductType,
     subscription: &SubscriptionInfo,
 ) -> Result<String, Error> {
     let data = base64::encode(serde_json::to_vec(subscription)?);
@@ -218,32 +218,34 @@ async fn setup(_param: Value) -> Result<(), Error> {
             }
             Action::UpdateOfflineSubscription => {
                 let server_id = proxmox_subscription::get_hardware_address()?;
-                let subscriptions: Vec<(&SubscriptionInfo, &str)> = state
+                let mut subscriptions: Vec<((ProductType, &SubscriptionInfo), &str)> = state
                     .subscriptions
                     .iter()
-                    .filter_map(|s| {
-                        if let Some(key) = s.key.as_ref() {
-                            if let Ok(product) = key[..3].parse::<ProductType>() {
-                                if product == ProductType::Pom {
-                                    return None;
-                                } else {
-                                    return Some((s, key.as_str()));
-                                }
-                            }
-                        }
-                        None
+                    .filter(|sub| sub.serverid.as_ref() == Some(&server_id))
+                    .filter_map(|sub| {
+                        sub.get_product_type()
+                            .ok()
+                            .and_then(|prod| Some(((prod, sub), sub.key.as_ref()?.as_str())))
                     })
+                    .filter(|((found_product, _), _)| found_product != &ProductType::Pom)
                     .collect();

+                subscriptions.sort_by(|((prod_a, sub_a), _), ((prod_b, sub_b), _)| {
+                    if prod_a == prod_b {
+                        return sub_b
+                            .get_next_due_date()
+                            .ok()
+                            .cmp(&sub_a.get_next_due_date().ok());
+                    }
+
+                    prod_a.cmp(prod_b)
+                });
+
                 if subscriptions.is_empty() {
-                    println!(
-                        "No matching subscription key found for server ID '{}'",
-                        server_id
-                    );
+                    println!("No matching subscription key found for server ID '{server_id}'");
                 } else {
-                    let info = read_selection_from_tty("Select key", &subscriptions, None)?;
-                    // safe unwrap, checked above!
-                    let product: ProductType = info.key.as_ref().unwrap()[..3].parse()?;
+                    let (product, info) =
+                        read_selection_from_tty("Select key", &subscriptions, None)?;
                     set_subscription_key(product, info)?;
                 }
             }
@@ -265,6 +267,7 @@ async fn setup(_param: Value) -> Result<(), Error> {
             },
             product: {
                 type: ProductType,
+                optional: true,
             },
         },
     },
@@ -272,10 +275,10 @@ async fn setup(_param: Value) -> Result<(), Error> {
 /// Configures and offline subscription key
 async fn setup_offline_key(
     mountpoint: String,
-    product: ProductType,
+    product: Option<ProductType>,
     _param: Value,
 ) -> Result<(), Error> {
-    if product == ProductType::Pom {
+    if product == Some(ProductType::Pom) {
         param_bail!(
             "product",
             format_err!("Proxmox Offline Mirror does not support offline operations.")
@@ -299,27 +302,45 @@ async fn setup_offline_key(
     );

     let server_id = proxmox_subscription::get_hardware_address()?;
-    let subscription = state.subscriptions.iter().find(|s| {
-        if let Some(key) = s.key.as_ref() {
-            if let Ok(found_product) = key[..3].parse::<ProductType>() {
-                return product == found_product;
-            }
+
+    let mut subscriptions: Vec<(ProductType, &SubscriptionInfo)> = state
+        .subscriptions
+        .iter()
+        .filter(|sub| sub.serverid.as_ref() == Some(&server_id))
+        .filter_map(|sub| sub.get_product_type().ok().map(|prod| (prod, sub)))
+        .filter(|(found_product, _)| {
+            (product.is_none() || Some(found_product) == product.as_ref())
+                && found_product != &ProductType::Pom
+        })
+        .collect();
+
+    if subscriptions.is_empty() {
+        bail!("No matching subscription key found for server ID '{server_id}'");
+    }
+
+    subscriptions.sort_by(|(prod_a, sub_a), (prod_b, sub_b)| {
+        if prod_a == prod_b {
+            return sub_b
+                .get_next_due_date()
+                .ok()
+                .cmp(&sub_a.get_next_due_date().ok());
         }
-        false
+
+        prod_a.cmp(prod_b)
     });

-    match subscription {
-        Some(subscription) => {
-            eprintln!("Setting offline subscription key for {product}..");
-            match set_subscription_key(product, subscription) {
-                Ok(output) if !output.is_empty() => eprintln!("success: {output}"),
-                Ok(_) => eprintln!("success."),
-                Err(err) => eprintln!("error: {err}"),
-            }
-            Ok(())
+    subscriptions.dedup_by(|(prod_a, _), (prod_b, _)| prod_a == prod_b);
+
+    for (product, subscription) in subscriptions {
+        eprintln!("Setting offline subscription key for {product}...");
+        match set_subscription_key(&product, subscription) {
+            Ok(output) if !output.is_empty() => eprintln!("success: {output}"),
+            Ok(_) => eprintln!("success."),
+            Err(err) => eprintln!("error: {err}"),
         }
-        None => bail!("No matching subscription key found for product '{product}' and server ID '{server_id}'"),
     }
+
+    Ok(())
 }

 #[api(
--
2.39.2

Re: [pbs-devel] [PATCH proxmox-offline-mirror 4/6] helper: improve handling of multiple keys when activating them

[Fabian Grünbichler]

On November 9, 2023 4:34 pm, Stefan Sterz wrote:
> [..]
> 
>      let server_id = proxmox_subscription::get_hardware_address()?;
> -    let subscription = state.subscriptions.iter().find(|s| {
> -        if let Some(key) = s.key.as_ref() {
> -            if let Ok(found_product) = key[..3].parse::<ProductType>() {
> -                return product == found_product;
> -            }
> +
> +    let mut subscriptions: Vec<(ProductType, &SubscriptionInfo)> = state
> +        .subscriptions
> +        .iter()
> +        .filter(|sub| sub.serverid.as_ref() == Some(&server_id))
> +        .filter_map(|sub| sub.get_product_type().ok().map(|prod| (prod, sub)))
> +        .filter(|(found_product, _)| {
> +            (product.is_none() || Some(found_product) == product.as_ref())
> +                && found_product != &ProductType::Pom
> +        })
> +        .collect();
> +
> +    if subscriptions.is_empty() {
> +        bail!("No matching subscription key found for server ID '{server_id}'");
> +    }
> +
> +    subscriptions.sort_by(|(prod_a, sub_a), (prod_b, sub_b)| {
> +        if prod_a == prod_b {
> +            return sub_b
> +                .get_next_due_date()
> +                .ok()
> +                .cmp(&sub_a.get_next_due_date().ok());
>          }
> -        false
> +
> +        prod_a.cmp(prod_b)
>      });
> 
> -    match subscription {
> -        Some(subscription) => {
> -            eprintln!("Setting offline subscription key for {product}..");
> -            match set_subscription_key(product, subscription) {
> -                Ok(output) if !output.is_empty() => eprintln!("success: {output}"),
> -                Ok(_) => eprintln!("success."),
> -                Err(err) => eprintln!("error: {err}"),
> -            }
> -            Ok(())
> +    subscriptions.dedup_by(|(prod_a, _), (prod_b, _)| prod_a == prod_b);

this is a bit opaque from a readability standpoint, would something like
this as follow-up be agreeable for you? it makes the whole "try to find
the single key per product" a bit more explicit.

diff --git a/src/bin/proxmox-offline-mirror-helper.rs b/src/bin/proxmox-offline-mirror-helper.rs
index a231e84..def10ad 100644
--- a/src/bin/proxmox-offline-mirror-helper.rs
+++ b/src/bin/proxmox-offline-mirror-helper.rs
@@ -303,7 +303,7 @@ async fn setup_offline_key(
 
     let server_id = proxmox_subscription::get_hardware_address()?;
 
-    let mut subscriptions: Vec<(ProductType, &SubscriptionInfo)> = state
+    let subscriptions: HashMap<ProductType, &SubscriptionInfo> = state
         .subscriptions
         .iter()
         .filter(|sub| sub.serverid.as_ref() == Some(&server_id))
@@ -312,25 +312,21 @@ async fn setup_offline_key(
             (product.is_none() || Some(found_product) == product.as_ref())
                 && found_product != &ProductType::Pom
         })
-        .collect();
+        .fold(HashMap::new(), |mut map, (found_product, sub)| {
+            if let Some(existing) = map.get(&found_product) {
+                if existing.get_next_due_date().ok() < sub.get_next_due_date().ok() {
+                    map.insert(found_product, sub);
+                }
+            } else {
+                map.insert(found_product, sub);
+            }
+            map
+        });
 
     if subscriptions.is_empty() {
         bail!("No matching subscription key found for server ID '{server_id}'");
     }
 
-    subscriptions.sort_by(|(prod_a, sub_a), (prod_b, sub_b)| {
-        if prod_a == prod_b {
-            return sub_b
-                .get_next_due_date()
-                .ok()
-                .cmp(&sub_a.get_next_due_date().ok());
-        }
-
-        prod_a.cmp(prod_b)
-    });
-
-    subscriptions.dedup_by(|(prod_a, _), (prod_b, _)| prod_a == prod_b);
-
     for (product, subscription) in subscriptions {
         eprintln!("Setting offline subscription key for {product}...");
         match set_subscription_key(&product, subscription) {

> +
> +    for (product, subscription) in subscriptions {
> +        eprintln!("Setting offline subscription key for {product}...");
> +        match set_subscription_key(&product, subscription) {
> +            Ok(output) if !output.is_empty() => eprintln!("success: {output}"),
> +            Ok(_) => eprintln!("success."),
> +            Err(err) => eprintln!("error: {err}"),
>          }
> -        None => bail!("No matching subscription key found for product '{product}' and server ID '{server_id}'"),
>      }
> +
> +    Ok(())
>  }
> 
>  #[api(
> --
> 2.39.2

[pbs-devel] [PATCH proxmox-offline-mirror 5/6] offline mirror binary: rustfmt clean up

[Stefan Sterz]

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/bin/proxmox-offline-mirror.rs | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/src/bin/proxmox-offline-mirror.rs b/src/bin/proxmox-offline-mirror.rs
index 383f1c3..0f47ee3 100644
--- a/src/bin/proxmox-offline-mirror.rs
+++ b/src/bin/proxmox-offline-mirror.rs
@@ -179,8 +179,8 @@ fn derive_debian_repo(
         (Release::Bookworm, DebianVariant::Security) => {
             "/usr/share/keyrings/debian-archive-bookworm-security-automatic.gpg"
         }
-        (Release::Bookworm, DebianVariant::Updates) |
-        (Release::Bookworm, DebianVariant::Backports) => {
+        (Release::Bookworm, DebianVariant::Updates)
+        | (Release::Bookworm, DebianVariant::Backports) => {
             "/usr/share/keyrings/debian-archive-bookworm-automatic.gpg"
         }
         (Release::Bookworm, _) => "/usr/share/keyrings/debian-archive-bookworm-stable.gpg",
@@ -239,13 +239,11 @@ fn action_add_mirror(config: &SectionConfigData) -> Result<Vec<MirrorConfig>, Er

                 let default_components = match release {
                     Release::Bookworm => "main contrib non-free non-free-firmware",
-                    _ => "main contrib non-free"
+                    _ => "main contrib non-free",
                 };

-                let components = read_string_from_tty(
-                    "Enter repository components",
-                    Some(default_components),
-                )?;
+                let components =
+                    read_string_from_tty("Enter repository components", Some(default_components))?;

                 derive_debian_repo(release, variant, &components)?
             }
--
2.39.2

[pbs-devel] [PATCH proxmox-backup 6/6] manager: check if offline subscription is for the correct product

[Stefan Sterz]

previously when an offline key was set it wasn't verified that the
subscription was for the correct product. while pom only applies
subscriptions for the corresponding products, a user could manually
invoke the `subscription set-offline-key` command to circumvent that.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/bin/proxmox_backup_manager/subscription.rs | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/bin/proxmox_backup_manager/subscription.rs b/src/bin/proxmox_backup_manager/subscription.rs
index 66161af7..12f09ecf 100644
--- a/src/bin/proxmox_backup_manager/subscription.rs
+++ b/src/bin/proxmox_backup_manager/subscription.rs
@@ -3,7 +3,7 @@ use serde_json::Value;

 use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
 use proxmox_schema::api;
-use proxmox_subscription::SubscriptionInfo;
+use proxmox_subscription::{ProductType, SubscriptionInfo};

 use proxmox_backup::api2::{self, node::subscription::subscription_file_opts};

@@ -51,6 +51,12 @@ pub fn set_offline_subscription_key(data: String) -> Result<(), Error> {
     if !info.is_signed() {
         bail!("Offline subscription key must be signed!");
     }
+
+    let product_type = info.get_product_type()?;
+    if product_type != ProductType::Pbs {
+        bail!("Subscription is not a PBS subscription ({product_type})!");
+    }
+
     info.check_signature(&[proxmox_subscription::files::DEFAULT_SIGNING_KEY]);
     info.check_age(false);
     info.check_server_id();
--
2.39.2

[pbs-devel] applied: [PATCH proxmox-backup 6/6] manager: check if offline subscription is for the correct product

[Fabian Grünbichler]

with the bumped proxmox-subscription dependency folded in

On November 9, 2023 4:34 pm, Stefan Sterz wrote:
> previously when an offline key was set it wasn't verified that the
> subscription was for the correct product. while pom only applies
> subscriptions for the corresponding products, a user could manually
> invoke the `subscription set-offline-key` command to circumvent that.
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
>  src/bin/proxmox_backup_manager/subscription.rs | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/bin/proxmox_backup_manager/subscription.rs b/src/bin/proxmox_backup_manager/subscription.rs
> index 66161af7..12f09ecf 100644
> --- a/src/bin/proxmox_backup_manager/subscription.rs
> +++ b/src/bin/proxmox_backup_manager/subscription.rs
> @@ -3,7 +3,7 @@ use serde_json::Value;
> 
>  use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
>  use proxmox_schema::api;
> -use proxmox_subscription::SubscriptionInfo;
> +use proxmox_subscription::{ProductType, SubscriptionInfo};
> 
>  use proxmox_backup::api2::{self, node::subscription::subscription_file_opts};
> 
> @@ -51,6 +51,12 @@ pub fn set_offline_subscription_key(data: String) -> Result<(), Error> {
>      if !info.is_signed() {
>          bail!("Offline subscription key must be signed!");
>      }
> +
> +    let product_type = info.get_product_type()?;
> +    if product_type != ProductType::Pbs {
> +        bail!("Subscription is not a PBS subscription ({product_type})!");
> +    }
> +
>      info.check_signature(&[proxmox_subscription::files::DEFAULT_SIGNING_KEY]);
>      info.check_age(false);
>      info.check_server_id();
> --
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>