diff --git a/README.md b/README.md index 09f4b69..b51f44a 100644 --- a/README.md +++ b/README.md @@ -776,7 +776,10 @@ the environment (for example, on a self-hosted runner where you do not want the assumed-role credentials to shadow an existing EC2 instance profile), pair `output-credentials: true` with `output-env-credentials: false`. In that mode, the action does not run its post-credential SDK-pickup validation step, since -the credentials were never written to the environment. +the credentials were never written to the environment. The action still +validates the resolved credentials by calling `sts:GetCallerIdentity` with the +explicit credentials, so the `allowed-account-ids` check can be enforced if +provided. ### Configure multiple AWS profiles in a single workflow diff --git a/src/CredentialsClient.ts b/src/CredentialsClient.ts index cfbf204..8347a99 100644 --- a/src/CredentialsClient.ts +++ b/src/CredentialsClient.ts @@ -53,54 +53,55 @@ export class CredentialsClient { public get stsClient(): STSClient { if (!this._stsClient || this.roleChaining) { - this._stsClient = new STSClient({ - customUserAgent: buildCustomUserAgent(), - ...(this.region !== undefined && { region: this.region }), - ...(this.stsEndpoint !== undefined && { endpoint: this.stsEndpoint }), - ...(this.requestHandler !== undefined && { requestHandler: this.requestHandler }), - }); + this._stsClient = this.createStsClient(); } return this._stsClient; } + // Builds an STS client using the action's configured region/endpoint/proxy. When explicit credentials are provided, + // the client uses them directly instead of the SDK default credential provider chain. + // This matters for validateAccountId. + private createStsClient(credentials?: AwsCredentialIdentity): STSClient { + return new STSClient({ + customUserAgent: buildCustomUserAgent(), + ...(this.region !== undefined && { region: this.region }), + ...(this.stsEndpoint !== undefined && { endpoint: this.stsEndpoint }), + ...(this.requestHandler !== undefined && { requestHandler: this.requestHandler }), + ...(credentials !== undefined && { credentials }), + }); + } + + // Validates that the credentials the action will hand to subsequent steps actually work, and returns the resolved + // caller identity (account + ARN). "Work" is proven by a sts:GetCallerIdentity call, which both confirms the + // credentials are accepted by AWS and returns the identity for later checks and outputs to use. public async validateCredentials( + credentials?: AwsCredentialIdentity, expectedAccessKeyId?: string, roleChaining?: boolean, - expectedAccountIds?: string[], - ) { - let credentials: AwsCredentialIdentity; - try { - credentials = await this.loadCredentials(); - if (!credentials.accessKeyId) { - throw new Error('Access key ID empty after loading credentials'); - } - } catch (error) { - throw new Error(`Credentials could not be loaded, please check your action inputs: ${errorMessage(error)}`); - } - if (expectedAccountIds && expectedAccountIds.length > 0 && expectedAccountIds[0] !== '') { - let callerIdentity: Awaited>; + ): Promise>> { + if (!credentials) { + let resolved: AwsCredentialIdentity; try { - callerIdentity = await getCallerIdentity(this.stsClient); + resolved = await this.loadCredentials(); + if (!resolved.accessKeyId) { + throw new Error('Access key ID empty after loading credentials'); + } } catch (error) { - throw new Error(`Could not validate account ID of credentials: ${errorMessage(error)}`); + throw new Error(`Credentials could not be loaded, please check your action inputs: ${errorMessage(error)}`); } - if (!callerIdentity.Account || !expectedAccountIds.includes(callerIdentity.Account)) { - throw new Error( - `The account ID of the provided credentials (${ - callerIdentity.Account ?? 'unknown' - }) does not match any of the expected account IDs: ${expectedAccountIds.join(', ')}`, - ); - } - } - - if (!roleChaining) { - const actualAccessKeyId = credentials.accessKeyId; - if (expectedAccessKeyId && expectedAccessKeyId !== actualAccessKeyId) { + if (!roleChaining && expectedAccessKeyId && expectedAccessKeyId !== resolved.accessKeyId) { throw new Error( 'Credentials loaded by the SDK do not match the expected access key ID configured by the action', ); } } + + const client = credentials ? this.createStsClient(credentials) : this.stsClient; + try { + return await getCallerIdentity(client); + } catch (error) { + throw new Error(`Credentials could not be loaded, please check your action inputs: ${errorMessage(error)}`); + } } private async loadCredentials() { diff --git a/src/helpers.ts b/src/helpers.ts index 4ad21e3..18295e4 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -3,6 +3,7 @@ import * as path from 'node:path'; import * as core from '@actions/core'; import type { Credentials, STSClient } from '@aws-sdk/client-sts'; import { GetCallerIdentityCommand } from '@aws-sdk/client-sts'; +import type { AwsCredentialIdentity } from '@aws-sdk/types'; import type { UserAgent } from '@smithy/types'; import type { CredentialsClient } from './CredentialsClient'; @@ -150,9 +151,8 @@ export async function getCallerIdentity(client: STSClient): Promise<{ Account: s return result; } -// Obtains account ID from STS Client and sets it as output -export async function exportAccountId(credentialsClient: CredentialsClient, maskAccountId?: boolean) { - const identity = await getCallerIdentity(credentialsClient.stsClient); +// Emits the account ID and ARN of an already-resolved caller identity as action outputs. +export function exportAccountId(identity: { Account: string; Arn: string }, maskAccountId?: boolean) { const accountId = identity.Account; const arn = identity.Arn; if (maskAccountId) { @@ -164,6 +164,35 @@ export async function exportAccountId(credentialsClient: CredentialsClient, mask return accountId; } +// Validates that the account of the already-resolved caller identity is in the allow-list provided via the +// `allowed-account-ids` input. +export function validateAccountId(expectedAccountIds: string[] | undefined, account: string | undefined): void { + if (!expectedAccountIds || expectedAccountIds.length === 0 || expectedAccountIds[0] === '') { + return; + } + if (!account || !expectedAccountIds.includes(account)) { + throw new Error( + `The account ID of the provided credentials (${ + account ?? 'unknown' + }) does not match any of the expected account IDs: ${expectedAccountIds.join(', ')}`, + ); + } +} + +// Converts the STS Credentials shape (returned by AssumeRole and provided as action inputs) into +// the AwsCredentialIdentity shape the SDK expects when credentials are supplied explicitly to a +// client. Returns undefined if the access key ID or secret access key is missing. +export function toCredentialIdentity(creds?: Partial): AwsCredentialIdentity | undefined { + if (!creds?.AccessKeyId || !creds.SecretAccessKey) { + return undefined; + } + return { + accessKeyId: creds.AccessKeyId, + secretAccessKey: creds.SecretAccessKey, + ...(creds.SessionToken && { sessionToken: creds.SessionToken }), + }; +} + // Tags have a more restrictive set of acceptable characters than GitHub environment variables can. // This replaces anything not conforming to the tag restrictions by inverting the regular expression. // See the AWS documentation for constraint specifics https://docs.aws.amazon.com/STS/latest/APIReference/API_Tag.html. diff --git a/src/index.ts b/src/index.ts index 2153a32..9d77a52 100644 --- a/src/index.ts +++ b/src/index.ts @@ -10,8 +10,10 @@ import { exportRegion, getBooleanInput, retryAndBackoff, + toCredentialIdentity, translateEnvVariables, unsetCredentials, + validateAccountId, verifyKeys, } from './helpers'; import { writeProfileFiles } from './profileManager'; @@ -51,8 +53,8 @@ export async function run() { }); const roleChaining = getBooleanInput('role-chaining', { required: false }); const outputCredentials = getBooleanInput('output-credentials', { required: false }); - // Default to always outputting environment credentials unless profile is specified. If profile is specified, default to - // no environment credentials (but still output them if the user specifically requests it). + // Default to always outputting environment credentials unless profile is specified. If profile is specified, default + // to no environment credentials (but still output them if the user specifically requests it). const outputEnvCredentials = getBooleanInput('output-env-credentials', { required: false, default: !awsProfile }); const unsetCurrentCredentials = getBooleanInput('unset-current-credentials', { required: false }); let disableRetry = getBooleanInput('disable-retry', { required: false }); @@ -198,27 +200,38 @@ export async function run() { writeProfileFiles(awsProfile, { AccessKeyId, SecretAccessKey, SessionToken }, region, overwriteAwsProfile); } } else if (!webIdentityTokenFile && !roleChaining) { - // Proceed only if credentials can be picked up - await withRetry( - () => credentialsClient.validateCredentials(undefined, roleChaining, expectedAccountIds), + // Proceed only if credentials can be picked up. validateCredentials resolves the ambient + // credentials via the SDK default chain, proves they work, and returns the caller identity. + const identity = await withRetry( + () => credentialsClient.validateCredentials(undefined, undefined, roleChaining), 'validateCredentials', ); - sourceAccountId = await withRetry(() => exportAccountId(credentialsClient, maskAccountId), 'exportAccountId'); + // Enforce the allowed-account-ids guardrail unless a role will be assumed, in which case the + // final account is validated after assumeRole (these ambient credentials are the source account). + if (!roleToAssume) { + validateAccountId(expectedAccountIds, identity.Account); + } + sourceAccountId = exportAccountId(identity, maskAccountId); } if (AccessKeyId || roleChaining) { - // Validate that the SDK can actually pick up credentials. - // This validates cases where this action is using existing environment credentials, - // and cases where the user intended to provide input credentials but the secrets inputs resolved to empty strings. - // Skip when output-env-credentials is false: input IAM keys were not written to env, so - // the default chain would resolve to ambient runner credentials and the access-key check - // would spuriously fail (see #1554). + // Validate that the credentials the action will use actually work, and resolve their identity. + const resolutionCredentials = + outputEnvCredentials || !AccessKeyId + ? undefined + : toCredentialIdentity({ AccessKeyId, SecretAccessKey, SessionToken }); + const identity = await withRetry( + () => credentialsClient.validateCredentials(resolutionCredentials, AccessKeyId, roleChaining), + 'validateCredentials', + ); + // Enforce the allowed-account-ids guardrail unless a role will be assumed (the final account is + // validated after assumeRole; these are the source credentials). + if (!roleToAssume) { + validateAccountId(expectedAccountIds, identity.Account); + } + sourceAccountId = identity.Account; if (outputEnvCredentials) { - await withRetry( - () => credentialsClient.validateCredentials(AccessKeyId, roleChaining, expectedAccountIds), - 'validateCredentials', - ); - sourceAccountId = await withRetry(() => exportAccountId(credentialsClient, maskAccountId), 'exportAccountId'); + exportAccountId(identity, maskAccountId); } } if (customTags && (useGitHubOIDCProvider() || webIdentityTokenFile)) { @@ -252,24 +265,15 @@ export async function run() { } while (specialCharacterWorkaround && !verifyKeys(roleCredentials.Credentials)); core.info(`Authenticated as assumedRoleId ${roleCredentials.AssumedRoleUser?.AssumedRoleId}`); exportCredentials(roleCredentials.Credentials, outputCredentials, outputEnvCredentials); - // Validate that the SDK can pick up the assumed-role credentials from the environment. - // Skip when output-env-credentials is false: the credentials were never written to env, - // so the default credential provider chain would resolve to ambient runner credentials - // (e.g. an EC2 instance profile) and the access-key-id check would spuriously fail. - // Skip when using a profile: validation runs after the profile file is written below. - if ((!process.env.GITHUB_ACTIONS || AccessKeyId) && !awsProfile && outputEnvCredentials) { - await withRetry( - () => - credentialsClient.validateCredentials( - roleCredentials.Credentials?.AccessKeyId, - roleChaining, - expectedAccountIds, - ), - 'validateCredentials', - ); - } + // Validate the assumed-role credentials and resolve their identity. + const identity = await withRetry( + () => credentialsClient.validateCredentials(toCredentialIdentity(roleCredentials.Credentials)), + 'validateCredentials', + ); + // Enforce the allowed-account-ids guardrail against the assumed (final) account. + validateAccountId(expectedAccountIds, identity.Account); if (outputEnvCredentials) { - await withRetry(() => exportAccountId(credentialsClient, maskAccountId), 'exportAccountId'); + exportAccountId(identity, maskAccountId); } // Write profile files if profile mode is enabled @@ -279,18 +283,8 @@ export async function run() { } // If user provided IAM User Credentials and then we assumed a role, overwrite the profile file to add // the session token. (this only overwrites the profile within a single run of the action). - // We then validate the credentials to make sure they work. if (AccessKeyId || !process.env.GITHUB_ACTIONS) { writeProfileFiles(awsProfile, roleCredentials.Credentials, region, true); - await withRetry( - () => - credentialsClient.validateCredentials( - roleCredentials.Credentials?.AccessKeyId, - roleChaining, - expectedAccountIds, - ), - 'validateCredentials', - ); } else { writeProfileFiles(awsProfile, roleCredentials.Credentials, region, overwriteAwsProfile); } diff --git a/test/index.test.ts b/test/index.test.ts index d03181a..500538a 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -618,6 +618,9 @@ describe('Configure AWS Credentials', {}, () => { }); it("doesn't export credentials as environment variables if told not to", {}, async () => { mockedSTSClient.on(AssumeRoleWithWebIdentityCommand).resolvesOnce(mocks.outputs.STS_CREDENTIALS); + // Credentials are validated (and their account resolved) even when not exported to the + // environment, so GetCallerIdentity is now called on the explicit assumed-role credentials. + mockedSTSClient.on(GetCallerIdentityCommand).resolves({ ...mocks.outputs.GET_CALLER_IDENTITY }); vi.mocked(core.getInput).mockImplementation(mocks.getInput(mocks.NO_ENV_CREDS_INPUTS)); vi.mocked(core.getIDToken).mockResolvedValue('testoidctoken'); process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN = 'fake-token'; @@ -628,6 +631,7 @@ describe('Configure AWS Credentials', {}, () => { }); it('can export creds as step outputs without exporting as env variables', {}, async () => { mockedSTSClient.on(AssumeRoleWithWebIdentityCommand).resolvesOnce(mocks.outputs.STS_CREDENTIALS); + mockedSTSClient.on(GetCallerIdentityCommand).resolves({ ...mocks.outputs.GET_CALLER_IDENTITY }); vi.mocked(core.getInput).mockImplementation(mocks.getInput(mocks.STEP_BUT_NO_ENV_INPUTS)); vi.mocked(core.getIDToken).mockResolvedValue('testoidctoken'); process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN = 'fake-token'; @@ -897,6 +901,65 @@ describe('Configure AWS Credentials', {}, () => { expect(core.info).toHaveBeenCalledWith('Authenticated as assumedRoleId AROAFAKEASSUMEDROLEID'); }); + it('fails with OIDC when account ID does not match allowed list', async () => { + // Regression test for the allowed-account-ids bypass: in a real runner (GITHUB_ACTIONS=true) + // authenticating via OIDC, the account-ID guardrail was previously never enforced. + vi.mocked(core.getInput).mockImplementation( + mocks.getInput({ + ...mocks.GH_OIDC_INPUTS, + 'allowed-account-ids': '999999999999', + }), + ); + vi.mocked(core.getIDToken).mockResolvedValue('testoidctoken'); + mockedSTSClient.on(AssumeRoleWithWebIdentityCommand).resolves(mocks.outputs.STS_CREDENTIALS); + mockedSTSClient.on(GetCallerIdentityCommand).resolves({ ...mocks.outputs.GET_CALLER_IDENTITY }); + process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN = 'fake-token'; + + await run(); + expect(core.setFailed).toHaveBeenCalledWith( + 'The account ID of the provided credentials (111111111111) does not match any of the expected account IDs: 999999999999', + ); + }); + + it('fails with OIDC and output-env-credentials false when account ID does not match', async () => { + // The guardrail must hold even when credentials are never written to the environment. + vi.mocked(core.getInput).mockImplementation( + mocks.getInput({ + ...mocks.NO_ENV_CREDS_INPUTS, + 'allowed-account-ids': '999999999999', + }), + ); + vi.mocked(core.getIDToken).mockResolvedValue('testoidctoken'); + mockedSTSClient.on(AssumeRoleWithWebIdentityCommand).resolves(mocks.outputs.STS_CREDENTIALS); + mockedSTSClient.on(GetCallerIdentityCommand).resolves({ ...mocks.outputs.GET_CALLER_IDENTITY }); + process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN = 'fake-token'; + + await run(); + expect(core.setFailed).toHaveBeenCalledWith( + 'The account ID of the provided credentials (111111111111) does not match any of the expected account IDs: 999999999999', + ); + }); + + it('fails with assume role when assumed account ID does not match allowed list', async () => { + vi.mocked(core.getInput).mockImplementation( + mocks.getInput({ + ...mocks.IAM_ASSUMEROLE_INPUTS, + 'allowed-account-ids': '999999999999', + }), + ); + mockedSTSClient.on(AssumeRoleCommand).resolves(mocks.outputs.STS_CREDENTIALS); + mockedSTSClient.on(GetCallerIdentityCommand).resolves({ ...mocks.outputs.GET_CALLER_IDENTITY }); + // biome-ignore lint/suspicious/noExplicitAny: any required to mock private method + vi.spyOn(CredentialsClient.prototype as any, 'loadCredentials') + .mockResolvedValueOnce({ accessKeyId: 'MYAWSACCESSKEYID' }) + .mockResolvedValueOnce({ accessKeyId: 'STSAWSACCESSKEYID' }); + + await run(); + expect(core.setFailed).toHaveBeenCalledWith( + 'The account ID of the provided credentials (111111111111) does not match any of the expected account IDs: 999999999999', + ); + }); + it('handles GetCallerIdentity API failure gracefully', async () => { vi.mocked(core.getInput).mockImplementation( mocks.getInput({ @@ -911,7 +974,11 @@ describe('Configure AWS Credentials', {}, () => { }); await run(); - expect(core.setFailed).toHaveBeenCalledWith('Could not validate account ID of credentials: API Error'); + // The account allow-list now reuses the single liveness GetCallerIdentity call, so an STS + // failure surfaces as a credential-loading failure rather than a dedicated account-check error. + expect(core.setFailed).toHaveBeenCalledWith( + 'Credentials could not be loaded, please check your action inputs: API Error', + ); }); it('ignores validation when allowed-account-ids is empty', async () => { @@ -1373,7 +1440,7 @@ describe('Configure AWS Credentials', {}, () => { }); describe('Retry Behavior', {}, () => { - it('retries exportAccountId on transient GetCallerIdentity failure', async () => { + it('retries validateCredentials on transient GetCallerIdentity failure', async () => { vi.mocked(core.getInput).mockImplementation(mocks.getInput(mocks.IAM_USER_INPUTS)); // biome-ignore lint/suspicious/noExplicitAny: any required to mock private method vi.spyOn(CredentialsClient.prototype as any, 'loadCredentials').mockResolvedValue({ @@ -1384,7 +1451,9 @@ describe('Configure AWS Credentials', {}, () => { .rejectsOnce(new Error('throttled')) .resolves({ ...mocks.outputs.GET_CALLER_IDENTITY }); await run(); - expect(core.info).toHaveBeenCalledWith(expect.stringContaining('Retry exportAccountId')); + // The single liveness GetCallerIdentity call lives in validateCredentials, so transient STS + // failures are retried under that label (the account ID is then resolved without a second call). + expect(core.info).toHaveBeenCalledWith(expect.stringContaining('Retry validateCredentials')); expect(core.setFailed).not.toHaveBeenCalled(); }); @@ -1414,7 +1483,7 @@ describe('Configure AWS Credentials', {}, () => { expect(core.info).not.toHaveBeenCalledWith(expect.stringContaining('Retry')); }); - it('retries exportAccountId after role assumption (issue #1681)', async () => { + it('retries the post-assume identity check on a transient invalid-token error (issue #1681)', async () => { vi.mocked(core.getInput).mockImplementation(mocks.getInput(mocks.GH_OIDC_INPUTS)); vi.mocked(core.getIDToken).mockResolvedValue('testoidctoken'); mockedSTSClient.on(AssumeRoleWithWebIdentityCommand).resolves(mocks.outputs.STS_CREDENTIALS); @@ -1424,7 +1493,9 @@ describe('Configure AWS Credentials', {}, () => { .resolves({ ...mocks.outputs.GET_CALLER_IDENTITY }); process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN = 'fake-token'; await run(); - expect(core.info).toHaveBeenCalledWith(expect.stringContaining('Retry exportAccountId')); + // Freshly-assumed credentials can be briefly rejected by STS (eventual consistency). The + // liveness GetCallerIdentity now runs inside validateCredentials, so the retry happens there. + expect(core.info).toHaveBeenCalledWith(expect.stringContaining('Retry validateCredentials')); expect(core.info).toHaveBeenCalledWith( expect.stringContaining('The security token included in the request is invalid'), );