[pbs-devel] [PATCH proxmox-backup v4 3/4] acme: change API impls to use proxmox-acme-api handlers

[Samuel Rufinatscha <s.rufinatscha@proxmox.com>]

PBS currently uses its own ACME client and API logic, while PDM uses the
factored out proxmox-acme and proxmox-acme-api crates. This duplication
risks differences in behaviour and requires ACME maintenance in two
places. This patch is part of a series to move PBS over to the shared
ACME stack.

Changes:
- Replace api2/config/acme.rs API logic with proxmox-acme-api handlers.
- Drop local caching and helper types that duplicate proxmox-acme-api.

Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
 src/api2/config/acme.rs                | 385 ++-----------------------
 src/api2/types/acme.rs                 |  16 -
 src/bin/proxmox_backup_manager/acme.rs |   6 +-
 src/config/acme/mod.rs                 |  44 +--
 4 files changed, 35 insertions(+), 416 deletions(-)

diff --git a/src/api2/config/acme.rs b/src/api2/config/acme.rs
index 02f88e2e..a112c8ee 100644
--- a/src/api2/config/acme.rs
+++ b/src/api2/config/acme.rs
@@ -1,31 +1,17 @@
-use std::fs;
-use std::ops::ControlFlow;
-use std::path::Path;
-use std::sync::{Arc, LazyLock, Mutex};
-use std::time::SystemTime;
-
-use anyhow::{bail, format_err, Error};
-use hex::FromHex;
-use serde::{Deserialize, Serialize};
-use serde_json::{json, Value};
-use tracing::{info, warn};
-
-use proxmox_router::{
-    http_bail, list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap,
-};
-use proxmox_schema::{api, param_bail};
-
-use proxmox_acme::types::AccountData as AcmeAccountData;
-
+use anyhow::Error;
 use pbs_api_types::{Authid, PRIV_SYS_MODIFY};
-
-use crate::api2::types::{AcmeChallengeSchema, KnownAcmeDirectory};
-use crate::config::acme::plugin::{
-    self, DnsPlugin, DnsPluginCore, DnsPluginCoreUpdater, PLUGIN_ID_SCHEMA,
+use proxmox_acme_api::{
+    AccountEntry, AccountInfo, AcmeAccountName, AcmeChallengeSchema, ChallengeSchemaWrapper,
+    DeletablePluginProperty, DnsPluginCore, DnsPluginCoreUpdater, KnownAcmeDirectory, PluginConfig,
+    DEFAULT_ACME_DIRECTORY_ENTRY, PLUGIN_ID_SCHEMA,
 };
-use proxmox_acme::async_client::AcmeClient;
-use proxmox_acme_api::AcmeAccountName;
+use proxmox_config_digest::ConfigDigest;
 use proxmox_rest_server::WorkerTask;
+use proxmox_router::{
+    http_bail, list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap,
+};
+use proxmox_schema::api;
+use tracing::info;
 
 pub(crate) const ROUTER: Router = Router::new()
     .get(&list_subdirs_api_method!(SUBDIRS))
@@ -67,19 +53,6 @@ const PLUGIN_ITEM_ROUTER: Router = Router::new()
     .put(&API_METHOD_UPDATE_PLUGIN)
     .delete(&API_METHOD_DELETE_PLUGIN);
 
-#[api(
-    properties: {
-        name: { type: AcmeAccountName },
-    },
-)]
-/// An ACME Account entry.
-///
-/// Currently only contains a 'name' property.
-#[derive(Serialize)]
-pub struct AccountEntry {
-    name: AcmeAccountName,
-}
-
 #[api(
     access: {
         permission: &Permission::Privilege(&["system", "certificates"], PRIV_SYS_MODIFY, false),
@@ -93,40 +66,7 @@ pub struct AccountEntry {
 )]
 /// List ACME accounts.
 pub fn list_accounts() -> Result<Vec<AccountEntry>, Error> {
-    let mut entries = Vec::new();
-    crate::config::acme::foreach_acme_account(|name| {
-        entries.push(AccountEntry { name });
-        ControlFlow::Continue(())
-    })?;
-    Ok(entries)
-}
-
-#[api(
-    properties: {
-        account: { type: Object, properties: {}, additional_properties: true },
-        tos: {
-            type: String,
-            optional: true,
-        },
-    },
-)]
-/// ACME Account information.
-///
-/// This is what we return via the API.
-#[derive(Serialize)]
-pub struct AccountInfo {
-    /// Raw account data.
-    account: AcmeAccountData,
-
-    /// The ACME directory URL the account was created at.
-    directory: String,
-
-    /// The account's own URL within the ACME directory.
-    location: String,
-
-    /// The ToS URL, if the user agreed to one.
-    #[serde(skip_serializing_if = "Option::is_none")]
-    tos: Option<String>,
+    proxmox_acme_api::list_accounts()
 }
 
 #[api(
