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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.


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