feat(rest): Add support for AWS SigV4 signing#2311
feat(rest): Add support for AWS SigV4 signing#2311rchowell wants to merge 4 commits intoapache:mainfrom
Conversation
|
Thanks for picking up the ball, @rchowell! Is this change working for you? If so, under what scenario, i.e. what endpoint, and how are you providing credentials? I've tried it against a Glue Iceberg REST endpoint ( |
Three issues prevented SigV4 signing from working against AWS Glue: 1. reqsign Context was missing OsEnv, so EnvCredentialProvider could not read AWS_ACCESS_KEY_ID etc. from the environment. 2. reqsign defaults to UNSIGNED-PAYLOAD for x-amz-content-sha256, but Glue rejects this. Compute the actual SHA256 of the request body and set the header explicitly. 3. update_with() inverted precedence: a /v1/config response containing sigv4 props would replace an existing signer with a freshly-built DefaultCredentialProvider one. Prefer the existing signer. Also stop copying application headers (content-type, user-agent, x-client-version) into signing parts -- including them in SignedHeaders caused mismatches against servers that don't verify them identically. In addition, open up the signing API so consumers can plug in their own credential providers (e.g. the AWS SDK's SharedCredentialsProvider for SSO / credential_process / profile auth): - Make SigV4Signer and HttpRequestSigner public. - Add SigV4Signer::with_credential_provider(...) accepting any impl ProvideCredential<Credential = Credential>. - Add RestCatalogBuilder::with_signer(...) to inject a pre-built signer. - Re-export the signing module as pub mod signing.
|
Rebased on latest main and incorporated @mdub's fixes from rchowell#1 (thanks!):
Verified end-to-end against the AWS Glue Iceberg REST endpoint ( |
Kurtiscwright
left a comment
Kurtiscwright
left a comment
There was a problem hiding this comment.
@rchowell thank you for working on this. I did the following testing:
I checked out the branch at 5c792d8 and verified it end-to-end against AWS S3Tables (https://s3tables.us-east-2.amazonaws.com/iceberg), using the AWS SDK's credential chain bridged into SigV4Signer via with_signer(...) + SigV4Signer::with_credential_provider(...). list_namespaces and list_tables both succeed, which confirms the patch_uri_for_signing double-encoding workaround is doing its job for ARN path segments.
Please see the comments, in general the PR looks good. I just had a two minor nitpicks and a question around intent for the new HttpRequestSigner trait's generalizability.
| /// The AWS service name to use for SigV4 signing (e.g. "s3", "execute-api") | ||
| pub const REST_CATALOG_PROP_SIGNING_NAME: &str = "rest.signing-name"; | ||
| /// The AWS region to use for SigV4 signing (e.g. "us-east-1") | ||
| pub const REST_CATALOG_PROP_SIGNING_REGION: &str = "rest.signing-region"; |
There was a problem hiding this comment.
Nitpick: Should the three SigV4 const variables, above this comment, live in signing.rs? They are AWS specific, and keeping them next to the signer would make the signing module self-contained as more auth schemes are added later.
| /// AWS SigV4 requires. By replacing `%` with `%25` before signing, reqsign's | ||
| /// decode→reencode cycle produces the correct double-encoded form. | ||
| /// | ||
| /// TODO: remove once fixed upstream in apache/opendal (reqsign-aws-v4). |
There was a problem hiding this comment.
Is there an upstream issue tracking this in apache/opendal?
|
|
||
| /// A trait for signing HTTP requests. | ||
| #[async_trait] | ||
| pub trait HttpRequestSigner: Send + Sync + Debug { |
There was a problem hiding this comment.
The HttpRequestSigner trait is named generically, which suggests future signer implementations may be added. If that is the intent, the config-to-signer logic in RestCatalogConfig::signer (and the TryFrom impl it delegates to) currently only recognizes rest.sigv4-enabled and builds a SigV4 signer.
Adding a second signer later would then force if-else chaining (if cfg.sigv4_enabled() { ... } else if cfg.azure_enabled() { ... }) which isn't ergonomic. Alternatively a second signer could refactor to a single rest.signer-type discriminant property plus per-type namespaces, but would be a breaking change for anyone already using the SigV4 properties.
The current shape works fine for SigV4 but may surprise a future contributor who adds a second signer and finds their requests silently unsigned because the SigV4 check fell through first.
If SigV4 is the only signer this crate will ship, and other signers are expected to come in through the with_signer(...) method on RestCatalogBuilder, that is a reasonable position too. In that case the trait name could be tightened to something like SigV4RequestSigner to signal that intent.
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Suggestion (not blocking): would it be worth adding a small test that uses a mock HttpRequestSigner to confirm HttpClient::execute calls sign_request on the signer before executing? A recording mock with a counter would make this cheap and would protect the extension point from silent regression.
3 participants
Which issue does this PR close?
What changes are included in this PR?
signingmod inside therestmod.HttpRequestSignertrait for signing headers.Are these changes tested?
Additional Context
There are several PRs in this space, and I did my best to consolidate them all into this.
(1) #917
This is the first and @phillipleblanc suggested an implementation without the aws crates.
(2) #1241
This is the reqwest approach, but @xxchan found a bug in url encoding. It uses reqsign-aws-v4.
(3) #2088
This is a nice approach for more general auth, and it may be best to have a discussion on the design so that a large PR like this is reviewable. This new PR is intentionally smaller in scope to hopefully close at #1236.
(4) mdub@4400539
Credit to @mdub here which was the latest reference on the issue. It's a reqwest-based approach. I've adapted this based on the discussions in (1) and (2).