@@ -143,23 +83,7 @@ pub struct AccountInfo {
 )]
 /// Return existing ACME account information.
 pub async fn get_account(name: AcmeAccountName) -> Result<AccountInfo, Error> {
-    let account_info = proxmox_acme_api::get_account(name).await?;
-
-    Ok(AccountInfo {
-        location: account_info.location,
-        tos: account_info.tos,
-        directory: account_info.directory,
-        account: AcmeAccountData {
-            only_return_existing: false, // don't actually write this out in case it's set
-            ..account_info.account
-        },
-    })
-}
-
-fn account_contact_from_string(s: &str) -> Vec<String> {
-    s.split(&[' ', ';', ',', '\0'][..])
-        .map(|s| format!("mailto:{s}"))
-        .collect()
+    proxmox_acme_api::get_account(name).await
 }
 
 #[api(
@@ -224,15 +148,11 @@ fn register_account(
         );
     }
 
-    if Path::new(&crate::config::acme::account_path(&name)).exists() {
+    if std::path::Path::new(&proxmox_acme_api::account_config_filename(&name)).exists() {
         http_bail!(BAD_REQUEST, "account {} already exists", name);
     }
 
-    let directory = directory.unwrap_or_else(|| {
-        crate::config::acme::DEFAULT_ACME_DIRECTORY_ENTRY
-            .url
-            .to_owned()
-    });
+    let directory = directory.unwrap_or_else(|| DEFAULT_ACME_DIRECTORY_ENTRY.url.to_string());
 
     WorkerTask::spawn(
         "acme-register",
@@ -288,17 +208,7 @@ pub fn update_account(
         auth_id.to_string(),
         true,
         move |_worker| async move {
-            let data = match contact {
-                Some(data) => json!({
-                    "contact": account_contact_from_string(&data),
-                }),
-                None => json!({}),
-            };
-
-            proxmox_acme_api::load_client_with_account(&name)
-                .await?
-                .update_account(&data)
-                .await?;
+            proxmox_acme_api::update_account(&name, contact).await?;
 
             Ok(())
         },
@@ -336,18 +246,8 @@ pub fn deactivate_account(
         auth_id.to_string(),
         true,
         move |_worker| async move {
-            match proxmox_acme_api::load_client_with_account(&name)
-                .await?
-                .update_account(&json!({"status": "deactivated"}))
-                .await
-            {
-                Ok(_account) => (),
-                Err(err) if !force => return Err(err),
-                Err(err) => {
-                    warn!("error deactivating account {name}, proceeding anyway - {err}");
-                }
-            }
-            crate::config::acme::mark_account_deactivated(&name)?;
+            proxmox_acme_api::deactivate_account(&name, force).await?;
+
             Ok(())
         },
     )
@@ -374,15 +274,7 @@ pub fn deactivate_account(
 )]
 /// Get the Terms of Service URL for an ACME directory.
 async fn get_tos(directory: Option<String>) -> Result<Option<String>, Error> {
-    let directory = directory.unwrap_or_else(|| {
-        crate::config::acme::DEFAULT_ACME_DIRECTORY_ENTRY
-            .url
-            .to_owned()
-    });
-    Ok(AcmeClient::new(directory)
-        .terms_of_service_url()
-        .await?
-        .map(str::to_owned))
+    proxmox_acme_api::get_tos(directory).await
 }
 
 #[api(
@@ -397,52 +289,7 @@ async fn get_tos(directory: Option<String>) -> Result<Option<String>, Error> {
 )]
 /// Get named known ACME directory endpoints.
 fn get_directories() -> Result<&'static [KnownAcmeDirectory], Error> {
-    Ok(crate::config::acme::KNOWN_ACME_DIRECTORIES)
-}
-
-/// Wrapper for efficient Arc use when returning the ACME challenge-plugin schema for serializing
-struct ChallengeSchemaWrapper {
-    inner: Arc<Vec<AcmeChallengeSchema>>,
-}
-
-impl Serialize for ChallengeSchemaWrapper {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: serde::Serializer,
-    {
-        self.inner.serialize(serializer)
-    }
-}
-
-struct CachedSchema {
-    schema: Arc<Vec<AcmeChallengeSchema>>,
-    cached_mtime: SystemTime,
-}
-
-fn get_cached_challenge_schemas() -> Result<ChallengeSchemaWrapper, Error> {
-    static CACHE: LazyLock<Mutex<Option<CachedSchema>>> = LazyLock::new(|| Mutex::new(None));
-
-    // the actual loading code
-    let mut last = CACHE.lock().unwrap();
-
-    let actual_mtime = fs::metadata(crate::config::acme::ACME_DNS_SCHEMA_FN)?.modified()?;
-
-    let schema = match &*last {
-        Some(CachedSchema {
-            schema,
-            cached_mtime,
-        }) if *cached_mtime >= actual_mtime => schema.clone(),
-        _ => {
-            let new_schema = Arc::new(crate::config::acme::load_dns_challenge_schema()?);
-            *last = Some(CachedSchema {
-                schema: Arc::clone(&new_schema),
-                cached_mtime: actual_mtime,
-            });
-            new_schema
-        }
-    };
-
-    Ok(ChallengeSchemaWrapper { inner: schema })
+    Ok(proxmox_acme_api::KNOWN_ACME_DIRECTORIES)
 }
 
 #[api(
