[PATCH proxmox 0/3] s3-client: fix request signing and update request time on retry

[PATCH proxmox 0/3] s3-client: fix request signing and update request time on retry

[Christian Ebner]

This patch series fixes 2 issues with the current s3-client
implementation.

In particular:
- Patch 1 fixes an issue with s3-client request signing, encountered
  during development. The canonical request headers and query
  parameters were incorrectly sorted by key+value strings instead
  of key only, which could lead to sorting mismatches with the API
  server, resulting in signature mismatches and therefore rejected
  requests. In particular, this might be encountered if one header
  name is a prefix to another header name.
  Fixed by sorting headers and queries via their respective key only.
- Patches 2+3 fix a not updated request time for requests being
  retried. This could potentially lead to `RequestTimeTooSkewed`
  errors when the time in-between retried requests is very large and
  might explain such errors observed as reported in the community
  forum [0].
  Fixed by setting the current request time and updating the request
  signature each time before sending the request via the client.

[0] https://forum.proxmox.com/threads/186881/


proxmox:

Christian Ebner (3):
  s3-client: fix header and query parameter sorting during aws sign v4
  s3-client: factor out request signing and related header updates
  s3-client: update request time and signature on retries

 proxmox-s3-client/src/aws_sign_v4.rs | 30 ++++++---
 proxmox-s3-client/src/client.rs      | 95 ++++++++++++++--------------
 2 files changed, 67 insertions(+), 58 deletions(-)


Summary over all repositories:
  2 files changed, 67 insertions(+), 58 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH proxmox 1/3] s3-client: fix header and query parameter sorting during aws sign v4

[Christian Ebner]

Currently, canonical headers and query parameters are incorrectly
sorted by combined `key:value` or `key=value` strings respectively,
instead of by key only. This could result in signature mismatches
with the S3 API.

