fix(cli): credential provider plugins cannot be used with modern synthesis #11350
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
November 4, 2020 18:09
…Credentials if an environment is passed
…there's nothing found
|
|
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
IainCole
changed the title
|
Wait, how do you intend to use this? There is an assumed role, sure. Your credential provider is going to give you credentials to use to assume the role, is that right? Our intended use of this feature was that you would use your default (user/developer) credentials to account A to federate into another account B (assuming that account B had given you permissions to do so by using Can you describe your use case in more detail? What kind of credential provider are you using? |
rix0rrr
previously requested changes
Nov 16, 2020
There was a problem hiding this comment.
If we agree that this is a useful change, you're going to have to add a test to assert that the credentials returned by the provider will be used to assume the role.
Changing the call signatures of the function in existing tests does not constitute sufficient testing, we didn't test the new functionality yet :).
|
Yeah, I want the credentials to come from a credential provider. I work for a large IaaS company and have a lot of AWS accounts I need to be using with CDK, I also don't want to have loads of creds on my machine if at all possible. So I use a credential provider to authenticate me and retrieve credentials for use in CDK which works great, but I can't use any of the new bootstrap assumeRole stuff because it can't get the credentials from the plugin, happy to add tests and stuff if we think this is a useful change, changing the call signatures was just to make it build :). |
|
Okay I do see the use case here. Feeling slightly uneasy about the change (as there are now 2 federation mechanisms fighting), but given that we don't really have anything better in place and you need to be unblocked, I'll go ahead and okay it after you add that test. |
…edRole is called with an environment
mergify
bot
dismissed
rix0rrr’s stale review
Pull request has been modified.
rix0rrr
changed the title
rix0rrr
approved these changes
Nov 19, 2020
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
rix0rrr
added a commit
that referenced
this pull request
Dec 9, 2020
This addresses a regression introduced in #11350 and reported in #11792. The original PR added support for using a credential provider as a source for credentials that will be used to assume the CDK Deploy Roles (created by Modern Bootstrapping and used by Default Synthesis). However, by doing so, it broke support for using current AWS credentials to do the same. Closes #11792.
mergify bot
pushed a commit
that referenced
this pull request
Dec 11, 2020
This addresses a regression introduced in #11350 and reported in #11792. The original PR added support for using a credential provider as a source for credentials that will be used to assume the CDK Deploy Roles (created by Modern Bootstrapping and used by Default Synthesis). However, by doing so, it broke support for using current AWS credentials to do the same. ## Original Behavior - In case of legacy stack, use "current credentials" if they are for the right account, otherwise ask credential provider plugins, fail if current credentials are for the wrong account - In case of DefaultSynthesized stack, always use "current credentials" to assume role ## Broken Behavior - In case of legacy stack, use "current credentials" if they are for the right account, otherwise ask credential provider plugins for credentials to the target account, fail if current credentials are for the wrong account - In case of DefaultSynthesized stack, - Use "current credentials" if they are for the right account, otherwise ask credential provider plugins for credentials to the target account, fail if current credentials are for the wrong account (reusing the logic from legacy stacks) - Then assume role using credentials obtained in the previous step The reuse of the same "credential obtaining" logic here broke cross-account role assumption, because we'd fail as soon as the current credentials would be for the wrong account instead of still trying to use them for AssumeRole. ## New Behavior - Try to get "base credentials" for a target environment: - Use "current credentials" if they are for the right account - Ask credential provider plugins - Use "current credentials" if they are for the wrong account (but remember that they were wrong) - In case of a legacy stack: - If the credentials are for the wrong account, fail - In case of a DefaultSynthesized stack: - Use the set of "base credentials" to try to assume the target role - If that succeeds, we're done - (Fallback) If it fails and the base credentials were already for the right account, use them after all. This fallback will allow people already using credential plugins to keep on using them, even when interoperating with DefaultSynthesized stacks. It will also allow people who seed their terminal with "ReadOnly" credentials (which might not have `sts:AssumeRole` permissions) to still run `cdk diff` as in the past. ## Changes to tests There has been a major refactoring of the tests around this. The current way of testing (mocking some calls left and right) was completely insufficient to test these mechanisms properly. I've therefore resorted to implementing a fake, in-memory version of STS's `GetCallerIdentity` and `AssumeRole`, and using the testing library `nock` to redirect network calls to that in-memory service. This will allow us to test the entire chain from our code down to and including the SDK's behavior, and make sure the right behavior happens without worrying about exact call orders. At the same time, the single gigantic test fixture (with the `~/.aws/credentials` and `~/.aws/config` files) was becoming rather cumbersome. Instead, each test now includes just the one or two profile sections it uses, and a helper function creates both the config files as well as immediately creates those users in-memory in the fake STS service. Closes #11792. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
flochaz
pushed a commit
to flochaz/aws-cdk
that referenced
this pull request
Jan 5, 2021
This addresses a regression introduced in aws#11350 and reported in aws#11792. The original PR added support for using a credential provider as a source for credentials that will be used to assume the CDK Deploy Roles (created by Modern Bootstrapping and used by Default Synthesis). However, by doing so, it broke support for using current AWS credentials to do the same. ## Original Behavior - In case of legacy stack, use "current credentials" if they are for the right account, otherwise ask credential provider plugins, fail if current credentials are for the wrong account - In case of DefaultSynthesized stack, always use "current credentials" to assume role ## Broken Behavior - In case of legacy stack, use "current credentials" if they are for the right account, otherwise ask credential provider plugins for credentials to the target account, fail if current credentials are for the wrong account - In case of DefaultSynthesized stack, - Use "current credentials" if they are for the right account, otherwise ask credential provider plugins for credentials to the target account, fail if current credentials are for the wrong account (reusing the logic from legacy stacks) - Then assume role using credentials obtained in the previous step The reuse of the same "credential obtaining" logic here broke cross-account role assumption, because we'd fail as soon as the current credentials would be for the wrong account instead of still trying to use them for AssumeRole. ## New Behavior - Try to get "base credentials" for a target environment: - Use "current credentials" if they are for the right account - Ask credential provider plugins - Use "current credentials" if they are for the wrong account (but remember that they were wrong) - In case of a legacy stack: - If the credentials are for the wrong account, fail - In case of a DefaultSynthesized stack: - Use the set of "base credentials" to try to assume the target role - If that succeeds, we're done - (Fallback) If it fails and the base credentials were already for the right account, use them after all. This fallback will allow people already using credential plugins to keep on using them, even when interoperating with DefaultSynthesized stacks. It will also allow people who seed their terminal with "ReadOnly" credentials (which might not have `sts:AssumeRole` permissions) to still run `cdk diff` as in the past. ## Changes to tests There has been a major refactoring of the tests around this. The current way of testing (mocking some calls left and right) was completely insufficient to test these mechanisms properly. I've therefore resorted to implementing a fake, in-memory version of STS's `GetCallerIdentity` and `AssumeRole`, and using the testing library `nock` to redirect network calls to that in-memory service. This will allow us to test the entire chain from our code down to and including the SDK's behavior, and make sure the right behavior happens without worrying about exact call orders. At the same time, the single gigantic test fixture (with the `~/.aws/credentials` and `~/.aws/config` files) was becoming rather cumbersome. Instead, each test now includes just the one or two profile sections it uses, and a helper function creates both the config files as well as immediately creates those users in-memory in the fake STS service. Closes aws#11792. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm not currently able to use CDK pipelines because I use a credential provider. I believe this is because the new bootstrap uses assumed roles. When assuming a role, only defaultCredentials is checked, I'm not sure if this was intentional or not because there is a comment that seems to mention this (that I will edit if this PR has any merit), but it seems odd to not allow a credential provider here.
I'm not 100% sure why I had to change the account ID in the tests, but I did and all the tests seem to still pass.