@@ -457,69 +304,7 @@ fn get_cached_challenge_schemas() -> Result<ChallengeSchemaWrapper, Error> {
 )]
 /// Get named known ACME directory endpoints.
 fn get_challenge_schema() -> Result<ChallengeSchemaWrapper, Error> {
-    get_cached_challenge_schemas()
-}
-
-#[api]
-#[derive(Default, Deserialize, Serialize)]
-#[serde(rename_all = "kebab-case")]
-/// The API's format is inherited from PVE/PMG:
-pub struct PluginConfig {
-    /// Plugin ID.
-    plugin: String,
-
-    /// Plugin type.
-    #[serde(rename = "type")]
-    ty: String,
-
-    /// DNS Api name.
-    #[serde(skip_serializing_if = "Option::is_none", default)]
-    api: Option<String>,
-
-    /// Plugin configuration data.
-    #[serde(skip_serializing_if = "Option::is_none", default)]
-    data: Option<String>,
-
-    /// Extra delay in seconds to wait before requesting validation.
-    ///
-    /// Allows to cope with long TTL of DNS records.
-    #[serde(skip_serializing_if = "Option::is_none", default)]
-    validation_delay: Option<u32>,
-
-    /// Flag to disable the config.
-    #[serde(skip_serializing_if = "Option::is_none", default)]
-    disable: Option<bool>,
-}
-
-// See PMG/PVE's $modify_cfg_for_api sub
-fn modify_cfg_for_api(id: &str, ty: &str, data: &Value) -> PluginConfig {
-    let mut entry = data.clone();
-
-    let obj = entry.as_object_mut().unwrap();
-    obj.remove("id");
-    obj.insert("plugin".to_string(), Value::String(id.to_owned()));
-    obj.insert("type".to_string(), Value::String(ty.to_owned()));
-
-    // FIXME: This needs to go once the `Updater` is fixed.
-    // None of these should be able to fail unless the user changed the files by hand, in which
-    // case we leave the unmodified string in the Value for now. This will be handled with an error
-    // later.
-    if let Some(Value::String(ref mut data)) = obj.get_mut("data") {
-        if let Ok(new) = proxmox_base64::url::decode_no_pad(&data) {
-            if let Ok(utf8) = String::from_utf8(new) {
-                *data = utf8;
-            }
-        }
-    }
-
-    // PVE/PMG do this explicitly for ACME plugins...
-    // obj.insert("digest".to_string(), Value::String(digest.clone()));
-
-    serde_json::from_value(entry).unwrap_or_else(|_| PluginConfig {
-        plugin: "*Error*".to_string(),
-        ty: "*Error*".to_string(),
-        ..Default::default()
-    })
+    proxmox_acme_api::get_cached_challenge_schemas()
 }
 
 #[api(
