fix(iam): role/group/user's path not included in ARN #13258

saltman424 · 2021-02-24T16:40:50Z

Solution to #13156

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

…, and users

gitpod-io · 2021-02-24T16:40:52Z

mergify · 2021-02-24T16:41:29Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

rix0rrr

You need to update tests, and I'm pretty sure the default / would already be included (so now your ARN would look like arn:aws:...:role//rolename

saltman424 · 2021-02-25T14:13:03Z

@rix0rrr I have not had a chance to familiarize myself with the repo's testing structure. Do you have specific recommendations on what I should update and where? Also, I got rid of the 'resourceName' argument to avoid the default slash because as I understand it, just using the 'resource' argument shouldn't include any separator. Is this understanding inaccurate?

rix0rrr · 2021-02-25T14:28:44Z

Do you have specific recommendations on what I should update and where?

There will be role.test.ts, user.test.ts, etc. files in the packages/@aws-cdk/aws-ec2/test directory. That would be a good place to add them.

Now this specific case is somewhat tricky to test, as it only comes into play when objects are being used transparently in a cross-env scenario. So in your unit test, you will need to create an App and two Stacks with different envs, create a Role/Group/User in one stack with a path and use its ARN in the other stack (just create a CfnResource with made-up properties, one of them using the ARN).

Then check that the value you're getting looks like what you expect.

I think there's even 3 cases I'd be interested in:

No path supplied
Path supplied that does NOT end in a /
Path supplied that ends in a /
(What if path STARTS with a /...?)

I wonder if IAM does any path normalization, or whether it rejects values with additional /es, or whether it just pastes them right in. We're going to have to mimic IAMs behavior here.

rix0rrr · 2021-02-25T14:29:45Z

As for what you did with the ARN components:

I liked the original better, where resource = 'role' and resourceName = 'path/to/role'. Seems more logical to me (although it would produce the same string ultimately)

saltman424 · 2021-02-25T15:20:17Z

@rix0rrr I can work on most of that. I just have one question (below).

Based on the documentation, my understanding was that IAM rejects any path that does not both start and end with a slash, so my implementation expects both. If you want CDK to support all of the cases you mentioned, we probably need to add path normalization in CDK as I believe CloudFormation will still require both. It shouldn't be hard, but do you want me to add that as part of this pull request?

I also did the pure resource implementation because it is slightly simpler when given both a beginning and ending slash, as I don't have to conditionally strip a leading slash from the path. But I can add the logic to allow us to use the resourceName argument if you think that'll be more readable.

With all that being said, I may not be able to get to it immediately. But let me know if you would like me to include path normalization or just add better error handling, and I'll let you know when I have had a chance to make these updates.

Pull request has been modified.

rix0rrr · 2021-03-03T15:05:21Z

Based on the documentation, my understanding was that IAM rejects any path that does not both start and end with a slash, so my implementation expects both.

Good call. That is fine then. If we want to make that requirement more flexible, that PR would then be the moment to do that.

rix0rrr

Needs tests

Pull request has been modified.

… from path

…N tests

mergify · 2021-04-22T13:14:59Z