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

[Samuel Rufinatscha <s.rufinatscha@proxmox.com>]

On 12/9/25 5:51 PM, Max R. Carrara wrote:
> 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>
>

Thank you Max for the detailed review and for testing! It's great to
hear that this refactor behaves as expected - for you too.

I will make sure to polish the imports as suggested (thanks for
providing them). Also, I will re-order the patches so that PBS can
depend on the newly introduced proxmox-acme-api function.

Regarding the unused Account functions you mentioned in
[PATCH proxmox v4 2/4] acme: reduce visibility of Request type, I agree,
we could probably remove them, especially now that their visibility
changed (pub -> pub(crate)) - will do that!

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



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