aws/credential: Add credential_process provider

[YakDriver]

Fixes #1834
Continuation of #1874

How It Works

If you have a method of sourcing credentials that isn't built in to the [SDK], you can integrate it by using credential_process in the config file. The [SDK] will call that command exactly as given and then read json data from stdout.

For example, you may have this configuration (this example uses awsprocesscreds for SAML integration):

[dev]
credential_process = awsprocesscreds-saml -e https://example.okta.com/home/amazon_aws/blob/123 -u 'monty@example.com' -p okta -a arn:aws:iam::123456789012:role/okta-dev

Assuming the process returns a valid json like this example (but with valid credentials):

{
  "Version": 1,
  "AccessKeyId": "<access key id>",
  "SecretAccessKey": "<secret access key>",
  "SessionToken": "<optional session token>",
  "Expiration": "<optional expiration date in ISO8601>"
}

You could use it in your code like this:

sess := session.Must(session.NewSessionWithOptions(session.Options{
    Profile: "dev",
}))

svc := s3.New(sess)

result, err := svc.ListBuckets(nil)
...

You can also provide the process command programmatically without using a config file.

creds := credentials.NewCredentials(&processcreds.ProcessProvider{
    Process: "/path/to/a/cred/process",
})

sess := session.Must(session.NewSessionWithOptions(session.Options{
    Credentials: creds,
}))

svc := s3.New(sess)

This Pull Request

This PR adds support for calling a credential_process and builds on the work of @micahhausler, taking into account review notes by @jasdel.

Notable changes over #1874:

• Rebased, fixing conflict
• Removes the processcreds provider from the default credential chain
• Adds further protection of credential_process including a timeout and small buffer limit (in case of a hung process or a process producing much data) - limits are configurable
• Uses goroutines for executing process, reading data from the process
• Moves the new provider and test into a processcreds package
• Creates const error messages and default values
• Completely avoids reading shared config file, leaving this to the existing providers
• Process is passed via input parameter to the provider allowing for programmatic defining without need for shared config file
• Makes Expiration of type Time
• Carefully handles the environment (which is tricky when tests clear the env)
• Avoids issues with string splitting the command by using subshells on Windows and Linux
• Lets exec.Command fail rather than doing pre-checks
• Captures both stdout and stderr in output
• Does not mess with profiles, leaving that to existing resources
• Does not use testify
• Includes tests providing near 100% code coverage

Please let me know if you have additional review.

See related:

• Implement usage of credential_process from AWS ini credentials file if it exists aws-sdk-js#1923
• Add support for process-based credential providers boto/botocore#1316
• Support for credential_process in ~/.aws/config serverless/serverless#4838
• 🌱 Support for credential_process oktadev/okta-aws-cli-assume-role#203
• Extensible Credential Providers aws-sdk-ruby#1820
• credential_process is not implemented aws-sdk-java-v2#455

[YakDriver]

Currently working on getting make ci-test to pass

[YakDriver]

@xibz @jasdel Any idea why some of the travis-ci builds are failing? They are unrelated to my changes.

[jasdel]

Looks like the build failure was an upstream golang.org/x/tool failure. Will restart tests.

[YakDriver]

Updated to work with #2210 (internal/ini: add support for ini parser). For what it's worth, locally make ci-test is working:

...
?   	github.com/aws/aws-sdk-go/service/xray	[no test files]
?   	github.com/aws/aws-sdk-go/service/xray/xrayiface	[no test files]
CI test validate no generated code changes

[YakDriver]

@jasdel @xibz x/tools are only supported for the latest two versions of Go: 1.10 and 1.11. Your travis-ci test mixes 1.7 and 1.8 with x/tools. Specifically, x/tools tries to call obj.IsAlias(), which didn't exist until 1.9. Is there anything I can do to avoid the 1.7 and 1.8 tests?

[jasdel]

@YakDriver thanks for letting us know about the build failure. We'll need to update the unit tests and code generation so that no test with x/tools dependency will be run. The SDK's core and client unit tests should all still be valid to run.

[YakDriver]

@jasdel it looks like all the Linux/Darwin 1.9, 1.10, 1.11 tests for this PR are working fine. I've run the the PR tests on an EC2 windows instance also and they ran fine. Looking forward to a review when you get a chance. 😃

[mikkeloscar]

This would also be a workaround for #1993 so I'm also in support for getting this reviewed/merged.

[xibz]

Hello @YakDriver, thank you for taking the time to create this PR. We have reviewed this internally and there are some concerns we have. I've decided to pull this in and refactor some of this based on the concerns and ensure that we get this reviewed internally as well. I just wanted to let you know that we have looked at this and we have decided to only allow the credential process provider to be enabled via code and not the shared config. So, I'll go ahead and make those changes and if you have any questions or feedback, please let us know.

[lorengordon]

we have looked at this and we have decided to only allow the credential process provider to be enabled via code and not the shared config

Ugh. As a user, that's a major bummer.

[xibz]

@lorengordon - Out of curiosity, why is this a major issue? Would it be difficult to not have a predefined location of said credential process, ie /usr/bin and have you code just reference that location? Why having it in shared config such a need, I guess is what I am asking? What is your use case?

Reading the AWS CLI docs it explicitly states,

Warning

The following describes a method of sourcing credentials from an external process. This can
potentially be dangerous, so proceed with caution. Other credential providers should be preferred
if at all possible. If using this option, you should make sure that the config file is as locked
down as possible using security best practices for your operating system.

Found here. This is a major reason why we are potentially deciding this to only be allowed via code.

[lorengordon]

<rant>
@xibz Because applications are written around the AWS SDK, and lean strongly on the AWS SDK to load and handle credentials for AWS. This is especially an issue for Go, since applications are generally compiled as static binaries.

The AWS Shared Config file is a common way for users of such applications to manage credentials for multiple accounts and multiple roles, in a way that, from a user's perspective, seems ought to work the same way across all applications, regardless of the backing AWS SDK. It would be great for that to work in a way that didn't require extra boilerplate in every application, and result in a lot of confusing issues for every application about why a particular credential method worked for some app "foo" but not some app "bar".

Right now, it sucks that Golang apps handle AWS credentials differently than Javascript apps, differently than Ruby apps, differently than Python apps, etc, etc. I certainly get that there may be some delay in support for new methods, with Python seeming to lead the way here, but simply choosing not to support the same methods in common across all SDKs... well, that's just a terrible user experience.

Amazon in the past had seemed to indicate we could expect this kind of parity across SDKs as the new way forward:

• https://aws.amazon.com/blogs/security/a-new-and-standardized-way-to-manage-credentials-in-the-aws-sdks/

We were just tackling one credential provider here, because we have a specific need for this provider in our environment, but the larger criticism is still valid. Without including support for loading the credential_process from the shared config, this contribution will likely end up being a waste of our time. That's a risk we take with all contributions to open source projects, but still, it stings. And I'm not looking forward to the maintenance tail of maintaining a fork just to build this in to open source apps we use.
</rant>

[lorengordon]

Found here. This is a major reason why we are potentially deciding this to only be allowed via code.

Yes. And yet, support for the option in the shared config is already baked in to the python and ruby SDKs. They nicely leave the option to the user, rather than trying to decide centrally for every user and use case.

Edit: This is what I'm talking about with the inconsistent user experience. I shouldn't need to know that the AWS CLI is written in Python and is using botocore to load the Shared Config. Nor should I need to know that Terraform is written in Go, and using the AWS Go SDK to load the Shared Config. Do both SDKs support the same options? If not, is it a subset of options? Is it a totally different set of options? Is there a master list of the possible options somewhere? I shouldn't need to care about this. They should both just work.

[xibz]

@lorengordon - Thank you for the feedback, and I want to let you know that we are not finalized on a decision as we want more feedback from users. However, some things to consider we want this to behave similarly to how the spec is defined internally. This is why we want to pull this down and make the changes. Also, having this only code enabled shouldn't be very different than how the other SDKs do it today. In addition, we aren't making a decision until we've received feedback from users and we have a meeting with a user tomorrow to ensure that we can implement this safe, secure, and easy to use. I believe we can achieve this and your feedback will help with that.

Here's what we were thinking,

cmd, err := processprovider.NewCommandFromConfig("/path/to/config", "profile")
if err != nil {
    return err
}

p := processprovider.NewProcessProvider(cmd)
cfg := &aws.Config{
    Credentials: credentials.NewCredential(p),
}

sess := session.Must(session.NewSession(cfg))

Please let us know if this will work for you

[mikkeloscar]

I just want to echo @lorengordon's concerns. If you only allow this provider via code it's pretty much useless. The reason it's useless is as @lorengordon says that it won't bring feature parity between the SDKs which will require every applicationn developer to do custom credentials initialization depending on the environment where the application is running. The whole point of the credentials chain is to avoid this and make it "just work" no matter the environment (or which language the SDK is written for).

I have been trying to argue the same thing in #1993 for months.

Having this supported the same way as python, would "solve"/work around the problem stated in #1993 for the Go SDK. I have created a doc on the SDK support for solving #1933 https://github.com/mikkeloscar/kube-aws-iam-controller/blob/master/docs/sdk-configuration.md

