Re: [pdm-devel] [PATCH datacenter-manager 2/3] fix #7179: cli: admin: expose acme commands

["Shan Shaji" <s.shaji@proxmox.com>]

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 <s.shaji@proxmox.com>
>> ---
>>  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(&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(())
>> +}
>> +
>> +#[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<String>,
>> +) -> 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<String, Error>
>
> 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::<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();
>> +                        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