implement credential_process on ProcessCredentials provider

[srchase]

reworked from #1923 to get tests passing

• npm run test passes
• changelog is added, npm run add-change
• run bundle exec rake docs:api and inspect doc/latest/index.html if documentation is changed

[codecov-io]

Codecov Report

Merging #2559 into master will decrease coverage by 0.02%.
The diff coverage is 91.93%.

@@            Coverage Diff             @@
##           master    #2559      +/-   ##
==========================================
- Coverage   96.85%   96.82%   -0.03%     
==========================================
  Files         281      283       +2     
  Lines        8527     8609      +82     
  Branches     1621     1639      +18     
==========================================
+ Hits         8259     8336      +77     
- Misses        268      273       +5

Impacted Files	Coverage Δ
lib/credentials/shared_ini_file_credentials.js	97.84% <ø> (ø)	⬆️
lib/credentials/credential_provider_chain.js	100% <ø> (ø)	⬆️
lib/node_loader.js	91.17% <40%> (+1.62%)	⬆️
lib/credentials/process_credentials.js	96.49% <96.49%> (ø)
clients/appmesh.js	76.47% <0%> (-23.53%)	⬇️
clients/textract.js	100% <0%> (ø)
lib/service.js	98.14% <0%> (ø)	⬆️
lib/s3/managed_upload.js	94.05% <0%> (+0.08%)	⬆️
lib/protocol/rest_json.js	94.91% <0%> (+0.17%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0cb104...cd5aaf2. Read the comment docs.

[lorengordon]

Awesome to see progress on this! FWIW, there is a PR on botocore to at least print stderr, so that users have some method to see and respond to things like a prompt for MFA... The implementation in the AWS SDK for GO does this already.

[AllanZhengYP]

It will helpful to customer if you add an example of using credential process in the documentation block.

[jstewmon]

@lorengordon I think it would be better to have a light-weight protocol to facilitate an interactive credential_process (defined by whomever owns the credential_process interface). A library dumping to stderr for a handled exception would violate my expectations as a user.

[lorengordon]

I can respect that. Perhaps my javascript naivete is showing, but is the stderr being returned to the calling process? That's the problem in botocore... stderr is swallowed, not returned. So the credential_process can't decide what to do with it; it's just gone.

[jstewmon]

@lorengordon Any stderr from the subprocess will be appended to the resulting Error.message property if the exit code is non-zero.

For example, if fail is an executable like so:

#!/usr/bin/env bash
echo some stdout output
echo some stderr output > /dev/stderr
echo some other stderr output > /dev/stderr
exit 1

...and we have a node script:

proc.exec('./fail', (err, stdout, stderr) => {
  console.error('err.message:', err.message);
});

Then, we will get:

err.message: Command failed: ./fail
some stderr output
some other stderr output

[AllanZhengYP]

@lorengordon

The child_process.exec() spins up a subprocess but only call the callback functions until the process returns, which means by the time you see the stderr content, the subprocess has returned. To support interactive CLI, maybe child_process.spawn() makes more sense here. Because you can wire up the main process' stdin to subprocess' stdin, and supply the stdin to the running subprocess. One thing to note is that while the credential process is spun up yet not return, the SDK excecution should be blocked to avoid more processes are spun up.

[lorengordon]

To support interactive CLI, maybe child_process.spawn() makes more sense here.

Yeah, that would definitely be necessary.

The primary use case I'm aware of for credential_process is to retrieve a temporary credential from a federated identity provider (ADFS, Okta, etc, maybe AWS SSO one day 🤞). The identity provider commonly requires username/password/MFA, and so the process may prompt for all these things.

The credential_process of course outputs the credential in the required JSON schema on stdout, and nothing else can be on stdout or the credential process spec is violated. That leaves stderr as the only method for interacting with the user.

[jstewmon]

Should this statement and the following if block be here?

The callback that runs when loadViaCredentialsProcess completes assigns this.expired and calls the callback. These statements appear to run on every load, which will eagerly invoke callback during a refresh, since this.accessKeyId and this.secretAccessKey will have truthy values during a refresh.

[jstewmon]

I think err should be checked before trying to process stdOut, since the condition is always checked.

[jstewmon]

Should all cases where err is not known to have already been wrapped, be wrapped with AWS.util.error and the code assigned?

[AllanZhengYP]

Is this part same to that in SharedIniFileCredential? It's a relatively minor one but I'd like to put this functionality(loading correct profile) in the util function and attach to AWS.util in node_loader.

[srchase]

It's almost the same, but it loads profilesFromConfig after profilesFromCreds. If credentials and config files had profiles with the same name, SharedIniFileCredentials and ProcessCredentials would give different precedence.

[AllanZhengYP]

I see. Thanks for clearing.

[AllanZhengYP]

I don't see the diff for EcsCredentials. Do I miss it?

[srchase]

ECSCredentials already throws an error if ENV_RELATIVE_URI or ENV_FULL_URI aren't set. isConfiguredForEcsCredentials could be removed if ECSCredentials is going to be put into the default providers.

[AllanZhengYP]

Ship it!

Just as a record: the codecov test fails because of the code diff in default credential chain.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.