Skip to content

feat(rest): support AWS SIGv4 #1241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 5 additions & 13 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ datafusion-cli = "45"
datafusion-sqllogictest = "45"
derive_builder = "0.20"
dirs = "6"
expect-test = "1"
enum-ordinalize = "4.3.0"
expect-test = "1"
faststr = "0.2.31"
fnv = "1.0.7"
fs-err = "3.1.0"
Expand Down Expand Up @@ -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" }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

reqwest = { version = "0.12.12", default-features = false, features = ["json"] }
roaring = { version = "0.10", git = "https://github.com/RoaringBitmap/roaring-rs.git" }
rust_decimal = "1.36"
Expand All @@ -108,7 +109,7 @@ tempfile = "3.18"
tera = "1"
thrift = "0.17.0"
tokio = { version = "1.44", default-features = false }
toml = "0.8"
toml = "0.8.9"
tracing = "0.1.37"
tracing-subscriber = "0.3.8"
typed-builder = "0.20"
Expand Down
1 change: 1 addition & 0 deletions crates/catalog/rest/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ tokio = { workspace = true, features = ["sync"] }
tracing = { workspace = true }
typed-builder = { workspace = true }
uuid = { workspace = true, features = ["v4"] }
reqsign = { workspace = true }

[dev-dependencies]
ctor = { workspace = true }
Expand Down
33 changes: 32 additions & 1 deletion crates/catalog/rest/src/catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@ use std::collections::HashMap;
use std::str::FromStr;

use async_trait::async_trait;
use iceberg::io::FileIO;
use iceberg::io::{FileIO, FileIOBuilder};
use iceberg::table::Table;
use iceberg::{
Catalog, Error, ErrorKind, Namespace, NamespaceIdent, Result, TableCommit, TableCreation,
TableIdent,
};
use itertools::Itertools;
use reqsign::{AwsConfig, AwsDefaultLoader, AwsV4Signer};
use reqwest::header::{
HeaderMap, HeaderName, HeaderValue, {self},
};
Expand Down Expand Up @@ -84,6 +85,29 @@ impl RestCatalogConfig {
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the keys should use constants rather magic strings.

if let Some("true") = self.props.get("rest.sigv4-enabled").map(|s| s.as_str()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use bool::from_str here?

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string should be formatted using constants

));
};
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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

));
};

let config = AwsConfig::default().from_profile().from_env();
let loader = AwsDefaultLoader::new(self.client().unwrap_or_default(), config);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let signer = AwsV4Signer::new(signing_name, signing_region);
Ok(Some((loader, signer)))
} else {
Ok(None)
}
}
fn namespaces_endpoint(&self) -> String {
self.url_prefixed(&["namespaces"])
}
Expand Down Expand Up @@ -306,6 +330,13 @@ impl RestCatalog {
None => None,
};

if let Some(warehouse_path) = warehouse_path {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

let file_io = match warehouse_path.or(metadata_location) {
Some(url) => FileIO::from_path(url)?.with_props(props).build()?,
None => {
Expand Down
42 changes: 41 additions & 1 deletion crates/catalog/rest/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@
use std::collections::HashMap;
use std::fmt::{Debug, Formatter};

use http::StatusCode;
use http::{HeaderValue, StatusCode};
use iceberg::{Error, ErrorKind, Result};
use reqsign::{AwsDefaultLoader, AwsV4Signer};
use reqwest::header::HeaderMap;
use reqwest::{Client, IntoUrl, Method, Request, RequestBuilder, Response};
use serde::de::DeserializeOwned;
Expand All @@ -43,6 +44,8 @@ pub(crate) struct HttpClient {
extra_headers: HeaderMap,
/// Extra oauth parameters to be added to each authentication request.
extra_oauth_params: HashMap<String, String>,

signer: Option<(AwsDefaultLoader, AwsV4Signer)>,
}

impl Debug for HttpClient {
Expand All @@ -65,6 +68,7 @@ impl HttpClient {
credential: cfg.credential(),
extra_headers,
extra_oauth_params: cfg.extra_oauth_params(),
signer: cfg.get_signer()?,
})
}

Expand All @@ -88,6 +92,7 @@ impl HttpClient {
extra_oauth_params: (!cfg.extra_oauth_params().is_empty())
.then(|| cfg.extra_oauth_params())
.unwrap_or(self.extra_oauth_params),
signer: cfg.get_signer()?,
})
}

Expand Down Expand Up @@ -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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to change request method.

Ok(Some(credential)) => {
const EMPTY_STRING_SHA256: &str =
"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";
request.headers_mut().insert(
"x-amz-content-sha256",
HeaderValue::from_str(EMPTY_STRING_SHA256).unwrap(),
);
Comment on lines +232 to +237

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to hardcode this here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also quite confused.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@liurenjie1024 liurenjie1024 Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if let Err(e) = signer.sign(&mut request, &credential) {
return Err(Error::new(
ErrorKind::Unexpected,
"Failed to sign request for sigv4 signing",
)
.with_source(e));
}
}
Ok(None) => {
return Err(Error::new(
ErrorKind::Unexpected,
"Credential not found for sigv4 signing",
));
}
Err(e) => {
return Err(Error::new(
ErrorKind::Unexpected,
"Failed to load credential for sigv4 signing",
)
.with_source(e));
}
}
}
Ok(self.client.execute(request).await?)
}

Expand Down Expand Up @@ -255,6 +293,7 @@ pub(crate) async fn deserialize_catalog_response<R: DeserializeOwned>(
/// codes that all endpoints share (400, 404, etc.).
pub(crate) async fn deserialize_unexpected_catalog_error(response: Response) -> Error {
let (status, headers) = (response.status(), response.headers().clone());
let url = response.url().to_string();
let bytes = match response.bytes().await {
Ok(bytes) => bytes,
Err(err) => return err.into(),
Expand All @@ -264,4 +303,5 @@ pub(crate) async fn deserialize_unexpected_catalog_error(response: Response) ->
.with_context("status", status.to_string())
.with_context("headers", format!("{:?}", headers))
.with_context("json", String::from_utf8_lossy(&bytes))
.with_context("url", url)
}
Loading