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

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

[Samuel Rufinatscha]

Hi,

this series proposes a change to ACME account registration in Proxmox
Backup Server (PBS), so that it also works with ACME servers that return
HTTP 204 No Content to the HEAD request for newNonce.

This behaviour was observed against a specific ACME deployment and
reported as bug #6939 [1]. Currently, PBS cannot register an ACME
account for this CA.

## 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]
says:

* 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 with status 204 No
  Content and an empty body [2].

Currently, our Rust ACME clients only accept 200 OK. PBS inherits that
strictness and aborts with:

*ACME server responded with unexpected status code: 204*

The author mentions, the issue did not appear with PVE9 [1].
After looking into PVE’s Perl ACME client [3] it appears it uses a GET
request instead of a HEAD request and accepts any 2xx success code
when retrieving the nonce [5]. This difference in behavior does not
affect functionality but is worth noting for consistency across
implementations.

## Ideas to solve the problem

To support ACME providers which return 204 No Content, the underlying
ACME clients need to tolerate both 200 OK and 204 No Content as valid
responses for the nonce HEAD request, as long as the Replay-Nonce is
provided.

I considered following solutions:

1. Change the `expected` field of the `AcmeRequest` type from `u16` to
   `Vec<u16>`, to support multiple success codes

2. Keep `expected: u16` and add a second field e.g. `expected_other:
   Vec<u16>` for "also allowed" codes.

3. Support any 2xx success codes, and remove the `expected` check

I thought (1) might be reasonable, because:

* It stays explicit and makes it clear which statuses are considered
  success.
* We don’t create two parallel concepts ("expected" vs
  "expected_other") which introduces additional complexity
* Can be extend later if we meet yet another harmless but not 200
  variant.
* We don’t allow arbitrary 2xx.

What do you think? Do you maybe have any other solution in mind that
would fit better?

## Testing

To prove the proposed fix, I reproduced the scenario:

Pebble (release 2.8.0) from Let's Encrypt [5] running on a Debian 9 VM
as the ACME server. 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. Trust the
Pebble and ngix CAs via `/usr/local/share/ca-certificates` +
`update-ca-certificates` on the PBS VM.

Then I ran following command against nginx:

```
proxmox-backup-manager acme account register proxytest root@backup.local
--directory 'https://nginx-address/dir

Attempting to fetch Terms of
Service from "https://acme-vm/dir"
Terms of Service:
data:text/plain,Do%20what%20thou%20wilt
Do you agree to the above terms?
[y|N]: y
Do you want to use external account binding? [y|N]: N
Attempting
to register account with "https://acme-vm/dir"...
Registration
successful, account URL: https://acme-vm/my-account/160e58b66bdd72da
```

When adjusting the nginx configuration to return any other non-expected
success status code, e.g. 205, PBS expectely rejects with `API
misbehaved: ACME server responded with unexpected status code: 205`.

## Maintainer notes:

The patch series involves the following components:

proxmox-acme: Apply PATCH 1 to change `expected` from `u16` to
`Vec<u16>`. This results in a breaking change, as it changes the public
API of the `AcmeRequest` type that is used by other components.

proxmox-acme-api: Needs to depend on the new proxmox-acme; patch bump

proxmox-backup: Apply PATCH 2 to use the new API changes; no breaking
change as of only internal changes; patch bump

proxmox-perl-rs / proxmox-datacenter-manager: Will need to use the
dependency version bumps to follow the new proxmox-acme.

## Patch summary

[PATCH 1/2] fix #6939: support providers returning 204 for nonce
requests

* Make the expected-status logic accept multiple allowed codes.
* Treat both 200 OK and 204 No Content as valid for HEAD /newNonce,
  provided Replay-Nonce is present.
* Keep rejecting other codes.

[PATCH 2/2] acme: accept HTTP 204 from newNonce endpoint

* Use the updated proxmox-acme behavior in PBS.
* PBS can now register an ACME account against servers that return 204
  for the nonce HEAD request.
* Still rejects unexpected codes.

Thanks for considering this patch series, I look forward to your
feedback.

Best,
Samuel Rufinatscha

## Changes from v1:

[PATCH 1/2] fix #6939: support providers returning 204 for nonce
requests
    * 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.

[PATCH 2/2] acme: accept HTTP 204 from newNonce endpoint
    * 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 (1):
  fix #6939: acme: support servers returning 204 for nonce requests

 proxmox-acme/src/account.rs      | 10 +++++-----
 proxmox-acme/src/async_client.rs |  6 +++---
 proxmox-acme/src/client.rs       |  2 +-
 proxmox-acme/src/lib.rs          |  4 ++++
 proxmox-acme/src/request.rs      | 15 ++++++++++++---
 5 files changed, 25 insertions(+), 12 deletions(-)


