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 a reqwest::Request for AWS SigV4, built on the official aws-sigv4 crate (already in the workspace lock via aws-config). It handles the parts Iceberg needs 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.
expect, connection and x-forwarded-for are excluded from signing, as Java's AbstractAws4Signer does — a proxy may rewrite them.
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 auth manager that uses this, and its catalog wiring, follow in #2660.

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.

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.

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.

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.

plusplusjiajia mentioned this pull request Aug 27, 2026

feat(rest): add SigV4 auth manager for the REST catalog #2660

Open

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

feat(rest): add a SigV4 request signer

47b0bc4

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 added 3 commits August 27, 2026 23:55

docs(rest): trim the SigV4 signer comments

c38496f

refactor(rest): break sign_at into named steps

9146e05

plusplusjiajia mentioned this pull request Aug 28, 2026

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

Draft

	[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"]

Uh oh!

Conversation

plusplusjiajia commented Aug 27, 2026

Which issue does this PR close?

What changes are included in this PR?

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CTTY Aug 27, 2026 •

edited

Loading