From 4d769d1dd9416902a8be9bc0a6f87e280314d4cc Mon Sep 17 00:00:00 2001 From: Zak <15427477+zakwalters@users.noreply.github.com> Date: Mon, 6 Apr 2020 15:58:14 +0100 Subject: [PATCH] feat(iam): add arbitrary conditions to existing principals (#7015) Closes #5855 Adds a `PrincipalWithConditions` wrapper that allows conditions to be added to any principal. Behaviour is consistent with the way that `PolicyStatement.addCondition` and `.addConditions` currently work - most notably in that adding an operator that is already present will merge their objects, but adding a condition to an operator/key combination that is already set will overwrite the existing value (rather than merge the values into an array). BREAKING CHANGES: every place an IAM Condition was expected it is now typed as `{[key: string]: any}`, instead of plain `any`. You were always supposed to pass a map/dictionary in these locations, but the type system didn't enforce it. It now does. This will not impact correct programs, but may cause compiler errors in programs that were incorrect. --- allowed-breaking-changes.txt | 5 + packages/@aws-cdk/aws-iam/README.md | 10 ++ .../@aws-cdk/aws-iam/lib/policy-statement.ts | 26 +++- packages/@aws-cdk/aws-iam/lib/principals.ts | 130 +++++++++++++++++- packages/@aws-cdk/aws-iam/package.json | 3 - .../aws-iam/test/policy-document.test.ts | 98 ++++++++++++- .../aws-iam/test/policy-statement.test.ts | 2 +- 7 files changed, 254 insertions(+), 20 deletions(-) diff --git a/allowed-breaking-changes.txt b/allowed-breaking-changes.txt index a84098fb3ab06..1f78ca52d6422 100644 --- a/allowed-breaking-changes.txt +++ b/allowed-breaking-changes.txt @@ -41,3 +41,8 @@ change-return-type:@aws-cdk/aws-lambda-destinations.LambdaDestination.bind change-return-type:@aws-cdk/aws-lambda-destinations.SnsDestination.bind change-return-type:@aws-cdk/aws-lambda-destinations.SqsDestination.bind removed:@aws-cdk/cdk-assets-schema.DockerImageDestination.imageUri +incompatible-argument:@aws-cdk/aws-iam.FederatedPrincipal. +incompatible-argument:@aws-cdk/aws-iam.PolicyStatement.addCondition +incompatible-argument:@aws-cdk/aws-iam.PolicyStatement.addConditions +incompatible-argument:@aws-cdk/aws-iam.PolicyStatement.addFederatedPrincipal +incompatible-argument:@aws-cdk/aws-iam.PrincipalPolicyFragment. diff --git a/packages/@aws-cdk/aws-iam/README.md b/packages/@aws-cdk/aws-iam/README.md index 59f0db61645c6..346971f5eb1d1 100644 --- a/packages/@aws-cdk/aws-iam/README.md +++ b/packages/@aws-cdk/aws-iam/README.md @@ -188,6 +188,16 @@ const role = new iam.Role(this, 'MyRole', { }); ``` +The `PrincipalWithConditions` class can be used to add conditions to a +principal, especially those that don't take a `conditions` parameter in their +constructor. The `principal.withConditions()` method can be used to create a +`PrincipalWithConditions` from an existing principal, for example: + +```ts +const principal = new iam.AccountPrincipal('123456789000') + .withConditions({ StringEquals: { foo: "baz" } }); +``` + ### 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/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts index 31aca5ad60b2c..5bba3ff6b824c 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts @@ -145,7 +145,7 @@ export class PolicyStatement { this.addPrincipals(new ServicePrincipal(service, opts)); } - public addFederatedPrincipal(federated: any, conditions: {[key: string]: any}) { + public addFederatedPrincipal(federated: any, conditions: Conditions) { this.addPrincipals(new FederatedPrincipal(federated, conditions)); } @@ -200,7 +200,7 @@ export class PolicyStatement { /** * Add a condition to the Policy */ - public addCondition(key: string, value: any) { + public addCondition(key: string, value: Condition) { const existingValue = this.condition[key]; this.condition[key] = existingValue ? { ...existingValue, ...value } : value; } @@ -208,7 +208,7 @@ export class PolicyStatement { /** * Add multiple conditions to the Policy */ - public addConditions(conditions: {[key: string]: any}) { + public addConditions(conditions: Conditions) { Object.keys(conditions).map(key => { this.addCondition(key, conditions[key]); }); @@ -303,6 +303,24 @@ export enum Effect { DENY = 'Deny', } +/** + * Condition for when an IAM policy is in effect. Maps from the keys in a request's context to + * a string value or array of string values. See the Conditions interface for more details. + */ +export type Condition = Record; + +/** + * Conditions for when an IAM Policy is in effect, specified in the following structure: + * + * `{ "Operator": { "keyInRequestContext": "value" } }` + * + * The value can be either a single string value or an array of string values. + * + * For more information, including which operators are supported, see [the IAM + * documentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html). + */ +export type Conditions = Record; + /** * Interface for creating a policy statement */ @@ -398,7 +416,7 @@ class JsonPrincipal extends PrincipalBase { this.policyFragment = { principalJson: json, - conditions: [] + conditions: {} }; } } diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts index 36c85d1aeabb7..ccf674cf6665f 100644 --- a/packages/@aws-cdk/aws-iam/lib/principals.ts +++ b/packages/@aws-cdk/aws-iam/lib/principals.ts @@ -1,6 +1,6 @@ import * as cdk from '@aws-cdk/core'; import { Default, RegionInfo } from '@aws-cdk/region-info'; -import { PolicyStatement } from './policy-statement'; +import { Condition, Conditions, PolicyStatement } from './policy-statement'; import { mergePrincipal } from './util'; /** @@ -78,10 +78,108 @@ export abstract class PrincipalBase implements IPrincipal { return JSON.stringify(this.policyFragment.principalJson); } + /** + * JSON-ify the principal + * + * Used when JSON.stringify() is called + */ + public toJSON() { + // Have to implement toJSON() because the default will lead to infinite recursion. + return this.policyFragment.principalJson; + } + + /** + * Returns a new PrincipalWithConditions using this principal as the base, with the + * passed conditions added. + * + * When there is a value for the same operator and key in both the principal and the + * conditions parameter, the value from the conditions parameter will be used. + * + * @returns a new PrincipalWithConditions object. + */ + public withConditions(conditions: Conditions): IPrincipal { + return new PrincipalWithConditions(this, conditions); + } +} + +/** + * An IAM principal with additional conditions specifying when the policy is in effect. + * + * 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; + private additionalConditions: Conditions; + + constructor( + private readonly principal: IPrincipal, + conditions: Conditions, + ) { + this.additionalConditions = conditions; + } + + /** + * Add a condition to the principal + */ + public addCondition(key: string, value: Condition) { + const existingValue = this.conditions[key]; + this.conditions[key] = existingValue ? { ...existingValue, ...value } : value; + } + + /** + * Adds multiple conditions to the principal + * + * Values from the conditions parameter will overwrite existing values with the same operator + * and key. + */ + public addConditions(conditions: Conditions) { + Object.entries(conditions).forEach(([key, value]) => { + this.addCondition(key, value); + }); + } + + /** + * 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). + */ + public get conditions() { + return this.mergeConditions(this.principal.policyFragment.conditions, this.additionalConditions); + } + + public get policyFragment(): PrincipalPolicyFragment { + return new PrincipalPolicyFragment(this.principal.policyFragment.principalJson, this.conditions); + } + + public addToPolicy(statement: PolicyStatement): boolean { + return this.principal.addToPolicy(statement); + } + + public toString() { + return this.principal.toString(); + } + + /** + * JSON-ify the principal + * + * Used when JSON.stringify() is called + */ public toJSON() { // Have to implement toJSON() because the default will lead to infinite recursion. return this.policyFragment.principalJson; } + + private mergeConditions(principalConditions: Conditions, additionalConditions: Conditions): Conditions { + const mergedConditions: Conditions = {}; + Object.entries(principalConditions).forEach(([operator, condition]) => { + mergedConditions[operator] = condition; + }); + Object.entries(additionalConditions).forEach(([operator, condition]) => { + mergedConditions[operator] = { ...mergedConditions[operator], ...condition }; + }); + return mergedConditions; + } } /** @@ -93,7 +191,11 @@ export abstract class PrincipalBase implements IPrincipal { export class PrincipalPolicyFragment { constructor( public readonly principalJson: { [key: string]: string[] }, - public readonly conditions: { [key: string]: any } = { }) { + /** + * 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). + */ + public readonly conditions: Conditions = {}) { } } @@ -103,7 +205,7 @@ export class ArnPrincipal extends PrincipalBase { } public get policyFragment(): PrincipalPolicyFragment { - return new PrincipalPolicyFragment({ AWS: [ this.arn ] }); + return new PrincipalPolicyFragment({ AWS: [this.arn] }); } public toString() { @@ -200,7 +302,7 @@ export class CanonicalUserPrincipal extends PrincipalBase { } public get policyFragment(): PrincipalPolicyFragment { - return new PrincipalPolicyFragment({ CanonicalUser: [ this.canonicalUserId ] }); + return new PrincipalPolicyFragment({ CanonicalUser: [this.canonicalUserId] }); } public toString() { @@ -213,7 +315,11 @@ export class FederatedPrincipal extends PrincipalBase { constructor( public readonly federated: string, - public readonly conditions: {[key: string]: any}, + /** + * 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). + */ + public readonly conditions: Conditions, assumeRoleAction: string = 'sts:AssumeRole') { super(); @@ -221,7 +327,7 @@ export class FederatedPrincipal extends PrincipalBase { } public get policyFragment(): PrincipalPolicyFragment { - return new PrincipalPolicyFragment({ Federated: [ this.federated ] }, this.conditions); + return new PrincipalPolicyFragment({ Federated: [this.federated] }, this.conditions); } public toString() { @@ -293,7 +399,7 @@ export class CompositePrincipal extends PrincipalBase { } public get policyFragment(): PrincipalPolicyFragment { - const principalJson: { [key: string]: string[] } = { }; + const principalJson: { [key: string]: string[] } = {}; for (const p of this.principals) { mergePrincipal(principalJson, p.policyFragment.principalJson); @@ -324,6 +430,11 @@ class StackDependentToken implements cdk.IResolvable { return cdk.Token.asString(this); } + /** + * JSON-ify the token + * + * Used when JSON.stringify() is called + */ public toJSON() { return ''; } @@ -349,6 +460,11 @@ class ServicePrincipalToken implements cdk.IResolvable { }); } + /** + * JSON-ify the token + * + * Used when JSON.stringify() is called + */ public toJSON() { return `<${this.service}>`; } diff --git a/packages/@aws-cdk/aws-iam/package.json b/packages/@aws-cdk/aws-iam/package.json index 78a6925e648cc..3cba4c68b787f 100644 --- a/packages/@aws-cdk/aws-iam/package.json +++ b/packages/@aws-cdk/aws-iam/package.json @@ -124,7 +124,6 @@ "docs-public-apis:@aws-cdk/aws-iam.CompositePrincipal", "docs-public-apis:@aws-cdk/aws-iam.CompositePrincipal.addPrincipals", "docs-public-apis:@aws-cdk/aws-iam.FederatedPrincipal", - "docs-public-apis:@aws-cdk/aws-iam.FederatedPrincipal.conditions", "docs-public-apis:@aws-cdk/aws-iam.FederatedPrincipal.federated", "docs-public-apis:@aws-cdk/aws-iam.Group", "docs-public-apis:@aws-cdk/aws-iam.LazyRole.roleId", @@ -146,8 +145,6 @@ "docs-public-apis:@aws-cdk/aws-iam.PolicyStatement.addResources", "docs-public-apis:@aws-cdk/aws-iam.PolicyStatement.toStatementJson", "docs-public-apis:@aws-cdk/aws-iam.PolicyStatement.toString", - "docs-public-apis:@aws-cdk/aws-iam.PrincipalBase.toJSON", - "docs-public-apis:@aws-cdk/aws-iam.PrincipalPolicyFragment.conditions", "docs-public-apis:@aws-cdk/aws-iam.PrincipalPolicyFragment.principalJson", "docs-public-apis:@aws-cdk/aws-iam.ServicePrincipal.service", "props-default-doc:@aws-cdk/aws-iam.GrantOnPrincipalOptions.scope", 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 9a926d33f201b..72203036df9ee 100644 --- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts +++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts @@ -1,8 +1,8 @@ import '@aws-cdk/assert/jest'; import { Lazy, Stack, Token } from '@aws-cdk/core'; import { - Anyone, AnyPrincipal, ArnPrincipal, CanonicalUserPrincipal, CompositePrincipal, Effect, FederatedPrincipal, - IPrincipal, PolicyDocument, PolicyStatement, PrincipalPolicyFragment, ServicePrincipal + AccountPrincipal, Anyone, AnyPrincipal, ArnPrincipal, CanonicalUserPrincipal, CompositePrincipal, Effect, + FederatedPrincipal, IPrincipal, PolicyDocument, PolicyStatement, PrincipalPolicyFragment, ServicePrincipal } from '../lib'; describe('IAM polocy document', () => { @@ -427,7 +427,7 @@ describe('IAM polocy document', () => { 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', { condition: 1 }))) + expect(() => p.addPrincipals(new FederatedPrincipal('federated', { StringEquals: { 'aws:some-key': 'some-value' } }))) .toThrow(/Components of a CompositePrincipal must not have conditions/); }); @@ -449,11 +449,11 @@ describe('IAM polocy document', () => { // add via policy statement statement.addArnPrincipal('aws-principal-3'); - statement.addCondition('cond2', { boom: 123 }); + statement.addCondition('cond2', { boom: '123' }); expect(stack.resolve(statement.toStatementJson())).toEqual({ Condition: { - cond2: { boom: 123 } + cond2: { boom: '123' } }, Effect: 'Allow', Principal: { @@ -473,6 +473,94 @@ describe('IAM polocy document', () => { }); }); + describe('PrincipalWithConditions can be used to add a principal with conditions', () => { + test('includes conditions from both the wrapped principal and the wrapper', () => { + const stack = new Stack(); + const principalOpts = { + conditions: { + BinaryEquals: { + 'principal-key': 'SGV5LCBmcmllbmQh', + }, + }, + }; + const p = new ServicePrincipal('s3.amazonaws.com', principalOpts) + .withConditions({ StringEquals: { 'wrapper-key': ['val-1', 'val-2'] } }); + const statement = new PolicyStatement(); + statement.addPrincipals(p); + expect(stack.resolve(statement.toStatementJson())).toEqual({ + Condition: { + BinaryEquals: { 'principal-key': 'SGV5LCBmcmllbmQh' }, + StringEquals: { 'wrapper-key': ['val-1', 'val-2'] }, + }, + Effect: 'Allow', + Principal: { + Service: 's3.amazonaws.com', + }, + }); + }); + + test('conditions from addCondition are merged with those from the principal', () => { + const stack = new Stack(); + const p = new AccountPrincipal('012345678900').withConditions({ StringEquals: { key: 'val' } }); + const statement = new PolicyStatement(); + statement.addPrincipals(p); + statement.addCondition('Null', { 'banned-key': 'true' }); + expect(stack.resolve(statement.toStatementJson())).toEqual({ + Effect: 'Allow', + Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::012345678900:root']] } }, + Condition: { StringEquals: { key: 'val' }, Null: { 'banned-key': 'true' } }, + }); + }); + + test('adding conditions via `withConditions` does not affect the original principal', () => { + const originalPrincipal = new ArnPrincipal('iam:an:arn'); + const principalWithConditions = originalPrincipal.withConditions({ StringEquals: { key: 'val' } }); + expect(originalPrincipal.policyFragment.conditions).toEqual({}); + expect(principalWithConditions.policyFragment.conditions).toEqual({ StringEquals: { key: 'val' } }); + }); + + test('conditions are merged when operators conflict', () => { + const p = new FederatedPrincipal('fed', { + OperatorOne: { 'fed-key': 'fed-val' }, + OperatorTwo: { 'fed-key': 'fed-val' }, + OperatorThree: { 'fed-key': 'fed-val' }, + }).withConditions({ + OperatorTwo: { 'with-key': 'with-val' }, + OperatorThree: { 'with-key': 'with-val' }, + }); + const statement = new PolicyStatement(); + statement.addCondition('OperatorThree', { 'add-key': 'add-val' }); + statement.addPrincipals(p); + expect(statement.toStatementJson()).toEqual({ + Effect: 'Allow', + Principal: { Federated: 'fed' }, + Condition: { + OperatorOne: { 'fed-key': 'fed-val' }, + OperatorTwo: { 'fed-key': 'fed-val', 'with-key': 'with-val' }, + OperatorThree: { 'fed-key': 'fed-val', 'with-key': 'with-val', 'add-key': 'add-val' }, + } + }); + }); + + test('values passed to `withConditions` overwrite values from the wrapped principal ' + + 'when keys conflict within an operator', () => { + const p = new FederatedPrincipal('fed', { + Operator: { key: 'p-val' }, + }).withConditions({ + Operator: { key: 'with-val' }, + }); + const statement = new PolicyStatement(); + statement.addPrincipals(p); + expect(statement.toStatementJson()).toEqual({ + Effect: 'Allow', + Principal: { Federated: 'fed' }, + Condition: { + Operator: { key: 'with-val' }, + }, + }); + }); + }); + describe('duplicate statements', () => { test('without tokens', () => { diff --git a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts index 24cbbdf2e2771..6cc6d067b6416 100644 --- a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts +++ b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts @@ -173,7 +173,7 @@ describe('IAM policy statement', () => { }).toThrow(/Fields must be either a string or an array of strings/); }); - test('throws error with field data being object', () => { + test('throws error with field data being array of non-strings', () => { expect(() => { PolicyStatement.fromJson({ Action: [{}]