fix(cli): cross-account deployment no longer works #11966

rix0rrr · 2020-12-09T16:31:16Z

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

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.

gitpod-io · 2020-12-09T16:31:19Z

rix0rrr · 2020-12-09T16:32:00Z

Still needs tests, and needs more explanation in the PR body. Getting this out as quickly as possible to start collecing comments.

redbaron · 2020-12-09T19:33:36Z

@rix0rrr , I am trying to understand, how does it help to do cross account deployment? For cdk deploy to work cross accounts, effective credentials need to be able to:

do all necessary lookups (lookup by id, CDKToolkit lookup, etc)
assume roles created by bootstrap stack

When we boostrap account A with --trust B and make depoyment from B, we still can't do lookups in A, right? Therefore new role in account A has to be created still.

rix0rrr · 2020-12-10T09:59:12Z

When we boostrap account A with --trust B and make depoyment from B, we still can't do lookups in A, right?

Lookups are out of scope for this, and a separate backlog item: #8905.

It will work if you deploy stacks that do not require context lookups though.

nija-at

Thanks for adding comments along. Made the logic so much easier to review.

Minor comments below.

nija-at · 2020-12-10T15:12:51Z

packages/aws-cdk/lib/api/aws-auth/credential-plugins.ts

-        return (providerOrCreds as any).resolvePromise();
-      }
-      return providerOrCreds;
+      const credentials = (providerOrCreds as any).resolvePromise ? await (providerOrCreds as any).resolvePromise() : providerOrCreds;


minor: could we do better than as any? What type are we expecting here?

// Backwards compatibility: if the plugin returns a ProviderChain, resolve that chain. // Otherwise it must have returned credentials.

nija-at · 2020-12-10T15:17:45Z

packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts

+    // We will proceed to AssumeRole using whatever we've been given.
+    const sdk = await this.withAssumedRole(baseCreds, options.assumeRoleArn, options.assumeRoleExternalId, env.region);
+
+    // Do a call on this SDK. We don't care about the answer, but we want to excercise the AssumeRoleCredentialsProvider


Suggested change

// Do a call on this SDK. We don't care about the answer, but we want to excercise the AssumeRoleCredentialsProvider

// Do a call on this SDK. We don't care about the answer, but we want to exercise the AssumeRoleCredentialsProvider

nija-at · 2020-12-10T15:20:46Z

packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts

+      // feed the CLI credentials which are sufficient by themselves. Prefer to assume the correct role if we can,
+      // but if we can't then let's just try with available credentials anyway.
+      if (baseCreds.source === 'correctDefault' || baseCreds.source === 'plugin') {
+        debug(e.message);


minor: also place the stack trace into debug logs.

nija-at · 2020-12-10T15:32:09Z

packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts

+    // Simple case is if we don't need to "assumeRole" here. If so, we must now have credentials for the right
+    // account.
+    if (options?.assumeRoleArn === undefined) {
+      if (baseCreds.source === 'incorrectDefault') { throw new Error(fmtObtainCredentialsError(env.account, baseCreds)); }


curious: why should credentials from a plugin when there is no role to assume produce an error?

That's not what this says though. This says if the default credentials are for the wrong account.

…nd-plugins

mergify · 2020-12-11T13:53:19Z

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-cdk-automation · 2020-12-11T13:54:43Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 230a874
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

redbaron · 2020-12-17T02:41:48Z

@rix0rrr , this change fixed cdk diff (it now assumes cdk-deploy role in the target account natively without plugins), but assets uploads during cdk deploy still produce following error:

[0%] start: Publishing 4a3609ad912843e581892f37ae9d6fb0fa1648b547693aaa562b0119452b8956:1454xxxxxxx-eu-west-2
[50%] fail: Need to perform AWS calls for account 1454xxxxxxx, but the current credentials are for 0147xxxxxxx

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*

rix0rrr requested a review from a team December 9, 2020 16:31

rix0rrr self-assigned this Dec 9, 2020

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Dec 9, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Dec 9, 2020

github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Dec 9, 2020

Make logic easier to follow

f4d0a6a

nija-at approved these changes Dec 10, 2020

View reviewed changes

rix0rrr added 6 commits December 11, 2020 14:11

Big test refactoring

33ae2ab

Merge remote-tracking branch 'origin/master' into huijbers/assuming-a…

e0d5218

…nd-plugins

Replace sdk-provider tests

1741a89

Cleaning up

55dddbb

Remove addl dep

8653360

Update yarn.lock

230a874

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Dec 11, 2020

mergify bot merged commit 6fb3448 into master Dec 11, 2020

mergify bot deleted the huijbers/assuming-and-plugins branch December 11, 2020 13:53

sbassin mentioned this pull request Dec 12, 2020

latest cdklocal/localstack fails to bootstrap localstack/aws-cdk-local#36

Closed

redbaron mentioned this pull request Dec 18, 2020

(cli): cross account deployments still don't work #12151

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): cross-account deployment no longer works #11966

fix(cli): cross-account deployment no longer works #11966

rix0rrr commented Dec 9, 2020 •

edited

Loading

gitpod-io bot commented Dec 9, 2020 •

edited

Loading

rix0rrr commented Dec 9, 2020

redbaron commented Dec 9, 2020

rix0rrr commented Dec 10, 2020

nija-at left a comment

nija-at Dec 10, 2020

rix0rrr Dec 11, 2020

nija-at Dec 10, 2020

nija-at Dec 10, 2020

nija-at Dec 10, 2020 •

edited

Loading

rix0rrr Dec 10, 2020

mergify bot commented Dec 11, 2020

aws-cdk-automation commented Dec 11, 2020

redbaron commented Dec 17, 2020 •

edited

Loading

	// Do a call on this SDK. We don't care about the answer, but we want to excercise the AssumeRoleCredentialsProvider
	// Do a call on this SDK. We don't care about the answer, but we want to exercise the AssumeRoleCredentialsProvider

fix(cli): cross-account deployment no longer works #11966

fix(cli): cross-account deployment no longer works #11966

Conversation

rix0rrr commented Dec 9, 2020 • edited Loading

Original Behavior

Broken Behavior

New Behavior

Changes to tests

gitpod-io bot commented Dec 9, 2020 • edited Loading

rix0rrr commented Dec 9, 2020

redbaron commented Dec 9, 2020

rix0rrr commented Dec 10, 2020

nija-at left a comment

Choose a reason for hiding this comment

nija-at Dec 10, 2020

Choose a reason for hiding this comment

rix0rrr Dec 11, 2020

Choose a reason for hiding this comment

nija-at Dec 10, 2020

Choose a reason for hiding this comment

nija-at Dec 10, 2020

Choose a reason for hiding this comment

nija-at Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

rix0rrr Dec 10, 2020

Choose a reason for hiding this comment

mergify bot commented Dec 11, 2020

aws-cdk-automation commented Dec 11, 2020

AWS CodeBuild CI Report

redbaron commented Dec 17, 2020 • edited Loading

rix0rrr commented Dec 9, 2020 •

edited

Loading

gitpod-io bot commented Dec 9, 2020 •

edited

Loading

nija-at Dec 10, 2020 •

edited

Loading

redbaron commented Dec 17, 2020 •

edited

Loading