Skip to content

Commit

Permalink
Fix S3 Express bug where SigV4 session token was incorrectly overriden (
Browse files Browse the repository at this point in the history
#3474)

## Description
S3 express canary exposed a bug introduced in smithy-rs#3457 where the
code overwrote the regular SigV4 session token name with the S3
Expression session token name when it shouldn't.
```
    4: Error { code: "InvalidRequest", message: "CreateSession request should not include \"x-amz-s3session-token\"", aws_request_id: "01c0c8864e00018e20558a130509f37740b906e4", s3_extended_request_id: "DGTJhRqVMbZdAHQ" }
```
For APIs like `ListDirectoryBuckets` or `CreateSession`, we should not
overwrite `x-amz-security-token` with `x-amz-s3session-token` in the
request header.

In the said PR, `aws_sdk_s3::s3_express::utils::for_s3_express` did not
take into account auth schemes attached to a resolved endpoint, failing
to detect that it should not override the session token name for the
said APIs. This PR will resolve that issue.

## Testing
- Added an integration test verifying the fix (this test currently fails
when run in the destination branch, `ysaito/s3express`)
- Verified all canary (including wasm, s3express) passed

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
  • Loading branch information
ysaito1001 authored Mar 11, 2024
1 parent 3c3843f commit 3b8f2f4
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 14 deletions.
17 changes: 12 additions & 5 deletions aws/rust-runtime/aws-inlineable/src/s3_express.rs
Original file line number Diff line number Diff line change
Expand Up @@ -798,14 +798,21 @@ pub(crate) mod utils {
use aws_smithy_types::{config_bag::ConfigBag, Document};

pub(crate) fn for_s3_express(cfg: &ConfigBag) -> bool {
// logic borrowed from aws_smithy_runtime::client::orchestrator::auth::extract_endpoint_auth_scheme_config
let endpoint = cfg
.load::<crate::config::endpoint::Endpoint>()
.expect("endpoint added to config bag by endpoint orchestrator");

if let Some(Document::String(backend)) = endpoint.properties().get("backend") {
backend.as_str() == "S3Express"
} else {
false
}
let auth_schemes = match endpoint.properties().get("authSchemes") {
Some(Document::Array(schemes)) => schemes,
_ => return false,
};
auth_schemes.iter().any(|doc| {
let config_scheme_id = doc
.as_object()
.and_then(|object| object.get("name"))
.and_then(Document::as_string);
config_scheme_id == Some(crate::s3_express::auth::SCHEME_ID.as_str())
})
}
}
38 changes: 30 additions & 8 deletions aws/sdk/integration-tests/s3/tests/express.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,34 @@ where
aws_sdk_s3::Client::from_conf(update_builder(config).build())
}

#[tokio::test]
async fn create_session_request_should_not_include_x_amz_s3session_token() {
let (http_client, request) = capture_request(None);
// There was a bug where a regular SigV4 session token was overwritten by an express session token
// even for CreateSession API request.
// To exercise that code path, it is important to include credentials with a session token below.
let conf = Config::builder()
.http_client(http_client)
.region(Region::new("us-west-2"))
.credentials_provider(::aws_credential_types::Credentials::for_tests_with_session_token())
.build();
let client = Client::from_conf(conf);

let _ = client
.list_objects_v2()
.bucket("s3express-test-bucket--usw2-az1--x-s3")
.send()
.await;

let req = request.expect_request();
assert!(
req.headers().get("x-amz-create-session-mode").is_some(),
"`x-amz-create-session-mode` should appear in headers of the first request when an express bucket is specified"
);
assert!(req.headers().get("x-amz-security-token").is_some());
assert!(req.headers().get("x-amz-s3session-token").is_none());
}

#[tokio::test]
async fn mixed_auths() {
let _logs = capture_test_logs();
Expand Down Expand Up @@ -292,10 +320,7 @@ async fn disable_s3_express_session_auth_at_service_client_level() {

let req = request.expect_request();
assert!(
!req.headers()
.get("authorization")
.unwrap()
.contains("x-amz-create-session-mode"),
req.headers().get("x-amz-create-session-mode").is_none(),
"x-amz-create-session-mode should not appear in headers when S3 Express session auth is disabled"
);
}
Expand All @@ -320,10 +345,7 @@ async fn disable_s3_express_session_auth_at_operation_level() {

let req = request.expect_request();
assert!(
!req.headers()
.get("authorization")
.unwrap()
.contains("x-amz-create-session-mode"),
req.headers().get("x-amz-create-session-mode").is_none(),
"x-amz-create-session-mode should not appear in headers when S3 Express session auth is disabled"
);
}
Expand Down
14 changes: 13 additions & 1 deletion tools/ci-cdk/lib/aws-sdk-rust/canary-stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,14 +193,26 @@ export class CanaryStack extends Stack {
}));

// Allow canaries to perform operations on test express bucket
// Unlike S3, no need to grant separate permissions for GetObject, PutObject, and so on because
// the session token enables access instead:
// https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-express-security-iam.html#s3-express-security-iam-actions
this.lambdaExecutionRole.addToPolicy(
new PolicyStatement({
actions: ['s3express:*'],
actions: ['s3express:CreateSession'],
effect: Effect.ALLOW,
resources: [`${this.canaryTestExpressBucket.attrArn}`],
})
);

// Allow canaries to list directory buckets
this.lambdaExecutionRole.addToPolicy(
new PolicyStatement({
actions: ['s3express:ListAllMyDirectoryBuckets'],
effect: Effect.ALLOW,
resources: ["*"],
})
);

// Allow canaries to call Transcribe's StartStreamTranscription
this.lambdaExecutionRole.addToPolicy(
new PolicyStatement({
Expand Down

0 comments on commit 3b8f2f4

Please sign in to comment.