@@ -535,12 +320,7 @@ fn modify_cfg_for_api(id: &str, ty: &str, data: &Value) -> PluginConfig {
 )]
 /// List ACME challenge plugins.
 pub fn list_plugins(rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<PluginConfig>, Error> {
-    let (plugins, digest) = plugin::config()?;
-    rpcenv["digest"] = hex::encode(digest).into();
-    Ok(plugins
-        .iter()
-        .map(|(id, (ty, data))| modify_cfg_for_api(id, ty, data))
-        .collect())
+    proxmox_acme_api::list_plugins(rpcenv)
 }
 
 #[api(
@@ -557,13 +337,7 @@ pub fn list_plugins(rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<PluginConfig>
 )]
 /// List ACME challenge plugins.
 pub fn get_plugin(id: String, rpcenv: &mut dyn RpcEnvironment) -> Result<PluginConfig, Error> {
-    let (plugins, digest) = plugin::config()?;
-    rpcenv["digest"] = hex::encode(digest).into();
-
-    match plugins.get(&id) {
-        Some((ty, data)) => Ok(modify_cfg_for_api(&id, ty, data)),
-        None => http_bail!(NOT_FOUND, "no such plugin"),
-    }
+    proxmox_acme_api::get_plugin(id, rpcenv)
 }
 
 // Currently we only have "the" standalone plugin and DNS plugins so we can just flatten a
@@ -595,30 +369,7 @@ pub fn get_plugin(id: String, rpcenv: &mut dyn RpcEnvironment) -> Result<PluginC
 )]
 /// Add ACME plugin configuration.
 pub fn add_plugin(r#type: String, core: DnsPluginCore, data: String) -> Result<(), Error> {
-    // Currently we only support DNS plugins and the standalone plugin is "fixed":
-    if r#type != "dns" {
-        param_bail!("type", "invalid ACME plugin type: {:?}", r#type);
-    }
-
-    let data = String::from_utf8(proxmox_base64::decode(data)?)
-        .map_err(|_| format_err!("data must be valid UTF-8"))?;
-
-    let id = core.id.clone();
-
-    let _lock = plugin::lock()?;
-
-    let (mut plugins, _digest) = plugin::config()?;
-    if plugins.contains_key(&id) {
-        param_bail!("id", "ACME plugin ID {:?} already exists", id);
-    }
-
-    let plugin = serde_json::to_value(DnsPlugin { core, data })?;
-
-    plugins.insert(id, r#type, plugin);
-
-    plugin::save_config(&plugins)?;
-
-    Ok(())
+    proxmox_acme_api::add_plugin(r#type, core, data)
 }
 
 #[api(
@@ -634,26 +385,7 @@ pub fn add_plugin(r#type: String, core: DnsPluginCore, data: String) -> Result<(
 )]
 /// Delete an ACME plugin configuration.
 pub fn delete_plugin(id: String) -> Result<(), Error> {
-    let _lock = plugin::lock()?;
-
-    let (mut plugins, _digest) = plugin::config()?;
-    if plugins.remove(&id).is_none() {
-        http_bail!(NOT_FOUND, "no such plugin");
-    }
-    plugin::save_config(&plugins)?;
-
-    Ok(())
-}
-
-#[api()]
-#[derive(Serialize, Deserialize)]
-#[serde(rename_all = "kebab-case")]
-/// Deletable property name
-pub enum DeletableProperty {
-    /// Delete the disable property
-    Disable,
-    /// Delete the validation-delay property
-    ValidationDelay,
+    proxmox_acme_api::delete_plugin(id)
 }
 
 #[api(
@@ -675,12 +407,12 @@ pub enum DeletableProperty {
                 type: Array,
                 optional: true,
                 items: {
-                    type: DeletableProperty,
+                    type: DeletablePluginProperty,
                 }
             },
             digest: {
-                description: "Digest to protect against concurrent updates",
                 optional: true,
+                type: ConfigDigest,
             },
         },
     },
