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 BCD031FF145 for ; Thu, 05 Feb 2026 15:26:10 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AC23912B15; Thu, 5 Feb 2026 15:26:43 +0100 (CET) Content-Type: text/plain; charset=UTF-8 Date: Thu, 05 Feb 2026 15:26:08 +0100 Message-Id: To: "Shan Shaji" , Subject: Re: [PATCH datacenter-manager v2 3/4] fix #7179: cli: admin: expose acme commands From: "Lukas Wagner" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260203175101.457724-1-s.shaji@proxmox.com> <20260203175101.457724-4-s.shaji@proxmox.com> In-Reply-To: <20260203175101.457724-4-s.shaji@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770301491357 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.038 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 Message-ID-Hash: MXXEAF77TO5DZBG32T2L4G4BLSGSPIIG X-Message-ID-Hash: MXXEAF77TO5DZBG32T2L4G4BLSGSPIIG X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Hi Shan, thanks for this updated version of your patch series. Looking good from what I can, some comments inline. On Tue Feb 3, 2026 at 6:51 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 > --- > > changes since v1: > - Fix formatting.=20 > - Use API register_account method instead of the proxmox-acme-api > crate's register_account method to register ACME account.=20 > - add `read_input` helper function.=20 > > cli/admin/Cargo.toml | 4 +- > cli/admin/src/acme.rs | 445 ++++++++++++++++++++++++++++++++++++++++++ > cli/admin/src/main.rs | 6 + > 3 files changed, 454 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 =3D true > proxmox-rest-server.workspace =3D true > proxmox-sys.workspace =3D true > proxmox-daemon.workspace =3D true > - > +proxmox-acme.workspace =3D true > +proxmox-acme-api.workspace =3D true > +proxmox-base64.workspace =3D true > pdm-api-types.workspace =3D true > pdm-config.workspace =3D true > pdm-buildcfg.workspace =3D true > diff --git a/cli/admin/src/acme.rs b/cli/admin/src/acme.rs > new file mode 100644 > index 0000000..be442cd > --- /dev/null > +++ b/cli/admin/src/acme.rs > @@ -0,0 +1,445 @@ > +use std::io::Write; > + > +use anyhow::{bail, Error}; > +use serde_json::Value; > + > +use pdm_api_types::acme::AcmeRegistrationParams; > +use proxmox_acme::async_client::AcmeClient; > +use proxmox_acme_api::{completion::*, AcmeAccountName, DnsPluginCore, KN= OWN_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 =3D 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) -> Resul= t<(), Error> { > + let output_format =3D get_output_format(¶m); > + let info =3D &dc_api::config::acme::API_METHOD_LIST_ACCOUNTS; > + let mut data =3D match info.handler { > + ApiHandler::Sync(handler) =3D> (handler)(param, info, rpcenv)?, > + _ =3D> unreachable!(), > + }; > + > + let options =3D default_table_format_options(); > + format_and_print_result_full(&mut data, &info.returns, &output_forma= t, &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) -> R= esult<(), Error> { > + let output_format =3D get_output_format(¶m); > + > + let info =3D &dc_api::config::acme::API_METHOD_GET_ACCOUNT; > + let mut data =3D match info.handler { > + ApiHandler::Async(handler) =3D> (handler)(param, info, rpcenv).a= wait?, > + _ =3D> unreachable!(), > + }; > + > + let options =3D default_table_format_options() > + .column( > + ColumnConfig::new("account") > + .renderer(|value, _record| Ok(serde_json::to_string_pret= ty(value)?)), > + ) > + .column(ColumnConfig::new("directory")) > + .column(ColumnConfig::new("location")) > + .column(ColumnConfig::new("tos")); > + format_and_print_result_full(&mut data, &info.returns, &output_forma= t, &options); > + > + Ok(()) > +} > + > +fn read_input(prompt: &str) -> Result { > + print!("{}: ", prompt); > + std::io::stdout().flush()?; > + let mut input =3D String::new(); > + std::io::stdin().read_line(&mut input)?; > + Ok(input) > +} > + > +#[api( > + input: { > + properties: { > + name: { type: AcmeAccountName }, > + contact: { > + description: "List of email addresses.", > + type: String, > + }, > + directory: { > + type: String, > + description: "The ACME Directory.", > + optional: true, > + }, > + } > + } > +)] > +///Register an ACME account. > +async fn register_account( > + name: AcmeAccountName, > + contact: String, > + directory: Option, > + rpcenv: &mut dyn RpcEnvironment, > +) -> Result<(), Error> { > + let (directory_url, custom_directory) =3D match directory { > + Some(directory) =3D> (directory, true), > + None =3D> { > + 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 =3D 0; > + loop { > + let mut input =3D read_input("Enter selection")?; > + match input.trim().parse::() { > + Ok(n) if n < KNOWN_ACME_DIRECTORIES.len() =3D> { > + break (KNOWN_ACME_DIRECTORIES[n].url.to_string()= , false); > + } > + Ok(n) if n =3D=3D KNOWN_ACME_DIRECTORIES.len() =3D> = { > + input.clear(); > + input =3D read_input("Enter custom directory URI= ")?; > + break (input.trim().to_owned(), true); > + } > + _ =3D> eprintln!("Invalid selection."), > + } > + > + attempt +=3D 1; > + if attempt >=3D 3 { > + bail!("Aborting."); > + } > + } > + } > + }; > + > + println!("Attempting to fetch Terms of Service from {directory_url:?= }"); > + let mut client =3D AcmeClient::new(directory_url.clone()); > + let directory =3D client.directory().await?; > + let tos_agreed =3D if let Some(tos_url) =3D directory.terms_of_servi= ce_url() { > + println!("Terms of Service: {tos_url}"); > + let input =3D read_input("Do you agree to the above terms? [y|N]= ")?; > + input.trim().eq_ignore_ascii_case("y") > + } else { > + println!("No Terms of Service found, proceeding."); > + true > + }; > + > + let mut eab_enabled =3D directory.external_account_binding_required(= ); > + if !eab_enabled && custom_directory { > + let input =3D read_input("Do you want to use external account bi= nding? [y|N]")?; > + eab_enabled =3D input.trim().eq_ignore_ascii_case("y"); > + } else if eab_enabled { > + println!("The CA requires external account binding."); > + } > + > + let eab_creds =3D if eab_enabled { > + println!("You should have received a key id and a key from your = CA."); > + let eab_kid =3D read_input("Enter EAB key id")?; > + let eab_hmac_key =3D read_input("Enter EAB key")?; > + Some((eab_kid.trim().to_owned(), eab_hmac_key.trim().to_owned())= ) > + } else { > + None > + }; > + > + let tos_url =3D tos_agreed > + .then(|| directory.terms_of_service_url().map(str::to_owned)) > + .flatten(); > + > + let (eab_kid, eab_hmac_key) =3D eab_creds.unzip(); > + let parameters =3D AcmeRegistrationParams { > + name: Some(name), > + contact: contact, > + tos_url: tos_url, > + directory: Some(directory_url), > + eab_kid: eab_kid, > + eab_hmac_key: eab_hmac_key, > + }; > + let param =3D serde_json::to_value(parameters)?; > + > + let info =3D &dc_api::config::acme::API_METHOD_REGISTER_ACCOUNT; > + let result =3D match info.handler { > + ApiHandler::Sync(handler) =3D> (handler)(param, info, rpcenv)?, > + _ =3D> unreachable!(), > + }; > + > + wait_for_local_worker(result.as_str().unwrap()).await?; > + > + Ok(()) > +} Two things came to mind when actually trying out this CLI: - The directory selection is a bit odd to use, since it uses 0-based indexing, something like 0.) Let's Encrypt 1.) Let's Encrypt Staging .... 1-based indexing probably is a bit nicer for users here. - Also, I wonder if there should be non-interactive version of this command as well, one that can be scripted, one where all parameters that are asked as parameters can be provided as flags. People can always use the API for this, but maybe it would still be nice to offer this in the CLI tool as well. Just thinking out loud, I'm aware that you just copied the approach from PBS, if we add something like this, this should be done in a separate series. - Might be worth moving this 'wizard' implementation to some helper module in proxmox-acme-api and then use the same implementation from both PDM and PBS. To avoid scope creep this can always be done in a follow-up series. I guess in this case the appropriate place for the new AcmeRegistrationParams is then also proxmox-acme-api, since the this would be the type that is returned from such a shared helper. (sorry, some of these I could have noticed in my earlier, less thorough review) All of these can be done in followup-patches, it's just some ideas for improvement over the status quo that we already have in PBS. > + > +#[api( > + input: { > + properties: { > + name: { type: AcmeAccountName }, > + contact: { > + description: "List of email addresses.", > + type: String, > + optional: true, > + } > + } > + } > +)] > +/// Update an ACME Account. > +async fn update_account(param: Value, rpcenv: &mut dyn RpcEnvironment) -= > Result<(), Error> { > + let info =3D &dc_api::config::acme::API_METHOD_UPDATE_ACCOUNT; > + let result =3D match info.handler { > + ApiHandler::Sync(handler) =3D> (handler)(param, info, rpcenv)?, > + _ =3D> unreachable!(), > + }; > + > + wait_for_local_worker(result.as_str().unwrap()).await?; > + > + Ok(()) > +} > + [...] > + > +pub fn cert_cli() -> CommandLineInterface { > + let cmd_def =3D CliCommandMap::new() > + .insert("order", CliCommand::new(&API_METHOD_ORDER_ACME_CERT)) > + .insert("revoke", CliCommand::new(&API_METHOD_REVOKE_ACME_CERT))= ; > + > + cmd_def.into() > +} > diff --git a/cli/admin/src/main.rs b/cli/admin/src/main.rs > index 02148e3..3fd3c2f 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; > =20 > +mod acme; > mod remotes; > mod support_status; > =20 > @@ -22,7 +23,11 @@ async fn run() -> Result<(), Error> { > pdm_buildcfg::configdir!("/access"), > ) > .context("failed to setup access control config")?; > + proxmox_acme_api::init(pdm_buildcfg::configdir!("/acme"), false) > + .context("failed to initialize acme config")?; > + > proxmox_log::Logger::from_env("PDM_LOG", proxmox_log::LevelFilter::I= NFO) > + .tasklog() I'd use the 'old' tasklog_pbs function here for now, since then we can apply these patches without bumping proxmox-log first. Changing this to `tasklog` is trivial and can be done in a followup patch. We should be getting a deprecation warning when building PDM once proxmox-log has been bumped, so there should be little chance to forget it. > .stderr() > .init() > .context("failed to set-up logger")?; > @@ -30,6 +35,7 @@ async fn run() -> Result<(), Error> { > server::context::init().context("could not set-up server context")?; > =20 > let cmd_def =3D CliCommandMap::new() > + .insert("acme", acme::acme_mgmt_cli()) > .insert("remote", remotes::cli()) > .insert( > "report",