proxmox-backup:

Samuel Rufinatscha (1):
  fix #6939: acme: accept HTTP 204 from newNonce endpoint

 src/acme/client.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)


Summary over all repositories:
  6 files changed, 29 insertions(+), 16 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] [PATCH proxmox v2 1/1] fix #6939: acme: support servers returning 204 for nonce requests

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

[1] https://datatracker.ietf.org/doc/html/rfc8555/#section-7.2
[2] https://bugzilla.proxmox.com/show_bug.cgi?id=6939
[3] https://git.proxmox.com/?p=proxmox-acme.git;a=blob;f=src/PVE/ACME.pm;h=f1e9bb7d316e3cea1e376c610b0479119217aecc;hb=HEAD#l219
[4] https://git.proxmox.com/?p=proxmox-acme.git;a=blob;f=src/PVE/ACME.pm;h=f1e9bb7d316e3cea1e376c610b0479119217aecc;hb=HEAD#l597

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/lib.rs          |  4 ++++
 proxmox-acme/src/request.rs      | 15 ++++++++++++---
 5 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/proxmox-acme/src/account.rs b/proxmox-acme/src/account.rs
index 73d786b8..44f9383f 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: &[crate::http_success::CREATED],
         };
 
         Ok(NewOrder::new(request))
@@ -107,7 +107,7 @@ impl Account {
             method: "POST",
             content_type: crate::request::JSON_CONTENT_TYPE,
             body,
-            expected: 200,
+            expected: &[crate::http_success::OK],
         })
     }
 
@@ -132,7 +132,7 @@ impl Account {
             method: "POST",
             content_type: crate::request::JSON_CONTENT_TYPE,
             body,
-            expected: 200,
+            expected: &[crate::http_success::OK],
         })
     }
 
@@ -157,7 +157,7 @@ impl Account {
             method: "POST",
             content_type: crate::request::JSON_CONTENT_TYPE,
             body,
-            expected: 200,
+            expected: &[crate::http_success::OK],
         })
     }
 
@@ -405,7 +405,7 @@ impl AccountCreator {
             method: "POST",
             content_type: crate::request::JSON_CONTENT_TYPE,
             body,
-            expected: crate::request::CREATED,
+            expected: &[crate::http_success::CREATED],
         })
     }
 
