Re: [PATCH datacenter-manager v2 3/4] fix #7179: cli: admin: expose acme commands

["Lukas Wagner" <l.wagner@proxmox.com>]

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 <s.shaji@proxmox.com>
> ---
>
> changes since v1:
>  - Fix formatting. 
>  - Use API register_account method instead of the proxmox-acme-api
>    crate's register_account method to register ACME account. 
> -  add `read_input` helper function. 
>
>  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 = 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..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, 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(&param);
> +    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(&param);
> +
> +    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(())
> +}
> +
> +fn read_input(prompt: &str) -> Result<String, Error> {
> +    print!("{}: ", prompt);
> +    std::io::stdout().flush()?;
> +    let mut input = 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<String>,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> 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 {
> +                let mut input = read_input("Enter selection")?;
> +                match input.trim().parse::<usize>() {
> +                    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();
> +                        input = read_input("Enter custom directory URI")?;
> +                        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}");
> +        let input = 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 = directory.external_account_binding_required();
> +    if !eab_enabled && custom_directory {
> +        let input = read_input("Do you want to use external account binding? [y|N]")?;
> +        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.");
> +        let eab_kid = read_input("Enter EAB key id")?;
> +        let eab_hmac_key = read_input("Enter EAB 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();
> +
> +    let (eab_kid, eab_hmac_key) = eab_creds.unzip();
> +    let parameters = 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 = serde_json::to_value(parameters)?;
> +
> +    let info = &dc_api::config::acme::API_METHOD_REGISTER_ACCOUNT;
> +    let result = match info.handler {
> +        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
> +        _ => 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 = &dc_api::config::acme::API_METHOD_UPDATE_ACCOUNT;
> +    let result = match info.handler {
> +        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
> +        _ => unreachable!(),
> +    };
> +
> +    wait_for_local_worker(result.as_str().unwrap()).await?;
> +
> +    Ok(())
> +}
> +

[...]

> +
> +pub fn cert_cli() -> CommandLineInterface {
> +    let cmd_def = 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;
>  
> +mod acme;
>  mod remotes;
>  mod support_status;
>  
> @@ -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::INFO)
> +        .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")?;
>  
>      let cmd_def = CliCommandMap::new()
> +        .insert("acme", acme::acme_mgmt_cli())
>          .insert("remote", remotes::cli())
>          .insert(
>              "report",