Fixes: 7c6ef846 ("s3 client: implement AWS signature v4 request authentication")
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-s3-client/src/aws_sign_v4.rs | 30 +++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/proxmox-s3-client/src/aws_sign_v4.rs b/proxmox-s3-client/src/aws_sign_v4.rs
index f172b4b4..54f4e54e 100644
--- a/proxmox-s3-client/src/aws_sign_v4.rs
+++ b/proxmox-s3-client/src/aws_sign_v4.rs
@@ -31,11 +31,11 @@ pub(crate) fn aws_sign_v4_signature(
     // headers are required. however, in order to prevent data tampering, you should consider
     // including all the headers in the signature calculation."
     // See https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html
-    let mut canonical_headers = Vec::new();
-    let mut signed_headers = Vec::new();
-    for (key, value) in request.headers() {
-        canonical_headers.push(format!(
-            "{}:{}",
+    let req_headers = request.headers();
+    let mut headers = Vec::with_capacity(req_headers.len());
+
+    for (key, value) in req_headers {
+        headers.push((
             // Header name has to be lower case, key.as_str() does guarantee that, see
             // https://docs.rs/http/0.2.0/http/header/struct.HeaderName.html
             key.as_str(),
@@ -43,14 +43,27 @@ pub(crate) fn aws_sign_v4_signature(
             // https://docs.rs/http/0.2.0/http/header/struct.HeaderValue.html
             value.to_str()?,
         ));
-        signed_headers.push(key.as_str());
     }
-    canonical_headers.sort();
-    signed_headers.sort();
+
+    headers.sort_unstable_by(|a, b| a.0.cmp(b.0));
+
+    let mut canonical_headers = Vec::with_capacity(headers.len());
+    let mut signed_headers = Vec::with_capacity(headers.len());
+    for (key, value) in headers {
+        canonical_headers.push(format!("{key}:{value}"));
+        signed_headers.push(key);
+    }
     let signed_headers_string = signed_headers.join(";");
 
     let mut canonical_queries = Url::parse(&request.uri().to_string())?
         .query_pairs()
+        .map(|(key, value)| (key.to_string(), value.to_string()))
+        .collect::<Vec<(String, String)>>();
+
+    canonical_queries.sort_unstable_by(|a, b| a.0.cmp(&b.0));
+
+    let canonical_queries = canonical_queries
+        .into_iter()
         .map(|(key, value)| {
             format!(
                 "{}={}",
@@ -59,7 +72,6 @@ pub(crate) fn aws_sign_v4_signature(
             )
         })
         .collect::<Vec<String>>();
-    canonical_queries.sort();
 
     let canonical_request = format!(
         "{}\n{}\n{}\n{}\n\n{}\n{}",
-- 
2.47.3

[PATCH proxmox 2/3] s3-client: factor out request signing and related header updates

[Christian Ebner]

Currently this is performed as part of the request prepare, but since
the request time might be to skewed on retries which can in principle
happen after a very long time, this needs to be updated on-demand.

Therefore move the required code into a dedicated reusable helper.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-s3-client/src/client.rs | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/proxmox-s3-client/src/client.rs b/proxmox-s3-client/src/client.rs
index 7903f090..631bbd0f 100644
--- a/proxmox-s3-client/src/client.rs
+++ b/proxmox-s3-client/src/client.rs
@@ -420,12 +420,6 @@ impl S3Client {
         let payload_digest = hex::encode(hasher.finish());
         let payload_len = contents.len();
 
-        let epoch = proxmox_time::epoch_i64();
-        let datetime = proxmox_time::strftime_utc(AWS_SIGN_V4_DATETIME_FORMAT, epoch)?;
-
-        request
-            .headers_mut()
-            .insert("x-amz-date", HeaderValue::from_str(&datetime)?);
         request
             .headers_mut()
             .insert("host", HeaderValue::from_str(&host_header)?);
@@ -452,13 +446,27 @@ impl S3Client {
                 .insert("Content-MD5", HeaderValue::from_str(&md5_digest)?);
         }
 
-        let signature = aws_sign_v4_signature(&request, &self.options, epoch, &payload_digest)?;
+        self.sign_request(&mut request, &payload_digest)?;
 
-        request
+        Ok(request)
+    }
+
+    /// Set or update the `x-amz-date` header to the current time, calculate the request signature
+    /// based on the provided payload digest and set or update the authorization header accordingly.
+    fn sign_request(&self, request: &mut Request<Body>, payload_digest: &str) -> Result<(), Error> {
+        let epoch = proxmox_time::epoch_i64();
+        let datetime = proxmox_time::strftime_utc(AWS_SIGN_V4_DATETIME_FORMAT, epoch)?;
+
+        let _prev_date = request
+            .headers_mut()
+            .insert("x-amz-date", HeaderValue::from_str(&datetime)?);
+
+        let signature = aws_sign_v4_signature(&request, &self.options, epoch, &payload_digest)?;
+        let _prev_auth = request
             .headers_mut()
             .insert(header::AUTHORIZATION, HeaderValue::from_str(&signature)?);
 
-        Ok(request)
+        Ok(())
     }
 
     /// Send API request to the configured endpoint using the inner https client.
-- 
2.47.3

[PATCH proxmox 3/3] s3-client: update request time and signature on retries

[Christian Ebner]

Currently request being retried do not update the request time
related information, keeping the initial requests state. This might
however cause issues if in-between the retries lays a long period of
time, resulting in `RequestTimeTooSkewed` errors.

To fix this, inline the prepare() helper into its only call site,
allowing to efficiently reuse already calculated payload digest and
size information within the send() helper. By early splitting the
request into parts and keeping the body as cheaply (via Bytes)
clone()-able state, the request is prepared setting common headers,
only calculating request time and signature right before sending
individual requests, including retries.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-s3-client/src/client.rs | 103 ++++++++++++++------------------
 1 file changed, 46 insertions(+), 57 deletions(-)

diff --git a/proxmox-s3-client/src/client.rs b/proxmox-s3-client/src/client.rs
index 631bbd0f..1cfac893 100644
--- a/proxmox-s3-client/src/client.rs
+++ b/proxmox-s3-client/src/client.rs
@@ -398,59 +398,6 @@ impl S3Client {
         ))
     }
 
-    /// Prepare API request by adding commonly required headers and perform request signing
-    async fn prepare(&self, mut request: Request<Body>) -> Result<Request<Body>, Error> {
-        let host_header = request
-            .uri()
-            .authority()
-            .ok_or_else(|| format_err!("request missing authority"))?
-            .to_string();
-
-        // Content verification for aws s3 signature
-        let mut hasher = Sha256::new();
-        let contents = request
-            .body()
-            .as_bytes()
-            .ok_or_else(|| format_err!("cannot prepare request with streaming body"))?;
-        hasher.update(contents);
-        // Use MD5 as upload integrity check, as other methods are not supported by all S3 object
-        // store providers and might be ignored and this is recommended by AWS as described in
-        // https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutObject.html#API_PutObject_RequestSyntax
-        let payload_md5 = md5::compute(contents);
-        let payload_digest = hex::encode(hasher.finish());
-        let payload_len = contents.len();
-
-        request
-            .headers_mut()
-            .insert("host", HeaderValue::from_str(&host_header)?);
-        request.headers_mut().insert(
-            "x-amz-content-sha256",
-            HeaderValue::from_str(&payload_digest)?,
-        );
-
-        let set_content_length_header = match request.method() {
-            &Method::PUT | &Method::POST => true,
-            &Method::DELETE if payload_len > 0 => true,
-            _ => false,
-        };
-        if set_content_length_header {
-            request.headers_mut().insert(
-                header::CONTENT_LENGTH,
-                HeaderValue::from_str(&payload_len.to_string())?,
-            );
-        }
-        if payload_len > 0 {
-            let md5_digest = proxmox_base64::encode(*payload_md5);
-            request
-                .headers_mut()
-                .insert("Content-MD5", HeaderValue::from_str(&md5_digest)?);
-        }
-
-        self.sign_request(&mut request, &payload_digest)?;
-
-        Ok(request)
-    }
-
     /// Set or update the `x-amz-date` header to the current time, calculate the request signature
     /// based on the provided payload digest and set or update the authorization header accordingly.
     fn sign_request(&self, request: &mut Request<Body>, payload_digest: &str) -> Result<(), Error> {
@@ -475,17 +422,57 @@ impl S3Client {
         request: Request<Body>,
         timeout: Option<Duration>,
     ) -> Result<Response<Incoming>, Error> {
-        let request = self.prepare(request).await?;
-
-        let (parts, body) = request.into_parts();
+        let (mut parts, body) = request.into_parts();
         let body_bytes = body
             .bytes()
             .ok_or_else(|| format_err!("cannot prepare request with streaming body"))?;
 
+        let host_header = parts
+            .uri
+            .authority()
+            .ok_or_else(|| format_err!("request missing authority"))?
+            .to_string();
+
+        // Content verification for aws s3 signature
+        let mut hasher = Sha256::new();
+        hasher.update(&body_bytes);
+        // Use MD5 as upload integrity check, as other methods are not supported by all S3 object
+        // store providers and might be ignored and this is recommended by AWS as described in
+        // https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutObject.html#API_PutObject_RequestSyntax
+        let payload_md5 = md5::compute(&body_bytes);
+        let payload_digest = hex::encode(hasher.finish());
+        let payload_len = body_bytes.len();
+
+        parts
+            .headers
+            .insert("host", HeaderValue::from_str(&host_header)?);
+        parts.headers.insert(
+            "x-amz-content-sha256",
+            HeaderValue::from_str(&payload_digest)?,
+        );
+
+        let set_content_length_header = match parts.method {
+            Method::PUT | Method::POST => true,
+            Method::DELETE if payload_len > 0 => true,
+            _ => false,
+        };
+        if set_content_length_header {
+            parts.headers.insert(
+                header::CONTENT_LENGTH,
+                HeaderValue::from_str(&payload_len.to_string())?,
+            );
+        }
+        if payload_len > 0 {
+            let md5_digest = proxmox_base64::encode(*payload_md5);
+            parts
+                .headers
+                .insert("Content-MD5", HeaderValue::from_str(&md5_digest)?);
+        }
+
         let deadline = timeout.map(|timeout| tokio::time::Instant::now() + timeout);
 
         for retry in 0..MAX_S3_HTTP_REQUEST_RETRY {
-            let request = Request::from_parts(parts.clone(), Body::from(body_bytes.clone()));
+            let mut request = Request::from_parts(parts.clone(), Body::from(body_bytes.clone()));
             if let Some(limiter) = &self.active_request_rate_limiter {
                 if matches!(parts.method, Method::PUT | Method::POST | Method::DELETE) {
                     let sleep = limiter.register_traffic(Instant::now(), 1);
@@ -509,6 +496,8 @@ impl S3Client {
                 }
             }
 
+            self.sign_request(&mut request, &payload_digest)?;
+
             let response = if let Some(deadline) = deadline {
                 tokio::time::timeout_at(deadline, self.client.request(request))
                     .await
-- 
2.47.3