@@ -694,65 +426,8 @@ pub fn update_plugin(
     id: String,
     update: DnsPluginCoreUpdater,
     data: Option<String>,
-    delete: Option<Vec<DeletableProperty>>,
-    digest: Option<String>,
+    delete: Option<Vec<DeletablePluginProperty>>,
+    digest: Option<ConfigDigest>,
 ) -> Result<(), Error> {
-    let data = data
-        .as_deref()
-        .map(proxmox_base64::decode)
-        .transpose()?
-        .map(String::from_utf8)
-        .transpose()
-        .map_err(|_| format_err!("data must be valid UTF-8"))?;
-
-    let _lock = plugin::lock()?;
-
-    let (mut plugins, expected_digest) = plugin::config()?;
-
-    if let Some(digest) = digest {
-        let digest = <[u8; 32]>::from_hex(digest)?;
-        crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?;
-    }
-
-    match plugins.get_mut(&id) {
-        Some((ty, ref mut entry)) => {
-            if ty != "dns" {
-                bail!("cannot update plugin of type {:?}", ty);
-            }
-
-            let mut plugin = DnsPlugin::deserialize(&*entry)?;
-
-            if let Some(delete) = delete {
-                for delete_prop in delete {
-                    match delete_prop {
-                        DeletableProperty::ValidationDelay => {
-                            plugin.core.validation_delay = None;
-                        }
-                        DeletableProperty::Disable => {
-                            plugin.core.disable = None;
-                        }
-                    }
-                }
-            }
-            if let Some(data) = data {
-                plugin.data = data;
-            }
-            if let Some(api) = update.api {
-                plugin.core.api = api;
-            }
-            if update.validation_delay.is_some() {
-                plugin.core.validation_delay = update.validation_delay;
-            }
-            if update.disable.is_some() {
-                plugin.core.disable = update.disable;
-            }
-
-            *entry = serde_json::to_value(plugin)?;
-        }
-        None => http_bail!(NOT_FOUND, "no such plugin"),
-    }
-
-    plugin::save_config(&plugins)?;
-
-    Ok(())
+    proxmox_acme_api::update_plugin(id, update, data, delete, digest)
 }
diff --git a/src/api2/types/acme.rs b/src/api2/types/acme.rs
index 7c9063c0..2905b41b 100644
--- a/src/api2/types/acme.rs
+++ b/src/api2/types/acme.rs
@@ -44,22 +44,6 @@ pub const ACME_DOMAIN_PROPERTY_SCHEMA: Schema =
         .format(&ApiStringFormat::PropertyString(&AcmeDomain::API_SCHEMA))
         .schema();
 
