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

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

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

## 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 is worth noting.

## Approach

This series changes the expected field of the internal Request type
from a single u16 to &'static [u16], so one request can explicitly
accept multiple success codes.

To avoid fixing the issue in PBS and in PDM (which uses the shared ACME stack),
this series fixes the bug in
proxmox-acme and then refactors PBS to use the shared ACME stack too.

## Testing

I tested the refactor using Pebble HTTP challenge type.
The DNS challange type will be tested as mentioned by Max (see v5).

*HTTP Challenge Type Test*

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 test 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 - proxmox: acme-api: add ACME completion helpers
0002 – proxmox: acme: introduce http_status module
0003 – proxmox: fix #6939: acme: support servers
returning 204 for nonce requests
0004 – proxmox-backup: acme: remove local AcmeClient and use
proxmox-acme-api handlers
0005 – proxmox-backup: acme: remove unused src/acme and plugin code

## Maintainer notes

proxmox-acme: requires version bump (breaking Request::expected change)
proxmox-backup: requires version bump
- NodeConfig::acme_config() signature changed from
Option<Result<AcmeConfig, Error>> to Result<AcmeConfig, Error>
- NodeConfig::acme_client() function removed

0001 - proxmox: acme-api: add ACME completion helpers could be applied
as an independent patch to make sure https://bugzilla.proxmox.com/show_bug.cgi?id=7179
is not blocked / avoid duplicate work

## Changelog

Changes from v5 to v6:

* rebased
* proxmox-acme: revert visibility changes and dead-code removal
* proxmox-acme-api: remove load_client_with_account
* proxmox-backup: remove pub Node::acme_client()
* proxmox-backup: Node::acme_config() inline transpose/default logic
* proxmox-backup: merge PBS Client removal and API handler changes in
one patch
* improve commit messages

Changes from v4 to v5:

* rebased
* 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 (3):
  acme-api: add ACME completion helpers
  acme: introduce http_status module
  fix #6939: acme: support servers returning 204 for nonce requests

 proxmox-acme-api/src/challenge_schemas.rs |  2 +-
 proxmox-acme-api/src/lib.rs               | 57 +++++++++++++++++++++++
 proxmox-acme/src/account.rs               | 10 ++--
 proxmox-acme/src/async_client.rs          |  6 +--
 proxmox-acme/src/client.rs                |  2 +-
 proxmox-acme/src/lib.rs                   |  2 +
 proxmox-acme/src/request.rs               | 15 ++++--
 7 files changed, 81 insertions(+), 13 deletions(-)


proxmox-backup:

Samuel Rufinatscha (2):
  acme: remove local AcmeClient and use proxmox-acme-api handlers
  acme: remove unused src/acme and plugin code

 Cargo.toml                             |   3 +
 src/acme/client.rs                     | 691 -------------------------
 src/acme/mod.rs                        |   5 -
 src/acme/plugin.rs                     | 335 ------------
 src/api2/config/acme.rs                | 399 ++------------
 src/api2/node/certificates.rs          | 221 +-------
 src/api2/types/acme.rs                 |  97 ----
 src/api2/types/mod.rs                  |   3 -
 src/bin/proxmox-backup-api.rs          |   2 +
 src/bin/proxmox-backup-manager.rs      |   3 +-
 src/bin/proxmox-backup-proxy.rs        |   1 +
 src/bin/proxmox_backup_manager/acme.rs |  37 +-
 src/config/acme/mod.rs                 | 168 ------
 src/config/acme/plugin.rs              | 189 -------
 src/config/mod.rs                      |   1 -
 src/config/node.rs                     |  43 +-
 src/lib.rs                             |   2 -
 17 files changed, 94 insertions(+), 2106 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
 delete mode 100644 src/config/acme/mod.rs
 delete mode 100644 src/config/acme/plugin.rs


Summary over all repositories:
  24 files changed, 175 insertions(+), 2119 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