[lorengordon]

@xibz I'm failing to see how handling this only in code addresses the linked issues, at all. But I'm open to the idea that is a failing on my part and not a failing of what you're currently proposing.

Here's a workflow I would consider a success:

1. AWS Go SDK merges support for credential_process and releases a new version.
2. Applications update the version of the AWS Go SDK they use, make no other changes to their code*, and release a new version.
3. Users of such applications are now able to use credential_process via the AWS Shared Config file to make credentials available to that application.

Any workflow that requires applications in Step 2 to make further changes to their code, I would consider to be failing to address the linked issues. I'd even wonder why bother merging, since no one seems to be asking for the feature being considered. And really, if that ends up being the case, the linked issues should be unlinked so they are not closed.

*There is probably an assumption here that the application is otherwise already creating the session correctly.

[YakDriver]

• Botocore, shared_config implementation of credential_process, November 20, 2017, by @JordonPhillips
• Ruby, shared_config implementation of credential_process, July 3, 2018, by community contributor (@YashoSharma)

[jasdel]

Thanks for the additional feedback. Our biggest concerns about enabling this feature by default is that users may unknowingly be opening them applications up to exploits by the SDK executing arbitrary executables from the shared config file. This is the primary reason we are concerned about enabling this feature in the default credential chain automatically within an application.

We understand the benefit of it should just work out of the box, enabling this feature automatically opens the application up to being exploited.

There are a couple use cases where we think the automatic behavior could be exploited:

• User's application has its ~/.aws/config or ~/.aws/credentials files compromised.
• User's application is instructed to use an alternative compromised shared config/credential file. e.g AWS_CONFIG_PATH and similar environment variables.

The following is the most obvious use case I immediately think of that combines these two vulnerabilities together. But, I have no doubt that there are others.

An given Go application using the SDK with automatic support for credential_process feature is spawned as a CGI "script". The web app allows external users to upload a file to be processed. That file is stored in a predictable pattern in the web app's host storage.

A nefarious user uses the web app to upload a shared config file with a malicious command in the credential_process field. The nefarious user also constructs a request to the web service that includes a AWS_CONFIG_PATH header value. The value of this header points to uploaded shared config file with the malicious command. The CGI caller will execute the Go app as a CGI "script" with the AWS_CONFIG_PATH header as a environment variable. This will instruct the SDK within the app to load the uploaded shared config with the malicious command, and execute the command automatically with the permissions of the CGI script.

Because of these risks, we think that something in the user's application code needs to explicitly enable this feature, beyond the shared config file, is needed to prevent unexpected execution of arbitrary executables. We see the value and need being able to define a way to source credentials from an external process, but we do not think it is appropriate for an automatic default feature within the SDK.

[YakDriver]

I don't dispute that there are risks. 🎃 😨 All features must face a risk/benefit determination. We just ask that a consistent risk/benefit determination be made across AWS CLI, Ruby, Python, and go.

This PR follows a similar mitigation pattern to the other implementations. It will only use credential_process if other, pre-existing credential mechanisms don't exist. At the very least, that makes it very noticeable that you are using credential_process. If it's setup to run an arbitrary command, you won't get credentials and CLI, Ruby, Python, go won't connect anymore. An unauthorized change to credential_process can't be slipped in without breaking existing apps.

In fact, CLI/Ruby/Python/go sharing the same shared_config and working the same way provides a great security benefit over one or the other working independently. If the config is compromised, you'll notice it across apps and platforms.

For example, here, a slipped in credential_process would be entirely ignored, and never executed, similar to the botocore and Ruby implementations.

[default]
aws_access_key_id = <YOUR_DEFAULT_ACCESS_KEY_ID>
aws_secret_access_key = <YOUR_DEFAULT_SECRET_ACCESS_KEY>
credential_process = <YOUR_PROCESS_COMMAND>

The very precise configuration is akin to purposely enabling the feature.

This PR's go implementation further mitigates beyond the botocore and Ruby implementations by providing a process timeout and a very limited stdout/stderr buffer.

[YakDriver]

@xibz @jasdel I made the requested changes. I added new commits to make review easier but happy to squash if you prefer.

Are either of you at re:Invent?

Reviewed by jasdel

[jasdel]

Thanks for working with us to create this PR, @YakDriver. The updates look good, and we'll be able to merge this PR in nearly next week after re:Invent.

@xibz and I are not at re:Invent this year, though there are a few AWS SDK team members at the developer tools booth this year.

[YakDriver]

Excellent news! We look forward to being among the first to use it.

[mikkeloscar]

Excellent news! We look forward to being among the first to use it.

I'll race you! :)

Awesome work, thanks!