Skip to content

Commit

Permalink
fix(cli): cross-account deployment no longer works (aws#11966)
Browse files Browse the repository at this point in the history
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*
  • Loading branch information
rix0rrr authored and flochaz committed Jan 5, 2021
1 parent 14db5b8 commit f659696
Show file tree
Hide file tree
Showing 12 changed files with 1,213 additions and 579 deletions.
1 change: 0 additions & 1 deletion packages/aws-cdk/lib/api/aws-auth/account-cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ export class AccountAccessKeyCache {
// try to get account ID based on this access key ID from disk.
const cached = await this.get(accessKeyId);
if (cached) {

debug(`Retrieved account ID ${cached.accountId} from disk cache`);
return cached;
}
Expand Down
1 change: 0 additions & 1 deletion packages/aws-cdk/lib/api/aws-auth/aws-sdk-inifile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ export class PatchedSharedIniFileCredentials extends AWS.SharedIniFileCredential
return;
}
sts.assumeRole(roleParams, callback);

}

private sourceProfileCredentials(sourceProfile: string, profiles: Record<string, Record<string, string>>) {
Expand Down
18 changes: 11 additions & 7 deletions packages/aws-cdk/lib/api/aws-auth/credential-plugins.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ import { CredentialProviderSource, Mode } from './credentials';
* for the given account.
*/
export class CredentialPlugins {
private readonly cache: {[key: string]: AWS.Credentials | undefined} = {};
private readonly cache: {[key: string]: PluginCredentials | undefined} = {};

public async fetchCredentialsFor(awsAccountId: string, mode: Mode): Promise<AWS.Credentials | undefined> {
public async fetchCredentialsFor(awsAccountId: string, mode: Mode): Promise<PluginCredentials | undefined> {
const key = `${awsAccountId}-${mode}`;
if (!(key in this.cache)) {
this.cache[key] = await this.lookupCredentials(awsAccountId, mode);
Expand All @@ -29,7 +29,7 @@ export class CredentialPlugins {
return PluginHost.instance.credentialProviderSources.map(s => s.name);
}

private async lookupCredentials(awsAccountId: string, mode: Mode): Promise<AWS.Credentials | undefined> {
private async lookupCredentials(awsAccountId: string, mode: Mode): Promise<PluginCredentials | undefined> {
const triedSources: CredentialProviderSource[] = [];
// Otherwise, inspect the various credential sources we have
for (const source of PluginHost.instance.credentialProviderSources) {
Expand All @@ -44,11 +44,15 @@ export class CredentialPlugins {

// Backwards compatibility: if the plugin returns a ProviderChain, resolve that chain.
// Otherwise it must have returned credentials.
if ((providerOrCreds as any).resolvePromise) {
return (providerOrCreds as any).resolvePromise();
}
return providerOrCreds;
const credentials = (providerOrCreds as any).resolvePromise ? await (providerOrCreds as any).resolvePromise() : providerOrCreds;

return { credentials, pluginName: source.name };
}
return undefined;
}
}

export interface PluginCredentials {
readonly credentials: AWS.Credentials;
readonly pluginName: string;
}
245 changes: 192 additions & 53 deletions packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as cxapi from '@aws-cdk/cx-api';
import * as AWS from 'aws-sdk';
import type { ConfigurationOptions } from 'aws-sdk/lib/config-base';
import * as fs from 'fs-extra';
import { debug } from '../../logging';
import { debug, warning } from '../../logging';
import { cached } from '../../util/functions';
import { CredentialPlugins } from '../aws-auth/credential-plugins';
import { Mode } from '../aws-auth/credentials';
Expand Down Expand Up @@ -79,12 +79,27 @@ const CACHED_ACCOUNT = Symbol('cached_account');
const CACHED_DEFAULT_CREDENTIALS = Symbol('cached_default_credentials');

/**
* Creates instances of the AWS SDK appropriate for a given account/region
* Creates instances of the AWS SDK appropriate for a given account/region.
*
* If an environment is given and the current credentials are NOT for the indicated
* account, will also search the set of credential plugin providers.
* Behavior is as follows:
*
* If no environment is given, the default credentials will always be used.
* - First, a set of "base" credentials are established
* - If a target environment is given and the default ("current") SDK credentials are for
* that account, return those; otherwise
* - If a target environment is given, scan all credential provider plugins
* for credentials, and return those if found; otherwise
* - Return default ("current") SDK credentials, noting that they might be wrong.
*
* - Second, a role may optionally need to be assumed. Use the base credentials
* established in the previous process to assume that role.
* - If assuming the role fails and the base credentials are for the correct
* account, return those. This is a fallback for people who are trying to interact
* with a Default Synthesized stack and already have right credentials setup.
*
* Typical cases we see in the wild:
* - Credential plugin setup that, although not recommended, works for them
* - Seeded terminal with `ReadOnly` credentials in order to do `cdk diff`--the `ReadOnly`
* role doesn't have `sts:AssumeRole` and will fail for no real good reason.
*/
export class SdkProvider {
/**
Expand Down Expand Up @@ -126,49 +141,53 @@ export class SdkProvider {
*
* The `environment` parameter is resolved first (see `resolveEnvironment()`).
*/
public async forEnvironment(environment: cxapi.Environment, mode: Mode): Promise<ISDK> {
public async forEnvironment(environment: cxapi.Environment, mode: Mode, options?: CredentialsOptions): Promise<ISDK> {
const env = await this.resolveEnvironment(environment);
const creds = await this.obtainCredentials(env.account, mode);
return new SDK(creds, env.region, this.sdkOptions);
}

/**
* Return an SDK which uses assumed role credentials
*
* The base credentials used to retrieve the assumed role credentials will be the
* same credentials returned by obtainCredentials if an environment and mode is passed,
* otherwise it will be the current credentials.
*
*/
public async withAssumedRole(roleArn: string, externalId: string | undefined, environment: cxapi.Environment | undefined, mode: Mode | undefined) {
debug(`Assuming role '${roleArn}'.`);
const baseCreds = await this.obtainBaseCredentials(env.account, mode);

let region: string;
let masterCredentials: AWS.Credentials;
// At this point, we need at least SOME credentials
if (baseCreds.source === 'none') { throw new Error(fmtObtainCredentialsError(env.account, baseCreds)); }

if (environment !== undefined && mode !== undefined) {
const env = await this.resolveEnvironment(environment);
masterCredentials = await this.obtainCredentials(env.account, mode);
region = env.region;
} else {
region = this.defaultRegion;
masterCredentials = await this.defaultCredentials();
// 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)); }
return new SDK(baseCreds.credentials, env.region, this.sdkOptions);
}

const creds = new AWS.ChainableTemporaryCredentials({
params: {
RoleArn: roleArn,
...externalId ? { ExternalId: externalId } : {},
RoleSessionName: `aws-cdk-${safeUsername()}`,
},
stsConfig: {
region,
...this.sdkOptions,
},
masterCredentials: masterCredentials,
});
// We will proceed to AssumeRole using whatever we've been given.
const sdk = await this.withAssumedRole(baseCreds, options.assumeRoleArn, options.assumeRoleExternalId, env.region);

// Exercise the AssumeRoleCredentialsProvider we've gotten at least once so
// we can determine whether the AssumeRole call succeeds or not.
try {
await sdk.forceCredentialRetrieval();
return sdk;
} catch (e) {
// AssumeRole failed. Proceed and warn *if and only if* the baseCredentials were already for the right account
// or returned from a plugin. This is to cover some current setups for people using plugins or preferring to
// 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);
warning(`${fmtObtainedCredentials(baseCreds)} could not be used to assume '${options.assumeRoleArn}', but are for the right account. Proceeding anyway.`);
return new SDK(baseCreds.credentials, env.region, this.sdkOptions);
}

return new SDK(creds, region, this.sdkOptions);
throw e;
}
}

/**
* Return the partition that base credentials are for
*
* Returns `undefined` if there are no base credentials.
*/
public async baseCredentialsPartition(environment: cxapi.Environment, mode: Mode): Promise<string | undefined> {
const env = await this.resolveEnvironment(environment);
const baseCreds = await this.obtainBaseCredentials(env.account, mode);
if (baseCreds.source === 'none') { return undefined; }
return (await new SDK(baseCreds.credentials, env.region, this.sdkOptions).currentAccount()).partition;
}

/**
Expand Down Expand Up @@ -230,30 +249,40 @@ export class SdkProvider {
/**
* Get credentials for the given account ID in the given mode
*
* Use the current credentials if the destination account matches the current credentials' account.
* Otherwise try all credential plugins.
* 1. Use the default credentials if the destination account matches the
* current credentials' account.
* 2. Otherwise try all credential plugins.
* 3. Fail if neither of these yield any credentials.
* 4. Return a failure if any of them returned credentials
*/
protected async obtainCredentials(accountId: string, mode: Mode): Promise<AWS.Credentials> {
private async obtainBaseCredentials(accountId: string, mode: Mode): Promise<ObtainBaseCredentialsResult> {
// First try 'current' credentials
const defaultAccountId = (await this.defaultAccount())?.accountId;
if (defaultAccountId === accountId) {
return this.defaultCredentials();
return { source: 'correctDefault', credentials: await this.defaultCredentials() };
}

// Then try the plugins
const pluginCreds = await this.plugins.fetchCredentialsFor(accountId, mode);
if (pluginCreds) {
return pluginCreds;
return { source: 'plugin', ...pluginCreds };
}

// No luck, format a useful error message
const error = [`Need to perform AWS calls for account ${accountId}`];
error.push(defaultAccountId ? `but the current credentials are for ${defaultAccountId}` : 'but no credentials have been configured');
if (this.plugins.availablePluginNames.length > 0) {
error.push(`and none of these plugins found any: ${this.plugins.availablePluginNames.join(', ')}`);
// Fall back to default credentials with a note that they're not the right ones yet
if (defaultAccountId !== undefined) {
return {
source: 'incorrectDefault',
accountId: defaultAccountId,
credentials: await this.defaultCredentials(),
unusedPlugins: this.plugins.availablePluginNames,
};
}

throw new Error(`${error.join(', ')}.`);
// Apparently we didn't find any at all
return {
source: 'none',
unusedPlugins: this.plugins.availablePluginNames,
};
}

/**
Expand All @@ -265,6 +294,40 @@ export class SdkProvider {
return this.defaultChain.resolvePromise();
});
}

/**
* Return an SDK which uses assumed role credentials
*
* The base credentials used to retrieve the assumed role credentials will be the
* same credentials returned by obtainCredentials if an environment and mode is passed,
* otherwise it will be the current credentials.
*/
private async withAssumedRole(
masterCredentials: Exclude<ObtainBaseCredentialsResult, { source: 'none' }>,
roleArn: string,
externalId: string | undefined,
region: string | undefined) {
debug(`Assuming role '${roleArn}'.`);

region = region ?? this.defaultRegion;

const creds = new AWS.ChainableTemporaryCredentials({
params: {
RoleArn: roleArn,
...externalId ? { ExternalId: externalId } : {},
RoleSessionName: `aws-cdk-${safeUsername()}`,
},
stsConfig: {
region,
...this.sdkOptions,
},
masterCredentials: masterCredentials.credentials,
});

return new SDK(creds, region, this.sdkOptions, {
assumeRoleCredentialsSourceDescription: fmtObtainedCredentials(masterCredentials),
});
}
}

/**
Expand Down Expand Up @@ -383,3 +446,79 @@ function readIfPossible(filename: string): string | undefined {
function safeUsername() {
return os.userInfo().username.replace(/[^\w+=,.@-]/g, '@');
}

/**
* Options for obtaining credentials for an environment
*/
export interface CredentialsOptions {
/**
* The ARN of the role that needs to be assumed, if any
*/
readonly assumeRoleArn?: string;

/**
* External ID required to assume the given role.
*/
readonly assumeRoleExternalId?: string;
}

/**
* Result of obtaining base credentials
*/
type ObtainBaseCredentialsResult =
{ source: 'correctDefault'; credentials: AWS.Credentials }
| { source: 'plugin'; pluginName: string, credentials: AWS.Credentials }
| { source: 'incorrectDefault'; credentials: AWS.Credentials; accountId: string; unusedPlugins: string[] }
| { source: 'none'; unusedPlugins: string[] };

/**
* Isolating the code that translates calculation errors into human error messages
*
* We cover the following cases:
*
* - No credentials are available at all
* - Default credentials are for the wrong account
*/
function fmtObtainCredentialsError(targetAccountId: string, obtainResult: ObtainBaseCredentialsResult & { source: 'none' | 'incorrectDefault' }): string {
const msg = [`Need to perform AWS calls for account ${targetAccountId}`];
switch (obtainResult.source) {
case 'incorrectDefault':
msg.push(`but the current credentials are for ${obtainResult.accountId}`);
break;
case 'none':
msg.push('but no credentials have been configured');
}
if (obtainResult.unusedPlugins.length > 0) {
msg.push(`and none of these plugins found any: ${obtainResult.unusedPlugins.join(', ')}`);
}
return msg.join(', ');
}

/**
* Format a message indicating where we got base credentials for the assume role
*
* We cover the following cases:
*
* - Default credentials for the right account
* - Default credentials for the wrong account
* - Credentials returned from a plugin
*/
function fmtObtainedCredentials(
obtainResult: Exclude<ObtainBaseCredentialsResult, { source: 'none' }>): string {
switch (obtainResult.source) {
case 'correctDefault':
return 'current credentials';
case 'plugin':
return `credentials returned by plugin '${obtainResult.pluginName}'`;
case 'incorrectDefault':
const msg = [];
msg.push(`current credentials (which are for account ${obtainResult.accountId}`);

if (obtainResult.unusedPlugins.length > 0) {
msg.push(`, and none of the following plugins provided credentials: ${obtainResult.unusedPlugins.join(', ')}`);
}
msg.push(')');

return msg.join('');
}
}
Loading

0 comments on commit f659696

Please sign in to comment.