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

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

Hi Lukas, thank you so much for the nice improvement feedbacks. 

Some comments inline.

On Thu Feb 5, 2026 at 3:26 PM CET, Lukas Wagner wrote:
> 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>
>> ---
>>

[...]

>> +///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.

I will add this in v3. Thanks!

>   - 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.

This is a nice improvement. We already have an option to provide custom
directory endpoint using --directory option. I will add additional options
to accept the TOS selection and EAB credentials.  

Do we need to provide a flag to choose the Lets encrypt endpoint  
as we already have a --directory option?

>   - 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.

Thanks! I will move the  wizard implementation to the proxmox-acme-api
crate and use the same on both PBS and PDM. Will send another series
once this series is applied. 

>> +
>> +#[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.
>

Will update it to `tasklog_pbs`. Thank You!

>>          .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",