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

enhancement(aws provider): Add ability to disable request signing #20973

Open
wants to merge 3 commits into
base: master
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
AWS components can now have request signing disabled by setting `auth.sign: false` on AWS
components. This can be useful, for example, when sending anonymous requests to AWS S3.
47 changes: 40 additions & 7 deletions src/aws/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,14 @@ pub enum AwsAuthentication {
/// [aws_region]: https://docs.aws.amazon.com/general/latest/gr/rande.html#regional-endpoints
#[configurable(metadata(docs::examples = "us-west-2"))]
region: Option<String>,

/// Whether to sign requests.
///
/// Setting this to false can be useful when writing to an AWS S3 bucket that allows
/// anonymous requests.
#[serde(default = "crate::serde::default_true")]
#[derivative(Default(value = "true"))]
sign: bool,
},
}

Expand Down Expand Up @@ -239,7 +247,7 @@ impl AwsAuthentication {
service_region: Region,
proxy: &ProxyConfig,
tls_options: &Option<TlsConfig>,
) -> crate::Result<SharedCredentialsProvider> {
) -> crate::Result<Option<SharedCredentialsProvider>> {
match self {
Self::AccessKey {
access_key_id,
Expand All @@ -265,9 +273,9 @@ impl AwsAuthentication {

let provider = builder.build_from_provider(provider).await;

return Ok(SharedCredentialsProvider::new(provider));
return Ok(Some(SharedCredentialsProvider::new(provider)));
}
Ok(provider)
Ok(Some(provider))
}
AwsAuthentication::File {
credentials_file,
Expand All @@ -288,7 +296,7 @@ impl AwsAuthentication {
.profile_name(profile)
.configure(&provider_config)
.build();
Ok(SharedCredentialsProvider::new(profile_provider))
Ok(Some(SharedCredentialsProvider::new(profile_provider)))
}
AwsAuthentication::Role {
assume_role,
Expand All @@ -313,17 +321,23 @@ impl AwsAuthentication {
)
.await;

Ok(SharedCredentialsProvider::new(provider))
Ok(Some(SharedCredentialsProvider::new(provider)))
}
AwsAuthentication::Default { imds, region, .. } => Ok(SharedCredentialsProvider::new(
AwsAuthentication::Default { sign: false, .. } => Ok(None),
AwsAuthentication::Default {
sign: true,
imds,
region,
..
} => Ok(Some(SharedCredentialsProvider::new(
default_credentials_provider(
region.clone().map(Region::new).unwrap_or(service_region),
proxy,
tls_options,
*imds,
)
.await?,
)),
))),
}
}

Expand Down Expand Up @@ -411,6 +425,7 @@ mod tests {
load_timeout_secs: Some(10),
imds: ImdsAuthentication { .. },
region: None,
sign: true,
}
));
}
Expand Down Expand Up @@ -453,6 +468,7 @@ mod tests {
connect_timeout: CONNECT_TIMEOUT,
read_timeout: READ_TIMEOUT,
},
sign: true,
}
));
}
Expand Down Expand Up @@ -676,4 +692,21 @@ mod tests {
_ => panic!(),
}
}

#[test]
fn parsing_sign_false() {
let config = toml::from_str::<ComponentConfig>(
r#"
auth.sign = false
"#,
)
.unwrap();

match config.auth {
AwsAuthentication::Default { sign, .. } => {
assert!(!sign);
}
_ => panic!(),
}
Comment on lines +705 to +710
Copy link
Member

Choose a reason for hiding this comment

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

This could probably be more concise as a assert!(matches!(...))

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, true, though I like it as-is since it matches the other tests here.

}
}
26 changes: 14 additions & 12 deletions src/aws/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use aws_smithy_runtime_api::client::{
runtime_components::RuntimeComponents,
};
use aws_smithy_types::body::SdkBody;
use aws_types::sdk_config::SharedHttpClient;
use aws_types::sdk_config::{SharedAsyncSleep, SharedHttpClient};
use bytes::Bytes;
use futures_util::FutureExt;
use http::HeaderMap;
Expand Down Expand Up @@ -201,25 +201,27 @@ pub async fn create_client_and_region<T: ClientBuilder>(
};

// Build the configuration first.
let mut config_builder = SdkConfig::builder()
.http_client(connector)
.sleep_impl(Arc::new(TokioSleep::new()))
.identity_cache(auth.credentials_cache().await?)
.credentials_provider(
let mut config_builder = SdkConfig::builder();

config_builder
.set_http_client(Some(SharedHttpClient::new(connector)))
.set_sleep_impl(Some(SharedAsyncSleep::new(Arc::new(TokioSleep::new()))))
.set_identity_cache(Some(auth.credentials_cache().await?))
.set_region(Some(region.clone()))
.set_retry_config(Some(retry_config.clone()))
Comment on lines +207 to +211
Copy link
Member

Choose a reason for hiding this comment

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

Why did these all become options? Somewhat confusing since the credentials provider doesn't appear to be one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good question, in hindsight I should have left a comment explaining this. This change was because I wanted to use set_credentials_provider to pass in an Option (since the credentials provider can be None). This requires a &mut self, and modifies in place, rather than a self where it returns an updated self. I updated all of the calls for consistency but I think I could leave the existing calls and just use set_credentials_provider for the one where I need to pass an Option if preferred.

Copy link
Member Author

@jszwedko jszwedko Jul 31, 2024

Choose a reason for hiding this comment

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

The builder has <option> and set_<option> methods for all configuration options. The ones with set_ take an Option (and &mut self) where the ones without do not (and take a self).

.set_credentials_provider(
auth.credentials_provider(region.clone(), proxy, tls_options)
.await?,
)
.region(region.clone())
.retry_config(retry_config.clone());
);

if let Some(endpoint_override) = endpoint {
config_builder = config_builder.endpoint_url(endpoint_override);
config_builder.set_endpoint_url(Some(endpoint_override));
}

if let Some(use_fips) =
aws_config::default_provider::use_fips::use_fips_provider(&provider_config).await
{
config_builder = config_builder.use_fips(use_fips);
config_builder.set_use_fips(Some(use_fips));
}

if let Some(timeout) = timeout {
Expand All @@ -234,7 +236,7 @@ pub async fn create_client_and_region<T: ClientBuilder>(
.set_connect_timeout(connect_timeout.map(Duration::from_secs))
.set_read_timeout(read_timeout.map(Duration::from_secs));

config_builder = config_builder.timeout_config(timeout_config_builder.build());
config_builder.set_timeout_config(Some(timeout_config_builder.build()));
}

let config = config_builder.build();
Expand Down
3 changes: 3 additions & 0 deletions src/sinks/aws_kinesis/firehose/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ async fn firehose_put_records_without_partition_key() {
load_timeout_secs: Some(5),
imds: ImdsAuthentication::default(),
region: None,
sign: true,
})),
endpoints: vec![elasticsearch_address()],
bulk: BulkConfig {
Expand Down Expand Up @@ -195,6 +196,7 @@ async fn firehose_put_records_with_partition_key() {
load_timeout_secs: Some(5),
imds: ImdsAuthentication::default(),
region: None,
sign: true,
})),
endpoints: vec![elasticsearch_address()],
bulk: BulkConfig {
Expand Down Expand Up @@ -278,6 +280,7 @@ async fn ensure_elasticsearch_domain(domain_name: String) -> String {
&None,
)
.await
.unwrap()
.unwrap(),
)
.endpoint_url(test_region_endpoint().endpoint().unwrap())
Expand Down
8 changes: 6 additions & 2 deletions src/sinks/elasticsearch/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,10 +269,14 @@ impl ElasticsearchCommon {
#[cfg(feature = "aws-core")]
pub async fn sign_request(
request: &mut http::Request<Bytes>,
credentials_provider: &aws_credential_types::provider::SharedCredentialsProvider,
credentials_provider: &Option<aws_credential_types::provider::SharedCredentialsProvider>,
region: &Option<aws_types::region::Region>,
) -> crate::Result<()> {
crate::aws::sign_request("es", request, credentials_provider, region).await
if let Some(credentials_provider) = credentials_provider {
crate::aws::sign_request("es", request, credentials_provider, region).await
} else {
Ok(())
}
}

async fn get_version(
Expand Down
3 changes: 3 additions & 0 deletions src/sinks/elasticsearch/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ async fn auto_version_aws() {
load_timeout_secs: Some(5),
imds: ImdsAuthentication::default(),
region: None,
sign: true,
},
)),
endpoints: vec![aws_server()],
Expand Down Expand Up @@ -396,6 +397,7 @@ async fn insert_events_on_aws() {
load_timeout_secs: Some(5),
imds: ImdsAuthentication::default(),
region: None,
sign: true,
},
)),
endpoints: vec![aws_server()],
Expand All @@ -422,6 +424,7 @@ async fn insert_events_on_aws_with_compression() {
load_timeout_secs: Some(5),
imds: ImdsAuthentication::default(),
region: None,
sign: true,
},
)),
endpoints: vec![aws_server()],
Expand Down
8 changes: 6 additions & 2 deletions src/sinks/prometheus/remote_write/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,14 @@ impl Service<RemoteWriteRequest> for RemoteWriteService {
#[cfg(feature = "aws-core")]
async fn sign_request(
request: &mut http::Request<Bytes>,
credentials_provider: &SharedCredentialsProvider,
credentials_provider: &Option<SharedCredentialsProvider>,
region: &Option<Region>,
) -> crate::Result<()> {
crate::aws::sign_request("aps", request, credentials_provider, region).await
if let Some(credentials_provider) = credentials_provider {
crate::aws::sign_request("aps", request, credentials_provider, region).await
} else {
Ok(())
}
}

pub(super) async fn build_request(
Expand Down
2 changes: 1 addition & 1 deletion src/sinks/util/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ pub enum Auth {
Basic(crate::http::Auth),
#[cfg(feature = "aws-core")]
Aws {
credentials_provider: aws_credential_types::provider::SharedCredentialsProvider,
credentials_provider: Option<aws_credential_types::provider::SharedCredentialsProvider>,
region: aws_types::region::Region,
},
}
1 change: 1 addition & 0 deletions src/sources/aws_sqs/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ async fn get_sqs_client() -> aws_sdk_sqs::Client {
AwsAuthentication::test_auth()
.credentials_provider(Region::new("custom"), &Default::default(), &None)
.await
.unwrap()
.unwrap(),
)
.endpoint_url(sqs_address())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ base: components: sinks: aws_cloudwatch_logs: configuration: {
required: true
type: string: examples: ["wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"]
}
sign: {
description: """
Whether to sign requests.

Setting this to false can be useful when writing to an AWS S3 bucket that allows
anonymous requests.
"""
required: false
type: bool: default: true
}
}
}
batch: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ base: components: sinks: aws_cloudwatch_metrics: configuration: {
required: true
type: string: examples: ["wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"]
}
sign: {
description: """
Whether to sign requests.

Setting this to false can be useful when writing to an AWS S3 bucket that allows
anonymous requests.
"""
required: false
type: bool: default: true
}
}
}
batch: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ base: components: sinks: aws_kinesis_firehose: configuration: {
required: true
type: string: examples: ["wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"]
}
sign: {
description: """
Whether to sign requests.

Setting this to false can be useful when writing to an AWS S3 bucket that allows
anonymous requests.
"""
required: false
type: bool: default: true
}
}
}
batch: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ base: components: sinks: aws_kinesis_streams: configuration: {
required: true
type: string: examples: ["wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"]
}
sign: {
description: """
Whether to sign requests.

Setting this to false can be useful when writing to an AWS S3 bucket that allows
anonymous requests.
"""
required: false
type: bool: default: true
}
}
}
batch: {
Expand Down
10 changes: 10 additions & 0 deletions website/cue/reference/components/sinks/base/aws_s3.cue
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,16 @@ base: components: sinks: aws_s3: configuration: {
required: true
type: string: examples: ["wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"]
}
sign: {
description: """
Whether to sign requests.

Setting this to false can be useful when writing to an AWS S3 bucket that allows
anonymous requests.
"""
required: false
type: bool: default: true
}
}
}
batch: {
Expand Down
10 changes: 10 additions & 0 deletions website/cue/reference/components/sinks/base/aws_sns.cue
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ base: components: sinks: aws_sns: configuration: {
required: true
type: string: examples: ["wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"]
}
sign: {
description: """
Whether to sign requests.

Setting this to false can be useful when writing to an AWS S3 bucket that allows
anonymous requests.
"""
required: false
type: bool: default: true
}
}
}
encoding: {
Expand Down
10 changes: 10 additions & 0 deletions website/cue/reference/components/sinks/base/aws_sqs.cue
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ base: components: sinks: aws_sqs: configuration: {
required: true
type: string: examples: ["wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"]
}
sign: {
description: """
Whether to sign requests.

Setting this to false can be useful when writing to an AWS S3 bucket that allows
anonymous requests.
"""
required: false
type: bool: default: true
}
}
}
encoding: {
Expand Down
Loading
Loading