From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 146E81FF187 for ; Tue, 2 Dec 2025 16:57:25 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2E86016C0D; Tue, 2 Dec 2025 16:57:46 +0100 (CET) From: Samuel Rufinatscha To: pbs-devel@lists.proxmox.com Date: Tue, 2 Dec 2025 16:56:54 +0100 Message-ID: <20251202155659.379848-4-s.rufinatscha@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20251202155659.379848-1-s.rufinatscha@proxmox.com> References: <20251202155659.379848-1-s.rufinatscha@proxmox.com> MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1764690986482 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.295 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] [PATCH proxmox-backup 3/4] acme: change API impls to use proxmox-acme-api handlers X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 --- 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, 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, + 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 { - 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 { - 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) -> Result, 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) -> Result, 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>, -} - -impl Serialize for ChallengeSchemaWrapper { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - self.inner.serialize(serializer) - } -} - -struct CachedSchema { - schema: Arc>, - cached_mtime: SystemTime, -} - -fn get_cached_challenge_schemas() -> Result { - static CACHE: LazyLock>> = 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 { )] /// Get named known ACME directory endpoints. fn get_challenge_schema() -> Result { - 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, - - /// Plugin configuration data. - #[serde(skip_serializing_if = "Option::is_none", default)] - data: Option, - - /// 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, - - /// Flag to disable the config. - #[serde(skip_serializing_if = "Option::is_none", default)] - disable: Option, -} - -// 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, 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 )] /// List ACME challenge plugins. pub fn get_plugin(id: String, rpcenv: &mut dyn RpcEnvironment) -> Result { - 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 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, - delete: Option>, - digest: Option, + delete: Option>, + digest: Option, ) -> 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::() { 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(mut func: F) -> Result<(), Error> where F: FnMut(AcmeAccountName) -> ControlFlow>, @@ -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, Error> { let raw = file_read_string(ACME_DNS_SCHEMA_FN)?; let schemas: serde_json::Map = 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