From 742538edb1fd9556e6f93ec7283cfc92c4da2d90 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 24 Nov 2021 18:12:43 +0100 Subject: [PATCH] feat(iam): enable session tagging To allow session tagging, the `sts:TagSession` permission needs to be added to the role's AssumeRolePolicyDocument. Introduce a new principal which enables this, and add a convenience method `.withSessionTags()` to the `PrincipalBase` class so all built-in principals will have this convenience method by default. To build this, we had to get rid of some cruft and assumptions around policy documents and statements, and defer more power to the `IPrincipal` objects themselves. In order not to break existing implementors, introduce a new interface `IAssumeRolePrincipal` which knows how to add itself to an AssumeRolePolicyDocument and gets complete freedom doing so. That same new interface could be used to lift some old limitations on `CompositePrincipal` so did that as well. Fixes #15908, closes #16725, fixes #2041, fixes #1578. --- packages/@aws-cdk/aws-iam/README.md | 30 +++- packages/@aws-cdk/aws-iam/lib/principals.ts | 148 +++++++++++++----- .../aws-iam/lib/private/assume-role-policy.ts | 25 +++ .../aws-iam/lib/private/policydoc-adapter.ts | 17 ++ packages/@aws-cdk/aws-iam/lib/role.ts | 42 ++--- .../aws-iam/test/policy-document.test.ts | 76 +++++++-- .../@aws-cdk/aws-iam/test/principals.test.ts | 32 ++++ 7 files changed, 288 insertions(+), 82 deletions(-) create mode 100644 packages/@aws-cdk/aws-iam/lib/private/assume-role-policy.ts create mode 100644 packages/@aws-cdk/aws-iam/lib/private/policydoc-adapter.ts diff --git a/packages/@aws-cdk/aws-iam/README.md b/packages/@aws-cdk/aws-iam/README.md index a81b25d53e6ba..5cb6a1f076371 100644 --- a/packages/@aws-cdk/aws-iam/README.md +++ b/packages/@aws-cdk/aws-iam/README.md @@ -155,7 +155,7 @@ To add a principal to a policy statement you can either use the abstract * `addServicePrincipal` or `new ServicePrincipal(service)` for `{ "Service": service }` * `addAccountRootPrincipal` or `new AccountRootPrincipal()` for `{ "AWS": { "Ref: "AWS::AccountId" } }` * `addCanonicalUserPrincipal` or `new CanonicalUserPrincipal(id)` for `{ "CanonicalUser": id }` -* `addFederatedPrincipal` or `new FederatedPrincipal(federated, conditions, assumeAction)` for +* `addFederatedPrincipal` or `new FederatedPrincipal(federated, conditions, [assumeAction], [sessionTags])` for `{ "Federated": arn }` and a set of optional conditions and the assume role action to use. * `addAnyPrincipal` or `new AnyPrincipal` for `{ "AWS": "*" }` @@ -209,13 +209,31 @@ The `WebIdentityPrincipal` class can be used as a principal for web identities l Cognito, Amazon, Google or Facebook, for example: ```ts -const principal = new iam.WebIdentityPrincipal('cognito-identity.amazonaws.com') - .withConditions({ - "StringEquals": { "cognito-identity.amazonaws.com:aud": "us-east-2:12345678-abcd-abcd-abcd-123456" }, - "ForAnyValue:StringLike": {"cognito-identity.amazonaws.com:amr": "unauthenticated" }, - }); +const principal = new iam.WebIdentityPrincipal('cognito-identity.amazonaws.com', { + 'StringEquals': { 'cognito-identity.amazonaws.com:aud': 'us-east-2:12345678-abcd-abcd-abcd-123456' }, + 'ForAnyValue:StringLike': {'cognito-identity.amazonaws.com:amr': 'unauthenticated' }, +}); ``` +If your identity provider is configured to assume a Role with [session +tags](https://docs.aws.amazon.com/IAM/latest/UserGuide/id_session-tags.html), you +need to call `.withSessionTags()` to add the required permissions to the Role's +policy document: + +```ts +new iam.Role(this, 'Role', { + assumedBy: new iam.WebIdentityPrincipal('cognito-identity.amazonaws.com', { + 'StringEquals': { + 'cognito-identity.amazonaws.com:aud': 'us-east-2:12345678-abcd-abcd-abcd-123456', + }, + 'ForAnyValue:StringLike': { + 'cognito-identity.amazonaws.com:amr': 'unauthenticated', + }, + }).withSessionTags(), +}); +``` + + ## Parsing JSON Policy Documents The `PolicyDocument.fromJson` and `PolicyStatement.fromJson` static methods can be used to parse JSON objects. For example: diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts index 001792cbcc475..d211916d07c11 100644 --- a/packages/@aws-cdk/aws-iam/lib/principals.ts +++ b/packages/@aws-cdk/aws-iam/lib/principals.ts @@ -1,7 +1,9 @@ import * as cdk from '@aws-cdk/core'; import { Default, RegionInfo } from '@aws-cdk/region-info'; import { IOpenIdConnectProvider } from './oidc-provider'; +import { PolicyDocument } from './policy-document'; import { Condition, Conditions, PolicyStatement } from './policy-statement'; +import { defaultAddPrincipalToAssumeRole } from './private/assume-role-policy'; import { ISamlProvider } from './saml-provider'; import { LITERAL_STRING_KEY, mergePrincipal } from './util'; @@ -68,6 +70,25 @@ export interface IPrincipal extends IGrantable { addToPrincipalPolicy(statement: PolicyStatement): AddToPrincipalPolicyResult; } +/** + * A type of principal that has more control over its own representation in AssumeRolePolicyDocuments + * + * More complex types of identity providers need more control over Role's policy documents + * than simply `{ Effect: 'Allow', Action: 'AssumeRole', Principal: }`. + * + * If that control is necessary, they can implement `IAssumeRolePrincipal` to get full + * access to a Role's AssumeRolePolicyDocument. + */ +export interface IAssumeRolePrincipal extends IPrincipal { + /** + * Add the princpial to the AssumeRolePolicyDocument + * + * Add the statements to the AssumeRolePolicyDocument necessary to give this principal + * permissions to assume the given role. + */ + addToAssumeRolePolicy(document: PolicyDocument): void; +} + /** * Result of calling `addToPrincipalPolicy` */ @@ -89,7 +110,7 @@ export interface AddToPrincipalPolicyResult { /** * Base class for policy principals */ -export abstract class PrincipalBase implements IPrincipal { +export abstract class PrincipalBase implements IAssumeRolePrincipal { public readonly grantPrincipal: IPrincipal = this; public readonly principalAccount: string | undefined = undefined; @@ -113,6 +134,14 @@ export abstract class PrincipalBase implements IPrincipal { return { statementAdded: false }; } + public addToAssumeRolePolicy(document: PolicyDocument): void { + // Default implementation of this protocol, compatible with the legacy behavior + document.addStatements(new PolicyStatement({ + actions: [this.assumeRoleAction], + principals: [this], + })); + } + public toString() { // This is a first pass to make the object readable. Descendant principals // should return something nicer. @@ -138,9 +167,39 @@ export abstract class PrincipalBase implements IPrincipal { * * @returns a new PrincipalWithConditions object. */ - public withConditions(conditions: Conditions): IPrincipal { + public withConditions(conditions: Conditions): PrincipalBase { return new PrincipalWithConditions(this, conditions); } + + /** + * Returns a new principal using this principal as the base, with session tags enabled. + * + * @returns a new SessionTagsPrincipal object. + */ + public withSessionTags(): PrincipalBase { + return new SessionTagsPrincipal(this); + } +} + +/** + * Base class for Principals that wrap other principals + */ +class PrincipalAdapter extends PrincipalBase { + public readonly assumeRoleAction = this.wrapped.assumeRoleAction; + public readonly principalAccount = this.wrapped.principalAccount; + + constructor(protected readonly wrapped: IPrincipal) { + super(); + } + + public get policyFragment(): PrincipalPolicyFragment { return this.wrapped.policyFragment; } + + addToPolicy(statement: PolicyStatement): boolean { + return this.wrapped.addToPolicy(statement); + } + addToPrincipalPolicy(statement: PolicyStatement): AddToPrincipalPolicyResult { + return this.wrapped.addToPrincipalPolicy(statement); + } } /** @@ -149,15 +208,11 @@ export abstract class PrincipalBase implements IPrincipal { * For more information about conditions, see: * https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html */ -export class PrincipalWithConditions implements IPrincipal { - public readonly grantPrincipal: IPrincipal = this; - public readonly assumeRoleAction: string = this.principal.assumeRoleAction; +export class PrincipalWithConditions extends PrincipalAdapter { private additionalConditions: Conditions; - constructor( - private readonly principal: IPrincipal, - conditions: Conditions, - ) { + constructor(principal: IPrincipal, conditions: Conditions) { + super(principal); this.additionalConditions = conditions; } @@ -186,27 +241,15 @@ export class PrincipalWithConditions implements IPrincipal { * See [the IAM documentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html). */ public get conditions() { - return this.mergeConditions(this.principal.policyFragment.conditions, this.additionalConditions); + return this.mergeConditions(this.wrapped.policyFragment.conditions, this.additionalConditions); } public get policyFragment(): PrincipalPolicyFragment { - return new PrincipalPolicyFragment(this.principal.policyFragment.principalJson, this.conditions); - } - - public get principalAccount(): string | undefined { - return this.principal.principalAccount; - } - - public addToPolicy(statement: PolicyStatement): boolean { - return this.addToPrincipalPolicy(statement).statementAdded; - } - - public addToPrincipalPolicy(statement: PolicyStatement): AddToPrincipalPolicyResult { - return this.principal.addToPrincipalPolicy(statement); + return new PrincipalPolicyFragment(this.wrapped.policyFragment.principalJson, this.conditions); } public toString() { - return this.principal.toString(); + return this.wrapped.toString(); } /** @@ -247,6 +290,30 @@ export class PrincipalWithConditions implements IPrincipal { } } +/** + * Enables session tags on role assumptions from a principal + * + * For more information on session tags, see: + * https://docs.aws.amazon.com/IAM/latest/UserGuide/id_session-tags.html + */ +export class SessionTagsPrincipal extends PrincipalAdapter { + constructor(principal: IPrincipal) { + super(principal); + } + + public addToAssumeRolePolicy(doc: PolicyDocument) { + // Lazy import to avoid circular import dependencies during startup + + // eslint-disable-next-line @typescript-eslint/no-require-imports + const adapter: typeof import('./private/policydoc-adapter') = require('./private/policydoc-adapter'); + + defaultAddPrincipalToAssumeRole(this.wrapped, new adapter.MutatingPolicyDocumentAdapter(doc, (statement) => { + statement.addActions('sts:TagSession'); + return statement; + })); + } +} + /** * A collection of the fields in a PolicyStatement that can be used to identify a principal. * @@ -440,6 +507,7 @@ export class FederatedPrincipal extends PrincipalBase { * @param federated federated identity provider (i.e. 'cognito-identity.amazonaws.com' for users authenticated through Cognito) * @param conditions The conditions under which the policy is in effect. * See [the IAM documentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html). + * @param sessionTags Whether to enable session tagging (see https://docs.aws.amazon.com/IAM/latest/UserGuide/id_session-tags.html) */ constructor( public readonly federated: string, @@ -470,6 +538,7 @@ export class WebIdentityPrincipal extends FederatedPrincipal { * @param identityProvider identity provider (i.e. 'cognito-identity.amazonaws.com' for users authenticated through Cognito) * @param conditions The conditions under which the policy is in effect. * See [the IAM documentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html). + * @param sessionTags Whether to enable session tagging (see https://docs.aws.amazon.com/IAM/latest/UserGuide/id_session-tags.html) */ constructor(identityProvider: string, conditions: Conditions = {}) { super(identityProvider, conditions ?? {}, 'sts:AssumeRoleWithWebIdentity'); @@ -605,9 +674,9 @@ export class StarPrincipal extends PrincipalBase { */ export class CompositePrincipal extends PrincipalBase { public readonly assumeRoleAction: string; - private readonly principals = new Array(); + private readonly principals = new Array(); - constructor(...principals: PrincipalBase[]) { + constructor(...principals: IPrincipal[]) { super(); if (principals.length === 0) { throw new Error('CompositePrincipals must be constructed with at least 1 Principal but none were passed.'); @@ -622,28 +691,29 @@ export class CompositePrincipal extends PrincipalBase { * * @param principals IAM principals that will be added to the composite principal */ - public addPrincipals(...principals: PrincipalBase[]): this { - for (const p of principals) { - if (p.assumeRoleAction !== this.assumeRoleAction) { - throw new Error( - 'Cannot add multiple principals with different "assumeRoleAction". ' + - `Expecting "${this.assumeRoleAction}", got "${p.assumeRoleAction}"`); - } + public addPrincipals(...principals: IPrincipal[]): this { + this.principals.push(...principals); + return this; + } + + public addToAssumeRolePolicy(doc: PolicyDocument) { + for (const p of this.principals) { + defaultAddPrincipalToAssumeRole(p, doc); + } + } + public get policyFragment(): PrincipalPolicyFragment { + // We only have a problem with conditions if we are trying to render composite + // princpals into a single statement (which is when `policyFragment` would get called) + for (const p of this.principals) { const fragment = p.policyFragment; if (fragment.conditions && Object.keys(fragment.conditions).length > 0) { throw new Error( 'Components of a CompositePrincipal must not have conditions. ' + `Tried to add the following fragment: ${JSON.stringify(fragment)}`); } - - this.principals.push(p); } - return this; - } - - public get policyFragment(): PrincipalPolicyFragment { const principalJson: { [key: string]: string[] } = {}; for (const p of this.principals) { diff --git a/packages/@aws-cdk/aws-iam/lib/private/assume-role-policy.ts b/packages/@aws-cdk/aws-iam/lib/private/assume-role-policy.ts new file mode 100644 index 0000000000000..9a2a474fcb2d4 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/private/assume-role-policy.ts @@ -0,0 +1,25 @@ +import { PolicyDocument } from '../policy-document'; +import { PolicyStatement } from '../policy-statement'; +import { IPrincipal, IAssumeRolePrincipal } from '../principals'; + +/** + * Add a principal to an AssumeRolePolicyDocument in the right way + * + * Delegate to the principal if it can do the job itself, do a default job if it can't. + */ +export function defaultAddPrincipalToAssumeRole(principal: IPrincipal, doc: PolicyDocument) { + if (isAssumeRolePrincipal(principal)) { + // Principal knows how to add itself + principal.addToAssumeRolePolicy(doc); + } else { + // Principal can't add itself, we do it for them + doc.addStatements(new PolicyStatement({ + actions: [principal.assumeRoleAction], + principals: [principal], + })); + } +} + +function isAssumeRolePrincipal(principal: IPrincipal): principal is IAssumeRolePrincipal { + return !!(principal as IAssumeRolePrincipal).addToAssumeRolePolicy; +} diff --git a/packages/@aws-cdk/aws-iam/lib/private/policydoc-adapter.ts b/packages/@aws-cdk/aws-iam/lib/private/policydoc-adapter.ts new file mode 100644 index 0000000000000..c6f95631ea495 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/private/policydoc-adapter.ts @@ -0,0 +1,17 @@ +import { PolicyDocument } from '../policy-document'; +import { PolicyStatement } from '../policy-statement'; + +/** + * A PolicyDocument adapter that can modify statements flowing through it + */ +export class MutatingPolicyDocumentAdapter extends PolicyDocument { + constructor(private readonly wrapped: PolicyDocument, private readonly mutator: (s: PolicyStatement) => PolicyStatement) { + super(); + } + + public addStatements(...statements: PolicyStatement[]): void { + for (const st of statements) { + this.wrapped.addStatements(this.mutator(st)); + } + } +} diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index b103b21231e23..ac48fdd3d7710 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -8,7 +8,9 @@ import { Policy } from './policy'; import { PolicyDocument } from './policy-document'; import { PolicyStatement } from './policy-statement'; import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; +import { defaultAddPrincipalToAssumeRole } from './private/assume-role-policy'; import { ImmutableRole } from './private/immutable-role'; +import { MutatingPolicyDocumentAdapter } from './private/policydoc-adapter'; import { AttachedPolicies, UniqueStringSet } from './util'; /** @@ -484,17 +486,21 @@ export interface IRole extends IIdentity { } function createAssumeRolePolicy(principal: IPrincipal, externalIds: string[]) { - const statement = new AwsStarStatement(); - statement.addPrincipals(principal); - statement.addActions(principal.assumeRoleAction); + const actualDoc = new PolicyDocument(); + + // If requested, add externalIds to every statement added to this doc + const addDoc = externalIds.length === 0 + ? actualDoc + : new MutatingPolicyDocumentAdapter(actualDoc, (statement) => { + statement.addCondition('StringEquals', { + 'sts:ExternalId': externalIds.length === 1 ? externalIds[0] : externalIds, + }); + return statement; + }); - if (externalIds.length) { - statement.addCondition('StringEquals', { 'sts:ExternalId': externalIds.length === 1 ? externalIds[0] : externalIds }); - } + defaultAddPrincipalToAssumeRole(principal, addDoc); - const doc = new PolicyDocument(); - doc.addStatements(statement); - return doc; + return actualDoc; } function validateMaxSessionDuration(duration?: number) { @@ -507,24 +513,6 @@ function validateMaxSessionDuration(duration?: number) { } } -/** - * A PolicyStatement that normalizes its Principal field differently - * - * Normally, "anyone" is normalized to "Principal: *", but this statement - * normalizes to "Principal: { AWS: * }". - */ -class AwsStarStatement extends PolicyStatement { - public toStatementJson(): any { - const stat = super.toStatementJson(); - - if (stat.Principal === '*') { - stat.Principal = { AWS: '*' }; - } - - return stat; - } -} - /** * Options for the `withoutPolicyUpdates()` modifier of a Role */ diff --git a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts index bf93e31901c6c..8815bd3b3e4ba 100644 --- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts +++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts @@ -2,7 +2,7 @@ import '@aws-cdk/assert-internal/jest'; import { Lazy, Stack, Token } from '@aws-cdk/core'; import { AccountPrincipal, Anyone, AnyPrincipal, ArnPrincipal, CanonicalUserPrincipal, CompositePrincipal, - Effect, FederatedPrincipal, IPrincipal, PolicyDocument, PolicyStatement, PrincipalPolicyFragment, ServicePrincipal, + Effect, FederatedPrincipal, IPrincipal, PolicyDocument, PolicyStatement, PrincipalPolicyFragment, ServicePrincipal, Role, } from '../lib'; describe('IAM policy document', () => { @@ -481,10 +481,46 @@ describe('IAM policy document', () => { expect(stack.resolve(statement.toStatementJson())).toEqual({ Effect: 'Allow', Principal: { AWS: 'i:am:an:arn' } }); }); - test('conditions are not allowed on individual principals of a composite', () => { - const p = new CompositePrincipal(new ArnPrincipal('i:am')); - expect(() => p.addPrincipals(new FederatedPrincipal('federated', { StringEquals: { 'aws:some-key': 'some-value' } }))) - .toThrow(/Components of a CompositePrincipal must not have conditions/); + test('conditions are allowed in an assumerolepolicydocument', () => { + const stack = new Stack(); + new Role(stack, 'Role', { + assumedBy: new CompositePrincipal( + new ArnPrincipal('i:am'), + new FederatedPrincipal('federated', { StringEquals: { 'aws:some-key': 'some-value' } }), + ), + }); + + expect(stack).toHaveResourceLike('AWS::IAM::Role', { + AssumeRolePolicyDocument: { + Statement: [ + { + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { AWS: 'i:am' }, + }, + { + Action: 'sts:AssumeRole', + Condition: { + StringEquals: { 'aws:some-key': 'some-value' }, + }, + Effect: 'Allow', + Principal: { Federated: 'federated' }, + }, + ], + }, + }); + }); + + test('conditions are not allowed when used in a single statement', () => { + + expect(() => { + new PolicyStatement({ + actions: ['s3:test'], + principals: [new CompositePrincipal( + new ArnPrincipal('i:am'), + new FederatedPrincipal('federated', { StringEquals: { 'aws:some-key': 'some-value' } }))], + }); + }).toThrow(/Components of a CompositePrincipal must not have conditions/); }); test('principals and conditions are a big nice merge', () => { @@ -519,13 +555,33 @@ describe('IAM policy document', () => { }); }); - test('cannot mix types of assumeRoleAction in a single composite', () => { - // GIVEN - const p = new CompositePrincipal(new ArnPrincipal('arn')); // assumeRoleAction is "sts:AssumeRule" + test('can mix types of assumeRoleAction in a single composite', () => { + const stack = new Stack(); + + // WHEN + new Role(stack, 'Role', { + assumedBy: new CompositePrincipal( + new ArnPrincipal('arn'), + new FederatedPrincipal('fed', {}, 'sts:Boom')), + }); // THEN - expect(() => p.addPrincipals(new FederatedPrincipal('fed', {}, 'sts:Boom'))) - .toThrow(/Cannot add multiple principals with different "assumeRoleAction". Expecting "sts:AssumeRole", got "sts:Boom"/); + expect(stack).toHaveResourceLike('AWS::IAM::Role', { + AssumeRolePolicyDocument: { + Statement: [ + { + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { AWS: 'arn' }, + }, + { + Action: 'sts:Boom', + Effect: 'Allow', + Principal: { Federated: 'fed' }, + }, + ], + }, + }); }); }); diff --git a/packages/@aws-cdk/aws-iam/test/principals.test.ts b/packages/@aws-cdk/aws-iam/test/principals.test.ts index 1bf7d47950875..c19d7ccd6d719 100644 --- a/packages/@aws-cdk/aws-iam/test/principals.test.ts +++ b/packages/@aws-cdk/aws-iam/test/principals.test.ts @@ -243,4 +243,36 @@ test('PrincipalWithConditions inherits principalAccount from AccountPrincipal ', // THEN expect(accountPrincipal.principalAccount).toStrictEqual('123456789012'); expect(principalWithConditions.principalAccount).toStrictEqual('123456789012'); +}); + +test('Can enable session tags', () => { + // GIVEN + const stack = new Stack(); + + // WHEN + new iam.Role(stack, 'Role', { + assumedBy: new iam.WebIdentityPrincipal( + 'cognito-identity.amazonaws.com', + { + 'StringEquals': { 'cognito-identity.amazonaws.com:aud': 'asdf' }, + 'ForAnyValue:StringLike': { 'cognito-identity.amazonaws.com:amr': 'authenticated' }, + }).withSessionTags(), + }); + + // THEN + expect(stack).toHaveResourceLike('AWS::IAM::Role', { + AssumeRolePolicyDocument: { + Statement: [ + { + Action: ['sts:AssumeRoleWithWebIdentity', 'sts:TagSession'], + Condition: { + 'StringEquals': { 'cognito-identity.amazonaws.com:aud': 'asdf' }, + 'ForAnyValue:StringLike': { 'cognito-identity.amazonaws.com:amr': 'authenticated' }, + }, + Effect: 'Allow', + Principal: { Federated: 'cognito-identity.amazonaws.com' }, + }, + ], + }, + }); }); \ No newline at end of file