Skip to content

Commit

Permalink
fix(cli): short-lived credentials are not refreshed (#32354)
Browse files Browse the repository at this point in the history
The CLI immediately invokes credential *providers* to produce
*credentials*, and passes the credentials around instead of the
providers. This means that if short-lived credentials expire (like
session credentials from roles), there is no way to refresh them.

CLI calls will start to fail if that happens.

To fix this, instead of resolving providers to credentials, pass
providers around instead.

Implications for auth plugins
-------------

This widens the plugin protocol: the new plugin protocol *forced* a
translation to V3 credentials, and had no way to return V3 providers.
While it is now possible to return V3 Credential Providers from the
plugin protocol, plugin writers cannot easily take advantage of that
protocol because there have been ~8 CLI releases that only support V3
credentials and will fail at runtime of V3 providers are returned.

To support this, pass a new options argument into `getProvider()`: this
will indicate whether V3 Providers are supported or not. Plugins can
return a provider if the CLI indicates that it supports V3 providers,
and avoid doing that if the CLI indicates it won't. That way, plugins
can be rewritten to take advantage of returning V3 providers without
crashing on CLI versions `2.167.0..(this releases)`.

This also affects #32111 in which the plugin contract is being moved.

Closes #32287.

----

*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 Dec 4, 2024
1 parent bf77e51 commit 058a0bf
Show file tree
Hide file tree
Showing 19 changed files with 462 additions and 109 deletions.
9 changes: 7 additions & 2 deletions packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { loadSharedConfigFiles } from '@smithy/shared-ini-file-loader';
import { AwsCredentialIdentityProvider, Logger } from '@smithy/types';
import * as promptly from 'promptly';
import { ProxyAgent } from 'proxy-agent';
import { makeCachingProvider } from './provider-caching';
import type { SdkHttpOptions } from './sdk-provider';
import { readIfPossible } from './util';
import { debug } from '../../logging';
Expand All @@ -23,6 +24,8 @@ const DEFAULT_TIMEOUT = 300000;
export class AwsCliCompatible {
/**
* Build an AWS CLI-compatible credential chain provider
*
* The credential chain returned by this function is always caching.
*/
public static async credentialChainBuilder(
options: CredentialChainOptions = {},
Expand All @@ -43,13 +46,13 @@ export class AwsCliCompatible {
* environment credentials still take precedence over AWS_PROFILE
*/
if (options.profile) {
return fromIni({
return makeCachingProvider(fromIni({
profile: options.profile,
ignoreCache: true,
mfaCodeProvider: tokenCodeFn,
clientConfig,
logger: options.logger,
});
}));
}

const envProfile = process.env.AWS_PROFILE || process.env.AWS_DEFAULT_PROFILE;
Expand All @@ -74,6 +77,8 @@ export class AwsCliCompatible {
* fromContainerMetadata()
* fromTokenFile()
* fromInstanceMetadata()
*
* The NodeProviderChain is already cached.
*/
const nodeProviderChain = fromNodeProviderChain({
profile: envProfile,
Expand Down
10 changes: 10 additions & 0 deletions packages/aws-cdk/lib/api/aws-auth/cached.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,13 @@ export function cached<A extends object, B>(obj: A, sym: symbol, fn: () => B): B
}
return (obj as any)[sym];
}

/**
* Like 'cached', but async
*/
export async function cachedAsync<A extends object, B>(obj: A, sym: symbol, fn: () => Promise<B>): Promise<B> {
if (!(sym in obj)) {
(obj as any)[sym] = await fn();
}
return (obj as any)[sym];
}
135 changes: 111 additions & 24 deletions packages/aws-cdk/lib/api/aws-auth/credential-plugins.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import type { AwsCredentialIdentity } from '@smithy/types';
import { inspect } from 'util';
import type { AwsCredentialIdentity, AwsCredentialIdentityProvider } from '@smithy/types';
import { debug, warning } from '../../logging';
import { CredentialProviderSource, Mode, PluginHost } from '../plugin';
import { CredentialProviderSource, PluginProviderResult, Mode, PluginHost, SDKv2CompatibleCredentials, SDKv3CompatibleCredentialProvider, SDKv3CompatibleCredentials } from '../plugin';
import { credentialsAboutToExpire, makeCachingProvider } from './provider-caching';

/**
* Cache for credential providers.
Expand All @@ -15,9 +17,14 @@ import { CredentialProviderSource, Mode, PluginHost } from '../plugin';
* for the given account.
*/
export class CredentialPlugins {
private readonly cache: { [key: string]: PluginCredentials | undefined } = {};
private readonly cache: { [key: string]: PluginCredentialsFetchResult | undefined } = {};
private readonly host: PluginHost;

public async fetchCredentialsFor(awsAccountId: string, mode: Mode): Promise<PluginCredentials | undefined> {
constructor(host?: PluginHost) {
this.host = host ?? PluginHost.instance;
}

public async fetchCredentialsFor(awsAccountId: string, mode: Mode): Promise<PluginCredentialsFetchResult | undefined> {
const key = `${awsAccountId}-${mode}`;
if (!(key in this.cache)) {
this.cache[key] = await this.lookupCredentials(awsAccountId, mode);
Expand All @@ -26,13 +33,13 @@ export class CredentialPlugins {
}

public get availablePluginNames(): string[] {
return PluginHost.instance.credentialProviderSources.map((s) => s.name);
return this.host.credentialProviderSources.map((s) => s.name);
}

private async lookupCredentials(awsAccountId: string, mode: Mode): Promise<PluginCredentials | undefined> {
private async lookupCredentials(awsAccountId: string, mode: Mode): Promise<PluginCredentialsFetchResult | undefined> {
const triedSources: CredentialProviderSource[] = [];
// Otherwise, inspect the various credential sources we have
for (const source of PluginHost.instance.credentialProviderSources) {
for (const source of this.host.credentialProviderSources) {
let available: boolean;
try {
available = await source.isAvailable();
Expand All @@ -59,28 +66,108 @@ export class CredentialPlugins {
continue;
}
debug(`Using ${source.name} credentials for account ${awsAccountId}`);
const providerOrCreds = await source.getProvider(awsAccountId, mode);

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

// Another layer of backwards compatibility: in SDK v2, the credentials object
// is both a container and a provider. So we need to force the refresh using getPromise.
// In SDK v3, these two responsibilities are separate, and the getPromise doesn't exist.
if ((credentials as any).getPromise) {
await (credentials as any).getPromise();
}

return { credentials, pluginName: source.name };
return {
credentials: await v3ProviderFromPlugin(() => source.getProvider(awsAccountId, mode, {
supportsV3Providers: true,
})),
pluginName: source.name,
};
}
return undefined;
}
}

export interface PluginCredentials {
readonly credentials: AwsCredentialIdentity;
/**
* Result from trying to fetch credentials from the Plugin host
*/
export interface PluginCredentialsFetchResult {
/**
* SDK-v3 compatible credential provider
*/
readonly credentials: AwsCredentialIdentityProvider;

/**
* Name of plugin that successfully provided credentials
*/
readonly pluginName: string;
}

/**
* Take a function that calls the plugin, and turn it into an SDKv3-compatible credential provider.
*
* What we will do is the following:
*
* - Query the plugin and see what kind of result it gives us.
* - If the result is self-refreshing or doesn't need refreshing, we turn it into an SDKv3 provider
* and return it directly.
* * If the underlying return value is a provider, we will make it a caching provider
* (because we can't know if it will cache by itself or not).
* * If the underlying return value is a static credential, caching isn't relevant.
* * If the underlying return value is V2 credentials, those have caching built-in.
* - If the result is a static credential that expires, we will wrap it in an SDKv3 provider
* that will query the plugin again when the credential expires.
*/
async function v3ProviderFromPlugin(producer: () => Promise<PluginProviderResult>): Promise<AwsCredentialIdentityProvider> {
const initial = await producer();

if (isV3Provider(initial)) {
// Already a provider, make caching
return makeCachingProvider(initial);
} else if (isV3Credentials(initial) && initial.expiration === undefined) {
// Static credentials that don't need refreshing nor caching
return () => Promise.resolve(initial);
} else if (isV3Credentials(initial) && initial.expiration !== undefined) {
// Static credentials that do need refreshing and caching
return refreshFromPluginProvider(initial, producer);
} else if (isV2Credentials(initial)) {
// V2 credentials that refresh and cache themselves
return v3ProviderFromV2Credentials(initial);
} else {
throw new Error(`Plugin returned a value that doesn't resemble AWS credentials: ${inspect(initial)}`);
}
}

/**
* Converts a V2 credential into a V3-compatible provider
*/
function v3ProviderFromV2Credentials(x: SDKv2CompatibleCredentials): AwsCredentialIdentityProvider {
return async () => {
// Get will fetch or refresh as necessary
await x.getPromise();

return {
accessKeyId: x.accessKeyId,
secretAccessKey: x.secretAccessKey,
sessionToken: x.sessionToken,
expiration: x.expireTime,
};
};
}

function refreshFromPluginProvider(current: AwsCredentialIdentity, producer: () => Promise<PluginProviderResult>): AwsCredentialIdentityProvider {
return async () => {
// eslint-disable-next-line no-console
console.error(current, Date.now());
if (credentialsAboutToExpire(current)) {
const newCreds = await producer();
if (!isV3Credentials(newCreds)) {
throw new Error(`Plugin initially returned static V3 credentials but now returned something else: ${inspect(newCreds)}`);
}
current = newCreds;
}
return current;
};
}

function isV3Provider(x: PluginProviderResult): x is SDKv3CompatibleCredentialProvider {
return typeof x === 'function';
}

function isV2Credentials(x: PluginProviderResult): x is SDKv2CompatibleCredentials {
return !!(x && typeof x === 'object' && x.accessKeyId && (x as SDKv2CompatibleCredentials).getPromise);
}

function isV3Credentials(x: PluginProviderResult): x is SDKv3CompatibleCredentials {
return !!(x && typeof x === 'object' && x.accessKeyId && !isV2Credentials(x));
}
24 changes: 24 additions & 0 deletions packages/aws-cdk/lib/api/aws-auth/provider-caching.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { memoize } from '@smithy/property-provider';
import { AwsCredentialIdentity, AwsCredentialIdentityProvider } from '@smithy/types';

/**
* Wrap a credential provider in a cache
*
* Some credential providers in the SDKv3 are cached (the default Node
* chain, specifically) but most others are not.
*
* Since we want to avoid duplicate calls to `AssumeRole`, or duplicate
* MFA prompts or what have you, we are going to liberally wrap providers
* in caches which will return the cached value until it expires.
*/
export function makeCachingProvider(provider: AwsCredentialIdentityProvider): AwsCredentialIdentityProvider {
return memoize(
provider,
credentialsAboutToExpire,
(token) => token.expiration !== undefined);
}

export function credentialsAboutToExpire(token: AwsCredentialIdentity) {
const expiryMarginSecs = 5;
return token.expiration !== undefined && token.expiration.getTime() - Date.now() < expiryMarginSecs * 1000;
}
40 changes: 14 additions & 26 deletions packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@ import { Environment, EnvironmentUtils, UNKNOWN_ACCOUNT, UNKNOWN_REGION } from '
import { AssumeRoleCommandInput } from '@aws-sdk/client-sts';
import { fromTemporaryCredentials } from '@aws-sdk/credential-providers';
import type { NodeHttpHandlerOptions } from '@smithy/node-http-handler';
import { AwsCredentialIdentity, AwsCredentialIdentityProvider, Logger } from '@smithy/types';
import { AwsCredentialIdentityProvider, Logger } from '@smithy/types';
import { AwsCliCompatible } from './awscli-compatible';
import { cached } from './cached';
import { CredentialPlugins } from './credential-plugins';
import { SDK } from './sdk';
import { debug, warning } from '../../logging';
import { traceMethods } from '../../util/tracing';
import { Mode } from '../plugin';
import { makeCachingProvider } from './provider-caching';

export type AssumeRoleAdditionalOptions = Partial<Omit<AssumeRoleCommandInput, 'ExternalId' | 'RoleArn'>>;

Expand Down Expand Up @@ -57,7 +58,6 @@ export interface SdkHttpOptions {
}

const CACHED_ACCOUNT = Symbol('cached_account');
const CACHED_DEFAULT_CREDENTIALS = Symbol('cached_default_credentials');

/**
* SDK configuration for a given environment
Expand Down Expand Up @@ -268,13 +268,7 @@ export class SdkProvider {
public async defaultAccount(): Promise<Account | undefined> {
return cached(this, CACHED_ACCOUNT, async () => {
try {
const credentials = await this.defaultCredentials();
const accessKeyId = credentials.accessKeyId;
if (!accessKeyId) {
throw new Error('Unable to resolve AWS credentials (setup with "aws configure")');
}

return await new SDK(credentials, this.defaultRegion, this.requestHandler, this.logger).currentAccount();
return await new SDK(this.defaultCredentialProvider, this.defaultRegion, this.requestHandler, this.logger).currentAccount();
} catch (e: any) {
// Treat 'ExpiredToken' specially. This is a common situation that people may find themselves in, and
// they are complaining about if we fail 'cdk synth' on them. We loudly complain in order to show that
Expand Down Expand Up @@ -307,7 +301,7 @@ export class SdkProvider {
if (defaultAccountId === accountId) {
return {
source: 'correctDefault',
credentials: await this.defaultCredentials(),
credentials: await this.defaultCredentialProvider,
};
}

Expand All @@ -322,7 +316,7 @@ export class SdkProvider {
return {
source: 'incorrectDefault',
accountId: defaultAccountId,
credentials: await this.defaultCredentials(),
credentials: await this.defaultCredentialProvider,
unusedPlugins: this.plugins.availablePluginNames,
};
}
Expand All @@ -334,16 +328,6 @@ export class SdkProvider {
};
}

/**
* Resolve the default chain to the first set of credentials that is available
*/
private async defaultCredentials(): Promise<AwsCredentialIdentity> {
return cached(this, CACHED_DEFAULT_CREDENTIALS, async () => {
debug('Resolving default credentials');
return this.defaultCredentialProvider();
});
}

/**
* Return an SDK which uses assumed role credentials
*
Expand All @@ -365,7 +349,7 @@ export class SdkProvider {
const sourceDescription = fmtObtainedCredentials(mainCredentials);

try {
const credentials = await fromTemporaryCredentials({
const credentials = await makeCachingProvider(fromTemporaryCredentials({
masterCredentials: mainCredentials.credentials,
params: {
RoleArn: roleArn,
Expand All @@ -380,7 +364,11 @@ export class SdkProvider {
customUserAgent: 'aws-cdk',
logger: this.logger,
},
})();
logger: this.logger,
}));

// Call the provider at least once here, to catch an error if it occurs
await credentials();

return new SDK(credentials, region, this.requestHandler, this.logger);
} catch (err: any) {
Expand Down Expand Up @@ -457,11 +445,11 @@ export interface CredentialsOptions {
* Result of obtaining base credentials
*/
type ObtainBaseCredentialsResult =
| { source: 'correctDefault'; credentials: AwsCredentialIdentity }
| { source: 'plugin'; pluginName: string; credentials: AwsCredentialIdentity }
| { source: 'correctDefault'; credentials: AwsCredentialIdentityProvider }
| { source: 'plugin'; pluginName: string; credentials: AwsCredentialIdentityProvider }
| {
source: 'incorrectDefault';
credentials: AwsCredentialIdentity;
credentials: AwsCredentialIdentityProvider;
accountId: string;
unusedPlugins: string[];
}
Expand Down
Loading

0 comments on commit 058a0bf

Please sign in to comment.