Re: [pbs-devel] [PATCH proxmox{-backup, } v4 0/8] fix #6939: acme: support servers returning 204 for nonce requests

["Max R. Carrara" <m.carrara@proxmox.com>]

On Wed Dec 3, 2025 at 11:22 AM CET, Samuel Rufinatscha wrote:
> Hi,
>
> this series fixes account registration for ACME providers that return
> HTTP 204 No Content to the newNonce request. Currently, both the PBS
> ACME client and the shared ACME client in proxmox-acme only accept
> HTTP 200 OK for this request. The issue was observed in PBS against a
> custom ACME deployment and reported as bug #6939 [1].
>
> [...]

Testing
-------

Tested this on my local PBS development instance with the DNS-01
challenge using one of my domains on OVH and Let's Encrypt Staging.

The cert was ordered without any problems. Everything worked just as
before.

Comments Regarding the Changes Made
-----------------------------------

Overall, looks pretty good! I only found a few minor things, see my
comments inline. 

What I would recommend overall is to make the changes in `proxmox`
first, and then use the new `async fn` you introduced in patch #4
(proxmox) in `proxmox-backup` instead of doing things the other way
around. That way you could perhaps use the function you introduced,
since I'm assuming you added it for good reason.

Conclusion
----------

LGTM—needs a teeny tiny bit more polish (see comments inline), but
otherwise works great already! :D Good to see a lot of redundant code
being removed.

The few things I mentioned inline aren't *strict* blockers IMO and can
maybe be addressed in a couple follow-up patches, if this gets merged as
is. Otherwise, should you release a v5 of this series, I'll do another
review.

Anyhow, should the maintainer decide to merge this series, please
consider:

Reviewed-by: Max R. Carrara <m.carrara@proxmox.com>
Tested-by: Max R. Carrara <m.carrara@proxmox.com>

>
> proxmox-backup:
>
> Samuel Rufinatscha (4):
>   acme: include proxmox-acme-api dependency
>   acme: drop local AcmeClient
>   acme: change API impls to use proxmox-acme-api handlers
>   acme: certificate ordering through proxmox-acme-api
>
>  Cargo.toml                             |   3 +
>  src/acme/client.rs                     | 691 -------------------------
>  src/acme/mod.rs                        |   5 -
>  src/acme/plugin.rs                     | 336 ------------
>  src/api2/config/acme.rs                | 407 ++-------------
>  src/api2/node/certificates.rs          | 240 ++-------
>  src/api2/types/acme.rs                 |  98 ----
>  src/api2/types/mod.rs                  |   3 -
>  src/bin/proxmox-backup-api.rs          |   2 +
>  src/bin/proxmox-backup-manager.rs      |   2 +
>  src/bin/proxmox-backup-proxy.rs        |   1 +
>  src/bin/proxmox_backup_manager/acme.rs |  21 +-
>  src/config/acme/mod.rs                 |  51 +-
>  src/config/acme/plugin.rs              |  99 +---
>  src/config/node.rs                     |  29 +-
>  src/lib.rs                             |   2 -
>  16 files changed, 103 insertions(+), 1887 deletions(-)
>  delete mode 100644 src/acme/client.rs
>  delete mode 100644 src/acme/mod.rs
>  delete mode 100644 src/acme/plugin.rs
>  delete mode 100644 src/api2/types/acme.rs
>
>
> proxmox:
>
> Samuel Rufinatscha (4):
>   acme-api: add helper to load client for an account
>   acme: reduce visibility of Request type
>   acme: introduce http_status module
>   fix #6939: acme: support servers returning 204 for nonce requests
>
>  proxmox-acme-api/src/account_api_impl.rs |  5 +++++
>  proxmox-acme-api/src/lib.rs              |  3 ++-
>  proxmox-acme/src/account.rs              | 27 +++++++++++++-----------
>  proxmox-acme/src/async_client.rs         |  8 +++----
>  proxmox-acme/src/authorization.rs        |  2 +-
>  proxmox-acme/src/client.rs               |  8 +++----
>  proxmox-acme/src/lib.rs                  |  6 ++----
>  proxmox-acme/src/order.rs                |  2 +-
>  proxmox-acme/src/request.rs              | 25 +++++++++++++++-------
>  9 files changed, 51 insertions(+), 35 deletions(-)
>
>
> Summary over all repositories:
>   25 files changed, 154 insertions(+), 1922 deletions(-)



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel