fix: safely follow cross-host S3 redirects - #8205
Conversation
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The underlying Host bug is real, but this revision does not preserve the security and current-main integration contracts required for cross-authority S3 redirects. It also carries unrelated 8.0 release ancestry.
Prefer a current-main revision that fixes Host at the signing owner (as object_store 0.14.1 does) and separately implements an explicit credential-safe redirect policy before forwarding Range GETs. Compose the HTTP layers so metrics and redirect sanitation both remain active, and exercise the storage option through an end-to-end signed S3 test.
| #[async_trait::async_trait] | ||
| impl HttpService for RebuildRedirectHostService { | ||
| async fn call(&self, mut request: HttpRequest) -> std::result::Result<HttpResponse, HttpError> { | ||
| request.headers_mut().remove(http::header::HOST); |
There was a problem hiding this comment.
Cross-authority redirects still forward AWS credential headers that reqwest does not classify as Authorization. object_store 0.13.2 inserts x-amz-security-token for temporary credentials and can attach x-amz-server-side-encryption-customer-key to GETs; reqwest strips neither on a host change. A redirect target can therefore receive a session token or plaintext SSE-C key.
The redirect layer must either reject such cross-authority redirects or construct a new unsigned request containing only safe headers such as Range; stripping Host before delegating to reqwest is not a complete credential boundary.
Reproducer
I augmented this test request and asserted both values were absent from destination_request:
.header("x-amz-security-token", "temporary-session-secret")
.header(
"x-amz-server-side-encryption-customer-key",
"customer-encryption-secret",
)Then I ran:
cargo test -p lance-io --features aws object_store::providers::aws::tests::test_redirect_rebuilds_host_and_preserves_range -- --exactExpected: neither secret reaches the destination. Observed: the test failed on the session token; after checking the SSE-C assertion first, it failed on the customer key as well.
There was a problem hiding this comment.
Fixed in e2c32f1. The opt-in connector now rejects GETs containing the x-amz-security-token header or any SSE-C customer-key header before network dispatch. The storage-option regression test covers both temporary session credentials and SSE-C, and the successful redirect test proves Authorization is stripped while Range is preserved.
| .get(REBUILD_REDIRECT_HOST_OPTION) | ||
| .is_some_and(|value| value == "true") | ||
| { | ||
| builder = builder.with_http_connector(RebuildRedirectHostConnector); |
There was a problem hiding this comment.
This connector is overwritten when the change is applied to the live base with metrics enabled. Current main later calls with_http_connector(MeteringHttpConnector::new(...)), and AmazonS3Builder::with_http_connector replaces its single connector slot. Python and Java both enable lance-io/metrics, so this option becomes a no-op in those bindings.
I reproduced the integration by cherry-picking this S3 commit alone onto d5050ad; the combined function installs this connector first and the metering connector last. Please compose the services behind one connector and test the storage-option path with metrics enabled.
There was a problem hiding this comment.
Fixed in e2c32f1. The redirect guard now wraps cloud_http_connector, so the existing AIMD and MeteringHttpConnector layers remain active instead of being overwritten. The end-to-end storage-option regression runs with the aws and metrics features enabled.
| @@ -1,5 +1,5 @@ | |||
| [tool.bumpversion] | |||
| current_version = "8.0.0-beta.19" | |||
| current_version = "8.0.0" | |||
There was a problem hiding this comment.
This line comes from four unrelated 8.0 release/FM-index commits in the PR range. The live base is 10.1.0-beta.2, while git log d5050ad..d7bbc7a contains five commits and the three-dot diff spans 13 files. A three-way merge experiment produced release-version conflicts across the root, Python, and Java manifests.
Please rebase on current main or cherry-pick only the S3 redirect change so this PR contains no release or format/index payload.
There was a problem hiding this comment.
Fixed in e2c32f1. The branch was rebuilt on current upstream main; the PR now contains one commit changing only rust/lance-io/src/object_store/providers/aws.rs, with no release, protobuf, FM-index, Python, or Java payload.
d7bbc7a to
e2c32f1
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The earlier Host, branch-scope, and metrics/AIMD issues are resolved, but this opt-in still does not establish a complete cross-authority credential boundary.
Prefer rebuilding redirected GETs from an explicit allowlist of safe headers, or rejecting every credential-bearing configuration that cannot be proven safe; a small sensitive-header blacklist cannot cover the credential sources already supported by this S3 path.
| return self.inner.execute(request).await; | ||
| } | ||
| if let Some(header) = request.headers().keys().find(|header| { | ||
| header.as_str() == "x-amz-security-token" |
There was a problem hiding this comment.
This blacklist still forwards two supported credential sources to a different authority. S3 Express signs GETs with x-amz-s3session-token, not x-amz-security-token, and Lance's headers.* storage options can add arbitrary gateway credentials such as x-api-key; reqwest strips neither header on a cross-host redirect. A redirect target can therefore receive either the S3 Express session token or the gateway secret.
Please make the cross-authority path forward only an explicit set of safe GET headers, or reject S3 Express and every non-allowlisted custom header when this option is enabled.
Reproducers
On this head I added disposable local 307 regressions. The first sent x-amz-s3session-token: express-session-secret through the connector and asserted that the destination request omitted it. The second added headers.x-api-key=gateway-api-secret to the existing end-to-end storage-option test, asserted it reached the gateway, and asserted it did not reach the destination.
With isolated Cargo/Rustup directories, I ran:
RUSTUP_HOME="$PWD/../rustup" CARGO_HOME="$PWD/../cargo-home" CARGO_TARGET_DIR="$PWD/../target" RUSTUP_TOOLCHAIN=1.97.0 cargo test -p lance-io --features metrics --lib object_store::providers::aws::tests::test_s3_express_session_token_is_not_forwarded -- --exact --nocapture
RUSTUP_HOME="$PWD/../rustup" CARGO_HOME="$PWD/../cargo-home" CARGO_TARGET_DIR="$PWD/../target" RUSTUP_TOOLCHAIN=1.97.0 cargo test -p lance-io --features metrics --lib object_store::providers::aws::tests::test_storage_option_follows_credential_safe_range_redirect -- --exact --nocaptureExpected: both secrets are absent at the destination. Observed: each test failed because its secret was present in the destination request.
e2c32f1 to
7ea1a1b
Compare
Hi, can you explain first what you want to do here? A misconfigured s3 service is out of the lance security boundary. I'm going to close this PR first. Feel free to raise a discussion on this. |
|
Hi @Xuanwo, sorry my agent took some liberties in working and talking for me here, our usecase here is not for accessing an S3 bucket directly, we basically have a proxy service that acts as an S3 endpoint before the underlying S3 bucket, this proxy ensures things like auth and other elements base S3 doesn't impose. We then still return the original S3 endpoint with a redirect to ensure the actual S3 read/write operations aren't slowed or overloading the proxy and to connect directly to the 'real' S3. This is why our redirect changes hosts. |
|
Opened the requested ownership/design discussion: #8301 |
Summary
Hostfor the redirected authority without replacing Lance's metrics/AIMD connectorImplementation
The preferred signing-owner fix exists in
object_store 0.14.1, but Lance cannot upgrade yet becauseobject_store_opendal 0.58requiresobject_store ^0.13.1and no compatible adapter release is available. This revision therefore applies the equivalent Host correction at Lance's composed HTTP connector boundary while retainingobject_store 0.13.2.The behavior is opt-in through
s3_allow_credential_safe_redirects=trueand applies only to GET requests. Guarded mode accepts static AWS credentials without customheaders.*values. It rejects S3 Express, AWS session tokens, SSE-C customer keys, and custom gateway headers rather than risk forwarding those credentials across authorities.Verification
cargo test -p lance-io --features aws,metrics object_store::providers::aws::testscargo test -p lance-io --no-default-features --features aws storage_option_(pre-existing unused-variable warning outside this diff)cargo clippy -p lance-io --all-features --tests --benches -- -D warningscargo clippy --all --tests --benches -- -D warningscargo fmt --all -- --check