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

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

On 11/4/25 3:11 PM, Thomas Lamprecht wrote:
> Am 03.11.25 um 11:13 schrieb Samuel Rufinatscha:
>> 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 not illegal. 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.
>>
>> Note: In comparison, PVE’s Perl ACME client performs a GET request [3]
>> instead of a HEAD request and accepts any 2xx success code when
>> retrieving the nonce [4]. This difference in behavior does not affect
>> functionality but is worth noting for consistency across
>> implementations.
> 
> The resulting code now looks mostly OK, but when doing a final review
> before pushing this out I noticed two things that I should have actually
> caught earlier:
> 
> 1. Introducing + using the new http_status module should be a separate
>     patch upfront, it has nothing to do with the actual bug fix but is
>     rather an independent cleanup.
> 
> 2. The Request struct and it's expected member are pub and is still
>     used directly in PBS - where it was originally factored out from
>     but not yet replaced. So the type change from `u16` to `&'static [u16]`
>     causes a breaking ABI change. That change itself can be manageable,
>     albeit if it can be easily avoided it's always nicer to do so; using
>     a constructor (potentially with builder pattern) and changing the
>     visibility of the members to pub(crate) or even making them private
>     to the module, can often be a good option to reduce friction for
>     any future change.
>     But here it would be nicer to clear the tech debt and switch PBS fully
>     over to the factored out impl, like e.g. PDM uses already. This should
>     then also allow us to reduce the visibility of the struct members and
>     the http_status module, which as of now I'd also rather see as
>     proxmox-acme specific and thus not exposing it to the public would be
>     better.
> 
> Do you want to give switching PBS over to the factored-out impl a try?
> It adds a bit to the scope, but we have to clean this up (or accumulate
> tech debt interest) sooner or later anyway, and if we do already a breaking
> change I'd prefer sooner.
> For the acme side of your changes nothing should change – besides maybe
> already reducing visibility of structs and/or their members in a separate
> patch.
> 
> In summary, a good order/split for the resulting patches could look
> something like:
> 
> 1. change PBS over to proxmox-acme, at least the client part.
> 2. reduce the visibility of types that now are only used in proxmox-acme
>     internally
> 3. introduce and use http_status mod in proxmox-acme
> 4. fix #6939
> 
> What do you think? If moving PBS to proxmox-acme turns out to be tricky,
> or even more work than expected, we can also go this route here as stop
> gap; but IMO it would be favorable to spend a bit more time in actually
> cleaning this up and reduce code duplication than doing so.
> Again, I should have caught that early, but FWIW, almost all of the work
> you done so far will still be relevant, so no real harm done FWICT.

Hi Thomas,

thanks a lot for having a second look and the detailed feedback -
sounds good to me! I agree that it would be better to split off the
fix-independent changes, and it would be a good opportunity now to
switch PBS over to the factored-out proxmox-acme implementation (get
at least rid of the duplicate client impl), and follow PDM. I think
this is manageable and will start by switching PBS to the shared
client, adjust visibility and add the http_status module before
applying the fix.

I’ll go ahead with the refactoring and will send a v4 with the
suggested patch structure.

Samuel


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