Re: [pbs-devel] [PATCH proxmox v5 1/4] acme: reduce visibility of Request type

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

On 1/13/26 2:46 PM, Fabian Grünbichler wrote:
> On January 8, 2026 12:26 pm, Samuel Rufinatscha wrote:
>> Currently, the low-level ACME Request type is publicly exposed, even
>> though users are expected to go through AcmeClient and
>> proxmox-acme-api handlers. This patch reduces visibility so that
>> the Request type and related fields/methods are crate-internal only.
> 
> it also removes a lot of public and private code entirely, not just
> changing visibility.. I think those were intentionally there to allow
> usage without the need to using either of the provided client
> implementations (which are guarded behind feature flags).
> 
> if we say the crate should only be used via either the `client` or the
> `async-client` then that's fine, but it should be made explicit and
> discussed.. right now this is sort of half-way there - e.g., the
> Account::new_order method was not made private, even though it makes no
> sense anymore with those other methods/helpers removed..
> 
> this patch also breaks a few reference in doc comments that would need
> to be dropped.
> 
> a note that this breaks the current usage of proxmox-acme in PBS would
> also be good to have here, if this is kept..
>

Makes sense.

I think the best here is to drop the visibility reductions and removals
and keep the low-level API intact (as it’s currently documented and
feature-gated).

This will keep this series focused on fixing the 204 nonce handling and
switch PBS to the factored-out client / API handlers (to be on the same
base as PDM).

