feat(aws-cdk): be aware of IAM and SecurityGroup changes #1240
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add awareness of potentially risky permissions changes to the cfnspec and diff tool, and surface those changes in an easily digestible format. This is an initial commit. Feature TODO: - [ ] Recognize AWS::Lambda::Permission objects - [ ] Recognize managed policies - [ ] Add support for SecurityGroup rules - [ ] Reverse engineer construct paths from logical IDs - [ ] Apply same logic to format inline diffs for IAM policy property updates. - [ ] Automatically diff and confirm statement additions on every 'deploy'. Quality TODO: - [ ] property tests on statement equality, parsing - [ ] tests on 'uncfn' routines - [ ] Replace CLI table library in aws-cdk to be the same as this one (supports newlines in cells and generally looks better). Fixes #978.
|
Trivial example: |
Before, the json file would be loaded upon initialization of the library, but the library would also be loaded in the build tool. It's only because of some crazy lazy evaluation optimization done by NodeJS that that didn't blow up before, but due to code changes it does blow up now.
… into huijbers/iam-aware-diffs
|
Current status: |
|
Status so far |
|
eladb
reviewed
Dec 5, 2018
There was a problem hiding this comment.
-
Please add a statement at the bottom of the security diff indicating this is an experimental feature and that users should consult the JSON diff as well to ensure no unexpected broadening of permissions happens. We need to let this bake for a bit before we let people trust this blindly.
-
Might have missed that, but I expected
cdk diffto always print any security related changes (not only broadening).
packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk/bin/cdk.ts
Outdated
| @@ -53,6 +54,7 @@ async function parseCommandLineArguments() { | |||
| .command('bootstrap [ENVIRONMENTS..]', 'Deploys the CDK toolkit stack into an AWS environment', yargs => yargs | |||
| .option('toolkit-stack-name', { type: 'string', desc: 'the name of the CDK toolkit stack' })) | |||
| .command('deploy [STACKS..]', 'Deploys the stack(s) named STACKS into your AWS account', yargs => yargs | |||
| .option('prompt', { type: 'string', choices: ['never', 'on-add'], default: 'on-add', desc: 'prompt if security-sensitive changes are about to be deployed' }) | |||
There was a problem hiding this comment.
I'd rename this option to safety or security and I envision three options:
ignore-- like "never" - basically ignores any security changesrequire-approval- a future mode, which should be the default I think, where some form of an approval hash can be stored incdk.json. This will allow CI/CD to protect against unsolicited changes instead of ignoring everythingalways-prompt- similar to the currenton-addwhere it will always prompt even if there is an approval in cdk.json
There was a problem hiding this comment.
I think there are two axes: one the one hand, what the change are that should trigger the approval workflow. On the other hand, how the approval is given (viz. interactive vs CI/CD workflows).
This flag was intended to select along the first axis--though I agree that the name was poorly chosen.
There was a problem hiding this comment.
always-prompt never makes sense in CI/CD, so I don't see why we should offer that option.
It does. It's |
|
|
eladb
approved these changes
Dec 9, 2018
| @@ -23,18 +23,18 @@ | |||
| }, | |||
| "license": "Apache-2.0", | |||
| "dependencies": { | |||
| "@aws-cdk/cfnspec": "^0.18.1", | |||
There was a problem hiding this comment.
I don't know why this even shows up. But because 0.19.0 has been released on master in the mean time.
| formatter.formatIamChanges(templateDiff.iamChanges); | ||
| formatter.formatSecurityGroupChanges(templateDiff.securityGroupChanges); | ||
|
|
||
| formatter.warning(`(This feature is experimental -- it might not be complete. See http://bit.ly/cdk-2EhF7Np)`); |
There was a problem hiding this comment.
TBH, I am not even sure this is just because this is an experimental feature. There could always be security related changes at the application level that we will never know about. So maybe something like this:
WARNING: there may be security-related changes which do not appear in this list. See <URL> for details.
Maybe we should ask our security folks what would be the best wording here?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add awareness of potentially risky permissions changes to the
cfnspec and diff tool, and surface those changes in an easily
digestible format.
This PR is still a work in progress.
Feature TODO:
[ ] Apply same logic to format inline diffs for IAM policyproperty updates.
'deploy'.
Quality TODO:
this one (supports newlines in cells and generally looks better).
Fixes #978.
Pull Request Checklist
Please check all boxes (including N/A items)
Testing
tests
manually executed (paste output to the PR description)
(currently maintained in a private repo).
Documentation
Title and description
fix(module): <title>bug fix (patch)feat(module): <title>feature/capability (minor)chore(module): <title>won't appear in changelogbuild(module): <title>won't appear in changelogBREAKING CHANGE: <describe exactly what changed and how to achieve similar behavior + link to documentation/gist/issue if more details are required>Fixes #xxxorCloses #xxxBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.