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): CLI hangs for 10 minutes on expired credentials #21052

Merged
merged 7 commits into from
Aug 19, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 1 addition & 2 deletions packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,5 +344,4 @@ async function tokenCodeFn(serialArn: string, cb: (err?: Error, token?: string)
debug('Failed to get MFA token', err);
cb(err);
}
}

}
15 changes: 15 additions & 0 deletions packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,10 @@ export class SdkProvider {
await sdk.forceCredentialRetrieval();
return { sdk, didAssumeRole: true };
} catch (e) {
if (isUnrecoverableAwsError(e)) {
throw 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,
Expand Down Expand Up @@ -270,6 +274,10 @@ export class SdkProvider {

return await new SDK(creds, this.defaultRegion, this.sdkOptions).currentAccount();
} catch (e) {
if (isUnrecoverableAwsError(e)) {
throw e;
}

debug('Unable to determine the default AWS account:', e);
return undefined;
}
Expand Down Expand Up @@ -541,3 +549,10 @@ function fmtObtainedCredentials(
return msg.join('');
}
}

/**
* Return whether an error should not be recovered from
*/
export function isUnrecoverableAwsError(e: Error) {
return (e as any).code === 'ExpiredToken';
}
5 changes: 4 additions & 1 deletion packages/aws-cdk/lib/api/aws-auth/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { ConfigurationOptions } from 'aws-sdk/lib/config-base';
import { debug, trace } from './_env';
import { AccountAccessKeyCache } from './account-cache';
import { cached } from './cached';
import { Account } from './sdk-provider';
import { Account, isUnrecoverableAwsError } from './sdk-provider';

// We need to map regions to domain suffixes, and the SDK already has a function to do this.
// It's not part of the public API, but it's also unlikely to go away.
Expand Down Expand Up @@ -234,6 +234,9 @@ export class SDK implements ISDK {
try {
await this._credentials.getPromise();
} catch (e) {
if (isUnrecoverableAwsError(e)) {
throw e;
}
debug(`Assuming role failed: ${e.message}`);
throw new Error([
'Could not assume role in target account',
Expand Down
9 changes: 8 additions & 1 deletion packages/aws-cdk/test/api/fake-sts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export class FakeSts {
_attributes: { xmlns: 'https://sts.amazonaws.com/doc/2011-06-15/' },
Error: {
Type: 'Sender',
Code: 'Error',
Code: e.code ?? 'Error',
Message: e.message,
},
RequestId: '1',
Expand Down Expand Up @@ -159,6 +159,13 @@ export class FakeSts {
private handleAssumeRole(mockRequest: MockRequest): Record<string, any> {
const identity = this.identity(mockRequest);

const failureRequested = mockRequest.parsedBody.RoleArn.match(/<FAIL:([^>]+)>/);
if (failureRequested) {
const err = new Error(`STS failing by user request: ${failureRequested[1]}`);
(err as any).code = failureRequested[1];
throw err;
}

this.assumedRoles.push({
roleArn: mockRequest.parsedBody.RoleArn,
roleSessionName: mockRequest.parsedBody.RoleSessionName,
Expand Down
16 changes: 16 additions & 0 deletions packages/aws-cdk/test/api/sdk-provider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,22 @@ describe('with intercepted network calls', () => {
// THEN
expect((await sdk.currentAccount()).accountId).toEqual(uniq('88888'));
});

test('if AssumeRole fails because of ExpiredToken, then fail completely', async () => {
// GIVEN
prepareCreds({
fakeSts,
config: {
default: { aws_access_key_id: 'foo', $account: '88888' },
},
});
const provider = await providerFromProfile(undefined);

// WHEN - assumeRole fails with a specific error
await expect(async () => {
await provider.forEnvironment(env(uniq('88888')), Mode.ForReading, { assumeRoleArn: '<FAIL:ExpiredToken>' });
}).rejects.toThrow(/ExpiredToken/);
});
});

describe('Plugins', () => {
Expand Down