Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): profile AssumeRole credentials don't work via proxy #7292

Merged
merged 3 commits into from
Apr 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ export class AwsCliCompatible {
* 3. Respects $AWS_SHARED_CREDENTIALS_FILE.
* 4. Respects $AWS_DEFAULT_PROFILE in addition to $AWS_PROFILE.
*/
public static async credentialChain(profile: string | undefined, ec2creds: boolean | undefined, containerCreds: boolean | undefined) {
public static async credentialChain(
profile: string | undefined,
ec2creds: boolean | undefined,
containerCreds: boolean | undefined,
httpOptions: AWS.HTTPOptions | undefined) {
await forceSdkToReadConfigIfPresent();

profile = profile || process.env.AWS_PROFILE || process.env.AWS_DEFAULT_PROFILE || 'default';
Expand All @@ -41,11 +45,11 @@ export class AwsCliCompatible {
];

if (await fs.pathExists(credentialsFileName())) {
sources.push(() => new AWS.SharedIniFileCredentials({ profile, filename: credentialsFileName() }));
sources.push(() => new AWS.SharedIniFileCredentials({ profile, filename: credentialsFileName(), httpOptions }));
}

if (await fs.pathExists(configFileName())) {
sources.push(() => new AWS.SharedIniFileCredentials({ profile, filename: credentialsFileName() }));
sources.push(() => new AWS.SharedIniFileCredentials({ profile, filename: credentialsFileName(), httpOptions }));
}

if (containerCreds ?? hasEcsCredentials()) {
Expand Down
23 changes: 13 additions & 10 deletions packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,23 +88,23 @@ export class SdkProvider {
* class `AwsCliCompatible` for the details.
*/
public static async withAwsCliCompatibleDefaults(options: SdkProviderOptions = {}) {
const chain = await AwsCliCompatible.credentialChain(options.profile, options.ec2creds, options.containerCreds);
const sdkOptions = parseHttpOptions(options.httpOptions ?? {});

const chain = await AwsCliCompatible.credentialChain(options.profile, options.ec2creds, options.containerCreds, sdkOptions.httpOptions);
const region = await AwsCliCompatible.region(options.profile);

return new SdkProvider(chain, region, options.httpOptions);
return new SdkProvider(chain, region, sdkOptions);
}

private readonly plugins = new CredentialPlugins();
private readonly httpOptions: ConfigurationOptions;

public constructor(
private readonly defaultChain: AWS.CredentialProviderChain,
/**
* Default region
*/
public readonly defaultRegion: string,
httpOptions: SdkHttpOptions = {}) {
this.httpOptions = defaultHttpOptions(httpOptions);
private readonly sdkOptions: ConfigurationOptions = {}) {
}

/**
Expand All @@ -116,7 +116,7 @@ export class SdkProvider {
public async forEnvironment(accountId: string | undefined, region: string | undefined, mode: Mode): Promise<ISDK> {
const env = await this.resolveEnvironment(accountId, region);
const creds = await this.obtainCredentials(env.account, mode);
return new SDK(creds, env.region, this.httpOptions);
return new SDK(creds, env.region, this.sdkOptions);
}

/**
Expand All @@ -139,12 +139,12 @@ export class SdkProvider {
},
stsConfig: {
region,
...this.httpOptions,
...this.sdkOptions,
},
masterCredentials: await this.defaultCredentials(),
});

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

/**
Expand Down Expand Up @@ -199,7 +199,7 @@ export class SdkProvider {
throw new Error('Unable to resolve AWS credentials (setup with "aws configure")');
}

return new SDK(creds, this.defaultRegion, this.httpOptions).currentAccount();
return new SDK(creds, this.defaultRegion, this.sdkOptions).currentAccount();
} catch (e) {
debug('Unable to determine the default AWS account:', e);
return undefined;
Expand Down Expand Up @@ -269,8 +269,11 @@ export interface Account {
* Get HTTP options for the SDK
*
* Read from user input or environment variables.
*
* Returns a complete `ConfigurationOptions` object because that's where
* `customUserAgent` lives, but `httpOptions` is the most important attribute.
*/
function defaultHttpOptions(options: SdkHttpOptions) {
function parseHttpOptions(options: SdkHttpOptions) {
const config: ConfigurationOptions = {};
config.httpOptions = {};

Expand Down
48 changes: 48 additions & 0 deletions packages/aws-cdk/test/api/sdk-provider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ beforeEach(() => {
[foo]
aws_access_key_id=${uid}fooccess
aws_secret_access_key=secret

[assumer]
aws_access_key_id=${uid}assumer
aws_secret_access_key=secret
`),
'/home/me/.bxt/config': dedent(`
[default]
Expand All @@ -46,6 +50,13 @@ beforeEach(() => {
aws_access_key_id=${uid}booccess
aws_secret_access_key=boocret
# No region here

[profile assumable]
role_arn=arn:aws:iam::12356789012:role/Assumable
source_profile=assumer

[profile assumer]
region=us-east-2
`),
});

Expand Down Expand Up @@ -138,6 +149,43 @@ describe('CLI compatible credentials loading', () => {

await expect(provider.forEnvironment(`${uid}some_account_#`, 'def', Mode.ForReading)).rejects.toThrow('Need to perform AWS calls');
});

test('even when using a profile to assume another profile, STS calls goes through the proxy', async () => {
// Messy mocking
let called = false;
jest.mock('proxy-agent', () => {
// eslint-disable-next-line @typescript-eslint/no-require-imports
class FakeAgent extends require('https').Agent {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this not complain about using require?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course it does :)

public addRequest(_: any, __: any) {
// FIXME: this error takes 6 seconds to be completely handled. It
// might be retries in the SDK somewhere, or something about the Node
// event loop. I've spent an hour trying to figure it out and I can't,
// and I gave up. We'll just have to live with this until someone gets
// inspired.
const error = new Error('ABORTED BY TEST');
(error as any).code = 'RequestAbortedError';
(error as any).retryable = false;
called = true;
throw error;
}
}
return FakeAgent;
});

// WHEN
const provider = await SdkProvider.withAwsCliCompatibleDefaults({ ...defaultCredOptions,
ec2creds: false,
profile: 'assumable',
httpOptions: {
proxyAddress: 'http://DOESNTMATTER/',
}
});

await provider.defaultAccount();

// THEN -- the fake proxy agent got called, we don't care about the result
expect(called).toEqual(true);
});
});

describe('Plugins', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk/test/util/mock-sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export class MockSdkProvider extends SdkProvider {
private readonly sdk: ISDK;

constructor() {
super(new AWS.CredentialProviderChain([]), 'bermuda-triangle-1337', { userAgent: 'aws-cdk/jest' });
super(new AWS.CredentialProviderChain([]), 'bermuda-triangle-1337', { customUserAgent: 'aws-cdk/jest' });

// SDK contains a real SDK, since some test use 'AWS-mock' to replace the underlying
// AWS calls which a real SDK would do, and some tests use the 'stub' functionality below.
Expand Down