Skip to content

Commit

Permalink
fix(cli): can not assume role from 2-level SSO (#23702)
Browse files Browse the repository at this point in the history
The CLI used to support this:

```
[profile sso]
...

[profile one]
...
source_profile = sso
```

But not this:

```
[profile sso]
...

[profile one]
...
source_profile = sso

[profile two]
...
source_profile = one
```

The reason was that:

* We have to do an explicit detection of SSO source profiles in our `PatchedSharedIniFileCredentials` because the upstream `SharedIniFileCredentials` has no SSO support at all; and
* When we recursed we would recurse using the `SharedIniFileCredentials` class.

In combination, this means that we could only recurse **one level**, because in `SharedIniFileCredentials` we wouldn't support SSO profiles at all.

Fix this by recursing using `PatchedSharedIniFileCredentials`, so that we can support SSO source profiles an arbitrary amount of nesting levels deep.

While investigating this, also fixed the following issues:

- SSO profiles would be detected using the incorrect key: `sso_start_url` can be specified either on the profile section, or a new `[sso-session]` section. `sso_account_id` however always must be on the profile section, so check on that.
- Dropped support for reading the STS AssumeRole `region` from the `[default]` section. After investigating by both the JS SDK team and myself, noticed that the AWS CLI does **not** support reading the region from there. While we are both in agreement this is a bug, all customers expect the CDK CLI to behave exactly the same as the AWS CLI, so we have to keep bug-for-bug compatibility.
- Drop the `credentials/config` file loading patch. The upstream SDK has fixed this behavior in the mean time, so we can rely on the passed-in profile data again.

Fixes #23520.

----

*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 Jan 17, 2023
1 parent 1fefc88 commit c3a345b
Showing 1 changed file with 42 additions and 67 deletions.
109 changes: 42 additions & 67 deletions packages/aws-cdk/lib/api/aws-auth/aws-sdk-inifile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,13 @@ import * as AWS from 'aws-sdk';
* 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).
* 1. The upstream aws-sdk does not support the 'credential_source' option. Meaning credentials
* for assume-role cannot be fetched using EC2/ESC metadata.
*
* 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.
*
* 3. The 'credential_source' option is not supported. Meaning credentials
* for assume-role cannot be fetched using EC2/ESC metadata.
*
* See https://github.com/aws/aws-sdk-js/issues/3418 for all the gory details.
* See https://github.com/aws/aws-sdk-js/issues/1916 for some more glory details.
* 2. The upstream aws-sdk does not support SSO profiles as the source of RoleProfiles,
* because it will always use the `SharedIniFileCredentials` provider to load
* source credentials, but in order to support SSO profiles you must use a
* separate class (`SsoCredentials).
*/
export class PatchedSharedIniFileCredentials extends AWS.SharedIniFileCredentials {
declare private profile: string;
Expand Down Expand Up @@ -60,21 +49,29 @@ export class PatchedSharedIniFileCredentials extends AWS.SharedIniFileCredential
var sourceProfile = roleProfile.source_profile;
var credentialSource = roleProfile.credential_source;

const credentialError = (AWS as any).util.error(
new Error(`When using 'role_arn' in profile ('${this.profile}'), you must also configure exactly one of 'source_profile' or 'credential_source'`),
{ code: 'SharedIniFileCredentialsProviderFailure' },
);

if (sourceProfile && credentialSource) {
throw credentialError;
}

if (!sourceProfile && !credentialSource) {
throw credentialError;
if (!!sourceProfile === !!credentialSource) {
throw (AWS as any).util.error(
new Error(`When using 'role_arn' in profile ('${this.profile}'), you must also configure exactly one of 'source_profile' or 'credential_source'`),
{ code: 'SharedIniFileCredentialsProviderFailure' },
);
}

const profiles = loadProfilesProper(this.filename);
const region = profiles[this.profile]?.region ?? profiles.default?.region ?? 'us-east-1';
// Confirmed this against AWS CLI behavior -- the region must be in the assumED profile,
// otherwise `us-east-1`. From the upstream comment in `aws-sdk-js`:
// -------- comment from aws-sdk-js -------------------
// Experimentation shows that the AWS CLI (tested at version 1.18.136)
// ignores the following potential sources of a region for the purposes of
// this AssumeRole call:
//
// - The [default] profile
// - The AWS_REGION environment variable
//
// Ignoring the [default] profile for the purposes of AssumeRole is arguably
// a bug in the CLI since it does use the [default] region for service
// calls... but right now we're matching behavior of the other tool.
// -------------------------------------------------

const region = roleProfile?.region ?? 'us-east-1';

const stsCreds = sourceProfile ? this.sourceProfileCredentials(sourceProfile, creds) : this.credentialSourceCredentials(credentialSource);

Expand Down Expand Up @@ -121,7 +118,6 @@ export class PatchedSharedIniFileCredentials extends AWS.SharedIniFileCredential
}

private sourceProfileCredentials(sourceProfile: string, profiles: Record<string, Record<string, string>>) {

var sourceProfileExistanceTest = profiles[sourceProfile];

if (typeof sourceProfileExistanceTest !== 'object') {
Expand All @@ -132,11 +128,23 @@ export class PatchedSharedIniFileCredentials extends AWS.SharedIniFileCredential
);
}

if (sourceProfileExistanceTest.sso_start_url) {
// We need to do a manual check here if the source profile (providing the
// credentials for the AssumeRole) is an SSO profile. That's because
// `SharedIniFileCredentials` itself doesn't support providing credentials from
// arbitrary profiles, only for StaticCredentials and AssumeRole type
// profiles; if it's an SSO profile you need to instantiate a special
// Credential Provider for that.
//
// ---
//
// An SSO profile can be configured in 2 ways (put all the info in the profile
// section, or put half of it in an `[sso-session]` block), but in both cases
// the primary profile block must have the `sso_account_id` key
if (sourceProfileExistanceTest.sso_account_id) {
return new AWS.SsoCredentials({ profile: sourceProfile });
}

return new AWS.SharedIniFileCredentials(
return new PatchedSharedIniFileCredentials(
(AWS as any).util.merge(this.options || {}, {
profile: sourceProfile,
preferStaticCredentials: true,
Expand All @@ -148,7 +156,6 @@ export class PatchedSharedIniFileCredentials extends AWS.SharedIniFileCredential
// the aws-sdk for js does not support 'credential_source' (https://github.com/aws/aws-sdk-js/issues/1916)
// so unfortunately we need to implement this ourselves.
private credentialSourceCredentials(sourceCredential: string) {

// see https://docs.aws.amazon.com/credref/latest/refdocs/setting-global-credential_source.html
switch (sourceCredential) {
case 'Environment': {
Expand All @@ -166,36 +173,4 @@ export class PatchedSharedIniFileCredentials extends AWS.SharedIniFileCredential
}

}
}

/**
* 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;
}
}

0 comments on commit c3a345b

Please sign in to comment.