Unverified Commit 6ceabf8b authored by Russell Cohen's avatar Russell Cohen Committed by GitHub
Browse files

Use tokio::process to allow timeouts to occur (#3052) 

## Motivation and Context
The existing credentials provider was a DOS risk and didn't obey timeout
settings because it used `std::timeout::spawn` but relied on a
async-based timeout mechanism.

## Description
- Use `tokio::process` instead
- introduce new cargo feature

## Testing
- unit test

## 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._
parent 2a51e0bc

CHANGELOG.next.toml

+12 −0
Original line number Diff line number Diff line
@@ -427,6 +427,18 @@ references = ["smithy-rs#3043", "smithy-rs#3078"] 
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }

author = "rcoh"



[[aws-sdk-rust]]

message = "A bug was fixed where the credentials-process provider was executing the subprocess in the worker thread, potentially stalling the runtime."

references = ["smithy-rs#3052"]

meta = { "breaking" = false, "tada" = false, "bug" = true }

author = "rcoh"



[[aws-sdk-rust]]

message = "The `credentials-process` feature was added to `aws-config`. If you currently use `no-default-features` for `aws-config`, you MUST enable this feature to use the [`CredentialProcessProvider`](https://docs.rs/aws-config/latest/aws_config/credential_process/struct.CredentialProcessProvider.html) provider directly or via `~/.aws/config`."

references = ["smithy-rs#3052"]

meta = { "breaking" = true, "tada" = false, "bug" = false }

author = "rcoh"



[[smithy-rs]]

message = """

`aws_smithy_http::operation::error::{BuildError, SerializationError}` have been moved to `aws_smithy_types::error::operation::{BuildError, SerializationError}`. Type aliases for them are left in `aws_smithy_http` for backwards compatibility but are deprecated.

aws/rust-runtime/aws-config/Cargo.toml

+2 −1
Original line number Diff line number Diff line
@@ -14,8 +14,9 @@ rustls = ["aws-smithy-runtime/tls-rustls", "client-hyper"] 
allow-compilation = [] # our tests use `cargo test --all-features` and native-tls breaks CI

rt-tokio = ["aws-smithy-async/rt-tokio", "aws-smithy-runtime/rt-tokio", "tokio/rt"]

sso = ["dep:aws-sdk-sso", "dep:aws-sdk-ssooidc", "dep:ring", "dep:hex", "dep:zeroize", "aws-smithy-runtime-api/http-auth"]

credentials-process = ["tokio/process"]



default = ["client-hyper", "rustls", "rt-tokio"]

default = ["client-hyper", "rustls", "rt-tokio", "credentials-process", "sso"]



[dependencies]

aws-credential-types = { path = "../../sdk/build/aws-sdk/sdk/aws-credential-types" }

aws/rust-runtime/aws-config/src/credential_process.rs

+30 −50
Original line number Diff line number Diff line
@@ -3,57 +3,20 @@ 
 * SPDX-License-Identifier: Apache-2.0

 */



#![cfg(feature = "credentials-process")]



//! Credentials Provider for external process



use crate::json_credentials::{json_parse_loop, InvalidJsonCredentials, RefreshableCredentials};

use crate::sensitive_command::CommandWithSensitiveArgs;

use aws_credential_types::provider::{self, error::CredentialsError, future, ProvideCredentials};

use aws_credential_types::Credentials;

use aws_smithy_json::deserialize::Token;

use std::fmt;

use std::process::Command;

use std::time::SystemTime;

use time::format_description::well_known::Rfc3339;

use time::OffsetDateTime;



#[derive(Clone)]

pub(crate) struct CommandWithSensitiveArgs<T>(T);



impl<T> CommandWithSensitiveArgs<T>

where

    T: AsRef<str>,

{

    pub(crate) fn new(value: T) -> Self {

        Self(value)

    }



    pub(crate) fn unredacted(&self) -> &str {

        self.0.as_ref()

    }

}



impl<T> fmt::Display for CommandWithSensitiveArgs<T>

where

    T: AsRef<str>,

{

    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

        // Security: The arguments for command must be redacted since they can be sensitive

        let command = self.0.as_ref();

        match command.find(char::is_whitespace) {

            Some(index) => write!(f, "{} ** arguments redacted **", &command[0..index]),

            None => write!(f, "{}", command),

        }

    }

}



impl<T> fmt::Debug for CommandWithSensitiveArgs<T>

where

    T: AsRef<str>,

{

    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

        write!(f, "{:?}", format!("{}", self))

    }

}



/// External process credentials provider

///

/// This credentials provider runs a configured external process and parses
@@ -109,11 +72,17 @@ impl CredentialProcessProvider { 
        }

    }



    pub(crate) fn from_command(command: &CommandWithSensitiveArgs<&str>) -> Self {

        Self {

            command: command.to_owned_string(),

        }

    }



    async fn credentials(&self) -> provider::Result {

        // Security: command arguments must be redacted at debug level

        tracing::debug!(command = %self.command, "loading credentials from external process");



        let mut command = if cfg!(windows) {

        let command = if cfg!(windows) {

            let mut command = Command::new("cmd.exe");

            command.args(["/C", self.command.unredacted()]);

            command
@@ -122,8 +91,10 @@ impl CredentialProcessProvider { 
            command.args(["-c", self.command.unredacted()]);

            command

        };



        let output = command.output().map_err(|e| {

        let output = tokio::process::Command::from(command)

            .output()

            .await

            .map_err(|e| {

                CredentialsError::provider_error(format!(

                    "Error retrieving credentials from external process: {}",

                    e
@@ -131,7 +102,7 @@ impl CredentialProcessProvider { 
            })?;



        // Security: command arguments can be logged at trace level

        tracing::trace!(command = ?command, status = ?output.status, "executed command (unredacted)");

        tracing::trace!(command = ?self.command, status = ?output.status, "executed command (unredacted)");



        if !output.status.success() {

            let reason =
@@ -261,9 +232,10 @@ pub(crate) fn parse_credential_process_json_credentials( 
mod test {

    use crate::credential_process::CredentialProcessProvider;

    use aws_credential_types::provider::ProvideCredentials;

    use std::time::SystemTime;

    use std::time::{Duration, SystemTime};

    use time::format_description::well_known::Rfc3339;

    use time::OffsetDateTime;

    use tokio::time::timeout;



    #[tokio::test]

    async fn test_credential_process() {
@@ -285,4 +257,12 @@ mod test { 
            )

        );

    }



    #[tokio::test]

    async fn credentials_process_timeouts() {

        let provider = CredentialProcessProvider::new(String::from("sleep 1000"));

        let _creds = timeout(Duration::from_millis(1), provider.provide_credentials())

            .await

            .expect_err("timeout forced");

    }

}

aws/rust-runtime/aws-config/src/lib.rs

+1 −0
Original line number Diff line number Diff line
@@ -127,6 +127,7 @@ pub mod meta; 
pub mod profile;

pub mod provider_config;

pub mod retry;

mod sensitive_command;

#[cfg(feature = "sso")]

pub mod sso;

pub(crate) mod standard_property;

aws/rust-runtime/aws-config/src/profile/credentials.rs

+8 −2
Original line number Diff line number Diff line
@@ -262,6 +262,8 @@ pub enum ProfileFileError { 
    FeatureNotEnabled {

        /// The feature or comma delimited list of features that must be enabled

        feature: Cow<'static, str>,

        /// Additional information about the missing feature

        message: Option<Cow<'static, str>>,

    },

}
@@ -315,10 +317,11 @@ impl Display for ProfileFileError { 
                "profile `{}` did not contain credential information",

                profile

            ),

            ProfileFileError::FeatureNotEnabled { feature: message } => {

            ProfileFileError::FeatureNotEnabled { feature, message } => {

                let message = message.as_deref().unwrap_or_default();

                write!(

                    f,

                    "This behavior requires following cargo feature(s) enabled: {message}",

                    "This behavior requires following cargo feature(s) enabled: {feature}. {message}",

                )

            }

        }
@@ -488,7 +491,10 @@ mod test { 
    make_test!(retry_on_error);

    make_test!(invalid_config);

    make_test!(region_override);

    #[cfg(feature = "credentials-process")]

    make_test!(credential_process);

    #[cfg(feature = "credentials-process")]

    make_test!(credential_process_failure);

    #[cfg(feature = "credentials-process")]

    make_test!(credential_process_invalid);

}