feat(rest): add a SigV4 request signer by plusplusjiajia · Pull Request #3082 · apache/iceberg-rust

plusplusjiajia · 2026-08-27T05:20:50Z

Which issue does this PR close?

Split out of #2660 so the signing can be reviewed on its own.

What changes are included in this PR?

SigV4Signer signs an HttpRequest for AWS SigV4, built on the official aws-sigv4 crate, behind a new sigv4 feature that is off by default. On top of the crate's defaults:

PayloadHashMode::IcebergRest puts a base64 checksum in x-amz-content-sha256 while the canonical request hashes the body in hex, matching Java's SignerChecksumParams; StandardAws uses hex in both.
Aws4Signer defaults: path normalization and double URL-encoding.
Java's unsigned headers (connection, expect, transfer-encoding, user-agent, x-amzn-trace-id), plus x-forwarded-for, are excluded from signing, since a proxy may rewrite them.
An existing Authorization moves to Original-Authorization and is signed, as in Java.
A + in the query is rewritten to %20 before signing, since verifiers disagree on whether it means a literal plus or a space and reqwest writes spaces as +.

The signer takes credentials per call. The auth manager and session that resolve them follow in #3092, and the catalog wiring in #2660.

Are these changes tested?

Yes. Besides unit tests, signatures are checked against requests signed by Iceberg Java's RESTSigV4AuthSession (iceberg-aws 1.10.1) and against the AWS SigV4 test suite.

plusplusjiajia · 2026-08-27T05:37:45Z

@CTTY This is the signing half of #2660 (#2660), split out so it can be reviewed on its own — and it now builds on the aws-sigv4 crate rather than a hand-rolled implementation, which was your concern there.

DerGut · 2026-08-27T18:38:04Z

I think AWS-specific dependencies are worth to hide behind a feature flag like we already do for different FileIO deps with OpenDAL

iceberg-rust/crates/storage/opendal/Cargo.toml

Lines 30 to 48 in a5f162f

[features]

default = ["opendal-memory", "opendal-fs", "opendal-s3"]

opendal-all = [

"opendal-memory",

"opendal-fs",

"opendal-s3",

"opendal-gcs",

"opendal-oss",

"opendal-azdls",

"opendal-hf",

]

opendal-azdls = ["opendal/services-azdls"]

opendal-fs = ["opendal/services-fs"]

opendal-gcs = ["opendal/services-gcs"]

opendal-hf = ["opendal/services-hf"]

opendal-memory = ["opendal/services-memory"]

opendal-oss = ["opendal/services-oss"]

opendal-s3 = ["opendal/services-s3", "reqsign-aws-v4", "reqsign-core"]

@DerGut Good point, this wasn't on my radar. Added — sigv4, off by default, gating aws-sigv4, aws-credential-types, base64 and sha2.

DerGut · 2026-08-27T18:39:54Z

Since this file is empty, wouldn't an /auth/sigv4.rs suffice? Unless we expect to add a lot of logic soon

@DerGut Done — collapsed to auth/sigv4.rs

CTTY

Thanks for breaking this down to a smaller PR!

Just took a pass, and I'm not sure how we can allow the usage of non-static credentials. We should explore if we could use aws rust sdk directly

CTTY · 2026-08-27T23:15:13Z

