From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 89D1E1FF139 for ; Tue, 27 Jan 2026 12:14:18 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6D35A2041D; Tue, 27 Jan 2026 12:14:41 +0100 (CET) Mime-Version: 1.0 Date: Tue, 27 Jan 2026 12:14:37 +0100 Message-Id: From: "Shan Shaji" To: "Lukas Wagner" , "Proxmox Datacenter Manager development discussion" , "Gabriel Goller" X-Mailer: aerc 0.20.0 References: <20260123172910.244121-1-s.shaji@proxmox.com> <20260123172910.244121-3-s.shaji@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1769512412660 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.106 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pdm-devel] [PATCH datacenter-manager 2/3] fix #7179: cli: admin: expose acme commands X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" Hi Lukas, Thank you for your feedbacks. Some comments inline. On Tue Jan 27, 2026 at 10:17 AM CET, Lukas Wagner wrote: > Hi Shan, thanks for the patch! > > Some notes inline. > > On Fri Jan 23, 2026 at 6:29 PM CET, Shan Shaji wrote: >> Previously, ACME commands were not exposed through the admin CLI. >> Added the necessary functionality to manage ACME settings directly >> via the command line. >> >> Signed-off-by: Shan Shaji >> --- >> cli/admin/Cargo.toml | 4 +- >> cli/admin/src/acme.rs | 442 ++++++++++++++++++++++++++++++++++++++++++ >> cli/admin/src/main.rs | 5 + >> 3 files changed, 450 insertions(+), 1 deletion(-) >> create mode 100644 cli/admin/src/acme.rs >> >> diff --git a/cli/admin/Cargo.toml b/cli/admin/Cargo.toml >> index e566b39..01afc88 100644 >> --- a/cli/admin/Cargo.toml >> +++ b/cli/admin/Cargo.toml >> @@ -22,7 +22,9 @@ proxmox-access-control.workspace = true >> proxmox-rest-server.workspace = true >> proxmox-sys.workspace = true >> proxmox-daemon.workspace = true >> - >> +proxmox-acme.workspace = true >> +proxmox-acme-api.workspace = true >> +proxmox-base64.workspace = true >> pdm-api-types.workspace = true >> pdm-config.workspace = true >> pdm-buildcfg.workspace = true >> diff --git a/cli/admin/src/acme.rs b/cli/admin/src/acme.rs >> new file mode 100644 >> index 0000000..81c63f9 >> --- /dev/null >> +++ b/cli/admin/src/acme.rs >> @@ -0,0 +1,442 @@ >> +use std::io::Write; >> + >> +use anyhow::{bail, Error}; >> +use serde_json::Value; >> + >> +use proxmox_acme::async_client::AcmeClient; >> +use proxmox_acme_api::{completion::*, AcmeAccountName, DnsPluginCore, KNOWN_ACME_DIRECTORIES}; >> +use proxmox_rest_server::wait_for_local_worker; >> +use proxmox_router::{cli::*, ApiHandler, RpcEnvironment}; >> +use proxmox_schema::api; >> +use proxmox_sys::fs::file_get_contents; >> + >> +use server::api as dc_api; >> + >> +pub fn acme_mgmt_cli() -> CommandLineInterface { >> + let cmd_def = CliCommandMap::new() >> + .insert("account", account_cli()) >> + .insert("plugin", plugin_cli()) >> + .insert("certificate", cert_cli()); >> + >> + cmd_def.into() >> +} >> + >> +#[api( >> + input: { >> + properties: { >> + "output-format": { >> + schema: OUTPUT_FORMAT, >> + optional: true, >> + } >> + } >> + } >> +)] >> +/// List ACME accounts. >> +fn list_accounts(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> { >> + let output_format = get_output_format(¶m); >> + let info = &dc_api::config::acme::API_METHOD_LIST_ACCOUNTS; >> + let mut data = match info.handler { >> + ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?, >> + _ => unreachable!(), >> + }; >> + >> + let options = default_table_format_options(); >> + format_and_print_result_full(&mut data, &info.returns, &output_format, &options); >> + >> + Ok(()) >> +} >> + >> +#[api( >> + input: { >> + properties: { >> + name: { type: AcmeAccountName }, >> + "output-format": { >> + schema: OUTPUT_FORMAT, >> + optional: true, >> + }, >> + } >> + } >> +)] >> +/// Show ACME account information. >> +async fn get_account(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> { >> + let output_format = get_output_format(¶m); >> + >> + let info = &dc_api::config::acme::API_METHOD_GET_ACCOUNT; >> + let mut data = match info.handler { >> + ApiHandler::Async(handler) => (handler)(param, info, rpcenv).await?, >> + _ => unreachable!(), >> + }; >> + >> + let options = default_table_format_options() >> + .column( >> + ColumnConfig::new("account") >> + .renderer(|value, _record| Ok(serde_json::to_string_pretty(value)?)), >> + ) >> + .column(ColumnConfig::new("directory")) >> + .column(ColumnConfig::new("location")) >> + .column(ColumnConfig::new("tos")); >> + format_and_print_result_full(&mut data, &info.returns, &output_format, &options); >> + >> + Ok(()) >> +} >> + >> +#[api( >> + input: { >> + properties: { >> + name: { type: AcmeAccountName }, >> + contact: { >> + description: "List of email addresses.", >> + }, >> + directory: { >> + type: String, >> + description: "The ACME Directory.", >> + optional: true, }, > > Formatting is a bit off here :) Will fix it in the next one. >> + } >> + } >> +)] >> +///Register an ACME account. >> +async fn register_account( >> + name: AcmeAccountName, >> + contact: String, >> + directory: Option, >> +) -> Result<(), Error> { >> + let (directory_url, custom_directory) = match directory { >> + Some(directory) => (directory, true), >> + None => { >> + println!("Directory endpoints:"); >> + for (i, dir) in KNOWN_ACME_DIRECTORIES.iter().enumerate() { >> + println!("{}) {}", i, dir.url); >> + } >> + >> + println!("{}) Custom", KNOWN_ACME_DIRECTORIES.len()); >> + let mut attempt = 0; >> + loop { >> + print!("Enter selection: "); >> + std::io::stdout().flush()?; >> + >> + let mut input = String::new(); >> + std::io::stdin().read_line(&mut input)?; > > I know most of this function was copied from PBS, but maybe it would > make sense to have a > > fn read_input(prompt: &str) -> Result > > and use it for all those prompts here? > > I think this could make this function much more readable. I will seperate it to another function. Thank you! >> + >> + match input.trim().parse::() { >> + Ok(n) if n < KNOWN_ACME_DIRECTORIES.len() => { >> + break (KNOWN_ACME_DIRECTORIES[n].url.to_string(), false); >> + } >> + Ok(n) if n == KNOWN_ACME_DIRECTORIES.len() => { >> + input.clear(); >> + print!("Enter custom directory URI: "); >> + std::io::stdout().flush()?; >> + std::io::stdin().read_line(&mut input)?; >> + break (input.trim().to_owned(), true); >> + } >> + _ => eprintln!("Invalid selection."), >> + } >> + >> + attempt += 1; >> + if attempt >= 3 { >> + bail!("Aborting."); >> + } >> + } >> + } >> + }; >> + >> + println!("Attempting to fetch Terms of Service from {directory_url:?}"); >> + let mut client = AcmeClient::new(directory_url.clone()); >> + let directory = client.directory().await?; >> + let tos_agreed = if let Some(tos_url) = directory.terms_of_service_url() { >> + println!("Terms of Service: {tos_url}"); >> + print!("Do you agree to the above terms? [y|N]: "); >> + std::io::stdout().flush()?; >> + let mut input = String::new(); >> + std::io::stdin().read_line(&mut input)?; >> + input.trim().eq_ignore_ascii_case("y") >> + } else { >> + println!("No Terms of Service found, proceeding."); >> + true >> + }; >> + >> + let mut eab_enabled = directory.external_account_binding_required(); >> + if !eab_enabled && custom_directory { >> + print!("Do you want to use external account binding? [y|N]: "); >> + std::io::stdout().flush()?; >> + let mut input = String::new(); >> + std::io::stdin().read_line(&mut input)?; >> + eab_enabled = input.trim().eq_ignore_ascii_case("y"); >> + } else if eab_enabled { >> + println!("The CA requires external account binding."); >> + } >> + >> + let eab_creds = if eab_enabled { >> + println!("You should have received a key id and a key from your CA."); >> + >> + print!("Enter EAB key id: "); >> + std::io::stdout().flush()?; >> + let mut eab_kid = String::new(); >> + std::io::stdin().read_line(&mut eab_kid)?; >> + >> + print!("Enter EAB key: "); >> + std::io::stdout().flush()?; >> + let mut eab_hmac_key = String::new(); >> + std::io::stdin().read_line(&mut eab_hmac_key)?; >> + >> + Some((eab_kid.trim().to_owned(), eab_hmac_key.trim().to_owned())) >> + } else { >> + None >> + }; >> + >> + let tos_url = tos_agreed >> + .then(|| directory.terms_of_service_url().map(str::to_owned)) >> + .flatten(); >> + >> + println!("Registering ACME account '{}'...", &name,); > > No need to create a reference here. Also, name could be inlined into the > format string. Also, there is a trailing comma here, which is a bit odd. > > >> + let location = >> + proxmox_acme_api::register_account(&name, contact, tos_url, Some(directory_url), eab_creds) >> + .await?; > > I wonder if it would make sense to actually call the API handler here, > same as for the other commands? This would entail running registration > in a worker task, which would mean that it is visible in the task > history together with a task log. But maybe I'm overlooking something here, > I saw that PBS does this in the same way as here, and maybe there is a > good reason why the registration is not done in a worker. I also had that smilliar confusion here, anyways i will try using the API handler here and see if i am getting any issues. >> + println!("Registration successful, account URL: {}", location); >> + >> + Ok(()) >> +} >> + > > [...] > >> diff --git a/cli/admin/src/main.rs b/cli/admin/src/main.rs >> index bcaa0a6..15114c9 100644 >> --- a/cli/admin/src/main.rs >> +++ b/cli/admin/src/main.rs >> @@ -9,6 +9,7 @@ use proxmox_router::RpcEnvironment; >> use proxmox_schema::api; >> use proxmox_sys::fs::CreateOptions; >> >> +mod acme; >> mod remotes; >> mod support_status; >> >> @@ -21,13 +22,17 @@ async fn run() -> Result<(), Error> { >> &pdm_api_types::AccessControlConfig, >> pdm_buildcfg::configdir!("/access"), >> )?; >> + proxmox_acme_api::init(pdm_buildcfg::configdir!("/acme"), false)?; >> + >> proxmox_log::Logger::from_env("PDM_LOG", proxmox_log::LevelFilter::INFO) >> + .stderr_on_no_workertask() >> .stderr() >> .init()?; > > Doing this actually prints all messages logged outside a workertask > *twice*, since this adds two subscribers that will print to stdout. > > I think it would make more sense to: > > proxmox_log::Logger::from_env("PDM_LOG", proxmox_log::LevelFilter::INFO) > .tasklog_pbs() > .stderr() > .init()?; > > ... which should print *all* messages to stderr and messages from within > a task log will also be stored in the task log. or could we keep the `stderr_on_no_workertask()` and remove the `stderr`? > Maybe also ask Gabriel about these, he did a lot of work on logging. Will ask him about it. Thank You! > @Gabriel, would my proposal make sense here? > >> >> server::context::init()?; >> >> let cmd_def = CliCommandMap::new() >> + .insert("acme", acme::acme_mgmt_cli()) >> .insert("remote", remotes::cli()) >> .insert( >> "report", _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel