Skip to content

Commit

Permalink
Add credentials exposure test & fix STS + SSO (#2603)
Browse files Browse the repository at this point in the history
## Motivation and Context
- credentials providers may leak credentials in the HTTP body at the
debug level

## Description
This adds a test to aws-config that looks for leaked credentials in all
of our provider integration tests—since these test use AWS APIs under
the hood, this also serves to test AWS services in general.

To support this, `sensitive` was added to the ParseHttpResponse trait
and code was generated to take action based on this change.

- [x] Add environment variable to force logging of the body
- [x] consider if we want to suppress request body logging as well

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

---------

Co-authored-by: John DiSanti <jdisanti@amazon.com>
  • Loading branch information
rcoh and jdisanti authored Apr 20, 2023
1 parent 9c585a2 commit 9a41e35
Show file tree
Hide file tree
Showing 20 changed files with 346 additions and 18 deletions.
16 changes: 16 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,19 @@ message = "Fix server code generation bug affecting constrained shapes bound wit
references = ["smithy-rs#2583", "smithy-rs#2584"]
meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "server" }
author = "david-perez"

[[aws-sdk-rust]]
message = """Reduce several instances of credential exposure in the SDK logs:
- IMDS now suppresses the body of the response from logs
- `aws-sigv4` marks the `x-amz-session-token` header as sensitive
- STS & SSO credentials have been manually marked as sensitive which suppresses logging of response bodies for relevant operations
"""
author = "rcoh"
references = ["smithy-rs#2603"]
meta = { "breaking" = false, "tada" = false, "bug" = false }

[[smithy-rs]]
message = "Add a sensitive method to `ParseHttpResponse`. When this returns true, logging of the HTTP response body will be suppressed."
author = "rcoh"
references = ["smithy-rs#2603"]
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client" }
1 change: 1 addition & 0 deletions aws/rust-runtime/aws-config/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ zeroize = { version = "1", optional = true }
[dev-dependencies]
futures-util = { version = "0.3.16", default-features = false }
tracing-test = "0.2.1"
tracing-subscriber = { version = "0.3.16", features = ["fmt", "json"] }

tokio = { version = "1.23.1", features = ["full", "test-util"] }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,6 @@ impl Builder {

#[cfg(test)]
mod test {
use tracing_test::traced_test;

use aws_credential_types::provider::ProvideCredentials;

use crate::default_provider::credentials::DefaultCredentialsChain;
Expand Down Expand Up @@ -242,7 +240,6 @@ mod test {
make_test!($name, execute, $provider_config_builder);
};
($name: ident, $func: ident, $provider_config_builder: expr) => {
#[traced_test]
#[tokio::test]
async fn $name() {
crate::test_case::TestEnvironment::from_dir(concat!(
Expand Down Expand Up @@ -324,7 +321,6 @@ mod test {
}

#[tokio::test]
#[traced_test]
#[cfg(feature = "client-hyper")]
async fn no_providers_configured_err() {
use crate::provider_config::ProviderConfig;
Expand Down
4 changes: 4 additions & 0 deletions aws/rust-runtime/aws-config/src/http_credential_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,10 @@ impl ParseStrictResponse for CredentialsResponseParser {
)),
}
}

fn sensitive(&self) -> bool {
true
}
}

#[derive(Clone, Debug)]
Expand Down
4 changes: 4 additions & 0 deletions aws/rust-runtime/aws-config/src/imds/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,10 @@ impl ParseStrictResponse for ImdsGetResponseHandler {
Err(InnerImdsError::BadStatus)
}
}

fn sensitive(&self) -> bool {
true
}
}

/// IMDSv2 Endpoint Mode
Expand Down
4 changes: 4 additions & 0 deletions aws/rust-runtime/aws-config/src/imds/client/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,8 @@ impl ParseStrictResponse for GetTokenResponseHandler {
expiry: self.time.now() + Duration::from_secs(ttl),
})
}

fn sensitive(&self) -> bool {
true
}
}
4 changes: 2 additions & 2 deletions aws/rust-runtime/aws-config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ mod loader {
///
/// # Example: Using a custom profile file path
///
/// ```
/// ```no_run
/// use aws_config::profile::{ProfileFileCredentialsProvider, ProfileFileRegionProvider};
/// use aws_config::profile::profile_file::{ProfileFiles, ProfileFileKind};
///
Expand Down Expand Up @@ -417,7 +417,7 @@ mod loader {
///
/// # Example: Using a custom profile name
///
/// ```
/// ```no_run
/// use aws_config::profile::{ProfileFileCredentialsProvider, ProfileFileRegionProvider};
/// use aws_config::profile::profile_file::{ProfileFiles, ProfileFileKind};
///
Expand Down
2 changes: 0 additions & 2 deletions aws/rust-runtime/aws-config/src/profile/credentials.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,14 +455,12 @@ async fn build_provider_chain(

#[cfg(test)]
mod test {
use tracing_test::traced_test;

use crate::profile::credentials::Builder;
use crate::test_case::TestEnvironment;

macro_rules! make_test {
($name: ident) => {
#[traced_test]
#[tokio::test]
async fn $name() {
TestEnvironment::from_dir(concat!(
Expand Down
2 changes: 1 addition & 1 deletion aws/rust-runtime/aws-config/src/profile/profile_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use std::path::PathBuf;
///
/// # Example: Using a custom profile file path
///
/// ```
/// ```no_run
/// use aws_config::profile::{ProfileFileCredentialsProvider, ProfileFileRegionProvider};
/// use aws_config::profile::profile_file::{ProfileFiles, ProfileFileKind};
/// use std::sync::Arc;
Expand Down
103 changes: 102 additions & 1 deletion aws/rust-runtime/aws-config/src/test_case.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,17 @@ use serde::Deserialize;
use crate::connector::default_connector;
use aws_smithy_types::error::display::DisplayErrorContext;
use std::collections::HashMap;
use std::env;
use std::error::Error;
use std::fmt::Debug;
use std::future::Future;
use std::io::Write;
use std::path::{Path, PathBuf};
use std::sync::{Arc, Mutex};
use std::time::{Duration, UNIX_EPOCH};
use tracing::dispatcher::DefaultGuard;
use tracing::Level;
use tracing_subscriber::fmt::TestWriter;

/// Test case credentials
///
Expand Down Expand Up @@ -84,7 +90,7 @@ impl AsyncSleep for InstantSleep {
}
}

#[derive(Deserialize)]
#[derive(Deserialize, Debug)]
pub(crate) enum GenericTestResult<T> {
Ok(T),
ErrorContains(String),
Expand Down Expand Up @@ -129,6 +135,79 @@ pub(crate) struct Metadata {
name: String,
}

struct Tee<W> {
buf: Arc<Mutex<Vec<u8>>>,
quiet: bool,
inner: W,
}

/// Capture logs from this test.
///
/// The logs will be captured until the `DefaultGuard` is dropped.
///
/// *Why use this instead of traced_test?*
/// This captures _all_ logs, not just logs produced by the current crate.
fn capture_test_logs() -> (DefaultGuard, Rx) {
// it may be helpful to upstream this at some point
let (mut writer, rx) = Tee::stdout();
if env::var("VERBOSE_TEST_LOGS").is_ok() {
writer.loud();
} else {
eprintln!("To see full logs from this test set VERBOSE_TEST_LOGS=true");
}
let subscriber = tracing_subscriber::fmt()
.with_max_level(Level::TRACE)
.with_writer(Mutex::new(writer))
.finish();
let guard = tracing::subscriber::set_default(subscriber);
(guard, rx)
}

struct Rx(Arc<Mutex<Vec<u8>>>);
impl Rx {
pub(crate) fn contents(&self) -> String {
String::from_utf8(self.0.lock().unwrap().clone()).unwrap()
}
}

impl Tee<TestWriter> {
fn stdout() -> (Self, Rx) {
let buf: Arc<Mutex<Vec<u8>>> = Default::default();
(
Tee {
buf: buf.clone(),
quiet: true,
inner: TestWriter::new(),
},
Rx(buf),
)
}
}

impl<W> Tee<W> {
fn loud(&mut self) {
self.quiet = false;
}
}

impl<W> Write for Tee<W>
where
W: Write,
{
fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
self.buf.lock().unwrap().extend_from_slice(buf);
if !self.quiet {
self.inner.write(buf)
} else {
Ok(buf.len())
}
}

fn flush(&mut self) -> std::io::Result<()> {
self.inner.flush()
}
}

impl TestEnvironment {
pub(crate) async fn from_dir(dir: impl AsRef<Path>) -> Result<TestEnvironment, Box<dyn Error>> {
let dir = dir.as_ref();
Expand Down Expand Up @@ -232,12 +311,26 @@ impl TestEnvironment {
eprintln!("test case: {}. {}", self.metadata.name, self.metadata.docs);
}

fn lines_with_secrets<'a>(&'a self, logs: &'a str) -> Vec<&'a str> {
logs.lines().filter(|l| self.contains_secret(l)).collect()
}

fn contains_secret(&self, log_line: &str) -> bool {
assert!(log_line.lines().count() <= 1);
match &self.metadata.result {
// NOTE: we aren't currently erroring if the session token is leaked, that is in the canonical request among other things
TestResult::Ok(creds) => log_line.contains(&creds.secret_access_key),
TestResult::ErrorContains(_) => false,
}
}

/// Execute a test case. Failures lead to panics.
pub(crate) async fn execute<F, P>(&self, make_provider: impl Fn(ProviderConfig) -> F)
where
F: Future<Output = P>,
P: ProvideCredentials,
{
let (_guard, rx) = capture_test_logs();
let provider = make_provider(self.provider_config.clone()).await;
let result = provider.provide_credentials().await;
tokio::time::pause();
Expand All @@ -256,6 +349,14 @@ impl TestEnvironment {
Ok(()) => {}
Err(e) => panic!("{}", e),
}
let contents = rx.contents();
let leaking_lines = self.lines_with_secrets(&contents);
assert!(
leaking_lines.is_empty(),
"secret was exposed\n{:?}\nSee the following log lines:\n {}",
self.metadata.result,
leaking_lines.join("\n ")
)
}

#[track_caller]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[default]
source_profile = base
credential_process = echo '{ "Version": 1, "AccessKeyId": "ASIARTESTID", "SecretAccessKey": "TESTSECRETKEY", "SessionToken": "TESTSESSIONTOKEN", "Expiration": "2022-05-02T18:36:00+00:00" }'
# these fake credentials are base64 encoded to prevent triggering the unit test that scans logs for secrets
credential_process = echo eyAiVmVyc2lvbiI6IDEsICJBY2Nlc3NLZXlJZCI6ICJBU0lBUlRFU1RJRCIsICJTZWNyZXRBY2Nlc3NLZXkiOiAiVEVTVFNFQ1JFVEtFWSIsICJTZXNzaW9uVG9rZW4iOiAiVEVTVFNFU1NJT05UT0tFTiIsICJFeHBpcmF0aW9uIjogIjIwMjItMDUtMDJUMTg6MzY6MDArMDA6MDAiIH0K | base64 --decode

[profile base]
region = us-east-1
16 changes: 12 additions & 4 deletions aws/rust-runtime/aws-sigv4/src/http_request/sign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ fn calculate_signing_headers<'a>(
// Step 4: https://docs.aws.amazon.com/en_pv/general/latest/gr/sigv4-add-signature-to-request.html
let values = creq.values.as_headers().expect("signing with headers");
let mut headers = HeaderMap::new();
add_header(&mut headers, header::X_AMZ_DATE, &values.date_time);
add_header(&mut headers, header::X_AMZ_DATE, &values.date_time, false);
headers.insert(
"authorization",
build_authorization_header(params.access_key, &creq, sts, &signature),
Expand All @@ -266,18 +266,26 @@ fn calculate_signing_headers<'a>(
&mut headers,
header::X_AMZ_CONTENT_SHA_256,
&values.content_sha256,
false,
);
}

if let Some(security_token) = params.security_token {
add_header(&mut headers, header::X_AMZ_SECURITY_TOKEN, security_token);
add_header(
&mut headers,
header::X_AMZ_SECURITY_TOKEN,
security_token,
true,
);
}

Ok(SigningOutput::new(headers, signature))
}

fn add_header(map: &mut HeaderMap<HeaderValue>, key: &'static str, value: &str) {
map.insert(key, HeaderValue::try_from(value).expect(key));
fn add_header(map: &mut HeaderMap<HeaderValue>, key: &'static str, value: &str, sensitive: bool) {
let mut value = HeaderValue::try_from(value).expect(key);
value.set_sensitive(sensitive);
map.insert(key, value);
}

// add signature to authorization header
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import software.amazon.smithy.rustsdk.customize.route53.Route53Decorator
import software.amazon.smithy.rustsdk.customize.s3.S3Decorator
import software.amazon.smithy.rustsdk.customize.s3.S3ExtendedRequestIdDecorator
import software.amazon.smithy.rustsdk.customize.s3control.S3ControlDecorator
import software.amazon.smithy.rustsdk.customize.sso.SSODecorator
import software.amazon.smithy.rustsdk.customize.sts.STSDecorator
import software.amazon.smithy.rustsdk.endpoints.AwsEndpointsStdLib
import software.amazon.smithy.rustsdk.endpoints.OperationInputTestDecorator
Expand Down Expand Up @@ -65,6 +66,7 @@ val DECORATORS: List<ClientCodegenDecorator> = listOf(
),
S3ControlDecorator().onlyApplyTo("com.amazonaws.s3control#AWSS3ControlServiceV20180820"),
STSDecorator().onlyApplyTo("com.amazonaws.sts#AWSSecurityTokenServiceV20110615"),
SSODecorator().onlyApplyTo("com.amazonaws.sso#SWBPortalService"),

// Only build docs-rs for linux to reduce load on docs.rs
listOf(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package software.amazon.smithy.rustsdk.customize.sso

import software.amazon.smithy.model.Model
import software.amazon.smithy.model.shapes.ServiceShape
import software.amazon.smithy.model.shapes.Shape
import software.amazon.smithy.model.shapes.ShapeId
import software.amazon.smithy.model.shapes.StructureShape
import software.amazon.smithy.model.traits.SensitiveTrait
import software.amazon.smithy.model.transform.ModelTransformer
import software.amazon.smithy.rust.codegen.client.smithy.customize.ClientCodegenDecorator
import software.amazon.smithy.rust.codegen.core.util.letIf
import java.util.logging.Logger

class SSODecorator : ClientCodegenDecorator {
override val name: String = "SSO"
override val order: Byte = 0
private val logger: Logger = Logger.getLogger(javaClass.name)

private fun isAwsCredentials(shape: Shape): Boolean = shape.id == ShapeId.from("com.amazonaws.sso#RoleCredentials")

override fun transformModel(service: ServiceShape, model: Model): Model =
ModelTransformer.create().mapShapes(model) { shape ->
shape.letIf(isAwsCredentials(shape)) {
(shape as StructureShape).toBuilder().addTrait(SensitiveTrait()).build()
}
}
}
Loading

0 comments on commit 9a41e35

Please sign in to comment.