+
+/// Static AWS-style credentials used for SigV4 signing of catalog requests.
+#[derive(Clone)]
+pub struct AwsCredentials {


We should not add this in iceberg and should use predefined credentials: https://docs.rs/aws-credential-types/latest/aws_credential_types/struct.Credentials.html

@CTTY Agreed, and it turned out to go further than just the type. Removed; sign takes aws_credential_types::Credentials, and following that through, the signer now carries no credential state at all — matching Java, where one Aws4Signer is shared across sessions and the session resolves credentials per request. It keeps only region, service and payload mode.

CTTY · 2026-08-27T23:16:22Z

+/// AWS SigV4 signer following Iceberg Java's `RESTSigV4AuthSession`: it adds the
+/// required amz headers and signs all request headers except a small blacklist.
+#[derive(Clone)]
+pub struct SigV4Signer {


Should this be pub(crate) ? I only expect sigv4AuthSession to use this

@CTTY Not in this PR — nothing in the crate uses the signer yet, so pub(crate) makes it all dead code and -D warnings implies -D dead-code.
Works once #3092 lands, but new then can't take a SigV4Signer either — a public fn can't take a private type. Taking region/service/mode instead passes clippy and drops 8 lines from the public API, at the cost of a five-argument constructor.

CTTY · 2026-08-27T23:17:44Z

+/// required amz headers and signs all request headers except a small blacklist.
+#[derive(Clone)]
+pub struct SigV4Signer {
+    credentials: AwsCredentials,


How do we handle role based credentials? I think we should introduce https://docs.rs/aws-credential-types/latest/aws_credential_types/provider/future/struct.ProvideCredentials.html to the SigV4AuthSession and have users provide their own credential provider

@CTTY This shaped the follow-up, thanks. Provider lives in the session, not the signer: #3092's SigV4AuthManager holds a SharedCredentialsProvider and resolves it per request, as Java does inside sign. Static credentials come from Java's property names; anything else goes to new.

CTTY · 2026-08-27T23:20:36Z

+    ///
+    /// Fails rather than sign a request whose body is streaming or whose
+    /// headers are not UTF-8, since neither can be canonicalized faithfully.
+    pub fn sign(&self, request: &mut reqwest::Request) -> Result<()> {


This should take HttpRequest?

@CTTY Switched, thanks!

CTTY · 2026-08-27T23:23:50Z

+        self.sign_at(request, Utc::now())
+    }
+
+    fn sign_at(&self, request: &mut reqwest::Request, now: DateTime<Utc>) -> Result<()> {


Same as above, this should use HttpRequest

@CTTY Same change.

CTTY · 2026-08-27T23:25:43Z

+        // not itself signed.
+        let displaced_content_hash: Vec<_> = request
+            .headers()
+            .get_all("x-amz-content-sha256")


This should be a constant

@CTTY Pulled out, with x-amz-date and x-amz-security-token. No header-name literals left outside tests.

CTTY · 2026-08-28T00:03:35Z

+                value.set_sensitive(true);
+                request.headers_mut().append(RELOCATED_AUTHORIZATION, value);
+            }
+        }


Let's create more helpers like java's converHeaders and updateHeaders to make the main function body more readable

@CTTY Split out convert_headers and update_request_headers after Java's, plus four smaller ones.

laskoviymishka

This is careful work — the IcebergRest/StandardAws split, the relocation of caller headers into Original-* with sensitivity marking, and the coverage of userinfo stripping, hop-by-hop headers, and AWS double-encoding all track Java's RESTSigV4AuthSession closely. A stateless signer with credentials passed per call is the right primitive for the auth layer to build on.

I don't think it should merge quite yet, for two reasons. This crate is publish = true, so SigV4Signer and PayloadHashMode go semver-locked the moment this hits crates.io — and PayloadHashMode is an exhaustive public enum, so a third hash mode later is a breaking change. I'd lock the surface down before the first publish: #[non_exhaustive] on the enum, and a look at the positional new(String, String, ...) while we can still change it freely.

The other is test confidence, which for a signing primitive is the thing that matters most. The IcebergRest-with-body signature — the most security-sensitive path — is pinned from the previous hand-rolled signer, and the AWS golden vector only exercises a test-only key-derivation helper, not the signer's own canonicalization. Nothing cross-checks our output against Java or the AWS SigV4 suite, so a real divergence would stay green. One externally-sourced end-to-end vector would close that.

Separately, worth settling the credential question CTTY raised. Right now sign() binds to a static &Credentials snapshot, while Java resolves credentials on every call — so IAM-role/IRSA/STS rotation is automatic there and manual here. My lean: a snapshot-based primitive is fine as long as the follow-up auth layer owns refresh and we document that contract now; if we'd rather bind to a ProvideCredentials-style provider, better to decide that before the API ships. Either way I'd write the freshness expectation into the sign() doc.

Before merge I'd want:

the .unwrap() at the content-hash insert propagated (no-panic rule)
#[non_exhaustive] on PayloadHashMode, and a look at the constructor shape
one external Java/AWS-suite signature vector, plus the three sign() tests moved onto sign_at so they actually pin
the tracing-suppression comment corrected and its branch actually exercised (today the test only hits the else-branch)

The rest is small. Once the panic and the semver surface are handled and there's one real cross-check on the signature, I'm happy to take another pass.

laskoviymishka · 2026-09-30T19:41:00Z

+
+/// How the payload hash is encoded in the `x-amz-content-sha256` header.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub enum PayloadHashMode {


Since the crate is publish = true, this enum becomes public API — and it derives Eq, so callers will match it exhaustively. A third mode later (streaming, CRC32C) would then be a breaking change. I'd tag it #[non_exhaustive] now; it's free before the first publish and can't be retrofitted afterward without the break.

#[non_exhaustive] pub enum PayloadHashMode {

Good call, added.

laskoviymishka · 2026-09-30T19:41:00Z

+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub enum PayloadHashMode {
+    /// Iceberg Java's RESTSigV4 style: base64 header when there is a body, hex
+    /// when there is none; the canonical request always uses hex. A caller-set


Worth calling out in this doc that the base64 header is a Java-SDK quirk, not the SigV4 spec: botocore/PyIceberg and StandardAws both write hex here. Signatures still verify (the canonical body-hash line stays hex), but a server that independently checks x-amz-content-sha256 == hex(sha256(body)) will reject an IcebergRest-signed request, and the same logical request won't be byte-identical to a PyIceberg one. A sentence steering callers to StandardAws unless they're specifically talking to a Java REST server would keep the follow-up from defaulting to the quirk.

Added a note on where the base64 comes from. I kept it neutral on which mode to pick, since #3092 defaults to IcebergRest to match Java clients.

laskoviymishka · 2026-09-30T19:41:00Z

+
+impl SigV4Signer {
+    /// Creates a new SigV4 signer.
+    pub fn new(region: String, service: String, mode: PayloadHashMode) -> Self {


new takes owned Strings, so every call site does "...".to_string() — impl Into<String> for region/service drops that with no downside.

pub fn new(region: impl Into<String>, service: impl Into<String>, mode: PayloadHashMode) -> Self

While we're locking the surface down: a positional constructor also can't gain a parameter without a semver break, so if there's any chance this grows a field, #[non_exhaustive] on the struct keeps that door open too.

Switched to impl Into<String>, thanks. The struct's fields are private, so adding one later isn't breaking.

laskoviymishka · 2026-09-30T19:41:00Z

+    /// muted for the call; with no subscriber, or with `tracing`'s `log-always`
+    /// feature, its `log` bridge still forwards those events, so keep
+    /// `aws_sigv4` below trace level there.
+    pub fn sign(


The doc here is thorough on redirects and tracing but silent on credential freshness, which is the one that'll bite. This signs with whatever snapshot it's handed; Java resolves credentials on every sign(), so IAM-role/IRSA/STS rotation is automatic there. A long-lived caller that resolves once (an easy mistake in the #2660 wiring) would sign with expired creds and just eat 403s. I'd add a line to the contract: callers with temporary credentials must resolve fresh ones before each call.

Agreed, added to the sign() doc. The #3092 session resolves credentials per request, like Java.

laskoviymishka · 2026-09-30T19:41:00Z

+            .collect();
+        request
+            .headers_mut()
+            .insert(CONTENT_SHA256, content_header.parse().unwrap());


I'd propagate here rather than .unwrap(). It's unreachable today since content_header is always hex or base64, but that invariant only lives in our heads — a future change that puts a stray byte in the value would panic the whole process mid-sign, which is exactly what the no-panic rule exists to prevent.

let value = content_header.parse().map_err(|e| { Error::new(ErrorKind::Unexpected, "invalid computed x-amz-content-sha256 value").with_source(e) })?; request.headers_mut().insert(CONTENT_SHA256, value);

Makes sense, it returns an error now.

laskoviymishka · 2026-09-30T19:41:00Z

+        // "a dispatcher was installed" flag for good, so doing it unasked would
+        // silently divert every later event away from an app's `log` bridge —
+        // a worse trade than a trace-level exposure the operator opted into.
+        let signed = if tracing::dispatcher::has_been_set() {


The comment says with_default sets tracing's global "a dispatcher was installed" flag "for good" — that's not right. with_default only sets a thread-local current subscriber; has_been_set() flips only for set_global_default. The real reason to gate is avoiding the thread-local swap when no global subscriber is active, not a permanent side-effect.

That same fact means the protection is untested: signing_does_not_trace_a_relocated_bearer_token installs its capture with with_default, so inside sign_at has_been_set() is false and the else branch runs — the NoSubscriber arm never executes, and the test would still pass if we deleted the if. Since that branch is the bit that actually keeps the bearer token out of the logs, I'd pin it with a set_global_default (guarded by a Once, or an integration test) so the suppressing path is the one under test.

with_default does set it: State::set_default in tracing-core 0.1.36 stores EXISTS (dispatcher.rs:849), so the test takes the muting branch, and forcing false fails it. That said, has_been_set() is doc-hidden, so the gate now uses the public LevelFilter::current(), and the test asserts it.

laskoviymishka · 2026-09-30T19:41:00Z

+    // canonical request keeps hex.
+    settings.payload_checksum_kind = PayloadChecksumKind::NoHeader;
+    let mut excluded = settings.excluded_headers.take().unwrap_or_default();
+    excluded.extend([


Small thing while we're here: an_existing_authorization_is_never_signed leans on aws_sigv4 excluding user-agent by default, but it's not in our explicit list. With the "1.4" floor a point release could change that default without a semver bump, and we'd silently start signing user-agent — which reqwest rewrites on the wire, breaking every request. I'd pin it ourselves rather than inherit it:

excluded.extend([ "expect".into(), "connection".into(), "user-agent".into(), "x-forwarded-for".into(), // ... ]);

Sure, added along with the rest of Java's list.

laskoviymishka · 2026-09-30T19:41:00Z

+    }
+
+    #[test]
+    fn signs_request_iceberg_mode() {


These three (signs_request_iceberg_mode, signs_empty_body_and_all_headers, signs_request_standard_mode_uses_hex_header) go through the public sign(), so Utc::now() makes the date and signature vary each run and they can only assert header shape — a canonicalization regression like header reordering would pass. I'd route them through sign_at with a fixed timestamp and pin via assert_signature_is, like the rest of the file. Worth keeping exactly one on sign() as a clock smoke test that asserts x-amz-date matches YYYYMMDDTHHmmSSZ, since otherwise nothing exercises the live-clock path at all.

Moved all three to sign_at with pinned signatures, and kept one live-clock test for sign().

laskoviymishka · 2026-09-30T19:41:00Z

+    /// The signed `host` must include an explicit non-default port, matching
+    /// what reqwest/hyper put on the wire and what the AWS SDK signs.
+    #[test]
+    fn iceberg_mode_signs_the_hex_payload_hash_not_the_base64_header() {


This is the most security-sensitive path — base64 header, hex canonical — and the pinned signature came from the previous hand-rolled signer, not an external reference. If that impl had the split subtly wrong, the pin freezes the bug and this test stays green forever.

signing_key_and_signature_match_aws_vector doesn't close the gap either: it exercises the test-only signing_key helper, while production delegates HMAC to aws_sigv4 — so it validates the helper, not the signer. I'd add at least one end-to-end case whose signature is sourced externally: a Java RESTSigV4AuthSession request for this IcebergRest-with-body case, or an AWS SigV4 suite vector (get-vanilla/post-vanilla) in StandardAws mode. That's the check that would actually catch a divergence from Java or the spec.

Thanks. Added signatures_match_iceberg_java, which pins two requests signed by Java's RESTSigV4AuthSession (iceberg-aws 1.10.1); both match byte for byte. get-vanilla doesn't fit since we always sign x-amz-content-sha256, so the StandardAws test uses the suite's post-x-www-form-urlencoded instead.

laskoviymishka · 2026-09-30T19:41:00Z


+    /// The request URL.
+    #[cfg(feature = "sigv4")]
+    pub(crate) fn url(&self) -> &Url {


Small one: these are #[cfg(feature = "sigv4")] but url_str() just below isn't, and HttpRequest is crate-internal anyway — the gate doesn't hide anything, it just splits the accessors so the next non-sigv4 caller that wants the URL has to re-add it. I'd drop the cfg and leave them unconditional pub(crate).

Kept the gate: the signer is their only caller, so without it the default build fails clippy on dead code. (HttpRequest is public, too.)

plusplusjiajia · 2026-10-02T12:42:49Z

Thanks @laskoviymishka! Pushed fixes and replied inline. The signer is now checked against requests signed by Iceberg Java and an AWS test suite vector. I kept the tracing comment and the sigv4 gate on the URL accessors; reasons inline.

laskoviymishka

Thanks for the quick turnaround — most of round 1 landed cleanly. The content-hash insert propagates now instead of unwrapping, PayloadHashMode is #[non_exhaustive], the sign() tests moved onto sign_at with fixed clocks, signatures_match_iceberg_java plus the AWS-suite post-x-www-form-urlencoded vector give us the external cross-check I wanted, the TRACE branch is actually exercised via CapturedLog, and the freshness contract is spelled out on the sign() doc. That's six of the seven.

The one I'd still hold on is the positional new(region, service, mode) — two adjacent impl Into<String> on a publish = true crate, so the swap-prone shape locks in semver the moment it hits crates.io. typed-builder is already in the workspace, so a builder is cheap; newtypes work too. Specifics inline.

Two smaller things from this pass, both inline: rewrite_url_for_signing discards the set_username/set_password results, which silently defeats the userinfo strip on schemes that reject it, and the tracing comment's reason is still off even though the guard and its test are now correct. And while we're locking the surface — HttpRequestBody::as_bytes() collapses the absent/empty-body distinction that IcebergRest mode depends on, so a custom signer that reaches for it (the doc calls the return "the signable bytes") gets wrong hashes; a doc note or a dedicated three-way accessor would close that before publish.

The constructor's the gate for me — the rest is polish. Close it and I don't think there's anything else holding this up.

laskoviymishka · 2026-10-02T19:25:04Z

+
+impl SigV4Signer {
+    /// Creates a new SigV4 signer.
+    pub fn new(


This is the one round-1 point still open, and the one I'd really like closed before the first publish: new takes two adjacent impl Into<String>, so new("execute-api", "us-east-1", mode) compiles clean and only surfaces as a 403 from a live endpoint. Once this is on crates.io the positional shape is semver-locked, and the swap has no compile-time or runtime cue.

typed-builder is already a workspace dep and used elsewhere in this crate, so SigV4Signer::builder().region(…).service(…).mode(…) is cheap and kills the swap outright; typed newtypes (Region/ServiceName) work too if we'd rather keep a free function.

Done, new is replaced by a typed-builder builder.

laskoviymishka · 2026-10-02T19:25:04Z

+fn rewrite_url_for_signing(request: &mut crate::HttpRequest) {
+    if !request.url().username().is_empty() || request.url().password().is_some() {
+        let url = request.url_mut();
+        let _ = url.set_username("");


I'd propagate these instead of discarding them — set_username/set_password return Result<(), ()> and fail on schemes that don't carry userinfo, so on failure the credentials stay in the URL and we sign (and send) the host we meant to strip, which defeats the whole point of the function. Simplest fix is to have rewrite_url_for_signing return Result<()>, map the Err(()) to a DataInvalid, and ? it at the sign_at call site.

Propagated. (It can't actually fail: only host-less URLs and file: reject the setters, and neither can carry userinfo.)

laskoviymishka · 2026-10-02T19:25:04Z

+
+        // `aws_sigv4` traces the request, `Original-Authorization` included.
+        // Mute it only when a subscriber could record that (the max level is
+        // `OFF` until one is registered): `with_default` marks tracing as in


The branch is exercised now via CapturedLog — thanks, that's exactly what I was after. The comment's reasoning is still off, though: with_default is a scoped, thread-local override that restores the previous dispatcher on exit — it doesn't mark tracing in-use for good or disable the log bridge process-wide; only set_global_default does that. The guard is right, so I'd just correct the stated reason, otherwise someone later removes it thinking the documented risk doesn't apply and starts logging Original-Authorization.

I ran it to be sure. tracing 0.1.44 with the log feature and a counting logger: tracing::info! reaches log before with_default(NoSubscriber, || {}), and never again after the scope ends. set_default stores tracing-core's EXISTS flag, nothing clears it, and the log bridge checks it. The comment now names the flag.

plusplusjiajia · 2026-10-08T02:44:20Z

Thanks @laskoviymishka! Builder, userinfo strip and as_bytes doc done; tracing comment reworded, with a runnable check inline.

laskoviymishka

the gate's closed — new(region, service, mode) is a TypedBuilder now, .region().service().mode().build() at every call site, so the two adjacent Into<String> can't be transposed anymore. and the three smaller round-2 items came with it: rewrite_url_for_signing propagates the set_username/set_password failures as DataInvalid instead of dropping them, the tracing-suppression comment finally matches its guard, and the absent-vs-empty-body distinction is documented on as_bytes() with the signer matching on the variants directly. that's everything I was holding on.

one thing I noticed reading this pass that I don't think came up before: sign_at relocates Authorization, overwrites the content hash, and strips userinfo from the URL before the steps that can fail — so an error (a non-UTF-8 header is the realistic one) leaves the caller holding a half-signed request with its Authorization gone. hoisting the signable_headers validation above convert_headers closes the realistic path, and a line on the error contract covers the rest. that's the one I'd want sorted before merge.

the rest is small and inline — the aws_credential_types::Credentials type sitting bare in the public sign signature (I'd re-export it under the feature), a relocated_name arm that can't fire, and a test or two that'd read better as explicit equivalence checks. none of it changes the shape.

sort the error-path ordering and I'm happy — nice work getting this one over the line.

laskoviymishka · 2026-10-08T22:05:21Z

+    pub fn sign(
+        &self,
+        request: &mut crate::HttpRequest,
+        credentials: &aws_credential_types::Credentials,


I'd re-export this type through the crate under the sigv4 feature (pub use aws_credential_types::Credentials;) rather than naming it bare in the public sign signature. As it stands downstream has to add its own aws-credential-types dep on a compatible 1.x just to construct the argument, and the version coupling isn't visible from our API. Re-exporting makes the dependency explicit and leaves us a seam if we ever want to wrap it.

Re-exported Credentials under sigv4; the public signature now uses it.

laskoviymishka · 2026-10-08T22:05:21Z

+        let body = signable_body(request)?;
+        let content_header = content_sha256_header(body.as_deref(), self.mode);
+
+        convert_headers(request);


sign_at mutates the request here — relocates Authorization, overwrites x-amz-content-sha256, strips userinfo, rewrites the query — before the fallible steps further down (the non-UTF-8 check in signable_headers, SignableRequest::new, sign). On an error the caller gets its request back half-signed: original Authorization gone, our content hash and Original-* headers in place, userinfo and + already stripped from the URL. A caller that retries or falls back on that error then sends something unauthenticated.

The non-UTF-8 header is the only realistic trigger, and it's cheap to close: hoist signable_headers(request)? above convert_headers so we bail before touching anything. For the remaining near-unreachable failures, a line on the sign() doc saying the request is left partially modified on error would cover it.

Thanks for catching this. Headers are now validated before modifying the request, with a regression test. The remaining error behavior is documented.

laskoviymishka · 2026-10-08T22:05:21Z

+fn signable_body(request: &crate::HttpRequest) -> Result<Option<Vec<u8>>> {
+    match request.body() {
+        crate::HttpRequestBody::Empty => Ok(None),
+        crate::HttpRequestBody::Buffered(bytes) => Ok(Some(bytes.to_vec())),


Optional: this to_vec() copies the whole body, and it then gets hashed twice — once for the content header, once inside aws_sigv4 via SignableBody::Bytes. If we compute the hex digest once from the borrowed bytes and pass SignableBody::Precomputed(hex), we drop both the copy and the second hash. Mostly matters for large commit payloads.

The body is now borrowed and hashed once, using SignableBody::Precomputed.

laskoviymishka · 2026-10-08T22:05:21Z

+fn relocated_name(name: &str) -> Option<reqwest::header::HeaderName> {
+    match name {
+        n if n == AMZ_DATE => Some(RELOCATED_AMZ_DATE),
+        n if n == CONTENT_SHA256 => Some(RELOCATED_CONTENT_SHA256),


The CONTENT_SHA256 arm can't fire as things stand: signing_settings uses PayloadChecksumKind::NoHeader, so aws_sigv4 never emits x-amz-content-sha256 among the signing instructions, and the caller's content hash is relocated separately through displaced_content_hash. I'd drop the arm with a comment, or at least note the invariant at signing_settings — otherwise a future flip of the checksum kind would silently double-relocate it.

Removed the unreachable arm and documented the separate content-hash relocation.

laskoviymishka · 2026-10-08T22:05:21Z

+
+        assert_signature_is(
+            &req,
+            "0f4a3487bcff9dd16bf0a42d06c24dc49b2366e8a928dc9666f8424cf5b306b3",


This literal and the one in userinfo_is_stripped_before_signing are both the signature of a plain /v1/config GET — stripping userinfo and collapsing // should land on exactly that request. That equivalence is the real property under test, but it's buried in a magic hex string: if canonicalization drifted and the vectors were regenerated, both would move together and still pass. I'd sign an unmodified /v1/config request in the same test and assert the two signatures are equal.

Both tests now compare against a plain /v1/config request signed with the same credentials and timestamp.

The crate traces the request it signs, and its redaction list covers `authorization` but not the `Original-` copy we make, so a delegate's bearer token could reach trace logs. Also reject non-UTF-8 headers rather than leave them unsigned, and hash a present-but-empty body as Java does.

Java holds one `Aws4Signer` across sessions and resolves credentials from an `AwsCredentialsProvider` per request, so the signer itself carries no credential state. Follow that: drop `AwsCredentials`, take the AWS crate's `Credentials` as a `sign` argument, and leave the provider to the auth session. Also sign `HttpRequest` rather than the concrete request type, name the amz headers, split `convert_headers`/`update_request_headers` after their Java counterparts, collapse the one-file `sigv4` module, and put the AWS dependencies behind a `sigv4` feature.

…ing flag untouched

…new take impl Into<String>

…RL as is

plusplusjiajia · 2026-10-09T07:26:06Z

Thanks @laskoviymishka! All five comments are addressed, with regression coverage.

laskoviymishka · 2026-10-09T16:21:31Z

Would wait for @CTTY here before merging.

CTTY

LGTM! Thanks for your work and your patience

Thanks Andrei for helping with the review!

CTTY · 2026-08-28T21:44:56Z

+#[derive(Clone)]
+pub struct SigV4Signer {
+    region: String,
+    service: String,


This should be just name

CTTY · 2026-10-09T23:14:20Z

+    /// Send the result through a client that does not follow redirects: a
+    /// redirect replays the signature, and across hosts reqwest drops
+    /// `Authorization` but keeps `Original-Authorization`.


I'm happy if we are going to address this by disabling redirect for client in #3092

@CTTY Thanks for the review and merge! In #3092, the default client disables redirects for signing managers.

plusplusjiajia mentioned this pull request Aug 27, 2026

feat(rest): enable SigV4 authentication from catalog properties #2660

Draft

plusplusjiajia marked this pull request as ready for review August 27, 2026 05:37

plusplusjiajia force-pushed the feat/rest-sigv4-signer branch from 70b5cd9 to 47b0bc4 Compare August 27, 2026 05:49

DerGut reviewed Aug 27, 2026

View reviewed changes

CTTY reviewed Aug 28, 2026

View reviewed changes

plusplusjiajia mentioned this pull request Aug 28, 2026

feat(rest): add a SigV4 auth manager and session #3092

Open

plusplusjiajia force-pushed the feat/rest-sigv4-signer branch from 9146e05 to 3586d02 Compare September 10, 2026 06:27

plusplusjiajia force-pushed the feat/rest-sigv4-signer branch from 3586d02 to 8a8d7ae Compare September 20, 2026 18:36

plusplusjiajia force-pushed the feat/rest-sigv4-signer branch from 8649632 to f3e87e0 Compare September 28, 2026 02:53

laskoviymishka requested changes Sep 30, 2026

View reviewed changes

laskoviymishka requested changes Oct 2, 2026

View reviewed changes

plusplusjiajia force-pushed the feat/rest-sigv4-signer branch from 65f3160 to 433a0d8 Compare October 8, 2026 02:37

plusplusjiajia requested a review from laskoviymishka October 8, 2026 09:51

laskoviymishka approved these changes Oct 8, 2026

View reviewed changes

plusplusjiajia added 12 commits October 8, 2026 21:57

feat(rest): add a SigV4 request signer

b3e291e

docs(rest): trim the SigV4 signer comments

3e252bd

refactor(rest): break sign_at into named steps

fc32f42

docs(rest): state the redirect and log-always limits of the signer

89d8f3f

fix(rest): keep relocation headers out of the signature, and the trac…

26bf79d

…ing flag untouched

docs(rest): name the log bridge that stays unmuted

8edf544

docs(rest): log-always also bypasses the muted subscriber

df66c94

fix(rest): return an error instead of unwrapping the content hash header

b9fa6cb

refactor(rest): make PayloadHashMode non_exhaustive and SigV4Signer::…

5d6b772

…new take impl Into<String>

docs(rest): note the base64 header quirk and credential freshness

f147293

plusplusjiajia added 17 commits October 8, 2026 21:57

docs(rest): keep the plus note next to the rewrite it explains

b205a40

fix(rest): spell out the headers Java leaves unsigned

14e0f05

test(rest): check signatures against Iceberg Java

44033c9

test(rest): pin the signing tests to a fixed time

0f02a72

test(rest): check standard mode against the AWS test suite

5aab197

test(rest): drop the helper-only AWS vector and the hmac dev-dependency

22af036

fix(rest): decide the tracing mute with public tracing API

9e9ff82

docs(rest): tighten the payload hash and credential notes

c36c457

test(rest): bound the live-clock stamp by readings around the call

2e6f507

chore(rest): update the public API for the signer changes

7824a3b

docs(rest): simplify the SigV4 signer comments

aba93f6

refactor(rest): build SigV4Signer with a typed builder

e241d07

fix(rest): propagate a failed userinfo strip instead of signing the U…

ee1a9fc

…RL as is

docs(rest): name the flag the tracing mute is gated to avoid

7a79bfc

docs(rest): say that as_bytes merges an absent body with an empty one

edc9d36

chore(rest): update the public API for the signer builder

b367eb3

fix(rest): validate requests before signing

0dab39c

plusplusjiajia force-pushed the feat/rest-sigv4-signer branch from 433a0d8 to 0dab39c Compare October 9, 2026 02:22

fix(rest): address SigV4 review findings

693de9d

CTTY approved these changes Oct 9, 2026

View reviewed changes

CTTY added this pull request to the merge queue Oct 9, 2026

Merged via the queue into apache:main with commit b846751 Oct 9, 2026
24 checks passed

unconsolable mentioned this pull request Oct 10, 2026

feat(aws): support configurable Single/Double URI encoding in AWS SigV4 apache/reqsign#911

Open

	[features]
	default = ["opendal-memory", "opendal-fs", "opendal-s3"]
	opendal-all = [
	"opendal-memory",
	"opendal-fs",
	"opendal-s3",
	"opendal-gcs",
	"opendal-oss",
	"opendal-azdls",
	"opendal-hf",
	]

	opendal-azdls = ["opendal/services-azdls"]
	opendal-fs = ["opendal/services-fs"]
	opendal-gcs = ["opendal/services-gcs"]
	opendal-hf = ["opendal/services-hf"]
	opendal-memory = ["opendal/services-memory"]
	opendal-oss = ["opendal/services-oss"]
	opendal-s3 = ["opendal/services-s3", "reqsign-aws-v4", "reqsign-core"]

Conversation

plusplusjiajia commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

plusplusjiajia commented Aug 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CTTY left a comment

Choose a reason for hiding this comment

Uh oh!

CTTY Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

plusplusjiajia commented Aug 27, 2026 •

edited

Loading

CTTY Aug 27, 2026 •

edited

Loading