Re: [pbs-devel] [PATCH proxmox 1/1] fix #6939: acme: support servers returning 204 for nonce requests

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

On 10/29/25 11:35 AM, Wolfgang Bumiller wrote:
> On Wed, Oct 29, 2025 at 08:53:34AM +0100, Thomas Lamprecht wrote:
>> Am 29.10.25 um 08:23 schrieb Christian Ebner:
>>> Hi, thanks for the patches!
>>>
>>> comments inline
>>>
>>> On 10/28/25 8:34 PM, Samuel Rufinatscha wrote:
>>>> Some ACME servers (notably custom or legacy implementations) respond
>>>> to HEAD /newNonce with a 204 No Content instead of the
>>>> RFC 8555-recommended 200 OK [1]. While this behavior is technically
>>>> off-spec, it is functionally harmless. This issue was reported on our
>>>> bug tracker [2].
>>>>
>>>> The previous implementation treated any non-200 response as an error,
>>>> causing account registration to fail against such servers. Relax the
>>>> status-code check to accept both 200 and 204 responses (and potentially
>>>> support other 2xx codes) to improve interoperability.
>>>>
>>>> This aligns behavior with PVE’s more tolerant Perl ACME client and
>>>> avoids regressions.
>>>>
>>>> [1] https://datatracker.ietf.org/doc/html/rfc8555/#section-7.2
>>>> [2] https://bugzilla.proxmox.com/show_bug.cgi?id=6939
>>>>
>>>> Fixes: #6939
>>>> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
>>>> ---
>>>>    proxmox-acme/src/account.rs      | 10 +++++-----
>>>>    proxmox-acme/src/async_client.rs |  6 +++---
>>>>    proxmox-acme/src/client.rs       |  2 +-
>>>>    proxmox-acme/src/request.rs      |  4 ++--
>>>>    4 files changed, 11 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/proxmox-acme/src/account.rs b/proxmox-acme/src/account.rs
>>>> index 73d786b8..60719865 100644
>>>> --- a/proxmox-acme/src/account.rs
>>>> +++ b/proxmox-acme/src/account.rs
>>>> @@ -85,7 +85,7 @@ impl Account {
>>>>                method: "POST",
>>>>                content_type: crate::request::JSON_CONTENT_TYPE,
>>>>                body,
>>>> -            expected: crate::request::CREATED,
>>>> +            expected: vec![crate::request::CREATED],
>>>
>>> while this is defined as dedicated constant...
>>>
>>>>            };
>>>>              Ok(NewOrder::new(request))
>>>> @@ -107,7 +107,7 @@ impl Account {
>>>>                method: "POST",
>>>>                content_type: crate::request::JSON_CONTENT_TYPE,
>>>>                body,
>>>> -            expected: 200,
>>>> +            expected: vec![200],
>>>
>>> ... these and the others below are not. Same for the 204 status code you are about to add.
>>>
>>> So in preparation for adding the new status code, these should probably be defined as, either:
>>> - as dedicated status code constants as well, or
>>> - all moved over to directly use https://docs.rs/http/1.3.1/http/status/struct.StatusCode.html
>>>
>>> I feel like the latter is not done here intentionally to avoid the dependency on hyper or http (re-exported by hyper) for the api types only.
>>
>> While you are right that constants are generally nicer, IMO HTTP codes are
>> very stable and universal to be fine to be used directly as numbers in the few
>> limited instances here.
> 
> Mostly this, but we can also just add internal constants as well. 200
> just seemed common enough...
>>
>> If we already (even just transitively) would get them from a dependency we still
>> should switch to that, but I'd not introduce a new dependency just for that; IMO
>> to high of a cost.

First thanks for the review Christian, Thomas, Wolfgang - agree!

I checked the option of using `StatusCode`, but as you mentioned, that 
would require adding the `http` or `hyper` dependency, which we 
currently don’t include in the core types. I would therefore drop the 
constant as suggested and introduced a small, dedicated internal module 
to hold the ACME HTTP success constants. This would keep them together 
and would make imports handy.


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