feat(aws-iam): Add permissionsBoundary to IAM Role L2 construct #2919

robertd · 2019-06-19T03:45:03Z

@eladb @rix0rrr @skinny85 Please review... I hope I did it right 😅.

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)

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

eladb · 2019-06-19T07:46:34Z

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

+   * @link https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies_boundaries.html
+   */
+
+  readonly permissionsBoundary?: string;


Shouldn’t that be IPolicy?

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

robertd · 2019-07-03T00:26:39Z

@eladb I'm going to need a bit of help with this PR. I can't figure out why IPolicy change is not working as intended. Any help/pointers is more than appreciated.

robertd · 2019-07-06T19:19:39Z

@eladb @rix0rrr I've taken a stab at this. It needs a review. One can now pass permissions boundary policy in L2 role construct. I apologize for so many forced pushes, but master has been somewhat of a moving target lately :). I'm open for feedback.

Few things to consider. I was not able to figure out how to create setPermissionsBoundary method that would be used after role has been created (i.e. role.setPermissionsBoundary(boundaryPolicy). Perhaps someone can guide me how to accomplish that.

SanderKnape · 2019-08-07T19:48:36Z

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

+   *
+   * @default - No permissions boundaries required.
+   */
+  readonly permissionsBoundary?: IPolicy;


Pretty sure this needs to be a managed policy instead of a policy. AFAIK you can not attach an inline policy as a permissions boundary. Once this proposed change is through (#2974 (comment)), customer managed policies will then also be supported.

I created a PR for that change here #3578 so people can get some eyes on it

SanderKnape · 2019-08-07T19:49:40Z

@robertd are you able to make the required changes to this PR? If not I can have a look at implementing this.

robertd · 2019-08-08T00:14:30Z

Sander, go ahead ah work on PR. I don’t think i’ll have time to look on it (knee surgery).

…

On Wed, Aug 7, 2019 at 6:02 PM Iain Cole ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ***@***.***/aws-iam/lib/role.ts <#2919 (comment)>: > + * A permissions boundary is an advanced feature for using a managed policy + * to set the maximum permissions that an identity-based policy can grant to + * an IAM entity. An entity's permissions boundary allows it to perform only + * the actions that are allowed by both its identity-based policies and its + * permissions boundaries. + * + * You can use an AWS managed policy or a customer managed policy to set the + * boundary for an IAM entity (user or role). That policy limits the maximum + * permissions for the user or role. + * + * @link https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html#cfn-iam-role-permissionsboundary + * @link https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies_boundaries.html + * + * @default - No permissions boundaries required. + */ + readonly permissionsBoundary?: IPolicy; I created a PR for that change here #3578 <#3578> so people can get some eyes on it — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2919?email_source=notifications&email_token=AAAHWN232Y5LT3VCARBRHPDQDNPC7A5CNFSM4HZFTEEKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCA5MLCA#discussion_r311810070>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAHWNYPOZHCGXOBKHGIB5LQDNPC7ANCNFSM4HZFTEEA> .

rix0rrr · 2019-08-08T09:31:54Z

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

@@ -240,6 +262,13 @@ export class Role extends Resource implements IRole {
      }
      return result;
    }
+
+    function _setPermissionsBoundary(policy?: IPolicy) {


I'd rather inline this as a ternary.

rix0rrr · 2019-08-08T09:32:09Z

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

+   *
+   * @attribute
+   */
+  readonly permissionsBoundary?: string;


Why is it a string?

rix0rrr · 2019-08-09T09:00:02Z

Superseded by Sander's PR.

robertd requested a review from a team as a code owner June 19, 2019 03:45

robertd force-pushed the iam-role-add-permissions-boundary branch from 9244fbf to 9c2b7af Compare June 19, 2019 03:46

eladb reviewed Jun 19, 2019

View reviewed changes

robertd force-pushed the iam-role-add-permissions-boundary branch from 9c2b7af to cf1a17f Compare June 20, 2019 01:40

robertd force-pushed the iam-role-add-permissions-boundary branch 4 times, most recently from e4851b9 to 6fe42ee Compare July 6, 2019 02:07

robertd added 2 commits July 6, 2019 13:13

feat(aws-iam): Add permissionsBoundary to IAM Role L2 construct

74212b2

Supply permissions boundary as IPolicy instead of string

9891b25

robertd force-pushed the iam-role-add-permissions-boundary branch from 6fe42ee to 9891b25 Compare July 6, 2019 19:15

robertd mentioned this pull request Jul 9, 2019

Applying permissions boundary to aws-cdk globally #3242

Closed

5 tasks

NGL321 assigned rix0rrr Aug 1, 2019

SanderKnape reviewed Aug 7, 2019

View reviewed changes

Merge branch 'master' into iam-role-add-permissions-boundary

af255d0

rix0rrr reviewed Aug 8, 2019

View reviewed changes

SanderKnape mentioned this pull request Aug 8, 2019

feat(iam): support permissions boundary policy for User and Role #3584

Merged

rix0rrr closed this Aug 9, 2019

robertd deleted the iam-role-add-permissions-boundary branch August 9, 2019 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-iam): Add permissionsBoundary to IAM Role L2 construct #2919

feat(aws-iam): Add permissionsBoundary to IAM Role L2 construct #2919

robertd commented Jun 19, 2019

eladb Jun 19, 2019

robertd commented Jul 3, 2019 •

edited

Loading

robertd commented Jul 6, 2019 •

edited

Loading

SanderKnape Aug 7, 2019

IainCole Aug 8, 2019

SanderKnape commented Aug 7, 2019

robertd commented Aug 8, 2019 via email •

edited

Loading

rix0rrr Aug 8, 2019

rix0rrr Aug 8, 2019

rix0rrr commented Aug 9, 2019

feat(aws-iam): Add permissionsBoundary to IAM Role L2 construct #2919

feat(aws-iam): Add permissionsBoundary to IAM Role L2 construct #2919

Conversation

robertd commented Jun 19, 2019

Pull Request Checklist

eladb Jun 19, 2019

Choose a reason for hiding this comment

robertd commented Jul 3, 2019 • edited Loading

robertd commented Jul 6, 2019 • edited Loading

SanderKnape Aug 7, 2019

Choose a reason for hiding this comment

IainCole Aug 8, 2019

Choose a reason for hiding this comment

SanderKnape commented Aug 7, 2019

robertd commented Aug 8, 2019 via email • edited Loading

rix0rrr Aug 8, 2019

Choose a reason for hiding this comment

rix0rrr Aug 8, 2019

Choose a reason for hiding this comment

rix0rrr commented Aug 9, 2019

robertd commented Jul 3, 2019 •

edited

Loading

robertd commented Jul 6, 2019 •

edited

Loading

robertd commented Aug 8, 2019 via email •

edited

Loading