Feature/credential provider ini #25
Conversation
| { | ||
| "compilerOptions": { | ||
| "module": "commonjs", | ||
| "target": "es6", |
There was a problem hiding this comment.
I think we still are targetting es5
| * @param sourceCreds The credentials with which to assume a role. | ||
| * @param params | ||
| */ | ||
| roleAssumer?: ( |
There was a problem hiding this comment.
I really like that roleAssumer is something that is passed in, rather than trying to have each credential provider rewrite logic to assume a role (and pull in all those dependencies)
| } | ||
|
|
||
| async function resolveProfileData( | ||
| profile: string, |
There was a problem hiding this comment.
Nit: does profileName make more sense? Only reason I say that is because you're also passing in profiles, and I'd expect profile to be a singular value from profiles.
| source_profile, | ||
| } = data; | ||
|
|
||
| const sourceCreds = fromIni({ |
There was a problem hiding this comment.
If someone accidentally set their source_profile to be the same as their profile, would this cause a stack call exceeded error?
There was a problem hiding this comment.
It would. I can detect a simple cycle here and throw, but detecting more complex cycles like the following would require this function to keep a breadcrumb trail of all profiles visited:
default:
role_arn: foo
source_profile: bar
bar:
role_arn: baz
source_profile: quux
quux:
role_arn: fizz
source_profile: default
Is it worth it in that case to throw a "cycle detected" error?
There was a problem hiding this comment.
Actually, since fromIni returns a CredentialProvider, and that resolves a promise, I think we'll never exceed the call stack limit since promises are put on the microtask queue, unless I'm missing something. That does however mean instead of getting a call stack error, the promise 'loop' will just keep going forever and never resolve.
If that's the case, it might be worth detecting a cycle so we can reject the promise instead.
There was a problem hiding this comment.
Err, yes, the particular error will be memory exhaustion or a failed malloc, but the user will still see a lag followed by a supremely unhelpful error message.
Cycle detection (and an appropriate test) added.
| options: FromIniInit | ||
| ): Promise<Credentials> { | ||
| const data = profiles[profile]; | ||
| if (isStaticCredsProfile(data)) { |
There was a problem hiding this comment.
It's an edge case, but I could see someone including source creds and role arn in the same profile.
Do you know what the CLI does in that situation? We might need to handle this if it turns out other SDKs support that case.
There was a problem hiding this comment.
I had assumed the CLI would prefer static credentials, but it looks like I had that backwards. I'd like to verify with someone on the boto team because the semantics of this seem a bit weird. (If I've specified a role ARN, access key, and secret key, should the profile use those credentials to call STS? What if I supply a role ARN, static credentials, and a source profile?)
There was a problem hiding this comment.
I spoke with Kyle, and the CLI will require any profile with a role_arn to also specify a source_profile, and any static credentials in that profile would be ignored. I'll update this PR to make the behavior matches the CLI.
There was a problem hiding this comment.
I switched the order of the checks and added a test to verify that we match the CLI.
| }); | ||
| } | ||
|
|
||
| function slurpFile(path: string): Promise<string> { |
There was a problem hiding this comment.
| @@ -0,0 +1,9 @@ | |||
| { | |||
| "compilerOptions": { | |||
There was a problem hiding this comment.
Since we're using async in this package, what about setting "importHelpers": true and depending on tslib? That should reduce the amount of code the TypeScript compiler generates.
There was a problem hiding this comment.
Sure; using tslib really helps our code coverage numbers, too.
Feature/credential provider ini
|
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. |
Spun off from #5.
This PR adds support for loading credentials from the shared config and credentials ini files. It supports the full suite of recursive role assumption, MFA tokens, and configuration via environment variables.