feat(iam): validate role path at build time by i05nagai · Pull Request #16165 · aws/aws-cdk

i05nagai · 2021-08-22T14:20:15Z

Role paths can be validated at build time.

According to the API document, u007F, DELETE special char, is valid. However, the creation with a role path /\u007F/ fails due to validation failure. I don't see any use case for the special char, so I ignored the discrepancy.

closes #13747

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

0x007F is valid in the API document. However, role creation failed. Not sure if the document is not up-to-date or the API has a bug.

gitpod-io · 2021-08-22T14:20:17Z

TheRealAmazonKendra

Thank you for your contribution! Just a few changes requested. Additionally, I want to look into the issue you brought up in your description about the character /\u007F/.

TheRealAmazonKendra · 2022-04-27T04:31:47Z

packages/@aws-cdk/aws-iam/lib/role.ts

+  const validRolePath = /^(\/|\/[\u0021-\u007F]+\/)$/;
+
+  if (path.length == 0 || path.length > 512) {
+    throw new Error(`role path is set to ${path}, but the length must be >= 1 and <= 512.`);


Suggested change

throw new Error(`role path is set to ${path}, but the length must be >= 1 and <= 512.`);

throw new Error(`Role path must be between 1 and 512 characters. The provided role path is ${path.length} characters.`);

Fixed in 8d9afeb

TheRealAmazonKendra · 2022-04-27T04:36:23Z

packages/@aws-cdk/aws-iam/lib/role.ts

+  if (path.length == 0 || path.length > 512) {
+    throw new Error(`role path is set to ${path}, but the length must be >= 1 and <= 512.`);
+  } else if (!validRolePath.test(path)) {
+    throw new Error(`role path is set to ${path}, but must match ${validRolePath.toString()}.`);


I'm not totally sure what the best error message is here but providing the regex itself here is not very user friendly. Perhaps you can describe the pattern needed instead of just using the regex. also, please capitalize the beginning of the sentence in the error message.

I've added a bit more explanation. Updated in ea3a324

TheRealAmazonKendra · 2022-04-27T04:38:58Z

packages/@aws-cdk/aws-iam/test/role.test.ts

+      });
+    });
+
+    test('must be between 1 chars and 512 chars', () => {


This may just be my preference here, but I'd prefer to only see one test case per test.

Do you mean 1 expect statement in a test? Fixed in ea3a324

TheRealAmazonKendra · 2022-04-27T04:39:50Z

packages/@aws-cdk/aws-iam/test/role.test.ts

+      const assumedBy = new ServicePrincipal('bla');
+
+      new Role(stack, 'MyRole1', { assumedBy, path: '/' });
+      new Role(stack, 'MyRole2', { assumedBy, path: '/p/a/t/h/' });


It doesn't look like either of these roles are being used in this test.

They are valid cases, When exceptions are raised, the test framework will catch them and make them fail tests. I made it explicit with expect statement in ea3a324

TheRealAmazonKendra · 2022-04-27T04:40:33Z

packages/@aws-cdk/aws-iam/test/role.test.ts

+        .toThrow(expected('/' + Array(512).join('a') + '/'));
+    });
+
+    test('must match regular expression', () => {


Same comment as the test above. One test case per test, please.

Fixed in ea3a324

TheRealAmazonKendra · 2022-04-27T04:47:43Z

packages/@aws-cdk/aws-iam/lib/role.ts

+    return;
+  }
+
+  const validRolePath = /^(\/|\/[\u0021-\u007F]+\/)$/;


I'm hesitant to allow validation of a character that will throw an error. I've put a ticket in with the IAM team to double check this. Can you provide the specific validation error you get with /\u007F/ to help me look into this?

That's great. I cannot investigate further on my side. A simple example with CDK is below. I've confirmed the same behavior with python client (boto3). I believe that the issue exists on IAM side.

import * as cdk from 'aws-cdk-lib'; import { aws_iam as iam } from 'aws-cdk-lib'; export class Example1Stack extends cdk.Stack { constructor(scope: cdk.App, id: string, props: cdk.StackProps) { super(scope, id, props); const role = new iam.Role(this, "role", { assumedBy: new iam.ServicePrincipal("lambda.amazonaws.com"), path: "/\u007F/", }) } }

Error messages

The stack named Example1Stack failed to deploy: UPDATE_ROLLBACK_COMPLETE: The specified value for path is invalid. It must begin and end with / and contain only alphanumeric characters and/or / characters. (Service: AmazonIdentityManagement; Status Code: 400; Error Code: ValidationError; Request ID: 08690aa1-f05c-4830-964d-acd4b2a84532; Proxy: null)

As far as I checked, other control charcters (e.g. /\u0x7E/) work and other patterns with \u0x007F (e. g. /aaaa\u0x007Faaaaa/) fail.

Pull request has been modified.

i05nagai · 2022-05-01T11:59:54Z

Thanks for the review! Please have a look again.

mergify · 2022-05-13T01:46:01Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-05-13T02:31:11Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 23600f2
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-05-13T02:31:25Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Role paths can be validated at build time. According to the [API document](https://docs.aws.amazon.com/IAM/latest/APIReference/API_Role.html), `u007F`, DELETE special char, is valid. However, the creation with a role path `/\u007F/` fails due to validation failure. I don't see any use case for the special char, so I ignored the discrepancy. closes aws#13747 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat: validate role path

1e3cd08

0x007F is valid in the API document. However, role creation failed. Not sure if the document is not up-to-date or the API has a bug.

peterwoodworth added p2 effort/small Small work item – less than a day of effort @aws-cdk/aws-iam Related to AWS Identity and Access Management labels Sep 1, 2021

RomainMuller force-pushed the master branch from 924c117 to ebfd5f2 Compare September 29, 2021 13:07

Merge branch 'master' into role-path-validation

1105f8a

rix0rrr added feature-request A feature should be added or improved. p2 and removed p2 @aws-cdk/aws-iam Related to AWS Identity and Access Management effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Mar 4, 2022

TheRealAmazonKendra previously requested changes Apr 27, 2022

View reviewed changes

TheRealAmazonKendra added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Apr 27, 2022

TheRealAmazonKendra changed the title ~~feat(aws-iam): validate role path~~ feat(iam): validate role path at build time Apr 27, 2022

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Apr 27, 2022

Merge branch 'aws:master' into role-path-validation

6775f13

i05nagai added 2 commits May 1, 2022 13:47

feat(iam): Update to CDK 2.x

8d9afeb

feat(iam): Add tests per a test case

ea3a324

TheRealAmazonKendra approved these changes May 13, 2022

View reviewed changes

Merge branch 'master' into role-path-validation

23600f2

mergify bot merged commit 65a5a46 into aws:master May 13, 2022

	throw new Error(`role path is set to ${path}, but the length must be >= 1 and <= 512.`);
	throw new Error(`Role path must be between 1 and 512 characters. The provided role path is ${path.length} characters.`);

Conversation

i05nagai commented Aug 22, 2021

Uh oh!

gitpod-io bot commented Aug 22, 2021

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i05nagai commented May 1, 2022

Uh oh!

mergify bot commented May 13, 2022

Uh oh!

aws-cdk-automation commented May 13, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented May 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants