Skip to content

Commit

Permalink
fix(cli): AssumeRole profiles require a [default] profile (#10032)
Browse files Browse the repository at this point in the history
This works around a bug in the AWS SDK for JS that only surfaced when
we switched to `AWS_STS_REGIONAL_ENDPOINTS=regional`, requiring a
`[default]` profile with a region for all users.

The bug was that the INI-file AssumeRole provider would ignore the
region in the profile, and always fall back to the region in:

* The profile specified using `$AWS_PROFILE` (which we don't use).
* Otherwise the region in the `[default]` profile (which a user
  may or may not have).

Traditionally it didn't really matter whether the STS client got a
region or not because it would always connect to `us-east-1` no matter
what, but when we switched to `AWS_STS_REGIONAL_ENDPOINTS=regional`, it
became illegal to not have a region.

Fix the upstream bug by basically replicating the important parts of
`SharedIniFileCredentials` of the AWS SDK in our codebase and patching
the bug.

Reported upstreeam as aws/aws-sdk-js#3418

Fixes #9937


----

*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 committed Aug 28, 2020
1 parent 9098e29 commit 95c0332
Show file tree
Hide file tree
Showing 3 changed files with 380 additions and 163 deletions.
161 changes: 161 additions & 0 deletions packages/aws-cdk/lib/api/aws-auth/aws-sdk-inifile.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
import * as AWS from 'aws-sdk';


/**
* Hack-fix
*
* There are a number of issues in the upstream version of SharedIniFileCredentials
* that need fixing:
*
* 1. The upstream aws-sdk contains an incorrect instantiation of an `AWS.STS`
* client, which *should* have taken the region from the requested profile
* but doesn't. It will use the region from the default profile, which
* may not exist, defaulting to `us-east-1` (since we switched to
* AWS_STS_REGIONAL_ENDPOINTS=regional, that default is not even allowed anymore
* and the absence of a default region will lead to an error).
*
* 2. The simple fix is to get the region from the `config` file. profiles
* are made up of a combination of `credentials` and `config`, and the region is
* generally in `config` with the rest in `credentials`. However, a bug in
* `getProfilesFromSharedConfig` overwrites ALL `config` data with `credentials`
* data, so we also need to do extra work to fish the `region` out of the config.
*
* See https://github.com/aws/aws-sdk-js/issues/3418 for all the gory details.
*/
export class PatchedSharedIniFileCredentials extends AWS.SharedIniFileCredentials {
declare private profile: string;
declare private filename: string;
declare private disableAssumeRole: boolean;
declare private options: Record<string, string>;
declare private roleArn: string;
declare private httpOptions?: AWS.HTTPOptions;
declare private tokenCodeFn?: (mfaSerial: string, callback: (err?: Error, token?: string) => void) => void;

public loadRoleProfile(
creds: Record<string, Record<string, string>>,
roleProfile: Record<string, string>,
callback: (err?: Error, data?: any) => void) {

// Need to duplicate the whole implementation here -- the function is long and has been written in
// such a way that there are no small monkey patches possible.

if (this.disableAssumeRole) {
throw (AWS as any).util.error(
new Error('Role assumption profiles are disabled. ' +
'Failed to load profile ' + this.profile +
' from ' + creds.filename),
{ code: 'SharedIniFileCredentialsProviderFailure' },
);
}

var self = this;
var roleArn = roleProfile.role_arn;
var roleSessionName = roleProfile.role_session_name;
var externalId = roleProfile.external_id;
var mfaSerial = roleProfile.mfa_serial;
var sourceProfileName = roleProfile.source_profile;

if (!sourceProfileName) {
throw (AWS as any).util.error(
new Error('source_profile is not set using profile ' + this.profile),
{ code: 'SharedIniFileCredentialsProviderFailure' },
);
}

var sourceProfileExistanceTest = creds[sourceProfileName];

if (typeof sourceProfileExistanceTest !== 'object') {
throw (AWS as any).util.error(
new Error('source_profile ' + sourceProfileName + ' using profile '
+ this.profile + ' does not exist'),
{ code: 'SharedIniFileCredentialsProviderFailure' },
);
}

var sourceCredentials = new AWS.SharedIniFileCredentials(
(AWS as any).util.merge(this.options || {}, {
profile: sourceProfileName,
preferStaticCredentials: true,
}),
);

// --------- THIS IS NEW ----------------------
const profiles = loadProfilesProper(this.filename);
const region = profiles[this.profile]?.region ?? profiles.default?.region ?? 'us-east-1';
// --------- /THIS IS NEW ----------------------

this.roleArn = roleArn;
var sts = new AWS.STS({
credentials: sourceCredentials,
region,
httpOptions: this.httpOptions,
});

var roleParams: AWS.STS.AssumeRoleRequest = {
RoleArn: roleArn,
RoleSessionName: roleSessionName || 'aws-sdk-js-' + Date.now(),
};

if (externalId) {
roleParams.ExternalId = externalId;
}

if (mfaSerial && self.tokenCodeFn) {
roleParams.SerialNumber = mfaSerial;
self.tokenCodeFn(mfaSerial, function(err, token) {
if (err) {
var message;
if (err instanceof Error) {
message = err.message;
} else {
message = err;
}
callback(
(AWS as any).util.error(
new Error('Error fetching MFA token: ' + message),
{ code: 'SharedIniFileCredentialsProviderFailure' },
));
return;
}

roleParams.TokenCode = token;
sts.assumeRole(roleParams, callback);
});
return;
}
sts.assumeRole(roleParams, callback);

}
}

/**
* A function to load profiles from disk that MERGES credentials and config instead of overwriting
*
* @see https://github.com/aws/aws-sdk-js/blob/5ae5a7d7d24d1000dbc089cc15f8ed2c7b06c542/lib/util.js#L956
*/
function loadProfilesProper(filename: string) {
const util = (AWS as any).util; // Does exists even though there aren't any typings for it
const iniLoader = util.iniLoader;
const profiles: Record<string, Record<string, string>> = {};
let profilesFromConfig: Record<string, Record<string, string>> = {};
if (process.env[util.configOptInEnv]) {
profilesFromConfig = iniLoader.loadFrom({
isConfig: true,
filename: process.env[util.sharedConfigFileEnv],
});
}
var profilesFromCreds: Record<string, Record<string, string>> = iniLoader.loadFrom({
filename: filename ||
(process.env[util.configOptInEnv] && process.env[util.sharedCredentialsFileEnv]),
});
for (const [name, profile] of Object.entries(profilesFromConfig)) {
profiles[name] = profile;
}
for (const [name, profile] of Object.entries(profilesFromCreds)) {
profiles[name] = {
...profiles[name],
...profile,
};
}
return profiles;
}
4 changes: 3 additions & 1 deletion packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as AWS from 'aws-sdk';
import * as fs from 'fs-extra';
import * as promptly from 'promptly';
import { debug } from '../../logging';
import { PatchedSharedIniFileCredentials } from './aws-sdk-inifile';
import { SharedIniFile } from './sdk_ini_file';

/**
Expand Down Expand Up @@ -44,7 +45,7 @@ export class AwsCliCompatible {
// Force reading the `config` file if it exists by setting the appropriate
// environment variable.
await forceSdkToReadConfigIfPresent();
sources.push(() => new AWS.SharedIniFileCredentials({
sources.push(() => new PatchedSharedIniFileCredentials({
profile,
filename: credentialsFileName(),
httpOptions: options.httpOptions,
Expand Down Expand Up @@ -310,3 +311,4 @@ async function tokenCodeFn(serialArn: string, cb: (err?: Error, token?: string)
cb(err);
}
}

0 comments on commit 95c0332

Please sign in to comment.