diff --git a/proxmox-acme/src/async_client.rs b/proxmox-acme/src/async_client.rs
index 60e1f359..b9df0f55 100644
--- a/proxmox-acme/src/async_client.rs
+++ b/proxmox-acme/src/async_client.rs
@@ -421,7 +421,7 @@ impl AcmeClient {
         };
 
         if parts.status.is_success() {
-            if status != request.expected {
+            if !request.expected.contains(&status) {
                 return Err(Error::InvalidApi(format!(
                     "ACME server responded with unexpected status code: {:?}",
                     parts.status
@@ -501,7 +501,7 @@ impl AcmeClient {
                 method: "GET",
                 content_type: "",
                 body: String::new(),
-                expected: 200,
+                expected: &[crate::http_success::OK],
             },
             nonce,
         )
@@ -553,7 +553,7 @@ impl AcmeClient {
                 method: "HEAD",
                 content_type: "",
                 body: String::new(),
-                expected: 200,
+                expected: &[crate::http_success::OK, crate::http_success::NO_CONTENT],
             },
             nonce,
         )
diff --git a/proxmox-acme/src/client.rs b/proxmox-acme/src/client.rs
index d8a62081..ea8a8655 100644
--- a/proxmox-acme/src/client.rs
+++ b/proxmox-acme/src/client.rs
@@ -203,7 +203,7 @@ impl Inner {
         let got_nonce = self.update_nonce(&mut response)?;
 
         if response.is_success() {
-            if response.status != request.expected {
+            if !request.expected.contains(&response.status) {
                 return Err(Error::InvalidApi(format!(
                     "API server responded with unexpected status code: {:?}",
                     response.status
diff --git a/proxmox-acme/src/lib.rs b/proxmox-acme/src/lib.rs
index df722629..ec586ec4 100644
--- a/proxmox-acme/src/lib.rs
+++ b/proxmox-acme/src/lib.rs
@@ -70,6 +70,10 @@ pub use order::Order;
 #[doc(inline)]
 pub use request::Request;
 
+#[cfg(feature = "impl")]
+#[doc(inline)]
+pub use request::http_success;
+
 // we don't inline these:
 #[cfg(feature = "impl")]
 pub use order::NewOrder;
diff --git a/proxmox-acme/src/request.rs b/proxmox-acme/src/request.rs
index 78a90913..0532528e 100644
--- a/proxmox-acme/src/request.rs
+++ b/proxmox-acme/src/request.rs
@@ -1,7 +1,6 @@
 use serde::Deserialize;
 
 pub(crate) const JSON_CONTENT_TYPE: &str = "application/jose+json";
-pub(crate) const CREATED: u16 = 201;
 
 /// A request which should be performed on the ACME provider.
 pub struct Request {
@@ -17,8 +16,18 @@ pub struct Request {
     /// The body to pass along with request, or an empty string.
     pub body: String,
 
-    /// The expected status code a compliant ACME provider will return on success.
-    pub expected: u16,
+    /// The set of HTTP status codes that indicate a successful response from an ACME provider.
+    pub expected: &'static [u16],
+}
+
+/// Common HTTP success status codes used in ACME responses.
+pub mod http_success {
+    /// 200 OK
+    pub const OK: u16 = 200;
+    /// 201 Created
+    pub const CREATED: u16 = 201;
+    /// 204 No Content
+    pub const NO_CONTENT: u16 = 204;
 }
 
 /// An ACME error response contains a specially formatted type string, and can optionally
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox-backup v2 1/1] fix #6939: acme: accept HTTP 204 from newNonce endpoint

[Samuel Rufinatscha]

When registering an ACME account, PBS fetches a fresh nonce by issuing a
HEAD request to the server's newNonce URL. Until now we assumed this
request would return HTTP 200 OK.

In practice, some ACME servers [1] respond with HTTP 204 No Content for
this HEAD request while still providing a valid Replay-Nonce header.
This causes PBS to abort registration with "ACME server responded with
unexpected status code: 204", even though the server would otherwise
issue certificates correctly. This behavior is technically
off-spec [2], however not forbidden.

Adjust the ACME client code in PBS to accept both 200 OK and 204 No
Content as successful results for the newNonce step. We continue to
reject other status codes so we don't silently accept arbitrary 2xx
responses.

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.

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=6939
[2] https://datatracker.ietf.org/doc/html/rfc8555/#section-7.2
[3] https://git.proxmox.com/?p=proxmox-acme.git;a=blob;f=src/PVE/ACME.pm;h=f1e9bb7d316e3cea1e376c610b0479119217aecc;hb=HEAD#l219
[4] https://git.proxmox.com/?p=proxmox-acme.git;a=blob;f=src/PVE/ACME.pm;h=f1e9bb7d316e3cea1e376c610b0479119217aecc;hb=HEAD#l597

Fixes: #6939
Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
 src/acme/client.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/acme/client.rs b/src/acme/client.rs
index 1c12a4b9..7c32940b 100644
--- a/src/acme/client.rs
+++ b/src/acme/client.rs
@@ -15,7 +15,7 @@ use serde::{Deserialize, Serialize};
 use proxmox_acme::account::AccountCreator;
 use proxmox_acme::order::{Order, OrderData};
 use proxmox_acme::types::AccountData as AcmeAccountData;
-use proxmox_acme::Request as AcmeRequest;
+use proxmox_acme::{http_success, Request as AcmeRequest};
 use proxmox_acme::{Account, Authorization, Challenge, Directory, Error, ErrorResponse};
 use proxmox_http::client::Client;
 use proxmox_sys::fs::{replace_file, CreateOptions};
@@ -530,7 +530,7 @@ impl AcmeClient {
         };
 
         if parts.status.is_success() {
-            if status != request.expected {
+            if !request.expected.contains(&status) {
                 return Err(Error::InvalidApi(format!(
                     "ACME server responded with unexpected status code: {:?}",
                     parts.status
@@ -609,7 +609,7 @@ impl AcmeClient {
                 method: "GET",
                 content_type: "",
                 body: String::new(),
-                expected: 200,
+                expected: &[http_success::OK],
             },
             nonce,
         )
@@ -657,7 +657,7 @@ impl AcmeClient {
                 method: "HEAD",
                 content_type: "",
                 body: String::new(),
-                expected: 200,
+                expected: &[http_success::OK, http_success::NO_CONTENT],
             },
             nonce,
         )
-- 
2.47.3



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