Skip to content
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

Fix S3 Express bug where SigV4 session token was incorrectly overriden #3474

Merged
merged 6 commits into from
Mar 11, 2024
Merged
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
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
Loading