>>
>> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
>> ---
>>   proxmox-acme/src/account.rs       | 94 ++-----------------------------
>>   proxmox-acme/src/async_client.rs  |  2 +-
>>   proxmox-acme/src/authorization.rs | 30 ----------
>>   proxmox-acme/src/client.rs        |  6 +-
>>   proxmox-acme/src/lib.rs           |  4 --
>>   proxmox-acme/src/order.rs         |  2 +-
>>   proxmox-acme/src/request.rs       | 12 ++--
>>   7 files changed, 16 insertions(+), 134 deletions(-)
>>
>> diff --git a/proxmox-acme/src/account.rs b/proxmox-acme/src/account.rs
>> index f763c1e9..d8eb3e73 100644
>> --- a/proxmox-acme/src/account.rs
>> +++ b/proxmox-acme/src/account.rs
>> @@ -8,12 +8,11 @@ use openssl::pkey::{PKey, Private};
>>   use serde::{Deserialize, Serialize};
>>   use serde_json::Value;
>>   
>> -use crate::authorization::{Authorization, GetAuthorization};
>>   use crate::b64u;
>>   use crate::directory::Directory;
>>   use crate::jws::Jws;
>>   use crate::key::{Jwk, PublicKey};
>> -use crate::order::{NewOrder, Order, OrderData};
>> +use crate::order::{NewOrder, OrderData};
>>   use crate::request::Request;
>>   use crate::types::{AccountData, AccountStatus, ExternalAccountBinding};
>>   use crate::Error;
>> @@ -92,7 +91,7 @@ impl Account {
>>       }
>>   
>>       /// Prepare a "POST-as-GET" request to fetch data. Low level helper.
>> -    pub fn get_request(&self, url: &str, nonce: &str) -> Result<Request, Error> {
>> +    pub(crate) fn get_request(&self, url: &str, nonce: &str) -> Result<Request, Error> {
>>           let key = PKey::private_key_from_pem(self.private_key.as_bytes())?;
>>           let body = serde_json::to_string(&Jws::new_full(
>>               &key,
>> @@ -112,7 +111,7 @@ impl Account {
>>       }
>>   
>>       /// Prepare a JSON POST request. Low level helper.
>> -    pub fn post_request<T: Serialize>(
>> +    pub(crate) fn post_request<T: Serialize>(
>>           &self,
>>           url: &str,
>>           nonce: &str,
>> @@ -136,31 +135,6 @@ impl Account {
>>           })
>>       }
>>   
>> -    /// Prepare a JSON POST request.
>> -    fn post_request_raw_payload(
>> -        &self,
>> -        url: &str,
>> -        nonce: &str,
>> -        payload: String,
>> -    ) -> Result<Request, Error> {
>> -        let key = PKey::private_key_from_pem(self.private_key.as_bytes())?;
>> -        let body = serde_json::to_string(&Jws::new_full(
>> -            &key,
>> -            Some(self.location.clone()),
>> -            url.to_owned(),
>> -            nonce.to_owned(),
>> -            payload,
>> -        )?)?;
>> -
>> -        Ok(Request {
>> -            url: url.to_owned(),
>> -            method: "POST",
>> -            content_type: crate::request::JSON_CONTENT_TYPE,
>> -            body,
>> -            expected: 200,
>> -        })
>> -    }
>> -
>>       /// Get the "key authorization" for a token.
>>       pub fn key_authorization(&self, token: &str) -> Result<String, Error> {
>>           let key = PKey::private_key_from_pem(self.private_key.as_bytes())?;
>> @@ -176,64 +150,6 @@ impl Account {
>>           Ok(b64u::encode(digest))
>>       }
>>   
>> -    /// Prepare a request to update account data.
>> -    ///
>> -    /// This is a rather low level interface. You should know what you're doing.
>> -    pub fn update_account_request<T: Serialize>(
>> -        &self,
>> -        nonce: &str,
>> -        data: &T,
>> -    ) -> Result<Request, Error> {
>> -        self.post_request(&self.location, nonce, data)
>> -    }
>> -
>> -    /// Prepare a request to deactivate this account.
>> -    pub fn deactivate_account_request<T: Serialize>(&self, nonce: &str) -> Result<Request, Error> {
>> -        self.post_request_raw_payload(
>> -            &self.location,
>> -            nonce,
>> -            r#"{"status":"deactivated"}"#.to_string(),
>> -        )
>> -    }
>> -
>> -    /// Prepare a request to query an Authorization for an Order.
>> -    ///
>> -    /// Returns `Ok(None)` if `auth_index` is out of out of range. You can query the number of
>> -    /// authorizations from via [`Order::authorization_len`] or by manually inspecting its
>> -    /// `.data.authorization` vector.
>> -    pub fn get_authorization(
>> -        &self,
>> -        order: &Order,
>> -        auth_index: usize,
>> -        nonce: &str,
>> -    ) -> Result<Option<GetAuthorization>, Error> {
>> -        match order.authorization(auth_index) {
>> -            None => Ok(None),
>> -            Some(url) => Ok(Some(GetAuthorization::new(self.get_request(url, nonce)?))),
>> -        }
>> -    }
>> -
>> -    /// Prepare a request to validate a Challenge from an Authorization.
>> -    ///
>> -    /// Returns `Ok(None)` if `challenge_index` is out of out of range. The challenge count is
>> -    /// available by inspecting the [`Authorization::challenges`] vector.
>> -    ///
>> -    /// This returns a raw `Request` since validation takes some time and the `Authorization`
>> -    /// object has to be re-queried and its `status` inspected.
>> -    pub fn validate_challenge(
>> -        &self,
>> -        authorization: &Authorization,
>> -        challenge_index: usize,
>> -        nonce: &str,
>> -    ) -> Result<Option<Request>, Error> {
>> -        match authorization.challenges.get(challenge_index) {
>> -            None => Ok(None),
>> -            Some(challenge) => self
>> -                .post_request_raw_payload(&challenge.url, nonce, "{}".to_string())
>> -                .map(Some),
>> -        }
>> -    }
>> -
>>       /// Prepare a request to revoke a certificate.
>>       ///
>>       /// The certificate can be either PEM or DER formatted.
>> @@ -274,7 +190,7 @@ pub struct CertificateRevocation<'a> {
>>   
>>   impl CertificateRevocation<'_> {
>>       /// Create the revocation request using the specified nonce for the given directory.
>> -    pub fn request(&self, directory: &Directory, nonce: &str) -> Result<Request, Error> {
>> +    pub(crate) fn request(&self, directory: &Directory, nonce: &str) -> Result<Request, Error> {
>>           let revoke_cert = directory.data.revoke_cert.as_ref().ok_or_else(|| {
>>               Error::Custom("no 'revokeCert' URL specified by provider".to_string())
>>           })?;
>> @@ -364,7 +280,7 @@ impl AccountCreator {
>>       /// the resulting request.
>>       /// Changing the private key between using the request and passing the response to
>>       /// [`response`](AccountCreator::response()) will render the account unusable!
>> -    pub fn request(&self, directory: &Directory, nonce: &str) -> Result<Request, Error> {
>> +    pub(crate) fn request(&self, directory: &Directory, nonce: &str) -> Result<Request, Error> {
>>           let key = self.key.as_deref().ok_or(Error::MissingKey)?;
>>           let url = directory.new_account_url().ok_or_else(|| {
>>               Error::Custom("no 'newAccount' URL specified by provider".to_string())
>> diff --git a/proxmox-acme/src/async_client.rs b/proxmox-acme/src/async_client.rs
>> index dc755fb9..2ff3ba22 100644
>> --- a/proxmox-acme/src/async_client.rs
>> +++ b/proxmox-acme/src/async_client.rs
>> @@ -10,7 +10,7 @@ use proxmox_http::{client::Client, Body};
>>   
>>   use crate::account::AccountCreator;
>>   use crate::order::{Order, OrderData};
>> -use crate::Request as AcmeRequest;
>> +use crate::request::Request as AcmeRequest;
>>   use crate::{Account, Authorization, Challenge, Directory, Error, ErrorResponse};
>>   
>>   /// A non-blocking Acme client using tokio/hyper.
>> diff --git a/proxmox-acme/src/authorization.rs b/proxmox-acme/src/authorization.rs
>> index 28bc1b4b..7027381a 100644
>> --- a/proxmox-acme/src/authorization.rs
>> +++ b/proxmox-acme/src/authorization.rs
>> @@ -6,8 +6,6 @@ use serde::{Deserialize, Serialize};
>>   use serde_json::Value;
>>   
>>   use crate::order::Identifier;
>> -use crate::request::Request;
>> -use crate::Error;
>>   
>>   /// Status of an [`Authorization`].
>>   #[derive(Clone, Copy, Debug, Eq, PartialEq, Deserialize, Serialize)]
>> @@ -132,31 +130,3 @@ impl Challenge {
>>   fn is_false(b: &bool) -> bool {
>>       !*b
>>   }
>> -
>> -/// Represents an in-flight query for an authorization.
>> -///
>> -/// This is created via [`Account::get_authorization`](crate::Account::get_authorization()).
>> -pub struct GetAuthorization {
>> -    //order: OrderData,
>> -    /// The request to send to the ACME provider. This is wrapped in an option in order to allow
>> -    /// moving it out instead of copying the contents.
>> -    ///
>> -    /// When generated via [`Account::get_authorization`](crate::Account::get_authorization()),
>> -    /// this is guaranteed to be `Some`.
>> -    ///
>> -    /// The response should be passed to the the [`response`](GetAuthorization::response()) method.
>> -    pub request: Option<Request>,
>> -}
>> -
>> -impl GetAuthorization {
>> -    pub(crate) fn new(request: Request) -> Self {
>> -        Self {
>> -            request: Some(request),
>> -        }
>> -    }
>> -
>> -    /// Deal with the response we got from the server.
>> -    pub fn response(self, response_body: &[u8]) -> Result<Authorization, Error> {
>> -        Ok(serde_json::from_slice(response_body)?)
>> -    }
>> -}
>> diff --git a/proxmox-acme/src/client.rs b/proxmox-acme/src/client.rs
>> index 931f7245..5c812567 100644
>> --- a/proxmox-acme/src/client.rs
>> +++ b/proxmox-acme/src/client.rs
>> @@ -7,8 +7,8 @@ use serde::{Deserialize, Serialize};
>>   use crate::b64u;
>>   use crate::error;
>>   use crate::order::OrderData;
>> -use crate::request::ErrorResponse;
>> -use crate::{Account, Authorization, Challenge, Directory, Error, Order, Request};
>> +use crate::request::{ErrorResponse, Request};
>> +use crate::{Account, Authorization, Challenge, Directory, Error, Order};
>>   
>>   macro_rules! format_err {
>>       ($($fmt:tt)*) => { Error::Client(format!($($fmt)*)) };
>> @@ -564,7 +564,7 @@ impl Client {
>>       }
>>   
>>       /// Low-level API to run an n API request. This automatically updates the current nonce!
>> -    pub fn run_request(&mut self, request: Request) -> Result<HttpResponse, Error> {
>> +    pub(crate) fn run_request(&mut self, request: Request) -> Result<HttpResponse, Error> {
>>           self.inner.run_request(request)
>>       }
>>   
>> diff --git a/proxmox-acme/src/lib.rs b/proxmox-acme/src/lib.rs
>> index df722629..6722030c 100644
>> --- a/proxmox-acme/src/lib.rs
>> +++ b/proxmox-acme/src/lib.rs
>> @@ -66,10 +66,6 @@ pub use error::Error;
>>   #[doc(inline)]
>>   pub use order::Order;
>>   
>> -#[cfg(feature = "impl")]
>> -#[doc(inline)]
>> -pub use request::Request;
>> -
>>   // we don't inline these:
>>   #[cfg(feature = "impl")]
>>   pub use order::NewOrder;
>> diff --git a/proxmox-acme/src/order.rs b/proxmox-acme/src/order.rs
>> index b6551004..432a81a4 100644
>> --- a/proxmox-acme/src/order.rs
>> +++ b/proxmox-acme/src/order.rs
>> @@ -153,7 +153,7 @@ pub struct NewOrder {
>>       //order: OrderData,
>>       /// The request to execute to place the order. When creating a [`NewOrder`] via
>>       /// [`Account::new_order`](crate::Account::new_order) this is guaranteed to be `Some`.
>> -    pub request: Option<Request>,
>> +    pub(crate) request: Option<Request>,
>>   }
>>   
>>   impl NewOrder {
>> diff --git a/proxmox-acme/src/request.rs b/proxmox-acme/src/request.rs
>> index 78a90913..dadfc5af 100644
>> --- a/proxmox-acme/src/request.rs
>> +++ b/proxmox-acme/src/request.rs
>> @@ -4,21 +4,21 @@ 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 {
>> +pub(crate) struct Request {
>>       /// The complete URL to send the request to.
>> -    pub url: String,
>> +    pub(crate) url: String,
>>   
>>       /// The HTTP method name to use.
>> -    pub method: &'static str,
>> +    pub(crate) method: &'static str,
>>   
>>       /// The `Content-Type` header to pass along.
>> -    pub content_type: &'static str,
>> +    pub(crate) content_type: &'static str,
>>   
>>       /// The body to pass along with request, or an empty string.
>> -    pub body: String,
>> +    pub(crate) body: String,
>>   
>>       /// The expected status code a compliant ACME provider will return on success.
>> -    pub expected: u16,
>> +    pub(crate) expected: u16,
>>   }
>>   
>>   /// 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 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