Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define IAM Permissions Boundary for Project #7144

Merged
merged 41 commits into from
Jun 3, 2021

Conversation

edwardfoyle
Copy link
Contributor

@edwardfoyle edwardfoyle commented Apr 20, 2021

Description of changes

Adds a new advanced project configuration option to specify a permissions boundary that will be applied to all IAM roles in the project. This is broken into 3 components:

  1. add a permissions boundary state manager to amplify-cli-core
  2. add a prompt to amplify configure project that writes the boundary to the state manager
  3. adds a permissions boundary template modifier to the pre-push cloudformation transformer that reads the value from the state manager and applies it to cfn templates

Issue #, if available

#4618

Description of how you validated changes

Manually validated as well as unit tested and e2e tested

Checklist

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

@edwardfoyle edwardfoyle requested a review from a team as a code owner April 20, 2021 23:25
@edwardfoyle edwardfoyle changed the title Param bound Define IAM Permission Boundary for Project Apr 20, 2021
@lgtm-com
Copy link

lgtm-com bot commented Apr 20, 2021

This pull request introduces 2 alerts when merging 838f7dc into c8069bd - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

cjihrig
cjihrig previously approved these changes Apr 22, 2021
@edwardfoyle edwardfoyle marked this pull request as draft April 22, 2021 17:33
@codecov-commenter
Copy link

codecov-commenter commented May 5, 2021

Codecov Report

Merging #7144 (fd65741) into master (3dbb3bf) will increase coverage by 0.11%.
The diff coverage is 79.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7144      +/-   ##
==========================================
+ Coverage   52.72%   52.83%   +0.11%     
==========================================
  Files         513      517       +4     
  Lines       25914    26038     +124     
  Branches     5058     5086      +28     
==========================================
+ Hits        13662    13758      +96     
- Misses      11286    11319      +33     
+ Partials      966      961       -5     
Impacted Files Coverage Δ
packages/amplify-cli/src/commands/env.ts 80.55% <ø> (ø)
...y-provider-awscloudformation/src/push-resources.ts 10.33% <0.00%> (-0.03%) ⬇️
packages/amplify-util-mock/src/func/index.ts 94.11% <0.00%> (ø)
...rovider-awscloudformation/src/aws-utils/aws-iam.ts 27.27% <27.27%> (ø)
...extensions/amplify-helpers/get-provider-plugins.ts 47.05% <33.33%> (-36.28%) ⬇️
...ackages/amplify-cli/src/init-steps/s9-onSuccess.ts 15.00% <50.00%> (+0.85%) ⬆️
...lify-provider-awscloudformation/src/initializer.ts 51.90% <85.71%> (ø)
...n/src/permissions-boundary/permissions-boundary.ts 91.22% <91.22%> (ø)
...s/amplify-cli-core/src/permissionsBoundaryState.ts 96.00% <96.00%> (ø)
packages/amplify-cli-core/src/index.ts 95.65% <100.00%> (+0.19%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c9c070...fd65741. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented May 5, 2021

This pull request introduces 2 alerts when merging f5fa7e2 into afbaa08 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

import { ResourceModifier } from '../pre-push-cfn-modifier';

export const iamRolePermissionBoundaryModifier: ResourceModifier<Role> = async resource => {
if (resource?.Properties?.PermissionsBoundary) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also check for type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the transformer isn't meant to be a CFN validator. In valid CFN it can only be a string so at this point I think we can assume if it exists at all we should leave it alone

packages/amplify-cli-core/src/permissionBoundaryState.ts Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented May 7, 2021

This pull request introduces 1 alert when merging 9ff685b into abb64f6 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

Copy link
Contributor

@yuth yuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 after changing global variable to module scoped one

@lgtm-com
Copy link

lgtm-com bot commented May 10, 2021

This pull request introduces 1 alert when merging ccfd139 into d28dd1c - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 20, 2021

This pull request introduces 1 alert when merging d0ad684 into 572ddbd - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@edwardfoyle edwardfoyle changed the title Define IAM Permission Boundary for Project Define IAM Permissions Boundary for Project May 21, 2021
@yuth yuth merged commit acf031b into aws-amplify:master Jun 3, 2021
edwardfoyle added a commit that referenced this pull request Jun 3, 2021
jhockett pushed a commit that referenced this pull request Jun 3, 2021
@edwardfoyle edwardfoyle mentioned this pull request Jun 10, 2021
4 tasks
@github-actions github-actions bot added the referenced-in-release Issues referenced in a published release changelog label Jun 15, 2021
@github-actions
Copy link

👋 Hi, this pull request was referenced in the v5.0.0 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v5.0.0.

cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021
…ws-amplify#7144)

Adds a new advanced project configuration option to specify a permissions boundary that will be applied to all IAM roles in the project. This is broken into 3 components:
1. add a permissions boundary state manager to amplify-cli-core
2. add a prompt to amplify configure project that writes the boundary to the state manager
3. adds a permissions boundary template modifier to the pre-push cloudformation transformer that reads the value from the state manager and applies it to cfn templates
cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
referenced-in-release Issues referenced in a published release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants