Credentials: partial migration to SDK v3 #1766
Conversation
|
|
JadenSimon
force-pushed
the
sijaden/credv3
branch
from
ffcf067 to
05a1320
Compare
|
SDK has broken types I guess. |
|
/runintegrationtests |
|
Still need to test for C9 and some edge cases. Will be adding a few more unit tests too. |
| const ssoCredentialProvider = this.makeSsoProvider() | ||
| return await ssoCredentialProvider.refreshCredentials() | ||
| // Patches 'source_profile' credentials as static representations, which the SDK can handle in all cases | ||
| private async patchSourceCredentials(): Promise<ParsedIniData> { |
There was a problem hiding this comment.
This part of the PR fixes issues with the JS SDK's credential handling. The SDK is unable to resolve source_profile fields when the source profile uses SSO/MFA/credential_process. We can handle this resolution ourselves, giving the SDK the resolved credentials by 'pre-loading' them.
There was a problem hiding this comment.
Has this come up as a case customers are using? Do we have any idea if the ASK CLI supports this? Seems like a nice improvement on paper but we should also tread carefully when adding custom credentials logic on top of what the SDK provides. It will increase the surface area for bugs to creep in (and credentials always seem to have goofy edge cases) and is maintainability overhead. Another approach is contributing changes back to the JS SDK if we feel this is a gap.
There was a problem hiding this comment.
The reason for doing this patching is to implement functionality that SDK V2/V3 lack. The CLI supports this behavior, so we should support it too. You're right that we should be updating the SDK to support this functionality, but I was looking for a fast solution to solve user issues. I do want to fix-up the SDK, but they require quite a bit more changes on their end to make it work correctly. With this PR, we effectively only use the SDK to parse the credential files (and retrieve creds from the environment), then resolve the credentials ourselves.
There was a problem hiding this comment.
Makes sense, thanks for the context. Agree this is the right solution for now since there's active customer pain on this issue.
| await getMfaTokenFromUser(mfaSerial, this.profileName, callback), | ||
| }) | ||
| private makeSharedIniFileCredentialsProvider(loadedCreds: ParsedIniData): AWS.CredentialProvider { | ||
| const assumeRole = async (credentials: AWS.Credentials, params: AssumeRoleParams) => { |
There was a problem hiding this comment.
V3 no longer performs role assumption for us; we must do it manually.
| @@ -78,11 +77,6 @@ describe('Ec2CredentialsProvider', function () { | |||
| assert.strictEqual(credentialsProvider.getDefaultRegion(), undefined) | |||
| }) | |||
|
|
|||
| it('returns credentials', async function () { | |||
| const credentials = await credentialsProvider.getCredentials() | |||
| assert(credentials instanceof EC2MetadataCredentials) | |||
There was a problem hiding this comment.
V3 credentials are no longer instances of a class, they are just plain old data instead. So this test (and the corresponding environment var provider test) are unnecessary.
|
/runintegrationtest |
|
/retrybuild |
There was a problem hiding this comment.
checking ec2 instance credentials
2021-08-11 23:24:14 [VERBOSE]: credentials: EC2 metadata region: eu-central-1
2021-08-11 23:24:14 [VERBOSE]: credentials: EC2 metadata service call took 3ms
2021-08-11 23:24:14 [ERROR]: Error trying to connect to AWS with Credentials Provider ec2:instance. Toolkit will now disconnect from AWS. [Error [CredentialsProviderError]: Invalid response received from instance metadata service.
at new i (/home/ec2-user/environment/.c9/extensions/aws-toolkit-vscode/dist/extension.js:1667:7169)
at /home/ec2-user/environment/.c9/extensions/aws-toolkit-vscode/dist/extension.js:1680:11728
at nt (/home/ec2-user/environment/.c9/extensions/aws-toolkit-vscode/dist/extension.js:1680:2892)
at Object.next (/home/ec2-user/environment/.c9/extensions/aws-toolkit-vscode/dist/extension.js:1680:2170)
at ke (/home/ec2-user/environment/.c9/extensions/aws-toolkit-vscode/dist/extension.js:1680:1726)
at processTicksAndRejections (internal/process/task_queues.js:97:5)] {
tryNextLink: true
}
edit: same behavior with 5a30d27 (current master)
Problem
SDK v2 has some known issues in regards to credentials from
source_profile. SDK v3 does not fix them out-right, however, it does expose some functionality that allows us to fix the bugs.Solution
Migrate the credentials subsystem to use SDK v3, then add additional logic to handle
source_profileresolution.The new SDK credentials API allows us to 'pre-load' credentials when calling relevant APIs. We can use this functionality to load source profile credentials ourselves, letting the SDK treat them as static credentials. Any valid profile should now be usable as a base profile as well (e.g. processes, SSO, MFA, etc.)
TODO: re-add test for when SDK never resolves a credential provider. Current test broke since the API is a little bit different with v3TODO: check if the STS region hack is still neededTODO: add regression testsExtra testing
Still thinking of more tests I can add.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.