-#[api(
-    properties: {
-        name: { type: String },
-        url: { type: String },
-    },
-)]
-/// An ACME directory endpoint with a name and URL.
-#[derive(Serialize)]
-pub struct KnownAcmeDirectory {
-    /// The ACME directory's name.
-    pub name: &'static str,
-
-    /// The ACME directory's endpoint URL.
-    pub url: &'static str,
-}
-
 #[api(
     properties: {
         schema: {
diff --git a/src/bin/proxmox_backup_manager/acme.rs b/src/bin/proxmox_backup_manager/acme.rs
index bb987b26..e7bd67af 100644
--- a/src/bin/proxmox_backup_manager/acme.rs
+++ b/src/bin/proxmox_backup_manager/acme.rs
@@ -8,10 +8,8 @@ use proxmox_schema::api;
 use proxmox_sys::fs::file_get_contents;
 
 use proxmox_acme::async_client::AcmeClient;
-use proxmox_acme_api::AcmeAccountName;
+use proxmox_acme_api::{AcmeAccountName, DnsPluginCore, KNOWN_ACME_DIRECTORIES};
 use proxmox_backup::api2;
-use proxmox_backup::config::acme::plugin::DnsPluginCore;
-use proxmox_backup::config::acme::KNOWN_ACME_DIRECTORIES;
 
 pub fn acme_mgmt_cli() -> CommandLineInterface {
     let cmd_def = CliCommandMap::new()
@@ -122,7 +120,7 @@ async fn register_account(
 
                 match input.trim().parse::<usize>() {
                     Ok(n) if n < KNOWN_ACME_DIRECTORIES.len() => {
-                        break (KNOWN_ACME_DIRECTORIES[n].url.to_owned(), false);
+                        break (KNOWN_ACME_DIRECTORIES[n].url.to_string(), false);
                     }
                     Ok(n) if n == KNOWN_ACME_DIRECTORIES.len() => {
                         input.clear();
diff --git a/src/config/acme/mod.rs b/src/config/acme/mod.rs
index d31b2bc9..35cda50b 100644
--- a/src/config/acme/mod.rs
+++ b/src/config/acme/mod.rs
@@ -1,8 +1,7 @@
 use std::collections::HashMap;
 use std::ops::ControlFlow;
-use std::path::Path;
 
-use anyhow::{bail, format_err, Error};
+use anyhow::Error;
 use serde_json::Value;
 
 use proxmox_sys::error::SysError;
@@ -10,8 +9,8 @@ use proxmox_sys::fs::{file_read_string, CreateOptions};
 
 use pbs_api_types::PROXMOX_SAFE_ID_REGEX;
 
-use crate::api2::types::{AcmeChallengeSchema, KnownAcmeDirectory};
-use proxmox_acme_api::AcmeAccountName;
+use crate::api2::types::AcmeChallengeSchema;
+use proxmox_acme_api::{AcmeAccountName, KnownAcmeDirectory, KNOWN_ACME_DIRECTORIES};
 
 pub(crate) const ACME_DIR: &str = pbs_buildcfg::configdir!("/acme");
 pub(crate) const ACME_ACCOUNT_DIR: &str = pbs_buildcfg::configdir!("/acme/accounts");
@@ -36,23 +35,8 @@ pub(crate) fn make_acme_dir() -> Result<(), Error> {
     create_acme_subdir(ACME_DIR)
 }
 
-pub const KNOWN_ACME_DIRECTORIES: &[KnownAcmeDirectory] = &[
-    KnownAcmeDirectory {
-        name: "Let's Encrypt V2",
-        url: "https://acme-v02.api.letsencrypt.org/directory",
-    },
-    KnownAcmeDirectory {
-        name: "Let's Encrypt V2 Staging",
-        url: "https://acme-staging-v02.api.letsencrypt.org/directory",
-    },
-];
-
 pub const DEFAULT_ACME_DIRECTORY_ENTRY: &KnownAcmeDirectory = &KNOWN_ACME_DIRECTORIES[0];
 
-pub fn account_path(name: &str) -> String {
-    format!("{ACME_ACCOUNT_DIR}/{name}")
-}
-
 pub fn foreach_acme_account<F>(mut func: F) -> Result<(), Error>
 where
     F: FnMut(AcmeAccountName) -> ControlFlow<Result<(), Error>>,
@@ -83,28 +67,6 @@ where
     }
 }
 
-pub fn mark_account_deactivated(name: &str) -> Result<(), Error> {
-    let from = account_path(name);
-    for i in 0..100 {
-        let to = account_path(&format!("_deactivated_{name}_{i}"));
-        if !Path::new(&to).exists() {
-            return std::fs::rename(&from, &to).map_err(|err| {
-                format_err!(
-                    "failed to move account path {:?} to {:?} - {}",
-                    from,
-                    to,
-                    err
-                )
-            });
-        }
-    }
-    bail!(
-        "No free slot to rename deactivated account {:?}, please cleanup {:?}",
-        from,
-        ACME_ACCOUNT_DIR
-    );
-}
-
 pub fn load_dns_challenge_schema() -> Result<Vec<AcmeChallengeSchema>, Error> {
     let raw = file_read_string(ACME_DNS_SCHEMA_FN)?;
     let schemas: serde_json::Map<String, Value> = serde_json::from_str(&raw)?;
-- 
2.47.3



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