Add support for `credential_process` from profiles #1356

jszwedko · 2022-04-29T23:05:58Z

Motivation and Context

Description

Starting to add support for credential_process in the default credentials chain. I'm just pushing this up as a draft to get early feedback before continuing implementation.

Testing

I tested this manually by using patching it into http://github.com/vectordotdev/vector but will follow through with tests here.

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
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.

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

jszwedko

Hey all! I'll push this forward more next week but wanted to get it up for any early feedback.

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

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

jszwedko · 2022-04-29T23:09:16Z

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

@@ -493,4 +501,5 @@ mod test {
    make_test!(retry_on_error);
    make_test!(invalid_config);
    make_test!(region_override);
+    make_test!(credentials_process);


Will implement.

we'll need to figure out how to mock this—probably some sort of "fake command" interface will need to get added to our existing OS shim

also, should validate that the credentials process can be used as the source profile for assume role

👍 I ended up using echo for the test in which should be supported on all platforms, but happy to try to sub in command mocking if you prefer.

Ref: a787b61

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

jszwedko · 2022-04-29T23:14:13Z

Ah, also, I integrated this into the profile file provider rather than creating a new one that is part of the default chain. I think it makes sense as part of the profile file provider (this is how rusoto modeled it too) rather than a separate provider that also tries to load profile files, but open to opinions here.

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

Adopt the AWS Java SDK approach of running through `cmd.exe` / `sh`. https://github.com/aws/aws-sdk-java/blob/fd409dee8ae23fb8953e0bb4dbde65536a7e0514/aws-java-sdk-core/src/main/java/com/amazonaws/auth/ProcessCredentialsProvider.java#L63-L82 Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

rcoh

overall this is exactly the right direction! I may end up asking you to not include it in the default chain..by default. We need to figure out if there are any security safeguards needed to start using this generally, but I'll get back to you on that ASAP!

Great work on this so far, this looks awesome!

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

rcoh · 2022-05-02T18:19:18Z

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

+    let mut command = Command::new(iter.next().ok_or_else(|| {
+        CredentialsError::provider_error(
+            "Error retrieving credentials from external process: provided command empty",
+        )
+    })?);


I wonder if we should spawn this into a task...the trouble is that we'd need to then abstract over spawning. I'll think about it.

👍 I'm open to thoughts here. I actually up refactoring this to take the Java SDK's approach of just passing the whole string through sh -c / cmd.exe /C in 14ad7a9 to avoid needing to do any parsing in the SDK itself.

rcoh · 2022-05-02T18:19:18Z

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

+    let mut command = Command::new(iter.next().ok_or_else(|| {
+        CredentialsError::provider_error(
+            "Error retrieving credentials from external process: provided command empty",
+        )
+    })?);


I wonder if we should spawn this into a task...the trouble is that we'd need to then abstract over spawning. I'll think about it.

rcoh · 2022-05-02T18:20:11Z

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

@@ -493,4 +501,5 @@ mod test {
    make_test!(retry_on_error);
    make_test!(invalid_config);
    make_test!(region_override);
+    make_test!(credentials_process);


we'll need to figure out how to mock this—probably some sort of "fake command" interface will need to get added to our existing OS shim

rcoh · 2022-05-02T18:20:46Z

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

@@ -100,6 +101,9 @@ impl ProviderChain {
                    })?
            }
            BaseProvider::AccessKey(key) => Arc::new(key.clone()),
+            BaseProvider::CredentialProcess(credential_process) => Arc::new(
+                CredentialProcessProvider::new(credential_process.to_string()),


if new returned a result, we could return a profile file error here which is probably the right thing

I mentioned this in #1356 (comment) but I actually ended up pulling out the command parsing logic, following suit with the Java SDK of just passing through the commands to sh -c / cmd.exe /C.

rcoh · 2022-05-02T18:20:47Z

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

@@ -100,6 +101,9 @@ impl ProviderChain {
                    })?
            }
            BaseProvider::AccessKey(key) => Arc::new(key.clone()),
+            BaseProvider::CredentialProcess(credential_process) => Arc::new(
+                CredentialProcessProvider::new(credential_process.to_string()),


if new returned a result, we could return a profile file error here which is probably the right thing

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

rcoh · 2022-05-02T18:23:43Z

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

@@ -493,4 +501,5 @@ mod test {
    make_test!(retry_on_error);
    make_test!(invalid_config);
    make_test!(region_override);
+    make_test!(credentials_process);


also, should validate that the credentials process can be used as the source profile for assume role

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

This required refactoring the existing JSON parsing a bit. Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

jszwedko · 2022-05-02T19:45:16Z

Thanks for taking a look @rcoh ! I pushed some more changes and think this is ready for full review.

jszwedko · 2022-05-02T19:47:00Z

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

@@ -177,7 +199,7 @@ pub(crate) fn parse_json_credentials(

 pub(crate) fn json_parse_loop<'a>(
    input: &'a [u8],
-    mut f: impl FnMut(Cow<'a, str>, Cow<'a, str>),
+    mut f: impl FnMut(Cow<'a, str>, &Token<'a>) -> Result<(), InvalidJsonCredentials>,


The changes to the JSON parsing to support IMDSv2, SSO, and the differing credential_process JSON formats were a bit more awkward than I was hoping. I started with changing this loop function to return all value types since the credential_process format has a number in it (Version) which spidered out a bit.

Velfi · 2022-05-06T18:27:25Z

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

@@ -115,7 +118,8 @@ pub(crate) fn parse_json_credentials(
    let mut expiration = None;
    let mut message = None;
    json_parse_loop(credentials_response.as_bytes(), |key, value| {
-        match key {
+        dbg!("calling loop with", &key, &value);


Don't forget to change this to a tracing call

The dbg! in this function probably aren't valuable to log and can just be removed.

Velfi · 2022-05-06T18:55:39Z

CHANGELOG.next.toml

@@ -22,3 +22,14 @@ message = "Log a debug event when a retry is going to be peformed"
 references = ["smithy-rs#1352"]
 meta = { "breaking" = false, "tada" = false, "bug" = false }
 author = "jdisanti"
+


Suggested change

[[aws-sdk-rust]]

missing the header for this entry

Velfi · 2022-05-06T19:03:43Z

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

+            )
+            .map_err(|_| {
+                InvalidJsonCredentials::Other(
+                    "credential expiration time cannot be represented by a SystemTime".into(),


I'm a bit confused by this message. Is it saying "you gave us data but we couldn't build a SystemTime from it" or "you gave us a SystemTime but that's not valid for this"?

Probably better to call it DateTime in this instance since that's what it's getting parsed into.

jdisanti

This looks great! Thanks a ton for putting in the work here.

I read through some internal documentation on this credential provider, which is the source of my redaction and process execution comments.

We should put // Security: <reason> comments around things like manual Debug implementations to avoid them getting converted back into derivations by a would be good intentioned, code simplifying, dev in the future 😃

jdisanti · 2022-05-06T21:16:21Z

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

+    /// [profile assume-role]
+    /// credential_process = /opt/bin/awscreds-custom --username helen
+    /// ```
+    CredentialProcess(&'a str),


The arguments to the credentials process are allowed to contain sensitive information, so we need to be careful not to log them by default. Thus, the Debug implementation for this enum needs to do some amount of redaction on this value. I recommend creating a new-type around the &'a str, and manually implementing Debug for it such that only the part before the first space is output, followed by something like <args redacted> if there was more to the string.

+1 for this—we should make something like:

struct<'a> Sensitive(Cow<'a, str>) impl AsDeref<str> for Sensitive ...

we can then use it here and in the provider to avoid needing to manually do a full debug impl

jdisanti · 2022-05-06T21:18:19Z

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

+#[derive(Debug)]
+pub struct CredentialProcessProvider {
+    command: String,


The comment about Debug argument redaction applies here.

jdisanti · 2022-05-06T21:30:29Z

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

+            ))
+        })?;
+
+        tracing::debug!(command = ?command, status = ?output.status, "executed command");


The command binary name can be logged at debug, but the arguments must be logged at trace since they can have sensitive information.

jdisanti · 2022-05-06T21:38:51Z

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

+        let mut command = if cfg!(windows) {
+            let mut command = Command::new("cmd.exe");
+            command.args(&["/C", &self.command]);
+            command
+        } else {
+            let mut command = Command::new("sh");
+            command.args(&["-c", &self.command]);
+            command
+        };


You shouldn't need the if windows / else since std::process::Command::new will use the PATH to find the executable if an absolute path wasn't given: https://doc.rust-lang.org/stable/std/process/struct.Command.html#method.new

The SDKs don't support things like shell aliases or argument variable substitution, so it shouldn't run through a shell.

Interestingly, this is the way the Go V2 SDK and Java V2 SDKs are doing it. Boto3 looks like it does its own platform-based splitting, with its own implementation for Windows and using shlex for other platforms. I'll dig into this some more to figure out what we should do for Rust.

Not going to block on this since Go V2 and Java V2 are doing it this way.

jdisanti · 2022-05-06T21:40:38Z

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

+        if !output.status.success() {
+            return Err(CredentialsError::provider_error(format!(
+                "Error retrieving credentials from external process: exited with code: {}",
+                output.status
+            )));
+        }


In the case of a non-zero exit status, the contents of stderr should be included in the returned error.

jdisanti · 2022-05-06T21:47:12Z

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

+#[derive(Debug, PartialEq, Eq)]
+pub(crate) struct RefreshableCredentials<'a> {


I think JsonCredentials implementing Debug is actually a bug. We should either remove it, or add a manual Debug implementation that redacts the credential values.

jdisanti · 2022-05-06T21:48:40Z

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

@@ -115,7 +118,8 @@ pub(crate) fn parse_json_credentials(
    let mut expiration = None;
    let mut message = None;
    json_parse_loop(credentials_response.as_bytes(), |key, value| {
-        match key {
+        dbg!("calling loop with", &key, &value);


The dbg! in this function probably aren't valuable to log and can just be removed.

jdisanti · 2022-05-06T21:52:14Z

aws/rust-runtime/aws-config/test-data/profile-provider/credential_process/fs/home/.aws/config

@@ -0,0 +1,6 @@
+[default]
+source_profile = base
+credential_process = echo '{ "Version": 1, "AccessKeyId": "ASIARTESTID", "SecretAccessKey": "TESTSECRETKEY", "SessionToken": "TESTSESSIONTOKEN", "Expiration": "2022-05-02T18:36:00+00:00" }'


Nice! Good thinking.

jdisanti · 2022-05-06T21:55:39Z

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

+             "Expiration": "2022-05-02T18:36:00+00:00"
+            */
+            (key, Token::ValueNumber { value, .. }) if key.eq_ignore_ascii_case("Version") => {
+                version = Some(value.to_u8())


Just to err on the safe side, the version should probably be an isize to avoid weird error messages or behavior if someone puts a large number in. It wouldn't surprise me if someone decides version numbers will be dates in the future (e.g., 20220506).

jdisanti · 2022-05-06T21:58:13Z

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

+            )
+            .map_err(|_| {
+                InvalidJsonCredentials::Other(
+                    "credential expiration time cannot be represented by a SystemTime".into(),


Probably better to call it DateTime in this instance since that's what it's getting parsed into.

mpetri · 2022-05-24T01:24:33Z

any update on this functionality?

jdisanti · 2022-05-24T20:55:10Z

any update on this functionality?

I'll try and get this wrapped up and merged this week.

rcoh

functionally, I think this is good to go. Before we merge, the main thing to get is the docs—the other comments can be a follow up, if preferred

rcoh · 2022-05-25T14:40:22Z

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

+    match version {
+        Some(1) => {
+            let access_key_id =
+                access_key_id.ok_or(InvalidJsonCredentials::MissingField("AccessKeyId"))?;


as a minor nit for readability, I'd probably handle the error cases & abort in the match then have the bulk of the code be un-nested

rcoh · 2022-05-25T14:43:59Z

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

+    /// [profile assume-role]
+    /// credential_process = /opt/bin/awscreds-custom --username helen
+    /// ```
+    CredentialProcess(&'a str),


+1 for this—we should make something like:

struct<'a> Sensitive(Cow<'a, str>) impl AsDeref<str> for Sensitive ...

we can then use it here and in the provider to avoid needing to manually do a full debug impl

rcoh · 2022-05-25T14:45:24Z

aws/rust-runtime/aws-config/test-data/profile-provider/credential_process/fs/home/.aws/config

@@ -0,0 +1,6 @@
+[default]


can we add a test where the command is something like exit 1? And a test where it returns invalid JSON?

…t-for-credential-process

rcoh

🚢

rcoh · 2022-05-26T19:53:57Z

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

    }
 }

+impl<T> Clone for CommandWithSensitiveArgs<T>


I think this is just #[derive(Clone)]?

It needs the where T: Clone clause, but maybe I can just put that bound on the struct itself and then derive...

If I take that approach, then each other trait impl needs to have a T: Clone bound added, so I don't think it's much of a savings.

jszwedko · 2022-05-26T21:27:58Z

Thank you for taking this across the line @jdisanti ! Thanks to @rcoh @Velfi for the review too. Apologies that I didn't get to circle back to address the comments yet. I'm happy to see this merged in though 😄

Velfi · 2022-05-27T15:47:42Z

@jszwedko No worries, we're all busy people. Thanks again for submitting this PR and doing the bulk of the work on it!

Starting to add support for credential_process from profiles

c3a595c

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

jszwedko changed the title ~~Starting to add support for credential_process from profiles~~ Add support for credential_process from profiles Apr 29, 2022

Remove dbg statement

f8b05ad

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

jszwedko commented Apr 29, 2022

View reviewed changes

jszwedko mentioned this pull request Apr 29, 2022

[request]: Add support for the external-process credential provider awslabs/aws-sdk-rust#261

Closed

2 tasks

jszwedko mentioned this pull request Apr 29, 2022

AWS Rust SDK doesn't support credential_process vectordotdev/vector#12271

Closed

jszwedko added 4 commits May 2, 2022 11:19

Re-order credential_process later to match boto ordering

14abfdd

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

correct provider name

b5f1c6b

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

Remove no longer needed dependency

865a6ee

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

rcoh reviewed May 2, 2022

View reviewed changes

jszwedko added 4 commits May 2, 2022 12:24

Add failing test case for credential_process

a787b61

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

Add JSON parsing for credential_process responses

7de0201

This required refactoring the existing JSON parsing a bit. Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

Add test

2e0ae8f

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

Update changelog

0e4f34c

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

jszwedko marked this pull request as ready for review May 2, 2022 19:39

jszwedko requested a review from a team as a code owner May 2, 2022 19:39

Merge branch 'main' into jszwedko/add-support-for-credential-process

6433a24

jszwedko requested a review from rcoh May 2, 2022 19:43

jszwedko commented May 2, 2022

View reviewed changes

Velfi reviewed May 6, 2022

View reviewed changes

jdisanti reviewed May 6, 2022

View reviewed changes

jdisanti added 3 commits May 24, 2022 10:14

Merge branch 'main' into jszwedko/add-support-for-credential-process

9f88a6b

Merge branch 'main' into jszwedko/add-support-for-credential-process

16a8495

Fix clippy lints

ce75253

Incorporate feedback

7b867af

rcoh approved these changes May 25, 2022

View reviewed changes

jdisanti added 3 commits May 26, 2022 11:46

Add more tests and refactor redaction

3be6523

Merge remote-tracking branch 'upstream/main' into jszwedko/add-suppor…

2a888d4

…t-for-credential-process

Add more documentation to CredentialProcessProvider

652345d

rcoh approved these changes May 26, 2022

View reviewed changes

jdisanti merged commit 53fab07 into smithy-lang:main May 26, 2022

jszwedko deleted the jszwedko/add-support-for-credential-process branch May 28, 2022 16:34

alexchen8 mentioned this pull request Oct 1, 2022

docs: Update AWS documentation vectordotdev/vector#14675

Merged

battesonb mentioned this pull request Oct 24, 2022

Unable to connect to ledger error awslabs/amazon-qldb-shell#493

Open

		#[derive(Debug, PartialEq, Eq)]
		pub(crate) struct RefreshableCredentials<'a> {

Add support for credential_process from profiles #1356

Add support for credential_process from profiles #1356

Conversation

jszwedko commented Apr 29, 2022 • edited

Motivation and Context

Description

Testing

Checklist

jszwedko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jszwedko commented Apr 29, 2022

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jszwedko commented May 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdisanti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpetri commented May 24, 2022

jdisanti commented May 24, 2022

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jszwedko commented May 26, 2022

Velfi commented May 27, 2022

Add support for `credential_process` from profiles #1356

Add support for `credential_process` from profiles #1356

jszwedko commented Apr 29, 2022 •

edited