public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox{, -backup} v5 0/9] fix #6939: acme: support servers returning 204 for nonce requests
Date: Tue, 13 Jan 2026 14:48:52 +0100	[thread overview]
Message-ID: <1768311979.9j5ilxdh2w.astroid@yuna.none> (raw)
In-Reply-To: <20260108112629.189670-1-s.rufinatscha@proxmox.com>

On January 8, 2026 12:26 pm, 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].

sent some feedback for individual patches, one thing to explicitly test
is that existing accounts and DNS plugin configuration continue to work
after the switch over - AFAICT from the testing description below that
was not done (or not noted?).

> 
> ## Problem
> 
> During ACME account registration, PBS first fetches an anti-replay
> nonce by sending a HEAD request to the CA’s newNonce URL.
> RFC 8555 §7.2 [2] states that:
> 
> * the server MUST include a Replay-Nonce header with a fresh nonce,
> * the server SHOULD use status 200 OK for the HEAD request,
> * the server MUST also handle GET on the same resource and may return
>   204 No Content with an empty body.
> 
> The reporter observed the following error message:
> 
>   *ACME server responded with unexpected status code: 204*
> 
> and mentioned that the issue did not appear with PVE 9 [1]. Looking at
> PVE’s Perl ACME client [3], it uses a GET request instead of HEAD and
> accepts any 2xx success code when retrieving the nonce. This difference
> in behavior does not affect functionality but is worth noting for
> consistency across implementations.
> 
> ## Approach
> 
> To support ACME providers which return 204 No Content, the Rust ACME
> clients in proxmox-backup and proxmox need to treat both 200 OK and 204
> No Content as valid responses for the nonce request, as long as a
> Replay-Nonce header is present.
> 
> This series changes the expected field of the internal Request type
> from a single u16 to a list of allowed status codes
> (e.g. &'static [u16]), so one request can explicitly accept multiple
> success codes.
> 
> To avoid fixing the issue twice (once in PBS’ own ACME client and once
> in the shared Rust client), this series first refactors PBS to use the
> shared AcmeClient from proxmox-acme / proxmox-acme-api, similar to PDM,
> and then applies the bug fix in that shared implementation so that all
> consumers benefit from the more tolerant behavior.
> 
> ## Testing
> 
> *Testing the refactor*
> 
> To test the refactor, I
> (1) installed latest stable PBS on a VM
> (2) created .deb package from latest PBS (master), containing the
>  refactor
> (3) installed created .deb package
> (4) installed Pebble from Let's Encrypt [5] on the same VM
> (5) created an ACME account and ordered the new certificate for the
>  host domain.
> 
> Steps to reproduce:
> 
> (1) install latest stable PBS on a VM, create .deb package from latest
>  PBS (master) containing the refactor, install created .deb package
> (2) install Pebble from Let's Encrypt [5] on the same VM:
> 
>     cd
>     apt update
>     apt install -y golang git
>     git clone https://github.com/letsencrypt/pebble
>     cd pebble
>     go build ./cmd/pebble
> 
> then, download and trust the Pebble cert:
> 
>     wget https://raw.githubusercontent.com/letsencrypt/pebble/main/test/certs/pebble.minica.pem
>     cp pebble.minica.pem /usr/local/share/ca-certificates/pebble.minica.crt
>     update-ca-certificates
> 
> We want Pebble to perform HTTP-01 validation against port 80, because
> PBS’s standalone plugin will bind port 80. Set httpPort to 80.
> 
>     nano ./test/config/pebble-config.json
> 
> Start the Pebble server in the background:
> 
>     ./pebble -config ./test/config/pebble-config.json &
> 
> Create a Pebble ACME account:
> 
>     proxmox-backup-manager acme account register default admin@example.com --directory 'https://127.0.0.1:14000/dir'
> 
> To verify persistence of the account I checked
> 
>     ls /etc/proxmox-backup/acme/accounts
> 
> Verified if update-account works
> 
>     proxmox-backup-manager acme account update default --contact "a@example.com,b@example.com"
>     proxmox-backup-manager acme account info default
> 
> In the PBS GUI, you can create a new domain. You can use your host
> domain name (see /etc/hosts). Select the created account and order the
> certificate.
> 
> After a page reload, you might need to accept the new certificate in the browser.
> In the PBS dashboard, you should see the new Pebble certificate.
> 
> *Note: on reboot, the created Pebble ACME account will be gone and you
> will need to create a new one. Pebble does not persist account info.
> In that case remove the previously created account in
> /etc/proxmox-backup/acme/accounts.
> 
> *Testing the newNonce fix*
> 
> To prove the ACME newNonce fix, I put nginx in front of Pebble, to
> intercept the newNonce request in order to return 204 No Content
> instead of 200 OK, all other requests are unchanged and forwarded to
> Pebble. Requires trusting the nginx CAs via
> /usr/local/share/ca-certificates + update-ca-certificates on the VM.
> 
> Then I ran following command against nginx:
> 
> proxmox-backup-manager acme account register proxytest root@backup.local --directory 'https://nginx-address/dir
> 
> The account could be created successfully. When adjusting the nginx
> configuration to return any other non-expected success status code,
> PBS rejects as expected.
> 
> ## Patch summary
> 
> 0001 – [PATCH proxmox v5 1/4] acme: reduce visibility of Request type
>  Restricts the visibility of the low-level Request type. Consumers
>  should rely on proxmox-acme-api or AcmeClient handlers.
> 
> 0002– [PATCH proxmox v5 2/4] acme: introduce http_status module
> 
> 0003 – [PATCH proxmox v5 3/4] fix #6939: acme: support servers
> returning 204 for nonce requests
>  Adjusts nonce handling to support ACME servers that return HTTP 204
>  (No Content) for new-nonce requests.
> 
> 0004 – [PATCH proxmox v5 4/4] acme-api: add helper to load client for
> an account
>  Introduces a helper function to load an ACME client instance for a
>  given account. Required for the following PBS ACME refactor.
> 
> 0005 – [PATCH proxmox-backup v5 1/5] acme: clean up ACME-related imports
> 
> 0006 – [PATCH proxmox-backup v5 2/5] acme: include proxmox-acme-api
> dependency
>  Prepares the codebase to use the factored out ACME API impl.
> 
> 0007 – [PATCH proxmox-backup v5 3/5] acme: drop local AcmeClient
>  Removes the local AcmeClient implementation. Represents the minimal
>  set of changes to replace it with the factored out AcmeClient.
> 
> 0008 – [PATCH proxmox-backup v5 4/5] acme: change API impls to use
> proxmox-acme-api handlers
> 
> 0009 – [PATCH proxmox-backup v5 5/5] acme: certificate ordering through
> proxmox-acme-api
> 
> Thanks for considering this patch series, I look forward to your
> feedback.
> 
> Best,
> Samuel Rufinatscha
> 
> ## Changelog
> 
> Changes from v4 to v5:
> 
> * rebased series
> * re-ordered series (proxmox-acme fix first)
> * proxmox-backup: cleaned up imports based on an initial clean-up patch
> * proxmox-acme: removed now unused post_request_raw_payload(),
>   update_account_request(), deactivate_account_request()
> * proxmox-acme: removed now obsolete/unused get_authorization() and
>   GetAuthorization impl
> 
> Verified removal by compiling PBS, PDM, and proxmox-perl-rs
> with all features.
> 
> Changes from v3 to v4:
> 
> * add proxmox-acme-api as a dependency and initialize it in
>  PBS so PBS can use the shared ACME API instead.
> * remove the PBS-local AcmeClient implementation and switch PBS
>  over to the shared proxmox-acme async client.
> * rework PBS’ ACME API endpoints to delegate to
>  proxmox-acme-api handlers instead of duplicating logic locally.
> * move PBS’ ACME certificate ordering logic over to
>  proxmox-acme-api, keeping only certificate installation/reload in PBS.
> * add a load_client_with_account helper in proxmox-acme-api so PBS
>  (and others) can construct an AcmeClient for a configured account
>  without duplicating boilerplate.
> * hide the low-level Request type and its fields behind constructors
>  / reduced visibility so changes to “expected” no longer affect the
>  public API as they did in v3.
> * split out the HTTP status constants into an internal http_status
>  module as a separate preparatory cleanup before the bug fix, instead
>  of doing this inline like in v3.
> * Rebased on top of the refactor: keep the same behavioural fix as in
>  v3 accept 204 for newNonce with Replay-Nonce present), but implement
>  it on top of the http_status module that is part of the refactor.
> 
> Changes from v2 to v3:
> 
> * rename `http_success` module to `http_status`
> * replace `http_success` usage
> * introduced `http_success` module to contain the http success codes
> * replaced `Vec<u16>` with `&[u16]` for expected codes to avoid allocations.
> * clarified the PVEs Perl ACME client behaviour in the commit message.
> * integrated the `http_success` module, replacing `Vec<u16>` with `&[u16]`
> * clarified the PVEs Perl ACME client behaviour in the commit message.
> 
> [1] Bugzilla report #6939:
> [https://bugzilla.proxmox.com/show_bug.cgi?id=6939](https://bugzilla.proxmox.com/show_bug.cgi?id=6939)
> [2] RFC 8555 (ACME):
> [https://datatracker.ietf.org/doc/html/rfc8555/#section-7.2](https://datatracker.ietf.org/doc/html/rfc8555/#section-7.2)
> [3] PVE’s Perl ACME client (allow 2xx codes for nonce requests):
> [https://git.proxmox.com/?p=proxmox-acme.git;a=blob;f=src/PVE/ACME.pm;h=f1e9bb7d316e3cea1e376c610b0479119217aecc;hb=HEAD#l597](https://git.proxmox.com/?p=proxmox-acme.git;a=blob;f=src/PVE/ACME.pm;h=f1e9bb7d316e3cea1e376c610b0479119217aecc;hb=HEAD#l597)
> [4] Pebble ACME server:
> [https://github.com/letsencrypt/pebble](https://github.com/letsencrypt/pebble)
> [5] Pebble ACME server (perform GET request:
> [https://git.proxmox.com/?p=proxmox-acme.git;a=blob;f=src/PVE/ACME.pm;h=f1e9bb7d316e3cea1e376c610b0479119217aecc;hb=HEAD#l219](https://git.proxmox.com/?p=proxmox-acme.git;a=blob;f=src/PVE/ACME.pm;h=f1e9bb7d316e3cea1e376c610b0479119217aecc;hb=HEAD#l219)
> 
> proxmox:
> 
> Samuel Rufinatscha (4):
>   acme: reduce visibility of Request type
>   acme: introduce http_status module
>   fix #6939: acme: support servers returning 204 for nonce requests
>   acme-api: add helper to load client for an account
> 
>  proxmox-acme-api/src/account_api_impl.rs |   5 ++
>  proxmox-acme-api/src/lib.rs              |   3 +-
>  proxmox-acme/src/account.rs              | 102 ++---------------------
>  proxmox-acme/src/async_client.rs         |   8 +-
>  proxmox-acme/src/authorization.rs        |  30 -------
>  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, 44 insertions(+), 145 deletions(-)
> 
> 
> proxmox-backup:
> 
> Samuel Rufinatscha (5):
>   acme: clean up ACME-related imports
>   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                | 406 ++-------------
>  src/api2/node/certificates.rs          | 232 ++-------
>  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      |  14 +-
>  src/bin/proxmox-backup-proxy.rs        |  15 +-
>  src/bin/proxmox_backup_manager/acme.rs |  21 +-
>  src/config/acme/mod.rs                 |  55 +-
>  src/config/acme/plugin.rs              |  92 +---
>  src/config/node.rs                     |  31 +-
>  src/lib.rs                             |   2 -
>  16 files changed, 109 insertions(+), 1897 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
> 
> 
> Summary over all repositories:
>   25 files changed, 153 insertions(+), 2042 deletions(-)
> 
> -- 
> Generated by git-murpp 0.8.1
> 
> 
> _______________________________________________
> 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

      parent reply	other threads:[~2026-01-13 13:48 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-01-08 11:26 Samuel Rufinatscha
2026-01-08 11:26 ` [pbs-devel] [PATCH proxmox v5 1/4] acme: reduce visibility of Request type Samuel Rufinatscha
2026-01-13 13:46   ` Fabian Grünbichler
2026-01-08 11:26 ` [pbs-devel] [PATCH proxmox v5 2/4] acme: introduce http_status module Samuel Rufinatscha
2026-01-13 13:45   ` Fabian Grünbichler
2026-01-14 10:29     ` Samuel Rufinatscha
2026-01-08 11:26 ` [pbs-devel] [PATCH proxmox v5 3/4] fix #6939: acme: support servers returning 204 for nonce requests Samuel Rufinatscha
2026-01-08 11:26 ` [pbs-devel] [PATCH proxmox v5 4/4] acme-api: add helper to load client for an account Samuel Rufinatscha
2026-01-13 13:45   ` Fabian Grünbichler
2026-01-13 16:57     ` Samuel Rufinatscha
2026-01-08 11:26 ` [pbs-devel] [PATCH proxmox-backup v5 1/5] acme: clean up ACME-related imports Samuel Rufinatscha
2026-01-13 13:45   ` [pbs-devel] applied: " Fabian Grünbichler
2026-01-08 11:26 ` [pbs-devel] [PATCH proxmox-backup v5 2/5] acme: include proxmox-acme-api dependency Samuel Rufinatscha
2026-01-13 13:45   ` Fabian Grünbichler
2026-01-13 16:41     ` Samuel Rufinatscha
2026-01-08 11:26 ` [pbs-devel] [PATCH proxmox-backup v5 3/5] acme: drop local AcmeClient Samuel Rufinatscha
2026-01-13 13:45   ` Fabian Grünbichler
2026-01-14  8:56     ` Samuel Rufinatscha
2026-01-14  9:58       ` Fabian Grünbichler
2026-01-14 10:52         ` Samuel Rufinatscha
2026-01-08 11:26 ` [pbs-devel] [PATCH proxmox-backup v5 4/5] acme: change API impls to use proxmox-acme-api handlers Samuel Rufinatscha
2026-01-13 13:45   ` Fabian Grünbichler
2026-01-13 16:53     ` Samuel Rufinatscha
2026-01-08 11:26 ` [pbs-devel] [PATCH proxmox-backup v5 5/5] acme: certificate ordering through proxmox-acme-api Samuel Rufinatscha
2026-01-13 13:45   ` Fabian Grünbichler
2026-01-13 16:51     ` Samuel Rufinatscha
2026-01-13 13:48 ` Fabian Grünbichler [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1768311979.9j5ilxdh2w.astroid@yuna.none \
    --to=f.gruenbichler@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal