feat(rest): support AWS SIGv4 #1241

xxchan · 2025-04-24T02:39:49Z

Signed-off-by: xxchan xxchan22f@gmail.com

Which issue does this PR close?

There's one missing piece: reqsign got canonical request like:

/iceberg/v1/arn%3Aaws%3As3tables%3Aap-southeast-1...
but should be /iceberg/v1/arn%253Aaws%253As3tables%253Aap-southeast-1%..., where reqsign wrongly decoded %3.

During my local testing, I tried to remove this percent_decode_str in
reqsign https://github.com/Xuanwo/reqsign/blob/26a1e224b5498a59ad53d9508be1e85df322f043/src/aws/v4.rs#L223 , then it runs successfully (I tested with the iceberg-rust CLI from #1220). But I'm not sure about the decode reason here and what's the best way to change it. cc @Xuanwo

What changes are included in this PR?

Are these changes tested?

Signed-off-by: xxchan <xxchan22f@gmail.com>

ananthaksr · 2025-04-24T03:57:52Z

crates/catalog/rest/src/client.rs

+                    const EMPTY_STRING_SHA256: &str =
+                        "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";
+                    request.headers_mut().insert(
+                        "x-amz-content-sha256",
+                        HeaderValue::from_str(EMPTY_STRING_SHA256).unwrap(),
+                    );


Why do we have to hardcode this here?

I'm also quite confused.

By default, reqsign will fill x-amz-content-sha256=UNSIGNED-PAYLOAD. If signing request body is needed, caller should fill a x-amz-content-sha256 themselves.

UNSIGNED-PAYLOAD works for s3, but it seems not working for s3tables. Therefore, I tried to include SHA256("")= e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855, and it works..

I also not sure if this is the most correct way, whether we should support this natively in reqsign.

I think this behavor should be hidden under reqsign. cc @Xuanwo WDYT?

ananthaksr · 2025-04-24T04:02:15Z

crates/catalog/rest/src/client.rs

@@ -220,6 +225,39 @@ impl HttpClient {
    /// Executes the given `Request` and returns a `Response`.
    pub async fn execute(&self, mut request: Request) -> Result<Response> {
        request.headers_mut().extend(self.extra_headers.clone());
+
+        if let Some((loader, signer)) = &self.signer {
+            match loader.load().await {


Do we want to load credentials with every request?

let config = AwsConfig::default().from_profile().from_env(); println!("access_key_id {:?}", config.access_key_id); let loader = AwsDefaultLoader::new(self.client().unwrap_or_default(), config);

We also need to change request method.

ananthaksr · 2025-04-24T04:03:35Z

Cargo.toml

@@ -93,6 +93,7 @@ port_scanner = "0.1.5"
 pretty_assertions = "1.4"
 rand = "0.8.5"
 regex = "1.10.5"
+reqsign = { version = "0.16.3" }


Is there a reason for not using https://crates.io/crates/aws-sigv4 ?

reqsign is lightweight, with minimal dependency footprints. And it's already depended by us (transitively via opendal ). So it's good not to introduce new heavy dependencies.

@Xuanwo could you share your opinions on this?

I'm fine with reqsign as its api will be easier to get adapted to different vendors, and also aws-sigv4 itself says it's not designed for used directly, see

Low-level SigV4 request signing implementations.

This crate is part of the AWS SDK for Rust and the smithy-rs code generator. In most cases, it should not be used directly.

ananthaksr · 2025-04-24T04:06:31Z

crates/catalog/rest/src/catalog.rs

+            if warehouse_path.starts_with("arn:aws:") {
+                let file_io = FileIOBuilder::new("s3").with_props(&props).build()?;
+                return Ok(file_io);
+            }


Is there a better way to know the rest catalog service e.g. using rest.signing-name?

ananthaksr · 2025-04-24T14:46:22Z

crates/catalog/rest/src/catalog.rs

+            let config = AwsConfig::default().from_profile().from_env();
+            println!("access_key_id {:?}", config.access_key_id);
+            let loader = AwsDefaultLoader::new(self.client().unwrap_or_default(), config);


Can we support customizing this like passing or reusing the aws profile / credentials configuration or better introduce a new set of rest specific configurations.

liurenjie1024

Thanks @xxchan for this pr, generally the direction is right, but there are some things to fix. Also one of my concerns is that there is no tests at all, how should we test this feature?

liurenjie1024 · 2025-04-28T13:27:33Z

Cargo.toml

@@ -93,6 +93,7 @@ port_scanner = "0.1.5"
 pretty_assertions = "1.4"
 rand = "0.8.5"
 regex = "1.10.5"
+reqsign = { version = "0.16.3" }


I'm fine with reqsign as its api will be easier to get adapted to different vendors, and also aws-sigv4 itself says it's not designed for used directly, see

Low-level SigV4 request signing implementations.

This crate is part of the AWS SDK for Rust and the smithy-rs code generator. In most cases, it should not be used directly.

liurenjie1024 · 2025-04-28T13:28:25Z

crates/catalog/rest/src/catalog.rs

@@ -84,6 +85,29 @@ impl RestCatalogConfig {
        }
    }

+    pub(crate) fn get_signer(&self) -> Result<Option<(AwsDefaultLoader, AwsV4Signer)>> {


I don't think we should credential store loader, why not store credential?

All the keys should use constants rather magic strings.

liurenjie1024 · 2025-04-28T13:30:16Z

crates/catalog/rest/src/catalog.rs

@@ -84,6 +85,29 @@ impl RestCatalogConfig {
        }
    }

+    pub(crate) fn get_signer(&self) -> Result<Option<(AwsDefaultLoader, AwsV4Signer)>> {
+        if let Some("true") = self.props.get("rest.sigv4-enabled").map(|s| s.as_str()) {


Why not use bool::from_str here?

liurenjie1024 · 2025-04-28T13:31:15Z

crates/catalog/rest/src/catalog.rs

+            let Some(signing_region) = self.props.get("rest.signing-region") else {
+                return Err(Error::new(
+                    ErrorKind::Unexpected,
+                    "rest.signing-region is not set when rest.sigv4-enabled is true",


This string should be formatted using constants

liurenjie1024 · 2025-04-28T13:31:22Z

crates/catalog/rest/src/catalog.rs

+            let Some(signing_name) = self.props.get("rest.signing-name") else {
+                return Err(Error::new(
+                    ErrorKind::Unexpected,
+                    "rest.signing-name is not set when rest.sigv4-enabled is true",


liurenjie1024 · 2025-04-28T13:32:09Z

crates/catalog/rest/src/catalog.rs

+            };
+
+            let config = AwsConfig::default().from_profile().from_env();
+            let loader = AwsDefaultLoader::new(self.client().unwrap_or_default(), config);


Not only from profile, we should also allow user to config using properties, for example aws_access_id

liurenjie1024 · 2025-04-28T13:33:20Z

crates/catalog/rest/src/catalog.rs

@@ -306,6 +330,13 @@ impl RestCatalog {
            None => None,
        };

+        if let Some(warehouse_path) = warehouse_path {


Look at below codes, we should also consider metadata_location. Also please add some comments to explain this change.

liurenjie1024 · 2025-04-28T13:34:12Z

crates/catalog/rest/src/client.rs

+                    const EMPTY_STRING_SHA256: &str =
+                        "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";
+                    request.headers_mut().insert(
+                        "x-amz-content-sha256",
+                        HeaderValue::from_str(EMPTY_STRING_SHA256).unwrap(),
+                    );


I'm also quite confused.

liurenjie1024 · 2025-04-28T13:35:15Z

crates/catalog/rest/src/client.rs

@@ -220,6 +225,39 @@ impl HttpClient {
    /// Executes the given `Request` and returns a `Response`.
    pub async fn execute(&self, mut request: Request) -> Result<Response> {
        request.headers_mut().extend(self.extra_headers.clone());
+
+        if let Some((loader, signer)) = &self.signer {
+            match loader.load().await {


We also need to change request method.

xxchan · 2025-04-28T14:51:01Z

Thanks @xxchan for this pr, generally the direction is right, but there are some things to fix.

Hi @liurenjie1024 , thanks for the review. This PR is more like a quick demo to see if it can work (seem to work via manual testing with the CLI), and if the general idea (of using reqsign) looks good. But as mentioned, there indeed are some minor problems related with reqsign. I might need some help from @Xuanwo to proceed. After that, I can resolve the PR comments to make it complete.

phillipleblanc · 2025-05-01T14:22:50Z

Here is another approach I implemented a while back that might be interesting to reference: #917

xxchan · 2025-05-01T16:09:30Z

@phillipleblanc Sorry for not noticing your PR! Will check it

But it seems at least we are on the same page about not to depend on the aws crates for signing (#917 (comment))

phillipleblanc · 2025-05-01T16:23:14Z

Yes, if we can implement this without depending on the aws sigv4 crate - that would be ideal.

feat(rest): support AWS SIGv4

1685873

Signed-off-by: xxchan <xxchan22f@gmail.com>

xxchan mentioned this pull request Apr 24, 2025

REST catalog: support AWS sigV4 #1236

Open

xxchan added 2 commits April 24, 2025 10:41

fix min lock

d7430ee

Signed-off-by: xxchan <xxchan22f@gmail.com>

cleanup

e698d79

Signed-off-by: xxchan <xxchan22f@gmail.com>

xxchan force-pushed the xxchan/cheerful-mite branch from 8a99af2 to e698d79 Compare April 24, 2025 02:51

ananthaksr reviewed Apr 24, 2025

View reviewed changes

liurenjie1024 reviewed Apr 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rest): support AWS SIGv4 #1241

feat(rest): support AWS SIGv4 #1241

xxchan commented Apr 24, 2025 •

edited

Loading

ananthaksr Apr 24, 2025

liurenjie1024 Apr 28, 2025

xxchan Apr 28, 2025

liurenjie1024 Apr 29, 2025 •

edited

Loading

ananthaksr Apr 24, 2025

liurenjie1024 Apr 28, 2025

ananthaksr Apr 24, 2025

xxchan Apr 24, 2025

liurenjie1024 Apr 28, 2025

ananthaksr Apr 24, 2025

ananthaksr Apr 24, 2025

liurenjie1024 left a comment

liurenjie1024 Apr 28, 2025

liurenjie1024 Apr 28, 2025

liurenjie1024 Apr 28, 2025

liurenjie1024 Apr 28, 2025

liurenjie1024 Apr 28, 2025

liurenjie1024 Apr 28, 2025

liurenjie1024 Apr 28, 2025

liurenjie1024 Apr 28, 2025

liurenjie1024 Apr 28, 2025

liurenjie1024 Apr 28, 2025

xxchan commented Apr 28, 2025

phillipleblanc commented May 1, 2025

xxchan commented May 1, 2025

phillipleblanc commented May 1, 2025

feat(rest): support AWS SIGv4 #1241

Are you sure you want to change the base?

feat(rest): support AWS SIGv4 #1241

Conversation

xxchan commented Apr 24, 2025 • edited Loading

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurenjie1024 Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan commented Apr 28, 2025

phillipleblanc commented May 1, 2025

xxchan commented May 1, 2025

phillipleblanc commented May 1, 2025

xxchan commented Apr 24, 2025 •

edited

Loading

liurenjie1024 Apr 29, 2025 •

edited

Loading