From 8ed929eef80b28822f4a3a4a54bf428a7a789073 Mon Sep 17 00:00:00 2001 From: Marco Frattallone Date: Thu, 28 Dec 2023 12:04:52 +0000 Subject: [PATCH 1/6] feat(aws-iam): validates roleName --- packages/aws-cdk-lib/aws-iam/lib/role.ts | 4 +++ .../aws-cdk-lib/aws-iam/test/role.test.ts | 35 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/packages/aws-cdk-lib/aws-iam/lib/role.ts b/packages/aws-cdk-lib/aws-iam/lib/role.ts index 5ed1302543d47..2e048bda43207 100644 --- a/packages/aws-cdk-lib/aws-iam/lib/role.ts +++ b/packages/aws-cdk-lib/aws-iam/lib/role.ts @@ -413,6 +413,10 @@ export class Role extends Resource implements IRole { physicalName: props.roleName, }); + if (props.roleName && !/^[\w_+=,.@-]{1,64}$/.test(props.roleName)) { + throw new Error('Invalid roleName. The name must be a string of characters consisting of upper and lowercase alphanumeric characters with no spaces. You can also include any of the following characters: _+=,.@-. The role name must be unique within the account. Role names are not distinguished by case. For example, you cannot create roles named both "Role1" and "role1". Length must be between 1 and 64 characters.'); + } + const externalIds = props.externalIds || []; if (props.externalId) { externalIds.push(props.externalId); diff --git a/packages/aws-cdk-lib/aws-iam/test/role.test.ts b/packages/aws-cdk-lib/aws-iam/test/role.test.ts index 5d239254d5bbc..10d61e84cb932 100644 --- a/packages/aws-cdk-lib/aws-iam/test/role.test.ts +++ b/packages/aws-cdk-lib/aws-iam/test/role.test.ts @@ -1325,3 +1325,38 @@ test('cross-env role ARNs include path', () => { }, }); }); + +test('throws with empty role name', () => { + const app = new App(); + const stack = new Stack(app, 'MyStack'); + expect(() => { + new Role(stack, 'Test', { + assumedBy: new ServicePrincipal('sns.amazonaws.com'), + roleName: '', + }); + }).toThrow('/Invalid roleName/'); +}); + +test('throws with name over 64 chars', () => { + const app = new App(); + const stack = new Stack(app, 'MyStack'); + const longName = 'a'.repeat(65); + + expect(() => { + new Role(stack, 'Test', { + assumedBy: new ServicePrincipal('sns.amazonaws.com'), + roleName: longName, + }); + }).toThrow('Invalid roleName'); +}); + +test('throws with invalid chars', () => { + const app = new App(); + const stack = new Stack(app, 'MyStack'); + expect(() => { + new Role(stack, 'Test', { + assumedBy: new ServicePrincipal('sns.amazonaws.com'), + roleName: 'invalid!name', + }); + }).toThrow('Invalid roleName'); +}); \ No newline at end of file From 594edd08f82518e16b6d50a61f17f10bf0a80ffb Mon Sep 17 00:00:00 2001 From: Marco Frattallone Date: Fri, 29 Dec 2023 13:33:49 +0000 Subject: [PATCH 2/6] remove unnecessary characher --- packages/aws-cdk-lib/aws-iam/lib/role.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/aws-iam/lib/role.ts b/packages/aws-cdk-lib/aws-iam/lib/role.ts index 2e048bda43207..ba63efbdede63 100644 --- a/packages/aws-cdk-lib/aws-iam/lib/role.ts +++ b/packages/aws-cdk-lib/aws-iam/lib/role.ts @@ -413,7 +413,7 @@ export class Role extends Resource implements IRole { physicalName: props.roleName, }); - if (props.roleName && !/^[\w_+=,.@-]{1,64}$/.test(props.roleName)) { + if (props.roleName && !/^[\w+=,.@-]{1,64}$/.test(props.roleName)) { throw new Error('Invalid roleName. The name must be a string of characters consisting of upper and lowercase alphanumeric characters with no spaces. You can also include any of the following characters: _+=,.@-. The role name must be unique within the account. Role names are not distinguished by case. For example, you cannot create roles named both "Role1" and "role1". Length must be between 1 and 64 characters.'); } From 791469db6b99ce0ab1ff5b39e63012661a7b7361 Mon Sep 17 00:00:00 2001 From: Marco Frattallone Date: Tue, 2 Jan 2024 11:32:08 +0100 Subject: [PATCH 3/6] feat(iam): manage Token --- packages/aws-cdk-lib/aws-iam/lib/role.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/aws-iam/lib/role.ts b/packages/aws-cdk-lib/aws-iam/lib/role.ts index ba63efbdede63..ff58f29849b53 100644 --- a/packages/aws-cdk-lib/aws-iam/lib/role.ts +++ b/packages/aws-cdk-lib/aws-iam/lib/role.ts @@ -413,7 +413,7 @@ export class Role extends Resource implements IRole { physicalName: props.roleName, }); - if (props.roleName && !/^[\w+=,.@-]{1,64}$/.test(props.roleName)) { + if (props.roleName && !Token.isUnresolved(props.roleName) && !/^[\w+=,.@-]{1,64}$/.test(props.roleName)) { throw new Error('Invalid roleName. The name must be a string of characters consisting of upper and lowercase alphanumeric characters with no spaces. You can also include any of the following characters: _+=,.@-. The role name must be unique within the account. Role names are not distinguished by case. For example, you cannot create roles named both "Role1" and "role1". Length must be between 1 and 64 characters.'); } From 2fd3b9c4fd7fdc894d57adf9c6614e2bbd4ebd4e Mon Sep 17 00:00:00 2001 From: Marco Frattallone Date: Tue, 2 Jan 2024 11:48:52 +0100 Subject: [PATCH 4/6] feat(iam): update error messages with only check performed --- packages/aws-cdk-lib/aws-iam/lib/role.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/aws-iam/lib/role.ts b/packages/aws-cdk-lib/aws-iam/lib/role.ts index ff58f29849b53..b8937d7b951aa 100644 --- a/packages/aws-cdk-lib/aws-iam/lib/role.ts +++ b/packages/aws-cdk-lib/aws-iam/lib/role.ts @@ -414,7 +414,7 @@ export class Role extends Resource implements IRole { }); if (props.roleName && !Token.isUnresolved(props.roleName) && !/^[\w+=,.@-]{1,64}$/.test(props.roleName)) { - throw new Error('Invalid roleName. The name must be a string of characters consisting of upper and lowercase alphanumeric characters with no spaces. You can also include any of the following characters: _+=,.@-. The role name must be unique within the account. Role names are not distinguished by case. For example, you cannot create roles named both "Role1" and "role1". Length must be between 1 and 64 characters.'); + throw new Error('Invalid roleName. The name must be a string of characters consisting of upper and lowercase alphanumeric characters with no spaces. You can also include any of the following characters: _+=,.@-. Length must be between 1 and 64 characters.'); } const externalIds = props.externalIds || []; From 1896e287ad06a98749aa5aecf1d3f5b47cc532e6 Mon Sep 17 00:00:00 2001 From: Marco Frattallone Date: Tue, 2 Jan 2024 11:50:00 +0100 Subject: [PATCH 5/6] feat(iam): test rolenames under 65 chars and with invalid chars --- .../aws-cdk-lib/aws-iam/test/role.test.ts | 36 ++++++++++++++----- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/packages/aws-cdk-lib/aws-iam/test/role.test.ts b/packages/aws-cdk-lib/aws-iam/test/role.test.ts index 10d61e84cb932..5c0d8ee499489 100644 --- a/packages/aws-cdk-lib/aws-iam/test/role.test.ts +++ b/packages/aws-cdk-lib/aws-iam/test/role.test.ts @@ -1326,15 +1326,17 @@ test('cross-env role ARNs include path', () => { }); }); -test('throws with empty role name', () => { +test('doesn\'t throw with name of 64 chars', () => { const app = new App(); const stack = new Stack(app, 'MyStack'); + const valdName = 'a'.repeat(64); + expect(() => { new Role(stack, 'Test', { assumedBy: new ServicePrincipal('sns.amazonaws.com'), - roleName: '', + roleName: valdName, }); - }).toThrow('/Invalid roleName/'); + }).not.toThrow('Invalid roleName'); }); test('throws with name over 64 chars', () => { @@ -1350,13 +1352,29 @@ test('throws with name over 64 chars', () => { }).toThrow('Invalid roleName'); }); -test('throws with invalid chars', () => { +describe('roleName validation', () => { const app = new App(); const stack = new Stack(app, 'MyStack'); - expect(() => { - new Role(stack, 'Test', { - assumedBy: new ServicePrincipal('sns.amazonaws.com'), - roleName: 'invalid!name', + const invalidChars = '!#$%^&*()'; + + it('rejects names with spaces', () => { + expect(() => { + new Role(stack, 'test spaces', { + assumedBy: new ServicePrincipal('sns.amazonaws.com'), + roleName: 'invalid name', + }); + }).toThrow('Invalid roleName'); + }); + + invalidChars.split('').forEach(char => { + it(`rejects name with ${char}`, () => { + expect(() => { + new Role(stack, `test ${char}`, { + assumedBy: new ServicePrincipal('sns.amazonaws.com'), + roleName: `invalid${char}`, + }); + }).toThrow('Invalid roleName'); }); - }).toThrow('Invalid roleName'); + }); + }); \ No newline at end of file From 6290df586ef1686790f00e1bbd2f216de81841d3 Mon Sep 17 00:00:00 2001 From: Marco Frattallone Date: Tue, 2 Jan 2024 13:09:32 +0100 Subject: [PATCH 6/6] feat(iam): test roleName as a Token --- .../aws-cdk-lib/aws-iam/test/role.test.ts | 34 +++++++++++++++++-- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/packages/aws-cdk-lib/aws-iam/test/role.test.ts b/packages/aws-cdk-lib/aws-iam/test/role.test.ts index 5c0d8ee499489..88ee4f2d6cbbc 100644 --- a/packages/aws-cdk-lib/aws-iam/test/role.test.ts +++ b/packages/aws-cdk-lib/aws-iam/test/role.test.ts @@ -1,7 +1,7 @@ import { testDeprecated } from '@aws-cdk/cdk-build-tools'; import { Construct } from 'constructs'; import { Template, Match, Annotations } from '../../assertions'; -import { Duration, Stack, App, CfnResource, RemovalPolicy, Lazy, Stage, DefaultStackSynthesizer, CliCredentialsStackSynthesizer, PERMISSIONS_BOUNDARY_CONTEXT_KEY, PermissionsBoundary } from '../../core'; +import { Duration, Stack, App, CfnResource, RemovalPolicy, Lazy, Stage, DefaultStackSynthesizer, CliCredentialsStackSynthesizer, PERMISSIONS_BOUNDARY_CONTEXT_KEY, PermissionsBoundary, Token } from '../../core'; import { AnyPrincipal, ArnPrincipal, CompositePrincipal, FederatedPrincipal, ManagedPolicy, PolicyStatement, Role, ServicePrincipal, User, Policy, PolicyDocument, Effect } from '../lib'; describe('isRole() returns', () => { @@ -1326,7 +1326,7 @@ test('cross-env role ARNs include path', () => { }); }); -test('doesn\'t throw with name of 64 chars', () => { +test('doesn\'t throw with roleName of 64 chars', () => { const app = new App(); const stack = new Stack(app, 'MyStack'); const valdName = 'a'.repeat(64); @@ -1339,7 +1339,7 @@ test('doesn\'t throw with name of 64 chars', () => { }).not.toThrow('Invalid roleName'); }); -test('throws with name over 64 chars', () => { +test('throws with roleName over 64 chars', () => { const app = new App(); const stack = new Stack(app, 'MyStack'); const longName = 'a'.repeat(65); @@ -1377,4 +1377,32 @@ describe('roleName validation', () => { }); }); +}); + +test('roleName validation with Tokens', () =>{ + const app = new App(); + const stack = new Stack(app, 'MyStack'); + const token = Lazy.string({ produce: () => 'token' }); + + // Mock isUnresolved to return false + jest.spyOn(Token, 'isUnresolved').mockReturnValue(false); + + expect(() => { + new Role(stack, 'Valid', { + assumedBy: new ServicePrincipal('sns.amazonaws.com'), + roleName: token, + }); + }).toThrow('Invalid roleName'); + + // Mock isUnresolved to return true + jest.spyOn(Token, 'isUnresolved').mockReturnValue(true); + + expect(() => { + new Role(stack, 'Invalid', { + assumedBy: new ServicePrincipal('sns.amazonaws.com'), + roleName: token, + }); + }).not.toThrow('Invalid roleName'); + + jest.clearAllMocks(); }); \ No newline at end of file