From cf3826389f3ac95520218ebb4502017b2a3057c8 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 24 Oct 2018 16:41:47 +0200 Subject: [PATCH 01/23] WIP --- .../@aws-cdk/cfnspec/build-tools/scrutiny.ts | 33 +++++++++++++++++++ packages/@aws-cdk/cfnspec/parse.py | 13 ++++++++ .../spec-source/200_Scrutinies_patch.json | 33 +++++++++++++++++++ .../@aws-cdk/cfnspec/test/test.scrutiny.ts | 26 +++++++++++++++ 4 files changed, 105 insertions(+) create mode 100644 packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts create mode 100644 packages/@aws-cdk/cfnspec/parse.py create mode 100644 packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json create mode 100644 packages/@aws-cdk/cfnspec/test/test.scrutiny.ts diff --git a/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts b/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts new file mode 100644 index 0000000000000..5da94a1e2bd91 --- /dev/null +++ b/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts @@ -0,0 +1,33 @@ +import { schema } from '../lib'; +import { ScrutinyType } from '../lib/schema'; + +/** + * Auto-detect common properties to apply scrutiny to by using heuristics + * + * Manually enhancing scrutiny attributes for each property does not scale + * well. Fortunately, the most important ones follow a common naming scheme and + * we tag all of them at once in this way. + * + * If the heuristic scheme gets it wrong in some individual cases, those can be + * fixed using schema patches. + */ +export function detectScrutinyTypes(spec: schema.Specification) { + for (const objectMap of [spec.PropertyTypes, spec.ResourceTypes]) { + for (const typeName of Object.keys(objectMap)) { + const typeSpec = objectMap[typeName]; // Instead of Object.entries(), to help the typechecker + + for (const [propertyName, propertySpec] of Object.entries(typeSpec.Properties || {})) { + if (propertySpec.ScrutinyType !== undefined) { continue; } // Only for unassigned + + const nameContainsPolicy = propertyName.indexOf('Policy') > -1; + const primitiveType = schema.isPrimitiveProperty(propertySpec) && propertySpec.PrimitiveType; + + if (nameContainsPolicy && primitiveType === 'Json') { + const isIamResource = typeName.indexOf('::IAM::') > 1; + propertySpec.ScrutinyType = isIamResource ? ScrutinyType.IdentityPolicy : ScrutinyType.ResourcePolicy; + continue; + } + } + } + } +} diff --git a/packages/@aws-cdk/cfnspec/parse.py b/packages/@aws-cdk/cfnspec/parse.py new file mode 100644 index 0000000000000..960723112fb95 --- /dev/null +++ b/packages/@aws-cdk/cfnspec/parse.py @@ -0,0 +1,13 @@ +import json + +def get_type(propspec): + return propspec.get('PrimitiveType', '???') + +model = json.load(open('spec-source/000_CloudFormationResourceSpecification.json')) + +types = ['PropertyTypes', 'ResourceTypes'] +for type in types: + for name, typespec in model[type].items(): + for propname, propspec in typespec['Properties'].items(): + if 'policy' in propname.lower(): + print('%s:%s -> %s' % (name, propname, get_type(propspec))) diff --git a/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json b/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json new file mode 100644 index 0000000000000..0b9ad762949fc --- /dev/null +++ b/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json @@ -0,0 +1,33 @@ +{ + "ResourceTypes": { + "AWS::SNS::Subscription": { + "patch": { + "description": "These are not IAM policies", + "operations": [ + { + "op": "add", + "path": "/Properties/DeliveryPolicy/ScrutinyType", + "value": "None" + }, + { + "op": "add", + "path": "/Properties/FilterPolicy/ScrutinyType", + "value": "None" + } + ] + } + }, + "AWS::SQS::RedrivePolicy": { + "patch": { + "description": "These are not IAM policies", + "operations": [ + { + "op": "add", + "path": "/Properties/RedrivePolicy/ScrutinyType", + "value": "None" + } + ] + } + } + } +} diff --git a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts new file mode 100644 index 0000000000000..63860265448f0 --- /dev/null +++ b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts @@ -0,0 +1,26 @@ +import { Test } from 'nodeunit'; +import { specification } from '../lib'; +import { ScrutinyType } from '../lib/schema'; + +export = { + 'spot-check IAM identity tags'(test: Test) { + const prop = specification.PropertyTypes['AWS::IAM::Role.Policy'].Properties!.PolicyDocument; + test.equals(prop.ScrutinyType, ScrutinyType.IdentityPolicy); + + test.done(); + }, + + 'spot-check IAM resource tags'(test: Test) { + const prop = specification.ResourceTypes['AWS::KMS::Key'].Properties!.KeyPolicy; + test.equals(prop.ScrutinyType, ScrutinyType.ResourcePolicy); + + test.done(); + }, + + 'spot-check no misclassified tags'(test: Test) { + const prop = specification.ResourceTypes['AWS::SNS::Subscription'].Properties!.DeliveryPolicy; + test.equals(prop.ScrutinyType, ScrutinyType.None); + + test.done(); + }, +}; \ No newline at end of file From 88bb44731bba927ee379f2099e3b38c2cfdc0221 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 23 Nov 2018 09:44:17 +0100 Subject: [PATCH 02/23] feat(aws-cdk): be aware of IAM and SecurityGroup changes 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. --- .../@aws-cdk/cfnspec/build-tools/build.ts | 3 + packages/@aws-cdk/cfnspec/lib/index.ts | 18 ++ .../@aws-cdk/cfnspec/lib/schema/property.ts | 28 ++++ .../spec-source/200_Scrutinies_patch.json | 6 +- .../@aws-cdk/cfnspec/test/spec-validators.ts | 68 +++++--- .../cloudformation-diff/lib/diff/index.ts | 32 ++-- .../cloudformation-diff/lib/diff/types.ts | 96 ++++++++++- .../cloudformation-diff/lib/format.ts | 17 ++ .../lib/iam/iam-changes.ts | 151 +++++++++++++++++ .../cloudformation-diff/lib/iam/statement.ts | 157 ++++++++++++++++++ .../cloudformation-diff/lib/iam/uncfn.ts | 63 +++++++ .../cloudformation-diff/package-lock.json | 27 ++- .../@aws-cdk/cloudformation-diff/package.json | 3 + .../test/test.diff-template.ts | 6 +- 14 files changed, 620 insertions(+), 55 deletions(-) create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/iam/uncfn.ts diff --git a/packages/@aws-cdk/cfnspec/build-tools/build.ts b/packages/@aws-cdk/cfnspec/build-tools/build.ts index 1657b8ca34859..424fdb3d44183 100644 --- a/packages/@aws-cdk/cfnspec/build-tools/build.ts +++ b/packages/@aws-cdk/cfnspec/build-tools/build.ts @@ -10,6 +10,7 @@ import fs = require('fs-extra'); import md5 = require('md5'); import path = require('path'); import { schema } from '../lib'; +import { detectScrutinyTypes } from './scrutiny'; async function main() { const inputDir = path.join(process.cwd(), 'spec-source'); @@ -25,6 +26,8 @@ async function main() { } } + detectScrutinyTypes(spec); + spec.Fingerprint = md5(JSON.stringify(normalize(spec))); const outDir = path.join(process.cwd(), 'spec'); diff --git a/packages/@aws-cdk/cfnspec/lib/index.ts b/packages/@aws-cdk/cfnspec/lib/index.ts index 61e3fb726f919..8d69a8249eaf4 100644 --- a/packages/@aws-cdk/cfnspec/lib/index.ts +++ b/packages/@aws-cdk/cfnspec/lib/index.ts @@ -64,3 +64,21 @@ function makePredicate(filter: string | RegExp | Filter): Filter { return s => s.match(filter) != null; } } + +/** + * Return the properties of the given type that require the given scrutiny type + */ +export function scrutinizablePropertyNames(resourceType: string, scrutinyType: schema.ScrutinyType): string[] { + const impl = specification.ResourceTypes[resourceType]; + if (!impl) { return []; } + + const ret = new Array(); + + for (const [propertyName, propertySpec] of Object.entries(impl.Properties || {})) { + if ((propertySpec.ScrutinyType || schema.ScrutinyType.None) === scrutinyType) { + ret.push(propertyName); + } + } + + return ret; +} diff --git a/packages/@aws-cdk/cfnspec/lib/schema/property.ts b/packages/@aws-cdk/cfnspec/lib/schema/property.ts index b9679d9ac98b3..ad555d76917e6 100644 --- a/packages/@aws-cdk/cfnspec/lib/schema/property.ts +++ b/packages/@aws-cdk/cfnspec/lib/schema/property.ts @@ -16,6 +16,13 @@ export interface PropertyBase extends Documented { * example, which other properties you updated. */ UpdateType: UpdateType; + + /** + * During a stack update, what kind of additional scrutiny changes to this property should be subjected to + * + * @default None + */ + ScrutinyType?: ScrutinyType; } export interface PrimitiveProperty extends PropertyBase { @@ -154,3 +161,24 @@ export function isUnionProperty(prop: Property): prop is UnionProperty { const castProp = prop as UnionProperty; return !!(castProp.ItemTypes || castProp.PrimitiveTypes || castProp.Types); } + +export enum ScrutinyType { + /** + * No additional scrutiny + */ + None = 'None', + + /** + * This is an IAM policy on an identity resource + */ + IdentityPolicy = 'IdentityPolicy', + + /** + * This is an IAM policy on a resource + */ + ResourcePolicy = 'ResourcePolicy', +} + +export function isScrutinyType(str: string): str is ScrutinyType { + return (ScrutinyType as any)[str] !== undefined; +} \ No newline at end of file diff --git a/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json b/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json index 0b9ad762949fc..1ef3b48263d8f 100644 --- a/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json +++ b/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json @@ -2,7 +2,7 @@ "ResourceTypes": { "AWS::SNS::Subscription": { "patch": { - "description": "These are not IAM policies", + "description": "SNS: These are not IAM policies", "operations": [ { "op": "add", @@ -17,9 +17,9 @@ ] } }, - "AWS::SQS::RedrivePolicy": { + "AWS::SQS::Queue": { "patch": { - "description": "These are not IAM policies", + "description": "SQS: Not an IAM policy", "operations": [ { "op": "add", diff --git a/packages/@aws-cdk/cfnspec/test/spec-validators.ts b/packages/@aws-cdk/cfnspec/test/spec-validators.ts index 3c0116afcce73..4211e5dcc64d7 100644 --- a/packages/@aws-cdk/cfnspec/test/spec-validators.ts +++ b/packages/@aws-cdk/cfnspec/test/spec-validators.ts @@ -30,64 +30,56 @@ function validateProperties(typeName: string, test: Test, properties: { [name: string]: schema.Property }, specification: Specification) { - const requiredKeys = ['Documentation', 'Required', 'UpdateType']; + const expectedKeys = ['Documentation', 'Required', 'UpdateType', 'ScrutinyType']; for (const name of Object.keys(properties)) { + const property = properties[name]; test.notEqual(property.Documentation, '', `${typeName}.Properties.${name} is documented`); test.ok(schema.isUpdateType(property.UpdateType), `${typeName}.Properties.${name} has valid UpdateType`); + if (property.ScrutinyType !== undefined) { + test.ok(schema.isScrutinyType(property.ScrutinyType), `${typeName}.Properties.${name} has valid ScrutinyType`); + } test.notEqual(property.Required, null, `${typeName}.Properties.${name} has required flag`); + if (schema.isPrimitiveProperty(property)) { - test.deepEqual(Object.keys(property).sort(), - [...requiredKeys, 'PrimitiveType'].sort(), - `${typeName}.Properties.${name} has no extra properties`); test.ok(schema.isPrimitiveType(property.PrimitiveType), `${typeName}.Properties.${name} has a valid PrimitiveType`); + expectedKeys.push('PrimitiveType'); + } else if (schema.isPrimitiveListProperty(property)) { - // The DuplicatesAllowed key is optional (absent === false) - const extraKeys = 'DuplicatesAllowed' in property ? ['DuplicatesAllowed'] : []; - test.deepEqual(Object.keys(property).sort(), - [...requiredKeys, ...extraKeys, 'PrimitiveItemType', 'Type'].sort(), - `${typeName}.Properties.${name} has no extra properties`); + expectedKeys.push('Type', 'DuplicatesAllowed', 'PrimitiveItemType'); test.ok(schema.isPrimitiveType(property.PrimitiveItemType), `${typeName}.Properties.${name} has a valid PrimitiveItemType`); + } else if (schema.isPrimitiveMapProperty(property)) { - // The DuplicatesAllowed key is optional (absent === false) - const extraKeys = 'DuplicatesAllowed' in property ? ['DuplicatesAllowed'] : []; - test.deepEqual(Object.keys(property).sort(), - [...requiredKeys, ...extraKeys, 'PrimitiveItemType', 'Type'].sort(), - `${typeName}.Properties.${name} has no extra properties`); + expectedKeys.push('Type', 'DuplicatesAllowed', 'PrimitiveItemType', 'Type'); test.ok(schema.isPrimitiveType(property.PrimitiveItemType), `${typeName}.Properties.${name} has a valid PrimitiveItemType`); test.ok(!property.DuplicatesAllowed, `${typeName}.Properties.${name} does not allow duplicates`); + } else if (schema.isComplexListProperty(property)) { - // The DuplicatesAllowed key is optional (absent === false) - const extraKeys = 'DuplicatesAllowed' in property ? ['DuplicatesAllowed'] : []; - test.deepEqual(Object.keys(property).sort(), - [...requiredKeys, ...extraKeys, 'ItemType', 'Type'].sort(), - `${typeName}.Properties.${name} has no extra properties`); + expectedKeys.push('Type', 'DuplicatesAllowed', 'ItemType', 'Type'); test.ok(property.ItemType, `${typeName}.Properties.${name} has a valid ItemType`); if (property.ItemType !== 'Tag') { const fqn = `${typeName.split('.')[0]}.${property.ItemType}`; const resolvedType = specification.PropertyTypes && specification.PropertyTypes[fqn]; test.ok(resolvedType, `${typeName}.Properties.${name} ItemType (${fqn}) resolves`); } + } else if (schema.isComplexMapProperty(property)) { - // The DuplicatesAllowed key is optional (absent === false) - const extraKeys = 'DuplicatesAllowed' in property ? ['DuplicatesAllowed'] : []; - test.deepEqual(Object.keys(property).sort(), - [...requiredKeys, ...extraKeys, 'ItemType', 'Type'].sort(), - `${typeName}.Properties.${name} has no extra properties`); + expectedKeys.push('Type', 'DuplicatesAllowed', 'ItemType', 'Type'); test.ok(property.ItemType, `${typeName}.Properties.${name} has a valid ItemType`); const fqn = `${typeName.split('.')[0]}.${property.ItemType}`; const resolvedType = specification.PropertyTypes && specification.PropertyTypes[fqn]; test.ok(resolvedType, `${typeName}.Properties.${name} ItemType (${fqn}) resolves`); test.ok(!property.DuplicatesAllowed, `${typeName}.Properties.${name} does not allow duplicates`); + } else if (schema.isComplexProperty(property)) { + expectedKeys.push('Type'); test.ok(property.Type, `${typeName}.Properties.${name} has a valid type`); const fqn = `${typeName.split('.')[0]}.${property.Type}`; const resolvedType = specification.PropertyTypes && specification.PropertyTypes[fqn]; test.ok(resolvedType, `${typeName}.Properties.${name} type (${fqn}) resolves`); - test.deepEqual(Object.keys(property).sort(), - [...requiredKeys, 'Type'].sort(), - `${typeName}.Properties.${name} has no extra properties`); + } else if (schema.isUnionProperty(property)) { + expectedKeys.push('PrimitiveTypes', 'PrimitiveItemTypes', 'ItemTypes', 'Types'); if (property.PrimitiveTypes) { for (const type of property.PrimitiveTypes) { test.ok(schema.isPrimitiveType(type), `${typeName}.Properties.${name} has only valid PrimitiveTypes`); @@ -107,9 +99,14 @@ function validateProperties(typeName: string, test.ok(resolvedType, `${typeName}.Properties.${name} type (${fqn}) resolves`); } } + } else { test.ok(false, `${typeName}.Properties.${name} has known type`); } + + test.deepEqual( + without(Object.keys(property), expectedKeys), [], + `${typeName}.Properties.${name} has no extra properties`); } } @@ -140,3 +137,20 @@ function validateAttributes(typeName: string, } } } + +/** + * Remove elements from a set + */ +function without(xs: T[], ...sets: T[][]) { + const ret = new Set(xs); + + for (const set of sets) { + for (const element of set) { + if (ret.has(element)) { + ret.delete(element); + } + } + } + + return Array.from(ret); +} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts index b8c744120fa27..7975d47e79f70 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts @@ -31,38 +31,44 @@ export function diffResource(oldValue: types.Resource, newValue: types.Resource) oldType: oldValue && oldValue.Type, newType: newValue && newValue.Type }; - let propertyChanges: { [key: string]: types.PropertyDifference } = {}; + let propertyUpdates: { [key: string]: types.PropertyDifference } = {}; let otherChanges: { [key: string]: types.Difference } = {}; if (resourceType.oldType === resourceType.newType) { // Only makes sense to inspect deeper if the types stayed the same const typeSpec = cfnspec.filteredSpecification(resourceType.oldType); const impl = typeSpec.ResourceTypes[resourceType.oldType]; - propertyChanges = diffKeyedEntities(oldValue.Properties, + propertyUpdates = diffKeyedEntities(oldValue.Properties, newValue.Properties, (oldVal, newVal, key) => _diffProperty(oldVal, newVal, key, impl)); otherChanges = diffKeyedEntities(oldValue, newValue, _diffOther); delete otherChanges.Properties; } - return new types.ResourceDifference(oldValue, newValue, { resourceType, propertyChanges, otherChanges }); + return new types.ResourceDifference(oldValue, newValue, { + resourceType, propertyUpdates, otherChanges, + oldProperties: (oldValue && oldValue.Properties) || {}, + newProperties: (newValue && newValue.Properties) || {} + }); function _diffProperty(oldV: any, newV: any, key: string, resourceSpec?: cfnspec.schema.ResourceType) { - let changeImpact: types.ResourceImpact | undefined; + let changeImpact; + const spec = resourceSpec && resourceSpec.Properties && resourceSpec.Properties[key]; if (spec) { switch (spec.UpdateType) { - case 'Immutable': - changeImpact = types.ResourceImpact.WILL_REPLACE; - break; - case 'Conditional': - changeImpact = types.ResourceImpact.MAY_REPLACE; - break; - default: - // In those cases, whatever is the current value is what we should keep - changeImpact = types.ResourceImpact.WILL_UPDATE; + case 'Immutable': + changeImpact = types.ResourceImpact.WILL_REPLACE; + break; + case 'Conditional': + changeImpact = types.ResourceImpact.MAY_REPLACE; + break; + default: + // In those cases, whatever is the current value is what we should keep + changeImpact = types.ResourceImpact.WILL_UPDATE; } } + return new types.PropertyDifference(oldV, newV, { changeImpact }); } diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index 55b23bdb1321f..2aa20569f7af2 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -1,4 +1,6 @@ +import cfnspec = require('@aws-cdk/cfnspec'); import { AssertionError } from 'assert'; +import { IamChanges } from '../iam/iam-changes'; import { deepEqual } from './util'; /** Semantic differences between two CloudFormation templates. */ @@ -15,6 +17,11 @@ export class TemplateDiff implements ITemplateDiff { /** The differences in unknown/unexpected parts of the template */ public unknown: DifferenceCollection>; + /** + * Changes to IAM policies + */ + public readonly iamChanges: IamChanges; + constructor(args: ITemplateDiff) { if (args.awsTemplateFormatVersion !== undefined) { this.awsTemplateFormatVersion = args.awsTemplateFormatVersion; @@ -33,6 +40,10 @@ export class TemplateDiff implements ITemplateDiff { this.parameters = args.parameters || new DifferenceCollection({}); this.resources = args.resources || new DifferenceCollection({}); this.unknown = args.unknown || new DifferenceCollection({}); + + this.iamChanges = new IamChanges( + this.scrutinizablePropertyChanges(cfnspec.schema.ScrutinyType.IdentityPolicy), + this.scrutinizablePropertyChanges(cfnspec.schema.ScrutinyType.ResourcePolicy)); } public get count() { @@ -62,6 +73,61 @@ export class TemplateDiff implements ITemplateDiff { public get isEmpty(): boolean { return this.count === 0; } + + /** + * Return all property changes of a given scrutiny type + * + * We don't just look at property updates; we also look at resource additions and deletions (in which + * case there is no further detail on property values), and resource type changes. + */ + public scrutinizablePropertyChanges(scrutinyType: cfnspec.schema.ScrutinyType): PropertyChange[] { + const ret = new Array(); + + for (const [resourceLogicalId, resourceChange] of Object.entries(this.resources.changes)) { + if (!resourceChange) { continue; } + + const props = cfnspec.scrutinizablePropertyNames(resourceChange.newResourceType!, scrutinyType); + for (const propertyName of props) { + ret.push({ + resourceLogicalId, propertyName, + oldValue: resourceChange.oldProperties[propertyName], + newValue: resourceChange.newProperties[propertyName], + }); + } + } + + return ret; + } +} + +/** + * A change in property values + * + * Not necessarily an update, it could be that there used to be no value there + * because there was no resource, and now there is (or vice versa). + * + * Therefore, we just contain plain values and not a PropertyDifference. + */ +export interface PropertyChange { + /** + * Logical ID of the resource where this property change was found + */ + resourceLogicalId: string; + + /** + * Name of the property that is changing + */ + propertyName: string; + + /** + * The old property value + */ + oldValue?: any; + + /** + * The new property value + */ + newValue?: any; } /** @@ -260,8 +326,18 @@ export interface Resource { [key: string]: any; } export class ResourceDifference extends Difference { + /** + * Old property values + */ + public readonly oldProperties: { [name: string]: any }; + + /** + * New property values + */ + public readonly newProperties: { [name: string]: any }; + /** Property-level changes on the resource */ - public readonly propertyChanges: { [key: string]: PropertyDifference }; + public readonly propertyUpdates: { [key: string]: PropertyDifference }; /** Changes to non-property level attributes of the resource */ public readonly otherChanges: { [key: string]: Difference }; @@ -272,14 +348,18 @@ export class ResourceDifference extends Difference { newValue: Resource | undefined, args: { resourceType: { oldType: string, newType: string }, - propertyChanges: { [key: string]: Difference }, + oldProperties: { [name: string]: any }, + newProperties: { [name: string]: any }, + propertyUpdates: { [key: string]: PropertyDifference }, otherChanges: { [key: string]: Difference } } ) { super(oldValue, newValue); this.resourceType = args.resourceType; - this.propertyChanges = args.propertyChanges; + this.propertyUpdates = args.propertyUpdates; this.otherChanges = args.otherChanges; + this.oldProperties = args.oldProperties; + this.newProperties = args.newProperties; } public get oldResourceType(): string | undefined { @@ -302,19 +382,19 @@ export class ResourceDifference extends Difference { return ResourceImpact.WILL_REPLACE; } - return Object.values(this.propertyChanges) + return Object.values(this.propertyUpdates) .map(elt => elt.changeImpact) .reduce(worstImpact, ResourceImpact.WILL_UPDATE); } public get count(): number { - return Object.keys(this.propertyChanges).length + return Object.keys(this.propertyUpdates).length + Object.keys(this.otherChanges).length; } public forEach(cb: (type: 'Property' | 'Other', name: string, value: Difference | PropertyDifference) => any) { - for (const key of Object.keys(this.propertyChanges).sort()) { - cb('Property', key, this.propertyChanges[key]); + for (const key of Object.keys(this.propertyUpdates).sort()) { + cb('Property', key, this.propertyUpdates[key]); } for (const key of Object.keys(this.otherChanges).sort()) { cb('Other', key, this.otherChanges[key]); @@ -330,4 +410,4 @@ class NoDifferenceError extends Error { constructor(message: string) { super(`No difference: ${message}`); } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index 9a700e2004f3e..1ce86b984f4a0 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -1,9 +1,11 @@ import cxapi = require('@aws-cdk/cx-api'); +import Table = require('cli-table'); import colors = require('colors/safe'); import { format } from 'util'; import { Difference, isPropertyDifference, ResourceDifference, ResourceImpact } from './diff-template'; import { DifferenceCollection, TemplateDiff } from './diff/types'; import { deepEqual } from './diff/util'; +import { IamChanges } from './iam/iam-changes'; /** * Renders template differences to the process' console. @@ -28,6 +30,12 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp printSectionFooter(); } + if (!templateDiff.iamChanges.empty) { + printSectionHeader('Summary of IAM Changes'); + formatIamChanges(templateDiff.iamChanges); + printSectionFooter(); + } + formatSection('Parameters', 'Parameter', templateDiff.parameters); formatSection('Metadata', 'Metadata', templateDiff.metadata); formatSection('Mappings', 'Mapping', templateDiff.mappings); @@ -269,4 +277,13 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp return `${p} ${colors.gray(logicalId)}`; } } + + function formatIamChanges(changes: IamChanges) { + const summary = changes.summarize(); + const head = summary.splice(0, 1)[0]; + + const table = new Table({ head, style: { head: [] } }); + table.push(...summary); + print(table.toString()); + } } diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts new file mode 100644 index 0000000000000..79a5765f69d38 --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts @@ -0,0 +1,151 @@ +import colors = require('colors/safe'); +import { PropertyChange } from "../diff/types"; +import { parseStatements, Statement, Targets } from "./statement"; +import { unCloudFormation } from "./uncfn"; + +/** + * Changes to IAM statements + */ +export class IamChanges { + public readonly additions: Statement[] = []; + public readonly removals: Statement[] = []; + private oldStatements: Statement[] = []; + private newStatements: Statement[] = []; + + constructor(identityPolicyChanges: PropertyChange[], resourcePolicyChanges: PropertyChange[]) { + for (const policyChange of identityPolicyChanges) { + this.oldStatements.push(...this.readIdentityStatements(policyChange.oldValue, policyChange.propertyName, policyChange.resourceLogicalId)); + this.newStatements.push(...this.readIdentityStatements(policyChange.newValue, policyChange.propertyName, policyChange.resourceLogicalId)); + } + for (const policyChange of resourcePolicyChanges) { + this.oldStatements.push(...this.readResourceStatements(policyChange.oldValue, policyChange.resourceLogicalId)); + this.newStatements.push(...this.readResourceStatements(policyChange.newValue, policyChange.resourceLogicalId)); + } + + this.additions.push(...this.newStatements.filter(s => !hasStatement(s, this.oldStatements))); + this.removals.push(...this.oldStatements.filter(s => !hasStatement(s, this.newStatements))); + } + + public get empty() { + return this.additions.length + this.removals.length === 0; + } + + public get hasAdditions() { + return this.additions.length > 0; + } + + public get hasRemovals() { + return this.removals.length > 0; + } + + /** + * Return a summary table of changes + */ + public summarize(): string[][] { + const ret: string[][] = []; + + const header = ['', 'Resource', 'Effect', 'Actions', 'Principals', 'Condition']; + + // First generate all lines, then sort on Resource so that similar resources are together + for (const statement of this.additions) { + ret.push([ + '+', + renderTargets(statement.resources), + statement.effect, + renderTargets(statement.actions), + renderTargets(statement.principals), + renderCondition(statement.condition) + ].map(s => colors.green(s))); + } + for (const statement of this.removals) { + ret.push([ + colors.red('-'), + renderTargets(statement.resources), + statement.effect, + renderTargets(statement.actions), + renderTargets(statement.principals), + renderCondition(statement.condition) + ].map(s => colors.red(s))); + } + + // Sort by 2nd column + ret.sort(makeComparator((row: string[]) => row[1])); + + ret.splice(0, 0, header); + + return ret; + } + + private readIdentityStatements(policy: any, propertyName: string, logicalId: string): Statement[] { + if (!policy) { return []; } + + // Stringify CloudFormation intrinsics so that the IAM parser has an easier domain to work in, + // then parse. + const statements = parseStatements(unCloudFormation(policy.Statement)); + + // If this is an assumeRolePolicy and the Resource list is empty, add in the logicalId into Resources, + // because that's what this statement is applying to. + if (propertyName === 'AssumeRolePolicyDocument') { + const rep = '${' + logicalId + '.Arn}'; + statements.forEach(s => s.resources.replaceEmpty(rep)); + } + + return statements; + } + + private readResourceStatements(policy: any, logicalId: string): Statement[] { + // Stringify CloudFormation intrinsics so that the IAM parser has an easier domain to work in, + // then parse. + const statements = parseStatements(unCloudFormation(policy.Statement)); + + // Since this is a ResourcePolicy, the meaning of the '*' in a Resource field + // actually scoped to the resource it's on, so replace * resources with what + // they're scoped to. + // + // We replace with the ARN of the resource, exemplified by a "GetAtt" + // expression. This might not strictly work in CFN for all resource types + // (because {X.Arn} might not exist, or it might have to be {Ref}), and for + // example for Buckets a * implies both the bucket and the resources in the + // bucket, but for human consumpion this is sufficient to be intelligible. + const rep = '${' + logicalId + '.Arn}'; + statements.forEach(s => s.resources.replaceStar(rep)); + + return statements; + } +} + +function hasStatement(statement: Statement, ss: Statement[]) { + return ss.some(s => statement.equal(s)); +} + +/** + * Render into a summary table cell + */ +function renderTargets(targets: Targets): string { + if (targets.not) { + return targets.values.map(s => `NOT ${s}`).join('\n'); + } + return targets.values.join('\n'); +} + +/** + * Render the Condition column + */ +function renderCondition(condition: any) { + if (!condition || Object.keys(condition).length === 0) { return ''; } + return JSON.stringify(condition, undefined, 2); +} + +/** + * Turn a key extraction function into a comparator for use in Array.sort() + */ +function makeComparator(keyFn: (x: T) => U) { + return (a: T, b: T) => { + const keyA = keyFn(a); + const keyB = keyFn(b); + + if (keyA < keyB) { return -1; } + if (keyB < keyA) { return 1; } + return 0; + }; +} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts new file mode 100644 index 0000000000000..0b1e192fe05d1 --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts @@ -0,0 +1,157 @@ +import deepEqual = require('fast-deep-equal'); + +export class Statement { + /** + * Statement ID + */ + public readonly sid: string | undefined; + + /** + * Statement effect + */ + public readonly effect: Effect; + + /** + * Resources + */ + public readonly resources: Targets; + + /** + * Principals + */ + public readonly principals: Targets; + + /** + * Actions + */ + public readonly actions: Targets; + + /** + * Object with conditions + */ + public readonly condition?: any; + + constructor(statement: UnknownMap) { + this.sid = expectString(statement.Sid); + this.effect = expectEffect(statement.Effect); + this.resources = new Targets(statement, 'Resource', 'NotResource'); + this.actions = new Targets(statement, 'Action', 'NotAction'); + this.principals = new Targets(statement, 'Principal', 'NotPrincipal'); + this.condition = statement.Condition; + } + + /** + * Whether this statement is equal to the other statement + */ + public equal(other: Statement) { + return (this.sid === other.sid + && this.effect === other.effect + && this.resources.equal(other.resources) + && this.actions.equal(other.actions) + && this.principals.equal(other.principals) + && deepEqual(this.condition, other.condition)); + } + +} + +/** + * Parse a list of statements from undefined, a Statement, or a list of statements + * @param x Pa + */ +export function parseStatements(x: any): Statement[] { + if (x === undefined) { x = []; } + if (!Array.isArray(x)) { x = [x]; } + return x.map((s: any) => new Statement(s)); +} + +/** + * Targets for a field + */ +export class Targets { + /** + * The values of the targets + */ + public readonly values: string[]; + + /** + * Whether positive or negative matchers + */ + public readonly not: boolean; + + constructor(statement: UnknownMap, positiveKey: string, negativeKey: string) { + if (negativeKey in statement) { + this.values = forceListOfStrings(statement[negativeKey]); + this.not = true; + } else { + this.values = forceListOfStrings(statement[positiveKey]); + this.not = false; + } + this.values.sort(); + } + + public get empty() { + return this.values.length === 0; + } + + /** + * Whether this set of targets is equal to the other set of targets + */ + public equal(other: Targets) { + return this.not === other.not && deepEqual(this.values.sort(), other.values.sort()); + } + + /** + * If the current value set is empty, put this in it + */ + public replaceEmpty(replacement: string) { + if (this.empty) { + this.values.push(replacement); + } + } + + /** + * If the actions contains a '*', replace with this string. + */ + public replaceStar(replacement: string) { + for (let i = 0; i < this.values.length; i++) { + if (this.values[i] === '*') { + this.values[i] = replacement; + } + } + this.values.sort(); + } +} + +type UnknownMap = {[key: string]: unknown}; + +export enum Effect { + Allow = 'Allow', + Deny = 'Deny', +} + +function expectString(x: unknown): string | undefined { + return typeof x === 'string' ? x : undefined; +} + +function expectEffect(x: unknown): Effect { + if (x === Effect.Allow || x === Effect.Deny) { return x as Effect; } + throw new Error(`Unknown effect: ${x}`); +} + +function forceListOfStrings(x: unknown): string[] { + if (typeof x === 'string') { return [x]; } + if (typeof x === 'undefined' || x === null) { return []; } + if (Array.isArray(x)) { + return x.map(el => `${el}`); + } + + if (typeof x === 'object') { + const ret: string[] = []; + for (const [key, value] of Object.entries(x)) { + ret.push(...forceListOfStrings(value).map(s => `${key}:${s}`)); + } + return ret; + } + + return [`${x}`]; +} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/uncfn.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/uncfn.ts new file mode 100644 index 0000000000000..f8bfa7845c957 --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/uncfn.ts @@ -0,0 +1,63 @@ +/** + * Turn CloudFormation intrinsics into strings + * + * ------ + * + * This stringification is not intended to be mechanically reversible! It's intended + * to be understood by humans! + * + * ------ + * + * Turns Fn::GetAtt and Fn::Ref objects into the same strings that can be + * parsed by Fn::Sub, but without the surrounding intrinsics. + * + * Evaluates Fn::Join directly if the second argument is a literal list of strings. + * + * For other intrinsics we choose a string representation that CloudFormation + * cannot actually parse, but is comprehensible to humans. + */ +export function unCloudFormation(x: any): any { + if (Array.isArray(x)) { + return x.map(unCloudFormation); + } + + const intrinsic = getIntrinsic(x); + if (intrinsic) { + if (intrinsic.fn === 'Ref') { return '${' + intrinsic.args + '}'; } + if (intrinsic.fn === 'Fn::GetAtt') { return '${' + intrinsic.args[0] + '.' + intrinsic.args[1] + '}'; } + if (intrinsic.fn === 'Fn::Join') { return unCloudFormationFnJoin(intrinsic.args[0], intrinsic.args[1]); } + return stringifyIntrinsic(intrinsic.fn, intrinsic.args); + } + + if (typeof x === 'object' && x !== null) { + const ret: any = {}; + for (const [key, value] of Object.entries(x)) { + ret[key] = unCloudFormation(value); + } + return ret; + } + return x; +} + +function unCloudFormationFnJoin(separator: string, args: any) { + if (Array.isArray(args)) { + return args.map(unCloudFormation).join(separator); + } + return stringifyIntrinsic('Fn::Join', [separator, args]); +} + +function stringifyIntrinsic(fn: string, args: any) { + return JSON.stringify({ [fn]: unCloudFormation(args) }); +} + +function getIntrinsic(x: any): Intrinsic | undefined { + if (x === undefined || x === null || Array.isArray(x)) { return undefined; } + if (typeof x !== 'object') { return undefined; } + const keys = Object.keys(x); + return keys.length === 1 && (keys[0] === 'Ref' || keys[0].startsWith('Fn::')) ? { fn: keys[0], args: x[keys[0]] } : undefined; +} + +interface Intrinsic { + fn: string; + args: any; +} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/package-lock.json b/packages/@aws-cdk/cloudformation-diff/package-lock.json index 445bc02dcc872..912138c9c82a7 100644 --- a/packages/@aws-cdk/cloudformation-diff/package-lock.json +++ b/packages/@aws-cdk/cloudformation-diff/package-lock.json @@ -1,19 +1,44 @@ { "name": "@aws-cdk/cloudformation-diff", - "version": "0.12.0", + "version": "0.18.1", "lockfileVersion": 1, "requires": true, "dependencies": { + "@types/cli-table": { + "version": "0.3.0", + "resolved": "https://registry.npmjs.org/@types/cli-table/-/cli-table-0.3.0.tgz", + "integrity": "sha512-QnZUISJJXyhyD6L1e5QwXDV/A5i2W1/gl6D6YMc8u0ncPepbv/B4w3S+izVvtAg60m6h+JP09+Y/0zF2mojlFQ==" + }, "buffer-from": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/buffer-from/-/buffer-from-1.1.1.tgz", "integrity": "sha512-MQcXEUbCKtEo7bhqEs6560Hyd4XaovZlO/k9V3hjVUF/zwW7KBVdSK4gIt/bzwS9MbR5qob+F5jusZsb0YQK2A==" }, + "cli-table": { + "version": "0.3.1", + "resolved": "https://registry.npmjs.org/cli-table/-/cli-table-0.3.1.tgz", + "integrity": "sha1-9TsFJmqLGguTSz0IIebi3FkUriM=", + "requires": { + "colors": "1.0.3" + }, + "dependencies": { + "colors": { + "version": "1.0.3", + "resolved": "http://registry.npmjs.org/colors/-/colors-1.0.3.tgz", + "integrity": "sha1-BDP0TYCWgP3rYO0mDxsMJi6CpAs=" + } + } + }, "colors": { "version": "1.3.2", "resolved": "https://registry.npmjs.org/colors/-/colors-1.3.2.tgz", "integrity": "sha512-rhP0JSBGYvpcNQj4s5AdShMeE5ahMop96cTeDl/v9qQQm2fYClE2QXZRi8wLzc+GmXSxdIqqbOIAhyObEXDbfQ==" }, + "fast-deep-equal": { + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/fast-deep-equal/-/fast-deep-equal-2.0.1.tgz", + "integrity": "sha1-ewUhjd+WZ79/Nwv3/bLLFf3Qqkk=" + }, "source-map": { "version": "0.6.1", "resolved": "https://registry.npmjs.org/source-map/-/source-map-0.6.1.tgz", diff --git a/packages/@aws-cdk/cloudformation-diff/package.json b/packages/@aws-cdk/cloudformation-diff/package.json index 4a0ecb1874814..6437a3ffcc7fe 100644 --- a/packages/@aws-cdk/cloudformation-diff/package.json +++ b/packages/@aws-cdk/cloudformation-diff/package.json @@ -25,7 +25,10 @@ "dependencies": { "@aws-cdk/cfnspec": "^0.18.1", "@aws-cdk/cx-api": "^0.18.1", + "@types/cli-table": "^0.3.0", + "cli-table": "^0.3.1", "colors": "^1.2.1", + "fast-deep-equal": "^2.0.1", "source-map-support": "^0.5.6" }, "devDependencies": { diff --git a/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts b/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts index c4e4d2e1f8a42..01cdc3663dcaf 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts @@ -142,7 +142,7 @@ exports.diffTemplate = { const difference = differences.resources.changes.BucketResource; test.notStrictEqual(difference, undefined, 'the difference is on the BucketResource logical ID'); test.equal(difference && difference.oldResourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); - test.deepEqual(difference && difference.propertyChanges, + test.deepEqual(difference && difference.propertyUpdates, { BucketName: { oldValue: bucketName, newValue: newBucketName, changeImpact: ResourceImpact.WILL_REPLACE } }, 'the difference reports property-level changes'); test.done(); @@ -210,7 +210,7 @@ exports.diffTemplate = { const difference = differences.resources.changes.BucketResource; test.notStrictEqual(difference, undefined, 'the difference is on the BucketResource logical ID'); test.equal(difference && difference.oldResourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); - test.deepEqual(difference && difference.propertyChanges, + test.deepEqual(difference && difference.propertyUpdates, { BucketName: { oldValue: bucketName, newValue: undefined, changeImpact: ResourceImpact.WILL_REPLACE } }, 'the difference reports property-level changes'); test.done(); @@ -249,7 +249,7 @@ exports.diffTemplate = { const difference = differences.resources.changes.BucketResource; test.notStrictEqual(difference, undefined, 'the difference is on the BucketResource logical ID'); test.equal(difference && difference.oldResourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); - test.deepEqual(difference && difference.propertyChanges, + test.deepEqual(difference && difference.propertyUpdates, { BucketName: { oldValue: undefined, newValue: bucketName, changeImpact: ResourceImpact.WILL_REPLACE } }, 'the difference reports property-level changes'); test.done(); From 81ee604b4892b993faea5ab6a55387774ac2c48e Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 26 Nov 2018 16:42:23 +0100 Subject: [PATCH 03/23] Recognize Lambda::Permissions objects --- .../@aws-cdk/cfnspec/build-tools/scrutiny.ts | 4 +- packages/@aws-cdk/cfnspec/lib/index.ts | 18 ++- .../@aws-cdk/cfnspec/lib/schema/property.ts | 8 +- .../cfnspec/lib/schema/resource-type.ts | 23 +++ .../spec-source/200_Scrutinies_patch.json | 12 ++ .../@aws-cdk/cfnspec/test/spec-validators.ts | 2 +- .../@aws-cdk/cfnspec/test/test.scrutiny.ts | 15 +- .../cloudformation-diff/lib/diff/index.ts | 12 +- .../cloudformation-diff/lib/diff/types.ts | 135 +++++++++++++++--- .../lib/iam/iam-changes.ts | 16 ++- .../cloudformation-diff/lib/iam/statement.ts | 44 +++++- 11 files changed, 247 insertions(+), 42 deletions(-) diff --git a/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts b/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts index 5da94a1e2bd91..30011d1e3d93b 100644 --- a/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts +++ b/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts @@ -1,5 +1,5 @@ import { schema } from '../lib'; -import { ScrutinyType } from '../lib/schema'; +import { PropertyScrutinyType } from '../lib/schema'; /** * Auto-detect common properties to apply scrutiny to by using heuristics @@ -24,7 +24,7 @@ export function detectScrutinyTypes(spec: schema.Specification) { if (nameContainsPolicy && primitiveType === 'Json') { const isIamResource = typeName.indexOf('::IAM::') > 1; - propertySpec.ScrutinyType = isIamResource ? ScrutinyType.IdentityPolicy : ScrutinyType.ResourcePolicy; + propertySpec.ScrutinyType = isIamResource ? PropertyScrutinyType.IdentityPolicy : PropertyScrutinyType.ResourcePolicy; continue; } } diff --git a/packages/@aws-cdk/cfnspec/lib/index.ts b/packages/@aws-cdk/cfnspec/lib/index.ts index 8d69a8249eaf4..90a864f3981cc 100644 --- a/packages/@aws-cdk/cfnspec/lib/index.ts +++ b/packages/@aws-cdk/cfnspec/lib/index.ts @@ -68,17 +68,31 @@ function makePredicate(filter: string | RegExp | Filter): Filter { /** * Return the properties of the given type that require the given scrutiny type */ -export function scrutinizablePropertyNames(resourceType: string, scrutinyType: schema.ScrutinyType): string[] { +export function scrutinizablePropertyNames(resourceType: string, scrutinyType: schema.PropertyScrutinyType): string[] { const impl = specification.ResourceTypes[resourceType]; if (!impl) { return []; } const ret = new Array(); for (const [propertyName, propertySpec] of Object.entries(impl.Properties || {})) { - if ((propertySpec.ScrutinyType || schema.ScrutinyType.None) === scrutinyType) { + if ((propertySpec.ScrutinyType || schema.PropertyScrutinyType.None) === scrutinyType) { ret.push(propertyName); } } return ret; } + +/** + * Return the names of the resource types that need to be subjected to additional scrutiny + */ +export function scrutinizableResourceTypes(scrutinyType: schema.ResourceScrutinyType): string[] { + const ret = new Array(); + for (const [resourceType, resourceSpec] of Object.entries(specification.ResourceTypes)) { + if ((resourceSpec.ScrutinyType || schema.PropertyScrutinyType.None) === scrutinyType) { + ret.push(resourceType); + } + } + + return ret; +} diff --git a/packages/@aws-cdk/cfnspec/lib/schema/property.ts b/packages/@aws-cdk/cfnspec/lib/schema/property.ts index ad555d76917e6..9c5bac57142d1 100644 --- a/packages/@aws-cdk/cfnspec/lib/schema/property.ts +++ b/packages/@aws-cdk/cfnspec/lib/schema/property.ts @@ -22,7 +22,7 @@ export interface PropertyBase extends Documented { * * @default None */ - ScrutinyType?: ScrutinyType; + ScrutinyType?: PropertyScrutinyType; } export interface PrimitiveProperty extends PropertyBase { @@ -162,7 +162,7 @@ export function isUnionProperty(prop: Property): prop is UnionProperty { return !!(castProp.ItemTypes || castProp.PrimitiveTypes || castProp.Types); } -export enum ScrutinyType { +export enum PropertyScrutinyType { /** * No additional scrutiny */ @@ -179,6 +179,6 @@ export enum ScrutinyType { ResourcePolicy = 'ResourcePolicy', } -export function isScrutinyType(str: string): str is ScrutinyType { - return (ScrutinyType as any)[str] !== undefined; +export function isPropertyScrutinyType(str: string): str is PropertyScrutinyType { + return (PropertyScrutinyType as any)[str] !== undefined; } \ No newline at end of file diff --git a/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts b/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts index 7517fcdbe1bf8..a77232666bde9 100644 --- a/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts +++ b/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts @@ -19,6 +19,13 @@ export interface ResourceType extends Documented { * What kind of value the 'Ref' operator refers to, if any. */ RefKind?: string; + + /** + * During a stack update, what kind of additional scrutiny changes to this resource should be subjected to + * + * @default None + */ + ScrutinyType?: ResourceScrutinyType; } export type Attribute = PrimitiveAttribute | ListAttribute; @@ -71,3 +78,19 @@ export enum SpecialRefKind { */ Arn = 'Arn' } + +export enum ResourceScrutinyType { + /** + * No additional scrutiny + */ + None = 'None', + + /** + * This is a Lambda Permission policy + */ + LambdaPermission = 'LambdaPermission', +} + +export function isResourceScrutinyType(str: string): str is ResourceScrutinyType { + return (ResourceScrutinyType as any)[str] !== undefined; +} \ No newline at end of file diff --git a/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json b/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json index 1ef3b48263d8f..4f5669c14fb2d 100644 --- a/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json +++ b/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json @@ -1,5 +1,17 @@ { "ResourceTypes": { + "AWS::Lambda::Permission": { + "patch": { + "description": "Permission scrutiny", + "operations": [ + { + "op": "add", + "path": "/ScrutinyType", + "value": "LambdaPermission" + } + ] + } + }, "AWS::SNS::Subscription": { "patch": { "description": "SNS: These are not IAM policies", diff --git a/packages/@aws-cdk/cfnspec/test/spec-validators.ts b/packages/@aws-cdk/cfnspec/test/spec-validators.ts index 4211e5dcc64d7..51c03bedda6ea 100644 --- a/packages/@aws-cdk/cfnspec/test/spec-validators.ts +++ b/packages/@aws-cdk/cfnspec/test/spec-validators.ts @@ -37,7 +37,7 @@ function validateProperties(typeName: string, test.notEqual(property.Documentation, '', `${typeName}.Properties.${name} is documented`); test.ok(schema.isUpdateType(property.UpdateType), `${typeName}.Properties.${name} has valid UpdateType`); if (property.ScrutinyType !== undefined) { - test.ok(schema.isScrutinyType(property.ScrutinyType), `${typeName}.Properties.${name} has valid ScrutinyType`); + test.ok(schema.isPropertyScrutinyType(property.ScrutinyType), `${typeName}.Properties.${name} has valid ScrutinyType`); } test.notEqual(property.Required, null, `${typeName}.Properties.${name} has required flag`); diff --git a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts index 63860265448f0..f85c0ff740906 100644 --- a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts +++ b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts @@ -1,25 +1,32 @@ import { Test } from 'nodeunit'; import { specification } from '../lib'; -import { ScrutinyType } from '../lib/schema'; +import { PropertyScrutinyType, ResourceScrutinyType } from '../lib/schema'; export = { 'spot-check IAM identity tags'(test: Test) { const prop = specification.PropertyTypes['AWS::IAM::Role.Policy'].Properties!.PolicyDocument; - test.equals(prop.ScrutinyType, ScrutinyType.IdentityPolicy); + test.equals(prop.ScrutinyType, PropertyScrutinyType.IdentityPolicy); test.done(); }, 'spot-check IAM resource tags'(test: Test) { const prop = specification.ResourceTypes['AWS::KMS::Key'].Properties!.KeyPolicy; - test.equals(prop.ScrutinyType, ScrutinyType.ResourcePolicy); + test.equals(prop.ScrutinyType, PropertyScrutinyType.ResourcePolicy); test.done(); }, 'spot-check no misclassified tags'(test: Test) { const prop = specification.ResourceTypes['AWS::SNS::Subscription'].Properties!.DeliveryPolicy; - test.equals(prop.ScrutinyType, ScrutinyType.None); + test.equals(prop.ScrutinyType, PropertyScrutinyType.None); + + test.done(); + }, + + 'check Lambda permission resource scrutiny'(test: Test) { + const resource = specification.ResourceTypes['AWS::Lambda::Permission']; + test.equals(resource.ScrutinyType, ResourceScrutinyType.LambdaPermission); test.done(); }, diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts index 7975d47e79f70..0add877280471 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts @@ -26,7 +26,7 @@ export function diffParameter(oldValue: types.Parameter, newValue: types.Paramet return new types.ParameterDifference(oldValue, newValue); } -export function diffResource(oldValue: types.Resource, newValue: types.Resource): types.ResourceDifference { +export function diffResource(oldValue?: types.Resource, newValue?: types.Resource): types.ResourceDifference { const resourceType = { oldType: oldValue && oldValue.Type, newType: newValue && newValue.Type @@ -34,12 +34,12 @@ export function diffResource(oldValue: types.Resource, newValue: types.Resource) let propertyUpdates: { [key: string]: types.PropertyDifference } = {}; let otherChanges: { [key: string]: types.Difference } = {}; - if (resourceType.oldType === resourceType.newType) { + if (resourceType.oldType !== undefined && resourceType.oldType === resourceType.newType) { // Only makes sense to inspect deeper if the types stayed the same const typeSpec = cfnspec.filteredSpecification(resourceType.oldType); const impl = typeSpec.ResourceTypes[resourceType.oldType]; - propertyUpdates = diffKeyedEntities(oldValue.Properties, - newValue.Properties, + propertyUpdates = diffKeyedEntities(oldValue!.Properties, + newValue!.Properties, (oldVal, newVal, key) => _diffProperty(oldVal, newVal, key, impl)); otherChanges = diffKeyedEntities(oldValue, newValue, _diffOther); delete otherChanges.Properties; @@ -47,8 +47,8 @@ export function diffResource(oldValue: types.Resource, newValue: types.Resource) return new types.ResourceDifference(oldValue, newValue, { resourceType, propertyUpdates, otherChanges, - oldProperties: (oldValue && oldValue.Properties) || {}, - newProperties: (newValue && newValue.Properties) || {} + oldProperties: oldValue && oldValue.Properties, + newProperties: newValue && newValue.Properties, }); function _diffProperty(oldV: any, newV: any, key: string, resourceSpec?: cfnspec.schema.ResourceType) { diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index 2aa20569f7af2..75af799e334dd 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -3,6 +3,8 @@ import { AssertionError } from 'assert'; import { IamChanges } from '../iam/iam-changes'; import { deepEqual } from './util'; +export type PropertyMap = {[key: string]: any }; + /** Semantic differences between two CloudFormation templates. */ export class TemplateDiff implements ITemplateDiff { public awsTemplateFormatVersion?: Difference; @@ -42,8 +44,10 @@ export class TemplateDiff implements ITemplateDiff { this.unknown = args.unknown || new DifferenceCollection({}); this.iamChanges = new IamChanges( - this.scrutinizablePropertyChanges(cfnspec.schema.ScrutinyType.IdentityPolicy), - this.scrutinizablePropertyChanges(cfnspec.schema.ScrutinyType.ResourcePolicy)); + this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.IdentityPolicy), + this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.ResourcePolicy), + this.scrutinizableResourceChanges(cfnspec.schema.ResourceScrutinyType.LambdaPermission), + ); } public get count() { @@ -80,7 +84,7 @@ export class TemplateDiff implements ITemplateDiff { * We don't just look at property updates; we also look at resource additions and deletions (in which * case there is no further detail on property values), and resource type changes. */ - public scrutinizablePropertyChanges(scrutinyType: cfnspec.schema.ScrutinyType): PropertyChange[] { + public scrutinizablePropertyChanges(scrutinyType: cfnspec.schema.PropertyScrutinyType): PropertyChange[] { const ret = new Array(); for (const [resourceLogicalId, resourceChange] of Object.entries(this.resources.changes)) { @@ -90,14 +94,60 @@ export class TemplateDiff implements ITemplateDiff { for (const propertyName of props) { ret.push({ resourceLogicalId, propertyName, - oldValue: resourceChange.oldProperties[propertyName], - newValue: resourceChange.newProperties[propertyName], + oldValue: resourceChange.oldProperties && resourceChange.oldProperties[propertyName], + newValue: resourceChange.newProperties && resourceChange.newProperties[propertyName], }); } } return ret; } + + /** + * Return all resource changes of a given scrutiny type + * + * We don't just look at resource updates; we also look at resource additions and deletions (in which + * case there is no further detail on property values), and resource type changes. + */ + public scrutinizableResourceChanges(scrutinyType: cfnspec.schema.ResourceScrutinyType): ResourceChange[] { + const ret = new Array(); + + const scrutinizableTypes = new Set(cfnspec.scrutinizableResourceTypes(scrutinyType)); + + for (const [resourceLogicalId, resourceChange] of Object.entries(this.resources.changes)) { + if (!resourceChange) { continue; } + + // Even though it's not physically possible in CFN, let's pretend to handle a change of 'Type'. + if (resourceChange.resourceTypeChanged) { + // Treat as DELETE+ADD + if (scrutinizableTypes.has(resourceChange.oldResourceType!)) { + ret.push({ + oldProperties: resourceChange.oldProperties, + resourceLogicalId, + resourceType: resourceChange.oldResourceType! + }); + } + if (scrutinizableTypes.has(resourceChange.newResourceType!)) { + ret.push({ + newProperties: resourceChange.newProperties, + resourceLogicalId, + resourceType: resourceChange.newResourceType! + }); + } + } else { + if (scrutinizableTypes.has(resourceChange.resourceType)) { + ret.push({ + oldProperties: resourceChange.oldProperties, + newProperties: resourceChange.newProperties, + resourceLogicalId, + resourceType: resourceChange.resourceType + }); + } + } + } + + return ret; + } } /** @@ -130,6 +180,33 @@ export interface PropertyChange { newValue?: any; } +/** + * A resource change + * + * Either a creation, deletion or update. + */ +export interface ResourceChange { + /** + * Logical ID of the resource where this property change was found + */ + resourceLogicalId: string; + + /** + * The type of the resource + */ + resourceType: string; + + /** + * The old properties value (might be undefined in case of creation) + */ + oldProperties?: PropertyMap; + + /** + * The new properties value (might be undefined in case of deletion) + */ + newProperties?: PropertyMap; +} + /** * Models an entity that changed between two versions of a CloudFormation template. */ @@ -329,12 +406,12 @@ export class ResourceDifference extends Difference { /** * Old property values */ - public readonly oldProperties: { [name: string]: any }; + public readonly oldProperties?: PropertyMap; /** * New property values */ - public readonly newProperties: { [name: string]: any }; + public readonly newProperties?: PropertyMap; /** Property-level changes on the resource */ public readonly propertyUpdates: { [key: string]: PropertyDifference }; @@ -342,20 +419,20 @@ export class ResourceDifference extends Difference { public readonly otherChanges: { [key: string]: Difference }; /** The resource type (or old and new type if it has changed) */ - private readonly resourceType: { readonly oldType: string, readonly newType: string }; + private readonly resourceTypes: { readonly oldType?: string, readonly newType?: string }; constructor(oldValue: Resource | undefined, newValue: Resource | undefined, args: { - resourceType: { oldType: string, newType: string }, - oldProperties: { [name: string]: any }, - newProperties: { [name: string]: any }, + resourceType: { oldType?: string, newType?: string }, + oldProperties?: PropertyMap, + newProperties?: PropertyMap, propertyUpdates: { [key: string]: PropertyDifference }, otherChanges: { [key: string]: Difference } } ) { super(oldValue, newValue); - this.resourceType = args.resourceType; + this.resourceTypes = args.resourceType; this.propertyUpdates = args.propertyUpdates; this.otherChanges = args.otherChanges; this.oldProperties = args.oldProperties; @@ -363,18 +440,42 @@ export class ResourceDifference extends Difference { } public get oldResourceType(): string | undefined { - return this.resourceType.oldType; + return this.resourceTypes.oldType; } public get newResourceType(): string | undefined { - return this.resourceType.newType; + return this.resourceTypes.newType; + } + + /** + * Return whether the resource type was changed in this diff + * + * This is not a valid operation in CloudFormation but to be defensive we're going + * to be aware of it anyway. + */ + public get resourceTypeChanged(): boolean { + return (this.resourceTypes.oldType !== undefined + && this.resourceTypes.newType !== undefined + && this.resourceTypes.oldType !== this.resourceTypes.newType); + } + + /** + * Return the resource type if it was unchanged + * + * If the resource type was changed, it's an error to call this. + */ + public get resourceType(): string { + if (this.resourceTypeChanged) { + throw new Error('Cannot get .resourceType, because the type was changed'); + } + return this.resourceTypes.oldType || this.resourceTypes.newType!; } public get changeImpact(): ResourceImpact { // Check the Type first - if (this.resourceType.oldType !== this.resourceType.newType) { - if (this.resourceType.oldType === undefined) { return ResourceImpact.WILL_CREATE; } - if (this.resourceType.newType === undefined) { + if (this.resourceTypes.oldType !== this.resourceTypes.newType) { + if (this.resourceTypes.oldType === undefined) { return ResourceImpact.WILL_CREATE; } + if (this.resourceTypes.newType === undefined) { return this.oldValue!.DeletionPolicy === 'Retain' ? ResourceImpact.WILL_ORPHAN : ResourceImpact.WILL_DESTROY; diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts index 79a5765f69d38..a8100ce9648ba 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts @@ -1,6 +1,6 @@ import colors = require('colors/safe'); -import { PropertyChange } from "../diff/types"; -import { parseStatements, Statement, Targets } from "./statement"; +import { PropertyChange, PropertyMap, ResourceChange } from "../diff/types"; +import { parseLambdaPermission, parseStatements, Statement, Targets } from "./statement"; import { unCloudFormation } from "./uncfn"; /** @@ -12,7 +12,7 @@ export class IamChanges { private oldStatements: Statement[] = []; private newStatements: Statement[] = []; - constructor(identityPolicyChanges: PropertyChange[], resourcePolicyChanges: PropertyChange[]) { + constructor(identityPolicyChanges: PropertyChange[], resourcePolicyChanges: PropertyChange[], lambdaPermissionChanges: ResourceChange[]) { for (const policyChange of identityPolicyChanges) { this.oldStatements.push(...this.readIdentityStatements(policyChange.oldValue, policyChange.propertyName, policyChange.resourceLogicalId)); this.newStatements.push(...this.readIdentityStatements(policyChange.newValue, policyChange.propertyName, policyChange.resourceLogicalId)); @@ -21,6 +21,10 @@ export class IamChanges { this.oldStatements.push(...this.readResourceStatements(policyChange.oldValue, policyChange.resourceLogicalId)); this.newStatements.push(...this.readResourceStatements(policyChange.newValue, policyChange.resourceLogicalId)); } + for (const lambdaChange of lambdaPermissionChanges) { + this.oldStatements.push(...this.readLambdaStatements(lambdaChange.oldProperties)); + this.newStatements.push(...this.readLambdaStatements(lambdaChange.newProperties)); + } this.additions.push(...this.newStatements.filter(s => !hasStatement(s, this.oldStatements))); this.removals.push(...this.oldStatements.filter(s => !hasStatement(s, this.newStatements))); @@ -112,6 +116,12 @@ export class IamChanges { return statements; } + + private readLambdaStatements(properties?: PropertyMap): Statement[] { + if (!properties) { return []; } + + return [parseLambdaPermission(unCloudFormation(properties))]; + } } function hasStatement(statement: Statement, ss: Statement[]) { diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts index 0b1e192fe05d1..62bb750876c7a 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts @@ -51,12 +51,10 @@ export class Statement { && this.principals.equal(other.principals) && deepEqual(this.condition, other.condition)); } - } /** * Parse a list of statements from undefined, a Statement, or a list of statements - * @param x Pa */ export function parseStatements(x: any): Statement[] { if (x === undefined) { x = []; } @@ -64,6 +62,45 @@ export function parseStatements(x: any): Statement[] { return x.map((s: any) => new Statement(s)); } +/** + * Parse a Statement from a Lambda::Permission object + * + * This is actually what Lambda adds to the policy document if you call AddPermission. + */ +export function parseLambdaPermission(x: any): Statement { + // Construct a statement from + const statement: any = { + Effect: 'Allow', + Action: x.Action, + Resource: x.FunctionName, + }; + + if (x.Principal !== undefined) { + if (x.Principal === '*') { + // * + statement.Principal = '*'; + } else if (/^\d{12}$/.test(x.Principal)) { + // Account number + statement.Principal = { AWS: `arn:aws:iam::${x.Principal}:root` }; + } else { + // Assume it's a service principal + // We might get this wrong vs. the previous one for tokens. Nothing to be done + // about that. It's only for human readable consumption after all. + statement.Principal = { Service: x.Principal }; + } + } + if (x.SourceArn !== undefined) { + if (statement.Condition === undefined) { statement.Condition = {}; } + statement.Condition.ArnLike = { 'AWS:SourceArn': x.SourceArn }; + } + if (x.SourceAccount !== undefined) { + if (statement.Condition === undefined) { statement.Condition = {}; } + statement.Condition.StringEquals = { 'AWS:SourceAccount': x.SourceAccount }; + } + + return new Statement(statement); +} + /** * Targets for a field */ @@ -141,11 +178,12 @@ function expectEffect(x: unknown): Effect { function forceListOfStrings(x: unknown): string[] { if (typeof x === 'string') { return [x]; } if (typeof x === 'undefined' || x === null) { return []; } + if (Array.isArray(x)) { return x.map(el => `${el}`); } - if (typeof x === 'object') { + if (typeof x === 'object' && x !== null) { const ret: string[] = []; for (const [key, value] of Object.entries(x)) { ret.push(...forceListOfStrings(value).map(s => `${key}:${s}`)); From b403e1c4bc09a0ca9cca20597f172f83d6c32582 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 26 Nov 2018 16:47:15 +0100 Subject: [PATCH 04/23] Make Condition representation more compact --- .../cloudformation-diff/lib/iam/iam-changes.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts index a8100ce9648ba..23555bba5e181 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts @@ -143,7 +143,20 @@ function renderTargets(targets: Targets): string { */ function renderCondition(condition: any) { if (!condition || Object.keys(condition).length === 0) { return ''; } - return JSON.stringify(condition, undefined, 2); + const jsonRepresentation = JSON.stringify(condition, undefined, 2); + + // The JSON representation looks like this: + // + // { + // "ArnLike": { + // "AWS:SourceArn": "${MyTopic86869434}" + // } + // } + // + // We can make it more compact without losing information by getting rid of the outermost braces + // and the indentation. + const lines = jsonRepresentation.split('\n'); + return lines.slice(1, lines.length - 1).map(s => s.substr(2)).join('\n'); } /** From 7fa16c7bf184130f0f2fe32292bef82df77db1a4 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 27 Nov 2018 14:22:49 +0100 Subject: [PATCH 05/23] Recognize and list managed policies as well --- .../cloudformation-diff/lib/iam/managed-policy.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts new file mode 100644 index 0000000000000..3f1e63b8d8a1a --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts @@ -0,0 +1,13 @@ +export class ManagedPolicyAttachment { + constructor(public readonly identityArn: string, public readonly managedPolicyArn: string) { + } + + public equal(other: ManagedPolicyAttachment) { + return this.identityArn === other.identityArn + && this.managedPolicyArn === other.managedPolicyArn; + } +} + +export function parseManagedPolicies(identityArn: string, arns: string[]): ManagedPolicyAttachment[] { + return arns.map((arn: string) => new ManagedPolicyAttachment(identityArn, arn)); +} \ No newline at end of file From 27bc21e73164556cd835b67963415ebc134e2200 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 27 Nov 2018 14:22:49 +0100 Subject: [PATCH 06/23] Recognize and list managed policies as well --- .../@aws-cdk/cfnspec/build-tools/scrutiny.ts | 7 + .../@aws-cdk/cfnspec/lib/schema/property.ts | 5 + .../@aws-cdk/cfnspec/test/test.scrutiny.ts | 7 + .../cloudformation-diff/lib/diff/types.ts | 11 +- .../cloudformation-diff/lib/format.ts | 25 +++- .../lib/iam/iam-changes.ts | 127 +++++++++++++++--- .../lib/iam/managed-policy.ts | 13 ++ 7 files changed, 164 insertions(+), 31 deletions(-) create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts diff --git a/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts b/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts index 30011d1e3d93b..ad47b80b12af0 100644 --- a/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts +++ b/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts @@ -19,6 +19,13 @@ export function detectScrutinyTypes(spec: schema.Specification) { for (const [propertyName, propertySpec] of Object.entries(typeSpec.Properties || {})) { if (propertySpec.ScrutinyType !== undefined) { continue; } // Only for unassigned + // Detect fields named like ManagedPolicyArns + if (propertyName === 'ManagedPolicyArns') { + propertySpec.ScrutinyType = PropertyScrutinyType.ManagedPolicies; + continue; + } + + // Detect fields named like '*Policy*' const nameContainsPolicy = propertyName.indexOf('Policy') > -1; const primitiveType = schema.isPrimitiveProperty(propertySpec) && propertySpec.PrimitiveType; diff --git a/packages/@aws-cdk/cfnspec/lib/schema/property.ts b/packages/@aws-cdk/cfnspec/lib/schema/property.ts index 9c5bac57142d1..92914e6e4ebf2 100644 --- a/packages/@aws-cdk/cfnspec/lib/schema/property.ts +++ b/packages/@aws-cdk/cfnspec/lib/schema/property.ts @@ -177,6 +177,11 @@ export enum PropertyScrutinyType { * This is an IAM policy on a resource */ ResourcePolicy = 'ResourcePolicy', + + /** + * A list of managed policies on a resource + */ + ManagedPolicies = 'ManagedPolicies', } export function isPropertyScrutinyType(str: string): str is PropertyScrutinyType { diff --git a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts index f85c0ff740906..093ed0de010e5 100644 --- a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts +++ b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts @@ -30,4 +30,11 @@ export = { test.done(); }, + + 'check role managedpolicyarns'(test: Test) { + const prop = specification.ResourceTypes['AWS::IAM::Role'].Properties!.ManagedPolicyArns; + test.equals(prop.ScrutinyType, PropertyScrutinyType.ManagedPolicies); + + test.done(); + }, }; \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index 75af799e334dd..2f08919e21be2 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -43,11 +43,12 @@ export class TemplateDiff implements ITemplateDiff { this.resources = args.resources || new DifferenceCollection({}); this.unknown = args.unknown || new DifferenceCollection({}); - this.iamChanges = new IamChanges( - this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.IdentityPolicy), - this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.ResourcePolicy), - this.scrutinizableResourceChanges(cfnspec.schema.ResourceScrutinyType.LambdaPermission), - ); + this.iamChanges = new IamChanges({ + identityPolicyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.IdentityPolicy), + resourcePolicyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.ResourcePolicy), + managedPolicyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.ManagedPolicies), + lambdaPermissionChanges: this.scrutinizableResourceChanges(cfnspec.schema.ResourceScrutinyType.LambdaPermission), + }); } public get count() { diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index 1ce86b984f4a0..dedd7a7707833 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -30,7 +30,7 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp printSectionFooter(); } - if (!templateDiff.iamChanges.empty) { + if (templateDiff.iamChanges.hasChanges) { printSectionHeader('Summary of IAM Changes'); formatIamChanges(templateDiff.iamChanges); printSectionFooter(); @@ -279,11 +279,24 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp } function formatIamChanges(changes: IamChanges) { - const summary = changes.summarize(); - const head = summary.splice(0, 1)[0]; + const tables: string[] = []; - const table = new Table({ head, style: { head: [] } }); - table.push(...summary); - print(table.toString()); + if (changes.hasStatementChanges) { + tables.push(renderTable(changes.summarizeStatements())); + } + + if (changes.hasManagedPolicyChanges) { + tables.push(renderTable(changes.summarizeManagedPolicies())); + } + + print(tables.join('\n\n')); } } + +function renderTable(cells: string[][]): string { + const head = cells.splice(0, 1)[0]; + + const table = new Table({ head, style: { head: [] } }); + table.push(...cells); + return table.toString(); +} diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts index 23555bba5e181..1d4bd81861153 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts @@ -1,57 +1,93 @@ import colors = require('colors/safe'); import { PropertyChange, PropertyMap, ResourceChange } from "../diff/types"; +import { ManagedPolicyAttachment, parseManagedPolicies } from './managed-policy'; import { parseLambdaPermission, parseStatements, Statement, Targets } from "./statement"; import { unCloudFormation } from "./uncfn"; +export interface IamChangesProps { + identityPolicyChanges: PropertyChange[]; + resourcePolicyChanges: PropertyChange[]; + lambdaPermissionChanges: ResourceChange[]; + managedPolicyChanges: PropertyChange[]; +} + /** * Changes to IAM statements */ export class IamChanges { - public readonly additions: Statement[] = []; - public readonly removals: Statement[] = []; + public readonly statementAdditions: Statement[] = []; + public readonly statementRemovals: Statement[] = []; + public readonly managedPolicyAdditions: ManagedPolicyAttachment[] = []; + public readonly managedPolicyRemovals: ManagedPolicyAttachment[] = []; + private oldStatements: Statement[] = []; private newStatements: Statement[] = []; + private oldPolicyAttachments: ManagedPolicyAttachment[] = []; + private newPolicyAttachments: ManagedPolicyAttachment[] = []; - constructor(identityPolicyChanges: PropertyChange[], resourcePolicyChanges: PropertyChange[], lambdaPermissionChanges: ResourceChange[]) { - for (const policyChange of identityPolicyChanges) { + constructor(props: IamChangesProps) { + for (const policyChange of props.identityPolicyChanges) { this.oldStatements.push(...this.readIdentityStatements(policyChange.oldValue, policyChange.propertyName, policyChange.resourceLogicalId)); this.newStatements.push(...this.readIdentityStatements(policyChange.newValue, policyChange.propertyName, policyChange.resourceLogicalId)); } - for (const policyChange of resourcePolicyChanges) { + for (const policyChange of props.resourcePolicyChanges) { this.oldStatements.push(...this.readResourceStatements(policyChange.oldValue, policyChange.resourceLogicalId)); this.newStatements.push(...this.readResourceStatements(policyChange.newValue, policyChange.resourceLogicalId)); } - for (const lambdaChange of lambdaPermissionChanges) { + for (const lambdaChange of props.lambdaPermissionChanges) { this.oldStatements.push(...this.readLambdaStatements(lambdaChange.oldProperties)); this.newStatements.push(...this.readLambdaStatements(lambdaChange.newProperties)); } + for (const managedPolicyChange of props.managedPolicyChanges) { + this.oldPolicyAttachments.push(...this.readManagedPolicies(managedPolicyChange.oldValue, managedPolicyChange.resourceLogicalId)); + this.newPolicyAttachments.push(...this.readManagedPolicies(managedPolicyChange.newValue, managedPolicyChange.resourceLogicalId)); + } + + this.statementAdditions.push(...difference(this.newStatements, this.oldStatements)); + this.statementRemovals.push(...difference(this.oldStatements, this.newStatements)); + + this.managedPolicyAdditions.push(...difference(this.newPolicyAttachments, this.oldPolicyAttachments)); + this.managedPolicyAdditions.push(...difference(this.oldPolicyAttachments, this.newPolicyAttachments)); + } + + public get hasChanges() { + return this.hasManagedPolicyChanges || this.hasStatementChanges; + } - this.additions.push(...this.newStatements.filter(s => !hasStatement(s, this.oldStatements))); - this.removals.push(...this.oldStatements.filter(s => !hasStatement(s, this.newStatements))); + public get hasStatementChanges() { + return this.statementAdditions.length + this.statementRemovals.length > 0; } - public get empty() { - return this.additions.length + this.removals.length === 0; + public get hasStatementAdditions() { + return this.statementAdditions.length > 0; } - public get hasAdditions() { - return this.additions.length > 0; + public get hasStatementRemovals() { + return this.statementRemovals.length > 0; } - public get hasRemovals() { - return this.removals.length > 0; + public get hasManagedPolicyChanges() { + return this.managedPolicyAdditions.length + this.managedPolicyRemovals.length > 0; + } + + public get hasManagedPolicyAdditions() { + return this.managedPolicyAdditions.length > 0; + } + + public get hasManagedPolicyRemovals() { + return this.managedPolicyRemovals.length > 0; } /** * Return a summary table of changes */ - public summarize(): string[][] { + public summarizeStatements(): string[][] { const ret: string[][] = []; - const header = ['', 'Resource', 'Effect', 'Actions', 'Principals', 'Condition']; + const header = ['', 'Resource', 'Effect', 'Action', 'Principal', 'Condition']; // First generate all lines, then sort on Resource so that similar resources are together - for (const statement of this.additions) { + for (const statement of this.statementAdditions) { ret.push([ '+', renderTargets(statement.resources), @@ -61,7 +97,7 @@ export class IamChanges { renderCondition(statement.condition) ].map(s => colors.green(s))); } - for (const statement of this.removals) { + for (const statement of this.statementRemovals) { ret.push([ colors.red('-'), renderTargets(statement.resources), @@ -80,6 +116,33 @@ export class IamChanges { return ret; } + public summarizeManagedPolicies(): string[][] { + const ret: string[][] = []; + const header = ['', 'Resource', 'Managed Policy ARN']; + + for (const att of this.managedPolicyAdditions) { + ret.push([ + '+', + att.identityArn, + att.managedPolicyArn, + ].map(s => colors.green(s))); + } + for (const att of this.managedPolicyRemovals) { + ret.push([ + '-', + att.identityArn, + att.managedPolicyArn, + ].map(s => colors.red(s))); + } + + // Sort by 2nd column + ret.sort(makeComparator((row: string[]) => row[1])); + + ret.splice(0, 0, header); + + return ret; + } + private readIdentityStatements(policy: any, propertyName: string, logicalId: string): Statement[] { if (!policy) { return []; } @@ -117,6 +180,13 @@ export class IamChanges { return statements; } + private readManagedPolicies(policyArns: string[] | undefined, logicalId: string): ManagedPolicyAttachment[] { + if (!policyArns) { return []; } + + const rep = '${' + logicalId + '.Arn}'; + return parseManagedPolicies(rep, unCloudFormation(policyArns)); + } + private readLambdaStatements(properties?: PropertyMap): Statement[] { if (!properties) { return []; } @@ -124,8 +194,25 @@ export class IamChanges { } } -function hasStatement(statement: Statement, ss: Statement[]) { - return ss.some(s => statement.equal(s)); +/** + * Things that can be compared to themselves (by value) + */ +interface Eq { + equal(other: T): boolean; +} + +/** + * Whether a collection contains some element (by value) + */ +function contains>(element: T, xs: T[]) { + return xs.some(x => x.equal(element)); +} + +/** + * Return collection except for elements + */ +function difference>(collection: T[], elements: T[]) { + return collection.filter(x => !contains(x, elements)); } /** diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts new file mode 100644 index 0000000000000..3f1e63b8d8a1a --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts @@ -0,0 +1,13 @@ +export class ManagedPolicyAttachment { + constructor(public readonly identityArn: string, public readonly managedPolicyArn: string) { + } + + public equal(other: ManagedPolicyAttachment) { + return this.identityArn === other.identityArn + && this.managedPolicyArn === other.managedPolicyArn; + } +} + +export function parseManagedPolicies(identityArn: string, arns: string[]): ManagedPolicyAttachment[] { + return arns.map((arn: string) => new ManagedPolicyAttachment(identityArn, arn)); +} \ No newline at end of file From 1899e6a9ef8bb04ee00ad8c89cdc7a32d1687a2f Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 27 Nov 2018 14:26:22 +0100 Subject: [PATCH 07/23] This file is not necessary --- packages/@aws-cdk/cfnspec/parse.py | 13 ------------- 1 file changed, 13 deletions(-) delete mode 100644 packages/@aws-cdk/cfnspec/parse.py diff --git a/packages/@aws-cdk/cfnspec/parse.py b/packages/@aws-cdk/cfnspec/parse.py deleted file mode 100644 index 960723112fb95..0000000000000 --- a/packages/@aws-cdk/cfnspec/parse.py +++ /dev/null @@ -1,13 +0,0 @@ -import json - -def get_type(propspec): - return propspec.get('PrimitiveType', '???') - -model = json.load(open('spec-source/000_CloudFormationResourceSpecification.json')) - -types = ['PropertyTypes', 'ResourceTypes'] -for type in types: - for name, typespec in model[type].items(): - for propname, propspec in typespec['Properties'].items(): - if 'policy' in propname.lower(): - print('%s:%s -> %s' % (name, propname, get_type(propspec))) From 9366966c62885f9c34c81651ba754b2ce3a7c4cd Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 27 Nov 2018 14:40:28 +0100 Subject: [PATCH 08/23] Make loading of specification.json lazy 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. --- packages/@aws-cdk/cfnspec/lib/index.ts | 29 ++++++++++++------- .../test/test.filtered-specification.ts | 2 +- .../@aws-cdk/cfnspec/test/test.namespaces.ts | 2 +- .../@aws-cdk/cfnspec/test/test.scrutiny.ts | 10 +++---- .../@aws-cdk/cloudformation-diff/package.json | 2 +- 5 files changed, 26 insertions(+), 19 deletions(-) diff --git a/packages/@aws-cdk/cfnspec/lib/index.ts b/packages/@aws-cdk/cfnspec/lib/index.ts index 90a864f3981cc..6db7b206b3e4f 100644 --- a/packages/@aws-cdk/cfnspec/lib/index.ts +++ b/packages/@aws-cdk/cfnspec/lib/index.ts @@ -5,18 +5,23 @@ export { schema }; /** * The complete AWS CloudFormation Resource specification, having any CDK patches and enhancements included in it. */ -// tslint:disable-next-line:no-var-requires -export const specification: schema.Specification = require('../spec/specification.json'); +export function specification(): schema.Specification { + return require('../spec/specification.json'); +} /** * The list of resource type names defined in the ``specification``. */ -export const resourceTypes = Object.keys(specification.ResourceTypes); +export function resourceTypes() { + return Object.keys(specification().ResourceTypes); +} /** * The list of namespaces defined in the ``specification``, that is resource name prefixes down to the second ``::``. */ -export const namespaces = Array.from(new Set(resourceTypes.map(n => n.split('::', 2).join('::')))); +export function namespaces() { + return Array.from(new Set(resourceTypes().map(n => n.split('::', 2).join('::')))); +} /** * Obtain a filtered version of the AWS CloudFormation specification. @@ -30,14 +35,16 @@ export const namespaces = Array.from(new Set(resourceTypes.map(n => n.split('::' * to the selected resource types. */ export function filteredSpecification(filter: string | RegExp | Filter): schema.Specification { - const result: schema.Specification = { ResourceTypes: {}, PropertyTypes: {}, Fingerprint: specification.Fingerprint }; + const spec = specification(); + + const result: schema.Specification = { ResourceTypes: {}, PropertyTypes: {}, Fingerprint: spec.Fingerprint }; const predicate: Filter = makePredicate(filter); - for (const type of resourceTypes) { + for (const type of resourceTypes()) { if (!predicate(type)) { continue; } - result.ResourceTypes[type] = specification.ResourceTypes[type]; + result.ResourceTypes[type] = spec.ResourceTypes[type]; const prefix = `${type}.`; - for (const propType of Object.keys(specification.PropertyTypes!).filter(n => n.startsWith(prefix))) { - result.PropertyTypes[propType] = specification.PropertyTypes![propType]; + for (const propType of Object.keys(spec.PropertyTypes!).filter(n => n.startsWith(prefix))) { + result.PropertyTypes[propType] = spec.PropertyTypes![propType]; } } result.Fingerprint = crypto.createHash('sha256').update(JSON.stringify(result)).digest('base64'); @@ -69,7 +76,7 @@ function makePredicate(filter: string | RegExp | Filter): Filter { * Return the properties of the given type that require the given scrutiny type */ export function scrutinizablePropertyNames(resourceType: string, scrutinyType: schema.PropertyScrutinyType): string[] { - const impl = specification.ResourceTypes[resourceType]; + const impl = specification().ResourceTypes[resourceType]; if (!impl) { return []; } const ret = new Array(); @@ -88,7 +95,7 @@ export function scrutinizablePropertyNames(resourceType: string, scrutinyType: s */ export function scrutinizableResourceTypes(scrutinyType: schema.ResourceScrutinyType): string[] { const ret = new Array(); - for (const [resourceType, resourceSpec] of Object.entries(specification.ResourceTypes)) { + for (const [resourceType, resourceSpec] of Object.entries(specification().ResourceTypes)) { if ((resourceSpec.ScrutinyType || schema.PropertyScrutinyType.None) === scrutinyType) { ret.push(resourceType); } diff --git a/packages/@aws-cdk/cfnspec/test/test.filtered-specification.ts b/packages/@aws-cdk/cfnspec/test/test.filtered-specification.ts index b96fb5b244cef..1296a1bd2486c 100644 --- a/packages/@aws-cdk/cfnspec/test/test.filtered-specification.ts +++ b/packages/@aws-cdk/cfnspec/test/test.filtered-specification.ts @@ -17,7 +17,7 @@ const tests: any = { } }; -for (const name of resourceTypes.sort()) { +for (const name of resourceTypes().sort()) { tests[`filteredSpecification(${JSON.stringify(name)})`] = (test: Test) => { const filteredSpec = filteredSpecification(name); test.notDeepEqual(filteredSpec, specification, `The filteredSpecification result is not the whole specification`); diff --git a/packages/@aws-cdk/cfnspec/test/test.namespaces.ts b/packages/@aws-cdk/cfnspec/test/test.namespaces.ts index f28d44c9e2173..2427bdb75ac5f 100644 --- a/packages/@aws-cdk/cfnspec/test/test.namespaces.ts +++ b/packages/@aws-cdk/cfnspec/test/test.namespaces.ts @@ -3,7 +3,7 @@ import { namespaces } from '../lib/index'; export = testCase({ 'expected namespaces are present'(test: Test) { - test.deepEqual(namespaces.sort(), expectedNamespaces.sort()); + test.deepEqual(namespaces().sort(), expectedNamespaces.sort()); test.done(); } }); diff --git a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts index 093ed0de010e5..401c11e27d661 100644 --- a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts +++ b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts @@ -4,35 +4,35 @@ import { PropertyScrutinyType, ResourceScrutinyType } from '../lib/schema'; export = { 'spot-check IAM identity tags'(test: Test) { - const prop = specification.PropertyTypes['AWS::IAM::Role.Policy'].Properties!.PolicyDocument; + const prop = specification().PropertyTypes['AWS::IAM::Role.Policy'].Properties!.PolicyDocument; test.equals(prop.ScrutinyType, PropertyScrutinyType.IdentityPolicy); test.done(); }, 'spot-check IAM resource tags'(test: Test) { - const prop = specification.ResourceTypes['AWS::KMS::Key'].Properties!.KeyPolicy; + const prop = specification().ResourceTypes['AWS::KMS::Key'].Properties!.KeyPolicy; test.equals(prop.ScrutinyType, PropertyScrutinyType.ResourcePolicy); test.done(); }, 'spot-check no misclassified tags'(test: Test) { - const prop = specification.ResourceTypes['AWS::SNS::Subscription'].Properties!.DeliveryPolicy; + const prop = specification().ResourceTypes['AWS::SNS::Subscription'].Properties!.DeliveryPolicy; test.equals(prop.ScrutinyType, PropertyScrutinyType.None); test.done(); }, 'check Lambda permission resource scrutiny'(test: Test) { - const resource = specification.ResourceTypes['AWS::Lambda::Permission']; + const resource = specification().ResourceTypes['AWS::Lambda::Permission']; test.equals(resource.ScrutinyType, ResourceScrutinyType.LambdaPermission); test.done(); }, 'check role managedpolicyarns'(test: Test) { - const prop = specification.ResourceTypes['AWS::IAM::Role'].Properties!.ManagedPolicyArns; + const prop = specification().ResourceTypes['AWS::IAM::Role'].Properties!.ManagedPolicyArns; test.equals(prop.ScrutinyType, PropertyScrutinyType.ManagedPolicies); test.done(); diff --git a/packages/@aws-cdk/cloudformation-diff/package.json b/packages/@aws-cdk/cloudformation-diff/package.json index 6437a3ffcc7fe..dc80f4397ac8f 100644 --- a/packages/@aws-cdk/cloudformation-diff/package.json +++ b/packages/@aws-cdk/cloudformation-diff/package.json @@ -25,13 +25,13 @@ "dependencies": { "@aws-cdk/cfnspec": "^0.18.1", "@aws-cdk/cx-api": "^0.18.1", - "@types/cli-table": "^0.3.0", "cli-table": "^0.3.1", "colors": "^1.2.1", "fast-deep-equal": "^2.0.1", "source-map-support": "^0.5.6" }, "devDependencies": { + "@types/cli-table": "^0.3.0", "cdk-build-tools": "^0.18.1", "pkglint": "^0.18.1" }, From 6bcf9a9650fde7c938d8f8fa62285dc476b45dc0 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 27 Nov 2018 15:16:56 +0100 Subject: [PATCH 09/23] Translate logical IDs to construct paths if possible --- .../cloudformation-diff/lib/format.ts | 71 +++++++++++++------ 1 file changed, 50 insertions(+), 21 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index dedd7a7707833..2bef4c3777645 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -19,6 +19,8 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp stream.write(colors.white(format(fmt, ...args)) + '\n'); } + completeLogicalPathMap(); + const ADDITION = colors.green('[+]'); const UPDATE = colors.yellow('[~]'); const REMOVAL = colors.red('[-]'); @@ -101,7 +103,7 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp const resourceType = diff.isRemoval ? diff.oldResourceType : diff.newResourceType; // tslint:disable-next-line:max-line-length - print(`${formatPrefix(diff)} ${formatValue(resourceType, colors.cyan)} ${formatLogicalId(logicalId, diff)} ${formatImpact(diff.changeImpact)}`); + print(`${formatPrefix(diff)} ${formatValue(resourceType, colors.cyan)} ${formatLogicalId(logicalId)} ${formatImpact(diff.changeImpact)}`); if (diff.isUpdate) { let processedCount = 0; @@ -226,28 +228,43 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp } } - function formatLogicalId(logicalId: string, diff?: ResourceDifference) { - // if we have a path in the map, return it - const path = logicalToPathMap[logicalId]; - if (path) { - // first component of path is the stack name, so let's remove that - return normalizePath(path); - } + /** + * Find 'aws:cdk:path' metadata in the diff and add it to the logicalToPathMap + * + * There are multiple sources of logicalID -> path mappings: synth metadata + * and resource metadata, and we combine all sources into a single map. + */ + function completeLogicalPathMap() { + for (const [logicalId, resourceDiff] of Object.entries(templateDiff.resources)) { + if (!resourceDiff) { continue; } + + const oldPathMetadata = resourceDiff.oldValue && resourceDiff.oldValue.Metadata && resourceDiff.oldValue.Metadata[cxapi.PATH_METADATA_KEY]; + if (oldPathMetadata && !(logicalId in logicalToPathMap)) { + logicalToPathMap[logicalId] = oldPathMetadata; + } - // if we don't have in our map, it might be a deleted resource, so let's try the - // template metadata - const oldPathMetadata = diff && diff.oldValue && diff.oldValue.Metadata && diff.oldValue.Metadata[cxapi.PATH_METADATA_KEY]; - if (oldPathMetadata) { - return normalizePath(oldPathMetadata); + const newPathMetadata = resourceDiff.newValue && resourceDiff.newValue.Metadata && resourceDiff.newValue.Metadata[cxapi.PATH_METADATA_KEY]; + if (newPathMetadata && !(logicalId in logicalToPathMap)) { + logicalToPathMap[logicalId] = newPathMetadata; + } } + } + + function formatLogicalId(logicalId: string) { + // if we have a path in the map, return it + const normalized = normalizedLogicalIdPath(logicalId); - const newPathMetadata = diff && diff.newValue && diff.newValue.Metadata && diff.newValue.Metadata[cxapi.PATH_METADATA_KEY]; - if (newPathMetadata) { - return normalizePath(newPathMetadata); + if (normalized) { + return `${normalized} ${colors.gray(logicalId)}`; } - // couldn't figure out the path, just return the logical ID return logicalId; + } + + function normalizedLogicalIdPath(logicalId: string): string | undefined { + // if we have a path in the map, return it + const path = logicalToPathMap[logicalId]; + return path ? normalizePath(path) : undefined; /** * Path is supposed to start with "/stack-name". If this is the case (i.e. path has more than @@ -273,8 +290,7 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp p = parts.join('/'); } - - return `${p} ${colors.gray(logicalId)}`; + return p; } } @@ -282,15 +298,28 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp const tables: string[] = []; if (changes.hasStatementChanges) { - tables.push(renderTable(changes.summarizeStatements())); + tables.push(renderTable(deepSubstituteBracedLogicalIds(changes.summarizeStatements()))); } if (changes.hasManagedPolicyChanges) { - tables.push(renderTable(changes.summarizeManagedPolicies())); + tables.push(renderTable(deepSubstituteBracedLogicalIds(changes.summarizeManagedPolicies()))); } print(tables.join('\n\n')); } + + function deepSubstituteBracedLogicalIds(rows: string[][]): string[][] { + return rows.map(row => row.map(substituteBracedLogicalIds)); + } + + /** + * Substitute all strings like ${LogId.xxx} with the path instead of the logical ID + */ + function substituteBracedLogicalIds(source: string): string { + return source.replace(/\$\{([^.}]+)(.[^}]+)?\}/ig, (_match, logId, suffix) => { + return '${' + (normalizedLogicalIdPath(logId) || logId) + (suffix || '') + '}'; + }); + } } function renderTable(cells: string[][]): string { From bb87db27afae57dd9fd5f5e847e65801e5e3fe84 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 28 Nov 2018 11:22:51 +0100 Subject: [PATCH 10/23] Add support for surfacing Security Group changes --- .../@aws-cdk/cfnspec/lib/schema/property.ts | 10 ++ .../cfnspec/lib/schema/resource-type.ts | 10 ++ .../spec-source/200_Scrutinies_patch.json | 41 ++++++ .../@aws-cdk/cfnspec/test/spec-validators.ts | 3 + .../@aws-cdk/cfnspec/test/test.scrutiny.ts | 18 +++ .../cloudformation-diff/lib/diff/types.ts | 13 ++ .../cloudformation-diff/lib/diffable.ts | 56 +++++++++ .../cloudformation-diff/lib/format.ts | 54 ++++++-- .../lib/iam/iam-changes.ts | 115 ++++++----------- .../lib/network/security-group-changes.ts | 101 +++++++++++++++ .../lib/network/security-group-rule.ts | 119 ++++++++++++++++++ 11 files changed, 453 insertions(+), 87 deletions(-) create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/diffable.ts create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/network/security-group-rule.ts diff --git a/packages/@aws-cdk/cfnspec/lib/schema/property.ts b/packages/@aws-cdk/cfnspec/lib/schema/property.ts index 92914e6e4ebf2..1c47bfa192f67 100644 --- a/packages/@aws-cdk/cfnspec/lib/schema/property.ts +++ b/packages/@aws-cdk/cfnspec/lib/schema/property.ts @@ -182,6 +182,16 @@ export enum PropertyScrutinyType { * A list of managed policies on a resource */ ManagedPolicies = 'ManagedPolicies', + + /** + * A set of ingress rules + */ + IngressRules = 'IngressRules', + + /** + * A set of egress rules + */ + EgressRules = 'EgressRules', } export function isPropertyScrutinyType(str: string): str is PropertyScrutinyType { diff --git a/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts b/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts index a77232666bde9..f2a4d2100abc8 100644 --- a/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts +++ b/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts @@ -89,6 +89,16 @@ export enum ResourceScrutinyType { * This is a Lambda Permission policy */ LambdaPermission = 'LambdaPermission', + + /** + * An ingress rule object + */ + IngressRuleResource = 'IngressRuleResource', + + /** + * A set of egress rules + */ + EgressRuleResource = 'EgressRuleResource', } export function isResourceScrutinyType(str: string): str is ResourceScrutinyType { diff --git a/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json b/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json index 4f5669c14fb2d..6f860d718ddda 100644 --- a/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json +++ b/packages/@aws-cdk/cfnspec/spec-source/200_Scrutinies_patch.json @@ -40,6 +40,47 @@ } ] } + }, + "AWS::EC2::SecurityGroup": { + "patch": { + "description": "SecurityGroup: Mark ingress/egress rules", + "operations": [ + { + "op": "add", + "path": "/Properties/SecurityGroupIngress/ScrutinyType", + "value": "IngressRules" + }, + { + "op": "add", + "path": "/Properties/SecurityGroupEgress/ScrutinyType", + "value": "EgressRules" + } + ] + } + }, + "AWS::EC2::SecurityGroupIngress": { + "patch": { + "description": "SecurityGroupIngress: Mark ingress rules", + "operations": [ + { + "op": "add", + "path": "/ScrutinyType", + "value": "IngressRuleResource" + } + ] + } + }, + "AWS::EC2::SecurityGroupEgress": { + "patch": { + "description": "SecurityGroupEgress: Mark egress rules", + "operations": [ + { + "op": "add", + "path": "/ScrutinyType", + "value": "EgressRuleResource" + } + ] + } } } } diff --git a/packages/@aws-cdk/cfnspec/test/spec-validators.ts b/packages/@aws-cdk/cfnspec/test/spec-validators.ts index 51c03bedda6ea..35e0cf29b12d1 100644 --- a/packages/@aws-cdk/cfnspec/test/spec-validators.ts +++ b/packages/@aws-cdk/cfnspec/test/spec-validators.ts @@ -12,6 +12,9 @@ function validateResourceTypes(test: Test, specification: Specification) { test.ok(typeName, 'Resource type name is not empty'); const type = specification.ResourceTypes[typeName]; test.notEqual(type.Documentation, null, `${typeName} is documented`); + if (type.ScrutinyType) { + test.ok(schema.isResourceScrutinyType(type.ScrutinyType), `${typeName}.ScrutinyType is not a valid ResourceScrutinyType`); + } if (type.Properties) { validateProperties(typeName, test, type.Properties, specification); } if (type.Attributes) { validateAttributes(typeName, test, type.Attributes, specification); } } diff --git a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts index 401c11e27d661..2105d81fa78d2 100644 --- a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts +++ b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts @@ -37,4 +37,22 @@ export = { test.done(); }, + + 'check securityGroup scrutinies'(test: Test) { + const inProp = specification().ResourceTypes['AWS::EC2::SecurityGroup'].Properties!.SecurityGroupIngress; + test.equals(inProp.ScrutinyType, PropertyScrutinyType.IngressRules); + + const eProp = specification().ResourceTypes['AWS::EC2::SecurityGroup'].Properties!.SecurityGroupEgress; + test.equals(eProp.ScrutinyType, PropertyScrutinyType.EgressRules); + + test.done(); + }, + + 'check securityGroupRule scrutinies'(test: Test) { + const inRes = specification().ResourceTypes['AWS::EC2::SecurityGroupIngress']; + test.equals(inRes.ScrutinyType, ResourceScrutinyType.IngressRuleResource); + + const eRes = specification().ResourceTypes['AWS::EC2::SecurityGroupEgress']; + test.equals(eRes.ScrutinyType, ResourceScrutinyType.EgressRuleResource); + } }; \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index 2f08919e21be2..caa83185019ea 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -1,6 +1,7 @@ import cfnspec = require('@aws-cdk/cfnspec'); import { AssertionError } from 'assert'; import { IamChanges } from '../iam/iam-changes'; +import { SecurityGroupChanges } from '../network/security-group-changes'; import { deepEqual } from './util'; export type PropertyMap = {[key: string]: any }; @@ -24,6 +25,11 @@ export class TemplateDiff implements ITemplateDiff { */ public readonly iamChanges: IamChanges; + /** + * Changes to Security Group ingress and egress rules + */ + public readonly securityGroupChanges: SecurityGroupChanges; + constructor(args: ITemplateDiff) { if (args.awsTemplateFormatVersion !== undefined) { this.awsTemplateFormatVersion = args.awsTemplateFormatVersion; @@ -49,6 +55,13 @@ export class TemplateDiff implements ITemplateDiff { managedPolicyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.ManagedPolicies), lambdaPermissionChanges: this.scrutinizableResourceChanges(cfnspec.schema.ResourceScrutinyType.LambdaPermission), }); + + this.securityGroupChanges = new SecurityGroupChanges({ + egressRulePropertyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.EgressRules), + ingressRulePropertyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.IngressRules), + egressRuleResourceChanges: this.scrutinizableResourceChanges(cfnspec.schema.ResourceScrutinyType.EgressRuleResource), + ingressRuleResourceChanges: this.scrutinizableResourceChanges(cfnspec.schema.ResourceScrutinyType.IngressRuleResource), + }); } public get count() { diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diffable.ts b/packages/@aws-cdk/cloudformation-diff/lib/diffable.ts new file mode 100644 index 0000000000000..7bc87b51144ef --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/lib/diffable.ts @@ -0,0 +1,56 @@ +/** + * Calculate differences of immutable elements + */ +export class DiffableCollection> { + public readonly additions: T[] = []; + public readonly removals: T[] = []; + + private readonly oldElements: T[] = []; + private readonly newElements: T[] = []; + + public addOld(...elements: T[]) { + this.oldElements.push(...elements); + } + + public addNew(...elements: T[]) { + this.newElements.push(...elements); + } + + public calculateDiff() { + this.additions.push(...difference(this.newElements, this.oldElements)); + this.removals.push(...difference(this.oldElements, this.newElements)); + } + + public get hasChanges() { + return this.additions.length + this.removals.length > 0; + } + + public get hasAdditions() { + return this.additions.length > 0; + } + + public get hasRemovals() { + return this.removals.length > 0; + } +} + +/** + * Things that can be compared to themselves (by value) + */ +interface Eq { + equal(other: T): boolean; +} + +/** + * Whether a collection contains some element (by value) + */ +function contains>(element: T, xs: T[]) { + return xs.some(x => x.equal(element)); +} + +/** + * Return collection except for elements + */ +function difference>(collection: T[], elements: T[]) { + return collection.filter(x => !contains(x, elements)); +} diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index 2bef4c3777645..f307906cb416e 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -6,6 +6,7 @@ import { Difference, isPropertyDifference, ResourceDifference, ResourceImpact } import { DifferenceCollection, TemplateDiff } from './diff/types'; import { deepEqual } from './diff/util'; import { IamChanges } from './iam/iam-changes'; +import { SecurityGroupChanges } from './network/security-group-changes'; /** * Renders template differences to the process' console. @@ -32,11 +33,8 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp printSectionFooter(); } - if (templateDiff.iamChanges.hasChanges) { - printSectionHeader('Summary of IAM Changes'); - formatIamChanges(templateDiff.iamChanges); - printSectionFooter(); - } + formatIamChanges(templateDiff.iamChanges); + formatSecurityGroupChanges(templateDiff.securityGroupChanges); formatSection('Parameters', 'Parameter', templateDiff.parameters); formatSection('Metadata', 'Metadata', templateDiff.metadata); @@ -295,17 +293,29 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp } function formatIamChanges(changes: IamChanges) { + if (!changes.hasChanges) { return; } + const tables: string[] = []; - if (changes.hasStatementChanges) { + if (changes.statements.hasChanges) { tables.push(renderTable(deepSubstituteBracedLogicalIds(changes.summarizeStatements()))); } - if (changes.hasManagedPolicyChanges) { + if (changes.managedPolicies.hasChanges) { tables.push(renderTable(deepSubstituteBracedLogicalIds(changes.summarizeManagedPolicies()))); } + printSectionHeader('Summary of IAM Changes'); print(tables.join('\n\n')); + printSectionFooter(); + } + + function formatSecurityGroupChanges(changes: SecurityGroupChanges) { + if (!changes.hasChanges) { return; } + + printSectionHeader('Summary of Security Group Changes'); + print(renderTable(deepSubstituteBracedLogicalIds(changes.summarize()))); + printSectionFooter(); } function deepSubstituteBracedLogicalIds(rows: string[][]): string[][] { @@ -327,5 +337,33 @@ function renderTable(cells: string[][]): string { const table = new Table({ head, style: { head: [] } }); table.push(...cells); - return table.toString(); + return stripHorizontalLines(table.toString()); } + +/** + * Strip horizontal lines in the table rendering if the second-column values are the same + * + * We couldn't find a table library that BOTH does newlines-in-cells correctly AND + * has an option to enable/disable separator lines on a per-row basis. So we're + * going to do some character post-processing on the table instead. + */ +function stripHorizontalLines(tableRendering: string) { + const lines = tableRendering.split('\n'); + + let i = 3; + while (i < lines.length - 3) { + if (secondColumnValue(lines[i]) === secondColumnValue(lines[i + 2])) { + lines.splice(i + 1, 1); + i += 1; + } else { + i += 2; + } + } + + return lines.join('\n'); + + function secondColumnValue(line: string) { + const cols = colors.stripColors(line).split('│').filter(x => x !== ''); + return cols[1]; + } +} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts index 1d4bd81861153..cef382f113375 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts @@ -1,5 +1,6 @@ import colors = require('colors/safe'); import { PropertyChange, PropertyMap, ResourceChange } from "../diff/types"; +import { DiffableCollection } from '../diffable'; import { ManagedPolicyAttachment, parseManagedPolicies } from './managed-policy'; import { parseLambdaPermission, parseStatements, Statement, Targets } from "./statement"; import { unCloudFormation } from "./uncfn"; @@ -15,67 +16,33 @@ export interface IamChangesProps { * Changes to IAM statements */ export class IamChanges { - public readonly statementAdditions: Statement[] = []; - public readonly statementRemovals: Statement[] = []; - public readonly managedPolicyAdditions: ManagedPolicyAttachment[] = []; - public readonly managedPolicyRemovals: ManagedPolicyAttachment[] = []; - - private oldStatements: Statement[] = []; - private newStatements: Statement[] = []; - private oldPolicyAttachments: ManagedPolicyAttachment[] = []; - private newPolicyAttachments: ManagedPolicyAttachment[] = []; + public readonly statements = new DiffableCollection(); + public readonly managedPolicies = new DiffableCollection(); constructor(props: IamChangesProps) { for (const policyChange of props.identityPolicyChanges) { - this.oldStatements.push(...this.readIdentityStatements(policyChange.oldValue, policyChange.propertyName, policyChange.resourceLogicalId)); - this.newStatements.push(...this.readIdentityStatements(policyChange.newValue, policyChange.propertyName, policyChange.resourceLogicalId)); + this.statements.addOld(...this.readIdentityStatements(policyChange.oldValue, policyChange.propertyName, policyChange.resourceLogicalId)); + this.statements.addNew(...this.readIdentityStatements(policyChange.newValue, policyChange.propertyName, policyChange.resourceLogicalId)); } for (const policyChange of props.resourcePolicyChanges) { - this.oldStatements.push(...this.readResourceStatements(policyChange.oldValue, policyChange.resourceLogicalId)); - this.newStatements.push(...this.readResourceStatements(policyChange.newValue, policyChange.resourceLogicalId)); + this.statements.addOld(...this.readResourceStatements(policyChange.oldValue, policyChange.resourceLogicalId)); + this.statements.addNew(...this.readResourceStatements(policyChange.newValue, policyChange.resourceLogicalId)); } for (const lambdaChange of props.lambdaPermissionChanges) { - this.oldStatements.push(...this.readLambdaStatements(lambdaChange.oldProperties)); - this.newStatements.push(...this.readLambdaStatements(lambdaChange.newProperties)); + this.statements.addOld(...this.readLambdaStatements(lambdaChange.oldProperties)); + this.statements.addNew(...this.readLambdaStatements(lambdaChange.newProperties)); } for (const managedPolicyChange of props.managedPolicyChanges) { - this.oldPolicyAttachments.push(...this.readManagedPolicies(managedPolicyChange.oldValue, managedPolicyChange.resourceLogicalId)); - this.newPolicyAttachments.push(...this.readManagedPolicies(managedPolicyChange.newValue, managedPolicyChange.resourceLogicalId)); + this.managedPolicies.addOld(...this.readManagedPolicies(managedPolicyChange.oldValue, managedPolicyChange.resourceLogicalId)); + this.managedPolicies.addNew(...this.readManagedPolicies(managedPolicyChange.newValue, managedPolicyChange.resourceLogicalId)); } - this.statementAdditions.push(...difference(this.newStatements, this.oldStatements)); - this.statementRemovals.push(...difference(this.oldStatements, this.newStatements)); - - this.managedPolicyAdditions.push(...difference(this.newPolicyAttachments, this.oldPolicyAttachments)); - this.managedPolicyAdditions.push(...difference(this.oldPolicyAttachments, this.newPolicyAttachments)); + this.statements.calculateDiff(); + this.managedPolicies.calculateDiff(); } public get hasChanges() { - return this.hasManagedPolicyChanges || this.hasStatementChanges; - } - - public get hasStatementChanges() { - return this.statementAdditions.length + this.statementRemovals.length > 0; - } - - public get hasStatementAdditions() { - return this.statementAdditions.length > 0; - } - - public get hasStatementRemovals() { - return this.statementRemovals.length > 0; - } - - public get hasManagedPolicyChanges() { - return this.managedPolicyAdditions.length + this.managedPolicyRemovals.length > 0; - } - - public get hasManagedPolicyAdditions() { - return this.managedPolicyAdditions.length > 0; - } - - public get hasManagedPolicyRemovals() { - return this.managedPolicyRemovals.length > 0; + return this.statements.hasChanges || this.managedPolicies.hasChanges; } /** @@ -87,7 +54,7 @@ export class IamChanges { const header = ['', 'Resource', 'Effect', 'Action', 'Principal', 'Condition']; // First generate all lines, then sort on Resource so that similar resources are together - for (const statement of this.statementAdditions) { + for (const statement of this.statements.additions) { ret.push([ '+', renderTargets(statement.resources), @@ -97,7 +64,7 @@ export class IamChanges { renderCondition(statement.condition) ].map(s => colors.green(s))); } - for (const statement of this.statementRemovals) { + for (const statement of this.statements.removals) { ret.push([ colors.red('-'), renderTargets(statement.resources), @@ -109,7 +76,7 @@ export class IamChanges { } // Sort by 2nd column - ret.sort(makeComparator((row: string[]) => row[1])); + ret.sort(makeComparator((row: string[]) => [row[1]])); ret.splice(0, 0, header); @@ -120,14 +87,14 @@ export class IamChanges { const ret: string[][] = []; const header = ['', 'Resource', 'Managed Policy ARN']; - for (const att of this.managedPolicyAdditions) { + for (const att of this.managedPolicies.additions) { ret.push([ '+', att.identityArn, att.managedPolicyArn, ].map(s => colors.green(s))); } - for (const att of this.managedPolicyRemovals) { + for (const att of this.managedPolicies.removals) { ret.push([ '-', att.identityArn, @@ -136,7 +103,7 @@ export class IamChanges { } // Sort by 2nd column - ret.sort(makeComparator((row: string[]) => row[1])); + ret.sort(makeComparator((row: string[]) => [row[1]])); ret.splice(0, 0, header); @@ -194,27 +161,6 @@ export class IamChanges { } } -/** - * Things that can be compared to themselves (by value) - */ -interface Eq { - equal(other: T): boolean; -} - -/** - * Whether a collection contains some element (by value) - */ -function contains>(element: T, xs: T[]) { - return xs.some(x => x.equal(element)); -} - -/** - * Return collection except for elements - */ -function difference>(collection: T[], elements: T[]) { - return collection.filter(x => !contains(x, elements)); -} - /** * Render into a summary table cell */ @@ -247,15 +193,26 @@ function renderCondition(condition: any) { } /** - * Turn a key extraction function into a comparator for use in Array.sort() + * Turn a (multi-key) extraction function into a comparator for use in Array.sort() */ -function makeComparator(keyFn: (x: T) => U) { +export function makeComparator(keyFn: (x: T) => U[]) { return (a: T, b: T) => { const keyA = keyFn(a); const keyB = keyFn(b); + const len = Math.min(keyA.length, keyB.length); - if (keyA < keyB) { return -1; } - if (keyB < keyA) { return 1; } - return 0; + for (let i = 0; i < len; i++) { + const c = compare(keyA[i], keyB[i]); + if (c !== 0) { return c; } + } + + // Arrays are the same up to the min length -- shorter array sorts first + return keyA.length - keyB.length; }; +} + +function compare(a: T, b: T) { + if (a < b) { return -1; } + if (b < a) { return 1; } + return 0; } \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts new file mode 100644 index 0000000000000..a914a3e882ab9 --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts @@ -0,0 +1,101 @@ +import colors = require('colors/safe'); +import { PropertyChange, ResourceChange } from "../diff/types"; +import { DiffableCollection } from "../diffable"; +import { makeComparator } from '../iam/iam-changes'; +import { unCloudFormation } from "../iam/uncfn"; +import { SecurityGroupRule } from "./security-group-rule"; + +export interface SecurityGroupChangesProps { + ingressRulePropertyChanges: PropertyChange[]; + ingressRuleResourceChanges: ResourceChange[]; + egressRuleResourceChanges: ResourceChange[]; + egressRulePropertyChanges: PropertyChange[]; +} + +/** + * Changes to IAM statements + */ +export class SecurityGroupChanges { + public readonly ingress = new DiffableCollection(); + public readonly egress = new DiffableCollection(); + + constructor(props: SecurityGroupChangesProps) { + // Group rules + for (const ingressProp of props.ingressRulePropertyChanges) { + this.ingress.addOld(...this.readInlineRules(ingressProp.oldValue, ingressProp.resourceLogicalId)); + this.ingress.addNew(...this.readInlineRules(ingressProp.newValue, ingressProp.resourceLogicalId)); + } + for (const egressProp of props.egressRulePropertyChanges) { + this.egress.addOld(...this.readInlineRules(egressProp.oldValue, egressProp.resourceLogicalId)); + this.egress.addNew(...this.readInlineRules(egressProp.newValue, egressProp.resourceLogicalId)); + } + + // Rule resources + for (const ingressRes of props.ingressRuleResourceChanges) { + this.ingress.addOld(...this.readRuleResource(ingressRes.oldProperties)); + this.ingress.addNew(...this.readRuleResource(ingressRes.newProperties)); + } + for (const egressRes of props.egressRuleResourceChanges) { + this.egress.addOld(...this.readRuleResource(egressRes.oldProperties)); + this.egress.addNew(...this.readRuleResource(egressRes.newProperties)); + } + + this.ingress.calculateDiff(); + this.egress.calculateDiff(); + } + + public get hasChanges() { + return this.ingress.hasChanges || this.egress.hasChanges; + } + + /** + * Return a summary table of changes + */ + public summarize(): string[][] { + const ret: string[][] = []; + + const header = ['', 'Group', 'Dir', 'Protocol', 'Peer']; + + const inWord = 'In'; + const outWord = 'Out'; + + // Render a single rule to the table (curried function so we can map it across rules easily--thank you JavaScript!) + const renderRule = (plusMin: string, inOut: string) => (rule: SecurityGroupRule) => [ + plusMin, + rule.groupId, + inOut, + rule.describeProtocol(), + rule.describePeer(), + ].map(s => plusMin === '+' ? colors.green(s) : colors.red(s)); + + // First generate all lines, sort later + ret.push(...this.ingress.additions.map(renderRule('+', inWord))); + ret.push(...this.egress.additions.map(renderRule('+', outWord))); + ret.push(...this.ingress.removals.map(renderRule('-', inWord))); + ret.push(...this.egress.removals.map(renderRule('-', outWord))); + + // Sort by group name then ingress/egress (ingress first) + ret.sort(makeComparator((row: string[]) => [row[1], row[2].indexOf(inWord) > -1 ? 0 : 1])); + + ret.splice(0, 0, header); + + return ret; + } + + private readInlineRules(rules: any, logicalId: string): SecurityGroupRule[] { + if (!rules) { return []; } + + // UnCloudFormation so the parser works in an easier domain + + const ref = '${' + logicalId + '.GroupId}'; + return rules.map((r: any) => new SecurityGroupRule(unCloudFormation(r), ref)); + } + + private readRuleResource(resource: any): SecurityGroupRule[] { + if (!resource) { return []; } + + // UnCloudFormation so the parser works in an easier domain + + return [new SecurityGroupRule(unCloudFormation(resource))]; + } +} diff --git a/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-rule.ts b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-rule.ts new file mode 100644 index 0000000000000..79af1cf60c237 --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-rule.ts @@ -0,0 +1,119 @@ +/** + * A single security group rule, either egress or ingress + */ +export class SecurityGroupRule { + /** + * Group ID of the group this rule applies to + */ + public readonly groupId: string; + + /** + * IP protocol this rule applies to + */ + public readonly ipProtocol: string; + + /** + * Start of port range this rule applies to, or ICMP type + */ + public readonly fromPort?: number; + + /** + * End of port range this rule applies to, or ICMP code + */ + public readonly toPort?: number; + + /** + * Peer of this rule + */ + public readonly peer?: RulePeer; + + constructor(ruleObject: any, groupRef?: string) { + this.ipProtocol = ruleObject.IpProtocol || '*unknown*'; + this.fromPort = ruleObject.FromPort; + this.toPort = ruleObject.ToPort; + this.groupId = ruleObject.GroupId || groupRef || '*unknown*'; // In case of an inline rule + + this.peer = + findFirst(ruleObject, + ['CidrIp', 'CidrIpv6'], + (ip) => ({ kind: 'cidr-ip', ip } as CidrIpPeer)) + || + findFirst(ruleObject, + ['DestinationSecurityGroupId', 'SourceSecurityGroupId'], + (securityGroupId) => ({ kind: 'security-group', securityGroupId } as SecurityGroupPeer)) + || + findFirst(ruleObject, + ['DestinationPrefixListId', 'SourcePrefixListId'], + (prefixListId) => ({ kind: 'prefix-list', prefixListId } as PrefixListPeer)); + } + + public equal(other: SecurityGroupRule) { + return this.ipProtocol === other.ipProtocol + && this.fromPort === other.fromPort + && this.toPort === other.toPort + && peerEqual(this.peer, other.peer); + } + + public describeProtocol() { + if (this.ipProtocol === '-1') { return 'Everything'; } + + const ipProtocol = this.ipProtocol.toUpperCase(); + + if (this.fromPort === -1) { return `All ${ipProtocol}`; } + if (this.fromPort === this.toPort) { return `${ipProtocol} ${this.fromPort}`; } + return `${ipProtocol} ${this.fromPort}-${this.toPort}`; + } + + public describePeer() { + if (this.peer) { + switch (this.peer.kind) { + case 'cidr-ip': + if (this.peer.ip === '0.0.0.0/0') { return 'Everyone (IPv4)'; } + if (this.peer.ip === '::/0') { return 'Everyone (IPv6)'; } + return `${this.peer.ip}`; + case 'prefix-list': return `${this.peer.prefixListId}`; + case 'security-group': return `${this.peer.securityGroupId}`; + } + } + + return '?'; + } +} + +export interface CidrIpPeer { + kind: 'cidr-ip'; + ip: string; +} + +export interface SecurityGroupPeer { + kind: 'security-group'; + securityGroupId: string; +} + +export interface PrefixListPeer { + kind: 'prefix-list'; + prefixListId: string; +} + +export type RulePeer = CidrIpPeer | SecurityGroupPeer | PrefixListPeer; + +function peerEqual(a?: RulePeer, b?: RulePeer) { + if ((a === undefined) !== (b === undefined)) { return false; } + if (a === undefined) { return true; } + + if (a.kind !== b!.kind) { return false; } + switch (a.kind) { + case 'cidr-ip': return a.ip === (b as typeof a).ip; + case 'security-group': return a.securityGroupId === (b as typeof a).securityGroupId; + case 'prefix-list': return a.prefixListId === (b as typeof a).prefixListId; + } +} + +function findFirst(obj: any, keys: string[], fn: (x: string) => T): T | undefined { + for (const key of keys) { + if (key in obj) { + return fn(obj[key]); + } + } + return undefined; +} \ No newline at end of file From dce115242887b3e35c84bc60d16b44101e75f7db Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 28 Nov 2018 17:21:46 +0100 Subject: [PATCH 11/23] Prompt for confirmation during deployments --- .../cloudformation-diff/lib/format.ts | 178 ++++++++++-------- packages/aws-cdk/bin/cdk.ts | 22 ++- packages/aws-cdk/lib/diff.ts | 28 ++- tools/cdk-integ-tools/bin/cdk-integ.ts | 2 +- 4 files changed, 145 insertions(+), 85 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index f307906cb416e..53d19aad0a734 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -16,55 +16,74 @@ import { SecurityGroupChanges } from './network/security-group-changes'; * case there is no aws:cdk:path metadata in the template. */ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: TemplateDiff, logicalToPathMap: { [logicalId: string]: string } = { }) { - function print(fmt: string, ...args: any[]) { - stream.write(colors.white(format(fmt, ...args)) + '\n'); - } - - completeLogicalPathMap(); + const formatter = new Formatter(stream, logicalToPathMap); - const ADDITION = colors.green('[+]'); const UPDATE = colors.yellow('[~]'); - const REMOVAL = colors.red('[-]'); + formatter.readConstructPathsFrom(templateDiff); if (templateDiff.awsTemplateFormatVersion || templateDiff.transform || templateDiff.description) { - printSectionHeader('Template'); - formatDifference('AWSTemplateFormatVersion', 'AWSTemplateFormatVersion', templateDiff.awsTemplateFormatVersion); - formatDifference('Transform', 'Transform', templateDiff.transform); - formatDifference('Description', 'Description', templateDiff.description); - printSectionFooter(); + formatter.printSectionHeader('Template'); + formatter.formatDifference('AWSTemplateFormatVersion', 'AWSTemplateFormatVersion', templateDiff.awsTemplateFormatVersion); + formatter.formatDifference('Transform', 'Transform', templateDiff.transform); + formatter.formatDifference('Description', 'Description', templateDiff.description); + formatter.printSectionFooter(); } - formatIamChanges(templateDiff.iamChanges); - formatSecurityGroupChanges(templateDiff.securityGroupChanges); + formatter.formatIamChanges(templateDiff.iamChanges); + formatter.formatSecurityGroupChanges(templateDiff.securityGroupChanges); + + formatter.formatSection('Parameters', 'Parameter', templateDiff.parameters); + formatter.formatSection('Metadata', 'Metadata', templateDiff.metadata); + formatter.formatSection('Mappings', 'Mapping', templateDiff.mappings); + formatter.formatSection('Conditions', 'Condition', templateDiff.conditions); + formatter.formatSection('Resources', 'Resource', templateDiff.resources, formatter.formatResourceDifference.bind(formatter)); + formatter.formatSection('Outputs', 'Output', templateDiff.outputs); + formatter.formatSection('Other Changes', 'Unknown', templateDiff.unknown); +} + +/** + * Renders a diff of security changes to the given stream + */ +export function formatSecurityChanges(stream: NodeJS.WriteStream, templateDiff: TemplateDiff, logicalToPathMap: {[logicalId: string]: string} = {}) { + const formatter = new Formatter(stream, logicalToPathMap); + formatter.readConstructPathsFrom(templateDiff); + + formatter.formatIamChanges(templateDiff.iamChanges); + formatter.formatSecurityGroupChanges(templateDiff.securityGroupChanges); +} + +const ADDITION = colors.green('[+]'); +const UPDATE = colors.yellow('[~]'); +const REMOVAL = colors.red('[-]'); - formatSection('Parameters', 'Parameter', templateDiff.parameters); - formatSection('Metadata', 'Metadata', templateDiff.metadata); - formatSection('Mappings', 'Mapping', templateDiff.mappings); - formatSection('Conditions', 'Condition', templateDiff.conditions); - formatSection('Resources', 'Resource', templateDiff.resources, formatResourceDifference); - formatSection('Outputs', 'Output', templateDiff.outputs); - formatSection('Other Changes', 'Unknown', templateDiff.unknown); +class Formatter { + constructor(private readonly stream: NodeJS.WriteStream, private readonly logicalToPathMap: { [logicalId: string]: string }) { + } - function formatSection>( + public print(fmt: string, ...args: any[]) { + this.stream.write(colors.white(format(fmt, ...args)) + '\n'); + } + + public formatSection>( title: string, entryType: string, collection: DifferenceCollection, - formatter: (type: string, id: string, diff: T) => void = formatDifference) { + formatter: (type: string, id: string, diff: T) => void = this.formatDifference.bind(this)) { if (collection.count === 0) { return; } - printSectionHeader(title); + this.printSectionHeader(title); collection.forEach((id, diff) => formatter(entryType, id, diff)); - printSectionFooter(); + this.printSectionFooter(); } - function printSectionHeader(title: string) { - print(colors.underline(colors.bold(title))); + public printSectionHeader(title: string) { + this.print(colors.underline(colors.bold(title))); } - function printSectionFooter() { - print(''); + public printSectionFooter() { + this.print(''); } /** @@ -73,13 +92,13 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp * @param logicalId the name of the entity that is different. * @param diff the difference to be rendered. */ - function formatDifference(type: string, logicalId: string, diff: Difference | undefined) { + public formatDifference(type: string, logicalId: string, diff: Difference | undefined) { if (!diff) { return; } let value; - const oldValue = formatValue(diff.oldValue, colors.red); - const newValue = formatValue(diff.newValue, colors.green); + const oldValue = this.formatValue(diff.oldValue, colors.red); + const newValue = this.formatValue(diff.newValue, colors.green); if (diff.isAddition) { value = newValue; } else if (diff.isUpdate) { @@ -88,7 +107,7 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp value = oldValue; } - print(`${formatPrefix(diff)} ${colors.cyan(type)} ${formatLogicalId(logicalId)}: ${value}`); + this.print(`${this.formatPrefix(diff)} ${colors.cyan(type)} ${this.formatLogicalId(logicalId)}: ${value}`); } /** @@ -97,22 +116,22 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp * @param logicalId the logical ID of the resource that changed. * @param diff the change to be rendered. */ - function formatResourceDifference(_type: string, logicalId: string, diff: ResourceDifference) { + public formatResourceDifference(_type: string, logicalId: string, diff: ResourceDifference) { const resourceType = diff.isRemoval ? diff.oldResourceType : diff.newResourceType; // tslint:disable-next-line:max-line-length - print(`${formatPrefix(diff)} ${formatValue(resourceType, colors.cyan)} ${formatLogicalId(logicalId)} ${formatImpact(diff.changeImpact)}`); + this.print(`${this.formatPrefix(diff)} ${this.formatValue(resourceType, colors.cyan)} ${this.formatLogicalId(logicalId)} ${this.formatImpact(diff.changeImpact)}`); if (diff.isUpdate) { let processedCount = 0; diff.forEach((_, name, values) => { processedCount += 1; - formatTreeDiff(name, values, processedCount === diff.count); + this.formatTreeDiff(name, values, processedCount === diff.count); }); } } - function formatPrefix(diff: Difference) { + public formatPrefix(diff: Difference) { if (diff.isAddition) { return ADDITION; } if (diff.isUpdate) { return UPDATE; } if (diff.isRemoval) { return REMOVAL; } @@ -125,7 +144,7 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp * * @returns the formatted string, with color applied. */ - function formatValue(value: any, color: (str: string) => string) { + public formatValue(value: any, color: (str: string) => string) { if (value == null) { return undefined; } if (typeof value === 'string') { return color(value); } return color(JSON.stringify(value)); @@ -135,7 +154,7 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp * @param impact the impact to be formatted * @returns a user-friendly, colored string representing the impact. */ - function formatImpact(impact: ResourceImpact) { + public formatImpact(impact: ResourceImpact) { switch (impact) { case ResourceImpact.MAY_REPLACE: return colors.italic(colors.yellow('may be replaced')); @@ -157,7 +176,7 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp * @param diff the difference on the tree. * @param last whether this is the last node of a parent tree. */ - function formatTreeDiff(name: string, diff: Difference, last: boolean) { + public formatTreeDiff(name: string, diff: Difference, last: boolean) { let additionalInfo = ''; if (isPropertyDifference(diff)) { if (diff.changeImpact === ResourceImpact.MAY_REPLACE) { @@ -166,8 +185,8 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp additionalInfo = ' (requires replacement)'; } } - print(' %s─ %s %s%s', last ? '└' : '├', changeTag(diff.oldValue, diff.newValue), name, additionalInfo); - return formatObjectDiff(diff.oldValue, diff.newValue, ` ${last ? ' ' : '│'}`); + this.print(' %s─ %s %s%s', last ? '└' : '├', this.changeTag(diff.oldValue, diff.newValue), name, additionalInfo); + return this.formatObjectDiff(diff.oldValue, diff.newValue, ` ${last ? ' ' : '│'}`); } /** @@ -178,15 +197,15 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp * @param newObject the new object. * @param linePrefix a prefix (indent-like) to be used on every line. */ - function formatObjectDiff(oldObject: any, newObject: any, linePrefix: string) { + public formatObjectDiff(oldObject: any, newObject: any, linePrefix: string) { if ((typeof oldObject !== typeof newObject) || Array.isArray(oldObject) || typeof oldObject === 'string' || typeof oldObject === 'number') { if (oldObject !== undefined && newObject !== undefined) { - print('%s ├─ %s %s', linePrefix, REMOVAL, formatValue(oldObject, colors.red)); - print('%s └─ %s %s', linePrefix, ADDITION, formatValue(newObject, colors.green)); + this.print('%s ├─ %s %s', linePrefix, REMOVAL, this.formatValue(oldObject, colors.red)); + this.print('%s └─ %s %s', linePrefix, ADDITION, this.formatValue(newObject, colors.green)); } else if (oldObject !== undefined /* && newObject === undefined */) { - print('%s └─ %s', linePrefix, formatValue(oldObject, colors.red)); + this.print('%s └─ %s', linePrefix, this.formatValue(oldObject, colors.red)); } else /* if (oldObject === undefined && newObject !== undefined) */ { - print('%s └─ %s', linePrefix, formatValue(newObject, colors.green)); + this.print('%s └─ %s', linePrefix, this.formatValue(newObject, colors.green)); } return; } @@ -199,12 +218,12 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp const newValue = newObject[key]; const treePrefix = key === lastKey ? '└' : '├'; if (oldValue !== undefined && newValue !== undefined) { - print('%s %s─ %s %s:', linePrefix, treePrefix, changeTag(oldValue, newValue), colors.blue(`.${key}`)); - formatObjectDiff(oldValue, newValue, `${linePrefix} ${key === lastKey ? ' ' : '│'}`); + this.print('%s %s─ %s %s:', linePrefix, treePrefix, this.changeTag(oldValue, newValue), colors.blue(`.${key}`)); + this.formatObjectDiff(oldValue, newValue, `${linePrefix} ${key === lastKey ? ' ' : '│'}`); } else if (oldValue !== undefined /* && newValue === undefined */) { - print('%s %s─ %s Removed: %s', linePrefix, treePrefix, REMOVAL, colors.blue(`.${key}`)); + this.print('%s %s─ %s Removed: %s', linePrefix, treePrefix, REMOVAL, colors.blue(`.${key}`)); } else /* if (oldValue === undefined && newValue !== undefined */ { - print('%s %s─ %s Added: %s', linePrefix, treePrefix, ADDITION, colors.blue(`.${key}`)); + this.print('%s %s─ %s Added: %s', linePrefix, treePrefix, ADDITION, colors.blue(`.${key}`)); } } } @@ -216,7 +235,7 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp * @returns a tag to be rendered in the diff, reflecting whether the difference * was an ADDITION, UPDATE or REMOVAL. */ - function changeTag(oldValue: any | undefined, newValue: any | undefined): string { + public changeTag(oldValue: any | undefined, newValue: any | undefined): string { if (oldValue !== undefined && newValue !== undefined) { return UPDATE; } else if (oldValue !== undefined /* && newValue === undefined*/) { @@ -232,25 +251,25 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp * There are multiple sources of logicalID -> path mappings: synth metadata * and resource metadata, and we combine all sources into a single map. */ - function completeLogicalPathMap() { + public readConstructPathsFrom(templateDiff: TemplateDiff) { for (const [logicalId, resourceDiff] of Object.entries(templateDiff.resources)) { if (!resourceDiff) { continue; } const oldPathMetadata = resourceDiff.oldValue && resourceDiff.oldValue.Metadata && resourceDiff.oldValue.Metadata[cxapi.PATH_METADATA_KEY]; - if (oldPathMetadata && !(logicalId in logicalToPathMap)) { - logicalToPathMap[logicalId] = oldPathMetadata; + if (oldPathMetadata && !(logicalId in this.logicalToPathMap)) { + this.logicalToPathMap[logicalId] = oldPathMetadata; } const newPathMetadata = resourceDiff.newValue && resourceDiff.newValue.Metadata && resourceDiff.newValue.Metadata[cxapi.PATH_METADATA_KEY]; - if (newPathMetadata && !(logicalId in logicalToPathMap)) { - logicalToPathMap[logicalId] = newPathMetadata; + if (newPathMetadata && !(logicalId in this.logicalToPathMap)) { + this.logicalToPathMap[logicalId] = newPathMetadata; } } } - function formatLogicalId(logicalId: string) { + public formatLogicalId(logicalId: string) { // if we have a path in the map, return it - const normalized = normalizedLogicalIdPath(logicalId); + const normalized = this.normalizedLogicalIdPath(logicalId); if (normalized) { return `${normalized} ${colors.gray(logicalId)}`; @@ -259,9 +278,9 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp return logicalId; } - function normalizedLogicalIdPath(logicalId: string): string | undefined { + public normalizedLogicalIdPath(logicalId: string): string | undefined { // if we have a path in the map, return it - const path = logicalToPathMap[logicalId]; + const path = this.logicalToPathMap[logicalId]; return path ? normalizePath(path) : undefined; /** @@ -292,46 +311,49 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp } } - function formatIamChanges(changes: IamChanges) { + public formatIamChanges(changes: IamChanges) { if (!changes.hasChanges) { return; } - const tables: string[] = []; - if (changes.statements.hasChanges) { - tables.push(renderTable(deepSubstituteBracedLogicalIds(changes.summarizeStatements()))); + this.printSectionHeader('IAM Statement Changes'); + this.print(renderTable(this.deepSubstituteBracedLogicalIds(changes.summarizeStatements()))); } if (changes.managedPolicies.hasChanges) { - tables.push(renderTable(deepSubstituteBracedLogicalIds(changes.summarizeManagedPolicies()))); + this.printSectionHeader('IAM Policy Changes'); + this.print(renderTable(this.deepSubstituteBracedLogicalIds(changes.summarizeManagedPolicies()))); } - printSectionHeader('Summary of IAM Changes'); - print(tables.join('\n\n')); - printSectionFooter(); + this.printSectionFooter(); } - function formatSecurityGroupChanges(changes: SecurityGroupChanges) { + public formatSecurityGroupChanges(changes: SecurityGroupChanges) { if (!changes.hasChanges) { return; } - printSectionHeader('Summary of Security Group Changes'); - print(renderTable(deepSubstituteBracedLogicalIds(changes.summarize()))); - printSectionFooter(); + this.printSectionHeader('Summary of Security Group Changes'); + this.print(renderTable(this.deepSubstituteBracedLogicalIds(changes.summarize()))); + this.printSectionFooter(); } - function deepSubstituteBracedLogicalIds(rows: string[][]): string[][] { - return rows.map(row => row.map(substituteBracedLogicalIds)); + public deepSubstituteBracedLogicalIds(rows: string[][]): string[][] { + return rows.map(row => row.map(this.substituteBracedLogicalIds.bind(this))); } /** * Substitute all strings like ${LogId.xxx} with the path instead of the logical ID */ - function substituteBracedLogicalIds(source: string): string { + public substituteBracedLogicalIds(source: string): string { return source.replace(/\$\{([^.}]+)(.[^}]+)?\}/ig, (_match, logId, suffix) => { - return '${' + (normalizedLogicalIdPath(logId) || logId) + (suffix || '') + '}'; + return '${' + (this.normalizedLogicalIdPath(logId) || logId) + (suffix || '') + '}'; }); } } +/** + * Render a two-dimensional array to a visually attractive table + * + * First row is considered the table header. + */ function renderTable(cells: string[][]): string { const head = cells.splice(0, 1)[0]; @@ -366,4 +388,4 @@ function stripHorizontalLines(tableRendering: string) { const cols = colors.stripColors(line).split('│').filter(x => x !== ''); return cols[1]; } -} \ No newline at end of file +} diff --git a/packages/aws-cdk/bin/cdk.ts b/packages/aws-cdk/bin/cdk.ts index 2a26c19fa7a83..f1173e020331f 100644 --- a/packages/aws-cdk/bin/cdk.ts +++ b/packages/aws-cdk/bin/cdk.ts @@ -10,7 +10,7 @@ import yargs = require('yargs'); import { bootstrapEnvironment, deployStack, destroyStack, loadToolkitInfo, Mode, SDK } from '../lib'; import { environmentsFromDescriptors, globEnvironmentsFromStacks } from '../lib/api/cxapp/environments'; import { AppStacks, listStackNames } from '../lib/api/cxapp/stacks'; -import { printStackDiff } from '../lib/diff'; +import { printSecurityDiff, printStackDiff } from '../lib/diff'; import { availableInitLanguages, cliInit, printAvailableTemplates } from '../lib/init'; import { interactive } from '../lib/interactive'; import { data, debug, error, highlight, print, setVerbose, success, warning } from '../lib/logging'; @@ -22,6 +22,7 @@ import { VERSION } from '../lib/version'; // tslint:disable-next-line:no-var-requires const promptly = require('promptly'); +const confirm = util.promisify(promptly.confirm); const DEFAULT_TOOLKIT_STACK_NAME = 'CDKToolkit'; @@ -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' }) .option('toolkit-stack-name', { type: 'string', desc: 'the name of the CDK toolkit stack' })) .command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', yargs => yargs .option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' })) @@ -194,7 +196,7 @@ async function initCommandLine() { return await cliBootstrap(args.ENVIRONMENTS, toolkitStackName, args.roleArn); case 'deploy': - return await cliDeploy(args.STACKS, toolkitStackName, args.roleArn); + return await cliDeploy(args.STACKS, toolkitStackName, args.roleArn, args.prompt); case 'destroy': return await cliDestroy(args.STACKS, args.force, args.roleArn); @@ -323,7 +325,7 @@ async function initCommandLine() { return 0; // exit-code } - async function cliDeploy(stackNames: string[], toolkitStackName: string, roleArn: string | undefined) { + async function cliDeploy(stackNames: string[], toolkitStackName: string, roleArn: string | undefined, prompt: string) { const stacks = await appStacks.selectStacks(...stackNames); renames.validateSelectedStacks(stacks); @@ -336,6 +338,14 @@ async function initCommandLine() { const toolkitInfo = await loadToolkitInfo(stack.environment, aws, toolkitStackName); const deployName = renames.finalName(stack.name); + if (prompt !== 'never') { + const currentTemplate = await readCurrentTemplate(stack); + if (printSecurityDiff(currentTemplate, stack)) { + const confirmed = await confirm(`Do you wish to deploy these changes (y/n)?`); + if (!confirmed) { throw new Error('Aborted by user'); } + } + } + if (deployName !== stack.name) { print('%s: deploying... (was %s)', colors.bold(deployName), colors.bold(stack.name)); } else { @@ -375,7 +385,7 @@ async function initCommandLine() { if (!force) { // tslint:disable-next-line:max-line-length - const confirmed = await util.promisify(promptly.confirm)(`Are you sure you want to delete: ${colors.blue(stacks.map(s => s.name).join(', '))} (y/n)?`); + const confirmed = await confirm(`Are you sure you want to delete: ${colors.blue(stacks.map(s => s.name).join(', '))} (y/n)?`); if (!confirmed) { return; } @@ -413,8 +423,10 @@ async function initCommandLine() { const fileContent = await fs.readFile(templatePath, { encoding: 'UTF-8' }); return parseTemplate(fileContent); } else { - const cfn = await aws.cloudFormation(stack.environment, Mode.ForReading); const stackName = renames.finalName(stack.name); + debug(`Reading existing template for stack ${stackName}.`); + + const cfn = await aws.cloudFormation(stack.environment, Mode.ForReading); try { const response = await cfn.getTemplate({ StackName: stackName }).promise(); return (response.TemplateBody && parseTemplate(response.TemplateBody)) || {}; diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index da9ca8bb2642b..f1454d9390083 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -1,7 +1,7 @@ import cfnDiff = require('@aws-cdk/cloudformation-diff'); import cxapi = require('@aws-cdk/cx-api'); import colors = require('colors/safe'); -import { print } from './logging'; +import { print, warning } from './logging'; /** * Pretty-prints the differences between two template states to the console. @@ -33,6 +33,32 @@ export function printStackDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedS return diff.count; } +export function printSecurityDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedStack): boolean { + const diff = cfnDiff.diffTemplate(oldTemplate, newTemplate.template); + + if (diffHasSecurityImpact(diff)) { + warning(`This deployment will make potentially sensitive changes.`); + warning(`Please confirm you intend to make the following modifications:\n`); + + cfnDiff.formatSecurityChanges(process.stderr, diff, buildLogicalToPathMap(newTemplate)); + return true; + } + return false; +} + +/** + * Return whether the diff has security-impacting changes that need confirmation + * + * TODO: Filter the security impact determination based off of an enum that allows + * us to pick minimum "severities" to alert on. + */ +function diffHasSecurityImpact(diff: cfnDiff.TemplateDiff) { + return diff.iamChanges.statements.hasAdditions + || diff.iamChanges.managedPolicies.hasAdditions + || diff.securityGroupChanges.ingress.hasAdditions + || diff.securityGroupChanges.egress.hasAdditions; +} + function buildLogicalToPathMap(template: cxapi.SynthesizedStack) { const map: { [id: string]: string } = {}; for (const path of Object.keys(template.metadata)) { diff --git a/tools/cdk-integ-tools/bin/cdk-integ.ts b/tools/cdk-integ-tools/bin/cdk-integ.ts index 1398c0e6f2bce..62eb473b78e76 100644 --- a/tools/cdk-integ-tools/bin/cdk-integ.ts +++ b/tools/cdk-integ-tools/bin/cdk-integ.ts @@ -28,7 +28,7 @@ async function main() { } try { - await test.invoke([ ...args, 'deploy' ], { verbose: argv.verbose }); // Note: no context, so use default user settings! + await test.invoke([ ...args, 'deploy', '--prompt', 'never' ], { verbose: argv.verbose }); // Note: no context, so use default user settings! console.error(`Success! Writing out reference synth.`); From b239852c0c78685204e293c256b6661e05caf035 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 30 Nov 2018 13:57:06 +0100 Subject: [PATCH 12/23] Add tests on IAM, fix bugs found --- .../@aws-cdk/cfnspec/build-tools/scrutiny.ts | 97 +++++-- packages/@aws-cdk/cfnspec/lib/index.ts | 33 ++- .../@aws-cdk/cfnspec/lib/schema/property.ts | 14 +- .../cfnspec/lib/schema/resource-type.ts | 14 + .../@aws-cdk/cfnspec/test/test.scrutiny.ts | 41 ++- .../cloudformation-diff/lib/diff/types.ts | 77 ++++-- .../lib/iam/iam-changes.ts | 246 +++++++++++------ .../lib/iam/managed-policy.ts | 9 + .../cloudformation-diff/lib/iam/statement.ts | 73 ++++- .../lib/network/security-group-changes.ts | 22 +- .../lib/network/security-group-rule.ts | 18 ++ .../lib/{iam => }/uncfn.ts | 0 .../@aws-cdk/cloudformation-diff/lib/util.ts | 49 ++++ .../cloudformation-diff/package-lock.json | 34 ++- .../@aws-cdk/cloudformation-diff/package.json | 1 + .../test/iam/test.broadening.ts | 46 ++++ .../test/iam/test.detect-changes.ts | 256 ++++++++++++++++++ .../test/iam/test.statement.ts | 235 ++++++++++++++++ .../cloudformation-diff/test/test.uncfn.ts | 66 +++++ .../@aws-cdk/cloudformation-diff/test/util.ts | 22 ++ packages/aws-cdk/lib/diff.ts | 5 +- 21 files changed, 1180 insertions(+), 178 deletions(-) rename packages/@aws-cdk/cloudformation-diff/lib/{iam => }/uncfn.ts (100%) create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/util.ts create mode 100644 packages/@aws-cdk/cloudformation-diff/test/iam/test.broadening.ts create mode 100644 packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts create mode 100644 packages/@aws-cdk/cloudformation-diff/test/iam/test.statement.ts create mode 100644 packages/@aws-cdk/cloudformation-diff/test/test.uncfn.ts create mode 100644 packages/@aws-cdk/cloudformation-diff/test/util.ts diff --git a/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts b/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts index ad47b80b12af0..9a6bf3df097b3 100644 --- a/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts +++ b/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts @@ -1,5 +1,5 @@ import { schema } from '../lib'; -import { PropertyScrutinyType } from '../lib/schema'; +import { PropertyScrutinyType, ResourceScrutinyType } from '../lib/schema'; /** * Auto-detect common properties to apply scrutiny to by using heuristics @@ -12,29 +12,78 @@ import { PropertyScrutinyType } from '../lib/schema'; * fixed using schema patches. */ export function detectScrutinyTypes(spec: schema.Specification) { - for (const objectMap of [spec.PropertyTypes, spec.ResourceTypes]) { - for (const typeName of Object.keys(objectMap)) { - const typeSpec = objectMap[typeName]; // Instead of Object.entries(), to help the typechecker - - for (const [propertyName, propertySpec] of Object.entries(typeSpec.Properties || {})) { - if (propertySpec.ScrutinyType !== undefined) { continue; } // Only for unassigned - - // Detect fields named like ManagedPolicyArns - if (propertyName === 'ManagedPolicyArns') { - propertySpec.ScrutinyType = PropertyScrutinyType.ManagedPolicies; - continue; - } - - // Detect fields named like '*Policy*' - const nameContainsPolicy = propertyName.indexOf('Policy') > -1; - const primitiveType = schema.isPrimitiveProperty(propertySpec) && propertySpec.PrimitiveType; - - if (nameContainsPolicy && primitiveType === 'Json') { - const isIamResource = typeName.indexOf('::IAM::') > 1; - propertySpec.ScrutinyType = isIamResource ? PropertyScrutinyType.IdentityPolicy : PropertyScrutinyType.ResourcePolicy; - continue; - } - } + for (const [typeName, typeSpec] of Object.entries(spec.ResourceTypes)) { + if (typeSpec.ScrutinyType !== undefined) { continue; } // Already assigned + + detectResourceScrutiny(typeName, typeSpec); + + // If a resource scrutiny is set by now, we don't need to look at the properties anymore + if (typeSpec.ScrutinyType !== undefined) { continue; } + + for (const [propertyName, propertySpec] of Object.entries(typeSpec.Properties || {})) { + if (propertySpec.ScrutinyType !== undefined) { continue; } // Already assigned + + detectPropertyScrutiny(typeName, propertyName, propertySpec); + } } } + +/** + * Detect and assign a scrutiny type for the resource + */ +function detectResourceScrutiny(typeName: string, typeSpec: schema.ResourceType) { + const properties = Object.entries(typeSpec.Properties || {}); + + // If this resource is named like *Policy and has a PolicyDocument property + if (typeName.endsWith('Policy') && properties.some(app2(isPolicyDocumentProperty))) { + typeSpec.ScrutinyType = isIamType(typeName) ? ResourceScrutinyType.IdentityPolicyResource : ResourceScrutinyType.ResourcePolicyResource; + return; + } +} + +/** + * Detect and assign a scrutiny type for the property + */ +function detectPropertyScrutiny(_typeName: string, propertyName: string, propertySpec: schema.Property) { + // Detect fields named like ManagedPolicyArns + if (propertyName === 'ManagedPolicyArns') { + propertySpec.ScrutinyType = PropertyScrutinyType.ManagedPolicies; + return; + } + + if (propertyName === "Policies" && schema.isComplexListProperty(propertySpec) && propertySpec.ItemType === 'Policy') { + propertySpec.ScrutinyType = PropertyScrutinyType.InlineIdentityPolicies; + return; + } + + if (isPolicyDocumentProperty(propertyName, propertySpec)) { + propertySpec.ScrutinyType = PropertyScrutinyType.InlineResourcePolicy; + return; + } +} + +function isIamType(typeName: string) { + return typeName.indexOf('::IAM::') > 1; +} + +function isPolicyDocumentProperty(propertyName: string, propertySpec: schema.Property) { + const nameContainsPolicy = propertyName.indexOf('Policy') > -1; + const primitiveType = schema.isPrimitiveProperty(propertySpec) && propertySpec.PrimitiveType; + + if (nameContainsPolicy && primitiveType === 'Json') { + return true; + } + return false; +} + +/** + * Make a function that takes 2 arguments take an array of 2 elements instead + * + * Makes it possible to map it over an array of arrays. TypeScript won't allow + * me to overload this type declaration so we need a different function for + * every # of arguments. + */ +function app2(fn: (a1: T1, a2: T2) => R): (as: [T1, T2]) => R { + return (as) => fn.apply(fn, as); +} \ No newline at end of file diff --git a/packages/@aws-cdk/cfnspec/lib/index.ts b/packages/@aws-cdk/cfnspec/lib/index.ts index 6db7b206b3e4f..f114178ff937e 100644 --- a/packages/@aws-cdk/cfnspec/lib/index.ts +++ b/packages/@aws-cdk/cfnspec/lib/index.ts @@ -9,6 +9,31 @@ export function specification(): schema.Specification { return require('../spec/specification.json'); } +/** + * Return the resource specification for the given typename + * + * Validates that the resource exists. If you don't this validating behavior, read from + * specification() directly. + */ +export function resourceSpecification(typeName: string): schema.ResourceType { + const ret = specification().ResourceTypes[typeName]; + if (!ret) { + throw new Error(`No such resource type: ${typeName}`); + } + return ret; +} + +/** + * Return the property specification for the given resource's property + */ +export function propertySpecification(typeName: string, propertyName: string): schema.Property { + const ret = resourceSpecification(typeName).Properties![propertyName]; + if (!ret) { + throw new Error(`Resource ${typeName} has no property: ${propertyName}`); + } + return ret; +} + /** * The list of resource type names defined in the ``specification``. */ @@ -75,14 +100,14 @@ function makePredicate(filter: string | RegExp | Filter): Filter { /** * Return the properties of the given type that require the given scrutiny type */ -export function scrutinizablePropertyNames(resourceType: string, scrutinyType: schema.PropertyScrutinyType): string[] { +export function scrutinizablePropertyNames(resourceType: string, scrutinyTypes: schema.PropertyScrutinyType[]): string[] { const impl = specification().ResourceTypes[resourceType]; if (!impl) { return []; } const ret = new Array(); for (const [propertyName, propertySpec] of Object.entries(impl.Properties || {})) { - if ((propertySpec.ScrutinyType || schema.PropertyScrutinyType.None) === scrutinyType) { + if (scrutinyTypes.includes(propertySpec.ScrutinyType || schema.PropertyScrutinyType.None)) { ret.push(propertyName); } } @@ -93,10 +118,10 @@ export function scrutinizablePropertyNames(resourceType: string, scrutinyType: s /** * Return the names of the resource types that need to be subjected to additional scrutiny */ -export function scrutinizableResourceTypes(scrutinyType: schema.ResourceScrutinyType): string[] { +export function scrutinizableResourceTypes(scrutinyTypes: schema.ResourceScrutinyType[]): string[] { const ret = new Array(); for (const [resourceType, resourceSpec] of Object.entries(specification().ResourceTypes)) { - if ((resourceSpec.ScrutinyType || schema.PropertyScrutinyType.None) === scrutinyType) { + if (scrutinyTypes.includes(resourceSpec.ScrutinyType || schema.ResourceScrutinyType.None)) { ret.push(resourceType); } } diff --git a/packages/@aws-cdk/cfnspec/lib/schema/property.ts b/packages/@aws-cdk/cfnspec/lib/schema/property.ts index 1c47bfa192f67..16dbad3a016e5 100644 --- a/packages/@aws-cdk/cfnspec/lib/schema/property.ts +++ b/packages/@aws-cdk/cfnspec/lib/schema/property.ts @@ -169,27 +169,27 @@ export enum PropertyScrutinyType { None = 'None', /** - * This is an IAM policy on an identity resource + * This is an IAM policy directly on a resource */ - IdentityPolicy = 'IdentityPolicy', + InlineResourcePolicy = 'InlineResourcePolicy', /** - * This is an IAM policy on a resource + * Either an AssumeRolePolicyDocument or a dictionary of policy documents */ - ResourcePolicy = 'ResourcePolicy', + InlineIdentityPolicies = 'InlineIdentityPolicies', /** - * A list of managed policies on a resource + * A list of managed policies (on an identity resource) */ ManagedPolicies = 'ManagedPolicies', /** - * A set of ingress rules + * A set of ingress rules (on a security group) */ IngressRules = 'IngressRules', /** - * A set of egress rules + * A set of egress rules (on a security group) */ EgressRules = 'EgressRules', } diff --git a/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts b/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts index f2a4d2100abc8..1b00979d72cd6 100644 --- a/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts +++ b/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts @@ -85,6 +85,20 @@ export enum ResourceScrutinyType { */ None = 'None', + /** + * An externally attached policy document to a resource + * + * (Common for SQS, SNS, S3, ...) + */ + ResourcePolicyResource = 'ResourcePolicyResource', + + /** + * This is an IAM policy on an identity resource + * + * (Basically saying: this is AWS::IAM::Policy) + */ + IdentityPolicyResource = 'IdentityPolicyResource', + /** * This is a Lambda Permission policy */ diff --git a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts index 2105d81fa78d2..91c3a364c0111 100644 --- a/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts +++ b/packages/@aws-cdk/cfnspec/test/test.scrutiny.ts @@ -1,58 +1,73 @@ import { Test } from 'nodeunit'; -import { specification } from '../lib'; +import { propertySpecification, resourceSpecification } from '../lib'; import { PropertyScrutinyType, ResourceScrutinyType } from '../lib/schema'; export = { 'spot-check IAM identity tags'(test: Test) { - const prop = specification().PropertyTypes['AWS::IAM::Role.Policy'].Properties!.PolicyDocument; - test.equals(prop.ScrutinyType, PropertyScrutinyType.IdentityPolicy); + const prop = propertySpecification('AWS::IAM::Role', 'Policies'); + test.equals(prop.ScrutinyType, PropertyScrutinyType.InlineIdentityPolicies); + + test.done(); + }, + + 'IAM AssumeRolePolicy'(test: Test) { + // AssumeRolePolicyDocument is a resource policy, because it applies to the Role itself! + const prop = propertySpecification('AWS::IAM::Role', 'AssumeRolePolicyDocument'); + test.equals(prop.ScrutinyType, PropertyScrutinyType.InlineResourcePolicy); test.done(); }, 'spot-check IAM resource tags'(test: Test) { - const prop = specification().ResourceTypes['AWS::KMS::Key'].Properties!.KeyPolicy; - test.equals(prop.ScrutinyType, PropertyScrutinyType.ResourcePolicy); + const prop = propertySpecification('AWS::KMS::Key', 'KeyPolicy'); + test.equals(prop.ScrutinyType, PropertyScrutinyType.InlineResourcePolicy); + + test.done(); + }, + + 'spot-check resource policy resources'(test: Test) { + test.equals(resourceSpecification('AWS::S3::BucketPolicy').ScrutinyType, ResourceScrutinyType.ResourcePolicyResource); test.done(); }, 'spot-check no misclassified tags'(test: Test) { - const prop = specification().ResourceTypes['AWS::SNS::Subscription'].Properties!.DeliveryPolicy; + const prop = propertySpecification('AWS::SNS::Subscription', 'DeliveryPolicy'); test.equals(prop.ScrutinyType, PropertyScrutinyType.None); test.done(); }, 'check Lambda permission resource scrutiny'(test: Test) { - const resource = specification().ResourceTypes['AWS::Lambda::Permission']; - test.equals(resource.ScrutinyType, ResourceScrutinyType.LambdaPermission); + test.equals(resourceSpecification('AWS::Lambda::Permission').ScrutinyType, ResourceScrutinyType.LambdaPermission); test.done(); }, 'check role managedpolicyarns'(test: Test) { - const prop = specification().ResourceTypes['AWS::IAM::Role'].Properties!.ManagedPolicyArns; + const prop = propertySpecification('AWS::IAM::Role', 'ManagedPolicyArns'); test.equals(prop.ScrutinyType, PropertyScrutinyType.ManagedPolicies); test.done(); }, 'check securityGroup scrutinies'(test: Test) { - const inProp = specification().ResourceTypes['AWS::EC2::SecurityGroup'].Properties!.SecurityGroupIngress; + const inProp = propertySpecification('AWS::EC2::SecurityGroup', 'SecurityGroupIngress'); test.equals(inProp.ScrutinyType, PropertyScrutinyType.IngressRules); - const eProp = specification().ResourceTypes['AWS::EC2::SecurityGroup'].Properties!.SecurityGroupEgress; + const eProp = propertySpecification('AWS::EC2::SecurityGroup', 'SecurityGroupEgress'); test.equals(eProp.ScrutinyType, PropertyScrutinyType.EgressRules); test.done(); }, 'check securityGroupRule scrutinies'(test: Test) { - const inRes = specification().ResourceTypes['AWS::EC2::SecurityGroupIngress']; + const inRes = resourceSpecification('AWS::EC2::SecurityGroupIngress'); test.equals(inRes.ScrutinyType, ResourceScrutinyType.IngressRuleResource); - const eRes = specification().ResourceTypes['AWS::EC2::SecurityGroupEgress']; + const eRes = resourceSpecification('AWS::EC2::SecurityGroupEgress'); test.equals(eRes.ScrutinyType, ResourceScrutinyType.EgressRuleResource); + + test.done(); } }; \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index caa83185019ea..a72c95792adcb 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -50,17 +50,15 @@ export class TemplateDiff implements ITemplateDiff { this.unknown = args.unknown || new DifferenceCollection({}); this.iamChanges = new IamChanges({ - identityPolicyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.IdentityPolicy), - resourcePolicyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.ResourcePolicy), - managedPolicyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.ManagedPolicies), - lambdaPermissionChanges: this.scrutinizableResourceChanges(cfnspec.schema.ResourceScrutinyType.LambdaPermission), + propertyChanges: this.scrutinizablePropertyChanges(IamChanges.IamPropertyScrutinies), + resourceChanges: this.scrutinizableResourceChanges(IamChanges.IamResourceScrutinies), }); this.securityGroupChanges = new SecurityGroupChanges({ - egressRulePropertyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.EgressRules), - ingressRulePropertyChanges: this.scrutinizablePropertyChanges(cfnspec.schema.PropertyScrutinyType.IngressRules), - egressRuleResourceChanges: this.scrutinizableResourceChanges(cfnspec.schema.ResourceScrutinyType.EgressRuleResource), - ingressRuleResourceChanges: this.scrutinizableResourceChanges(cfnspec.schema.ResourceScrutinyType.IngressRuleResource), + egressRulePropertyChanges: this.scrutinizablePropertyChanges([cfnspec.schema.PropertyScrutinyType.EgressRules]), + ingressRulePropertyChanges: this.scrutinizablePropertyChanges([cfnspec.schema.PropertyScrutinyType.IngressRules]), + egressRuleResourceChanges: this.scrutinizableResourceChanges([cfnspec.schema.ResourceScrutinyType.EgressRuleResource]), + ingressRuleResourceChanges: this.scrutinizableResourceChanges([cfnspec.schema.ResourceScrutinyType.IngressRuleResource]), }); } @@ -92,22 +90,33 @@ export class TemplateDiff implements ITemplateDiff { return this.count === 0; } + /** + * Return true if any of the permissions objects involve a broadening of permissions + */ + public get permissionsBroadened(): boolean { + return this.iamChanges.permissionsBroadened + || this.securityGroupChanges.ingress.hasAdditions + || this.securityGroupChanges.egress.hasAdditions; + } + /** * Return all property changes of a given scrutiny type * * We don't just look at property updates; we also look at resource additions and deletions (in which * case there is no further detail on property values), and resource type changes. */ - public scrutinizablePropertyChanges(scrutinyType: cfnspec.schema.PropertyScrutinyType): PropertyChange[] { + private scrutinizablePropertyChanges(scrutinyTypes: cfnspec.schema.PropertyScrutinyType[]): PropertyChange[] { const ret = new Array(); for (const [resourceLogicalId, resourceChange] of Object.entries(this.resources.changes)) { if (!resourceChange) { continue; } - const props = cfnspec.scrutinizablePropertyNames(resourceChange.newResourceType!, scrutinyType); + const props = cfnspec.scrutinizablePropertyNames(resourceChange.newResourceType!, scrutinyTypes); for (const propertyName of props) { ret.push({ resourceLogicalId, propertyName, + resourceType: resourceChange.resourceType, + scrutinyType: cfnspec.propertySpecification(resourceChange.resourceType, propertyName).ScrutinyType!, oldValue: resourceChange.oldProperties && resourceChange.oldProperties[propertyName], newValue: resourceChange.newProperties && resourceChange.newProperties[propertyName], }); @@ -123,39 +132,33 @@ export class TemplateDiff implements ITemplateDiff { * We don't just look at resource updates; we also look at resource additions and deletions (in which * case there is no further detail on property values), and resource type changes. */ - public scrutinizableResourceChanges(scrutinyType: cfnspec.schema.ResourceScrutinyType): ResourceChange[] { + private scrutinizableResourceChanges(scrutinyTypes: cfnspec.schema.ResourceScrutinyType[]): ResourceChange[] { const ret = new Array(); - const scrutinizableTypes = new Set(cfnspec.scrutinizableResourceTypes(scrutinyType)); + const scrutinizableTypes = new Set(cfnspec.scrutinizableResourceTypes(scrutinyTypes)); for (const [resourceLogicalId, resourceChange] of Object.entries(this.resources.changes)) { if (!resourceChange) { continue; } + const commonProps = { + scrutinyType: cfnspec.resourceSpecification(resourceChange.resourceType).ScrutinyType!, + oldProperties: resourceChange.oldProperties, + newProperties: resourceChange.newProperties, + resourceLogicalId + }; + // Even though it's not physically possible in CFN, let's pretend to handle a change of 'Type'. if (resourceChange.resourceTypeChanged) { // Treat as DELETE+ADD if (scrutinizableTypes.has(resourceChange.oldResourceType!)) { - ret.push({ - oldProperties: resourceChange.oldProperties, - resourceLogicalId, - resourceType: resourceChange.oldResourceType! - }); + ret.push({ ...commonProps, newProperties: undefined, resourceType: resourceChange.oldResourceType! }); } if (scrutinizableTypes.has(resourceChange.newResourceType!)) { - ret.push({ - newProperties: resourceChange.newProperties, - resourceLogicalId, - resourceType: resourceChange.newResourceType! - }); + ret.push({ ...commonProps, oldProperties: undefined, resourceType: resourceChange.newResourceType! }); } } else { if (scrutinizableTypes.has(resourceChange.resourceType)) { - ret.push({ - oldProperties: resourceChange.oldProperties, - newProperties: resourceChange.newProperties, - resourceLogicalId, - resourceType: resourceChange.resourceType - }); + ret.push({ ...commonProps, resourceType: resourceChange.resourceType }); } } } @@ -178,6 +181,16 @@ export interface PropertyChange { */ resourceLogicalId: string; + /** + * Type of the resource + */ + resourceType: string; + + /** + * Scrutiny type for this property change + */ + scrutinyType: cfnspec.schema.PropertyScrutinyType; + /** * Name of the property that is changing */ @@ -205,6 +218,11 @@ export interface ResourceChange { */ resourceLogicalId: string; + /** + * Scrutiny type for this resource change + */ + scrutinyType: cfnspec.schema.ResourceScrutinyType; + /** * The type of the resource */ @@ -416,6 +434,7 @@ export interface Resource { [key: string]: any; } + export class ResourceDifference extends Difference { /** * Old property values @@ -525,4 +544,4 @@ class NoDifferenceError extends Error { constructor(message: string) { super(`No difference: ${message}`); } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts index cef382f113375..bb7991df8f309 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts @@ -1,40 +1,42 @@ +import cfnspec = require('@aws-cdk/cfnspec'); import colors = require('colors/safe'); import { PropertyChange, PropertyMap, ResourceChange } from "../diff/types"; import { DiffableCollection } from '../diffable'; -import { ManagedPolicyAttachment, parseManagedPolicies } from './managed-policy'; -import { parseLambdaPermission, parseStatements, Statement, Targets } from "./statement"; -import { unCloudFormation } from "./uncfn"; +import { unCloudFormation } from "../uncfn"; +import { deepRemoveUndefined, dropIfEmpty, flatMap, makeComparator } from '../util'; +import { ManagedPolicyAttachment, ManagedPolicyJson, parseManagedPolicies } from './managed-policy'; +import { parseLambdaPermission, parseStatements, renderCondition, Statement, StatementJson, Targets } from "./statement"; export interface IamChangesProps { - identityPolicyChanges: PropertyChange[]; - resourcePolicyChanges: PropertyChange[]; - lambdaPermissionChanges: ResourceChange[]; - managedPolicyChanges: PropertyChange[]; + propertyChanges: PropertyChange[]; + resourceChanges: ResourceChange[]; } /** * Changes to IAM statements */ export class IamChanges { + public static IamPropertyScrutinies = [ + cfnspec.schema.PropertyScrutinyType.InlineIdentityPolicies, + cfnspec.schema.PropertyScrutinyType.InlineResourcePolicy, + cfnspec.schema.PropertyScrutinyType.ManagedPolicies, + ]; + + public static IamResourceScrutinies = [ + cfnspec.schema.ResourceScrutinyType.ResourcePolicyResource, + cfnspec.schema.ResourceScrutinyType.IdentityPolicyResource, + cfnspec.schema.ResourceScrutinyType.LambdaPermission, + ]; + public readonly statements = new DiffableCollection(); public readonly managedPolicies = new DiffableCollection(); constructor(props: IamChangesProps) { - for (const policyChange of props.identityPolicyChanges) { - this.statements.addOld(...this.readIdentityStatements(policyChange.oldValue, policyChange.propertyName, policyChange.resourceLogicalId)); - this.statements.addNew(...this.readIdentityStatements(policyChange.newValue, policyChange.propertyName, policyChange.resourceLogicalId)); - } - for (const policyChange of props.resourcePolicyChanges) { - this.statements.addOld(...this.readResourceStatements(policyChange.oldValue, policyChange.resourceLogicalId)); - this.statements.addNew(...this.readResourceStatements(policyChange.newValue, policyChange.resourceLogicalId)); - } - for (const lambdaChange of props.lambdaPermissionChanges) { - this.statements.addOld(...this.readLambdaStatements(lambdaChange.oldProperties)); - this.statements.addNew(...this.readLambdaStatements(lambdaChange.newProperties)); + for (const propertyChange of props.propertyChanges) { + this.readPropertyChange(propertyChange); } - for (const managedPolicyChange of props.managedPolicyChanges) { - this.managedPolicies.addOld(...this.readManagedPolicies(managedPolicyChange.oldValue, managedPolicyChange.resourceLogicalId)); - this.managedPolicies.addNew(...this.readManagedPolicies(managedPolicyChange.newValue, managedPolicyChange.resourceLogicalId)); + for (const resourceChange of props.resourceChanges) { + this.readResourceChange(resourceChange); } this.statements.calculateDiff(); @@ -45,6 +47,18 @@ export class IamChanges { return this.statements.hasChanges || this.managedPolicies.hasChanges; } + /** + * Return whether the changes include broadened permissions + * + * Permissions are broadened if positive statements are added or + * negative statements are removed, or if managed policies are added. + */ + public get permissionsBroadened(): boolean { + return this.statements.additions.some(s => !s.isNegativeStatement) + || this.statements.removals.some(s => s.isNegativeStatement) + || this.managedPolicies.hasAdditions; + } + /** * Return a summary table of changes */ @@ -110,41 +124,114 @@ export class IamChanges { return ret; } - private readIdentityStatements(policy: any, propertyName: string, logicalId: string): Statement[] { - if (!policy) { return []; } + /** + * Return a machine-readable version of the changes + */ + public toJson(): IamChangesJson { + return deepRemoveUndefined({ + statementAdditions: dropIfEmpty(this.statements.additions.map(s => s.toJson())), + statementRemovals: dropIfEmpty(this.statements.removals.map(s => s.toJson())), + managedPolicyAdditions: dropIfEmpty(this.managedPolicies.additions.map(s => s.toJson())), + managedPolicyRemovals: dropIfEmpty(this.managedPolicies.removals.map(s => s.toJson())), + }); + } - // Stringify CloudFormation intrinsics so that the IAM parser has an easier domain to work in, - // then parse. - const statements = parseStatements(unCloudFormation(policy.Statement)); + private readPropertyChange(propertyChange: PropertyChange) { + switch (propertyChange.scrutinyType) { + case cfnspec.schema.PropertyScrutinyType.InlineIdentityPolicies: + // AWS::IAM::{ Role | User | Group }.Policies + this.statements.addOld(...this.readIdentityPolicies(propertyChange.oldValue, propertyChange.resourceLogicalId)); + this.statements.addNew(...this.readIdentityPolicies(propertyChange.newValue, propertyChange.resourceLogicalId)); + break; + case cfnspec.schema.PropertyScrutinyType.InlineResourcePolicy: + // Any PolicyDocument on a resource (including AssumeRolePolicyDocument) + this.statements.addOld(...this.readResourceStatements(propertyChange.oldValue, propertyChange.resourceLogicalId)); + this.statements.addNew(...this.readResourceStatements(propertyChange.newValue, propertyChange.resourceLogicalId)); + break; + case cfnspec.schema.PropertyScrutinyType.ManagedPolicies: + // Just a list of managed policies + this.managedPolicies.addOld(...this.readManagedPolicies(propertyChange.oldValue, propertyChange.resourceLogicalId)); + this.managedPolicies.addNew(...this.readManagedPolicies(propertyChange.newValue, propertyChange.resourceLogicalId)); + break; + } + } - // If this is an assumeRolePolicy and the Resource list is empty, add in the logicalId into Resources, - // because that's what this statement is applying to. - if (propertyName === 'AssumeRolePolicyDocument') { - const rep = '${' + logicalId + '.Arn}'; - statements.forEach(s => s.resources.replaceEmpty(rep)); + private readResourceChange(resourceChange: ResourceChange) { + switch (resourceChange.scrutinyType) { + case cfnspec.schema.ResourceScrutinyType.IdentityPolicyResource: + // AWS::IAM::Policy + this.statements.addOld(...this.readIdentityPolicyResource(resourceChange.oldProperties)); + this.statements.addNew(...this.readIdentityPolicyResource(resourceChange.newProperties)); + break; + case cfnspec.schema.ResourceScrutinyType.ResourcePolicyResource: + // AWS::*::{Bucket,Queue,Topic}Policy + this.statements.addOld(...this.readResourcePolicyResource(resourceChange.oldProperties)); + this.statements.addNew(...this.readResourcePolicyResource(resourceChange.newProperties)); + break; + case cfnspec.schema.ResourceScrutinyType.LambdaPermission: + this.statements.addOld(...this.readLambdaStatements(resourceChange.oldProperties)); + this.statements.addNew(...this.readLambdaStatements(resourceChange.newProperties)); + break; } + } + + /** + * Parse a list of policies on an identity + */ + private readIdentityPolicies(policies: any, logicalId: string): Statement[] { + if (policies === undefined) { return []; } + + const appliesToPrincipal = '${' + logicalId + '.Arn}'; + + return flatMap(policies, (policy: any) => { + return defaultPrincipal(appliesToPrincipal, parseStatements(unCloudFormation(policy.PolicyDocument.Statement))); + }); + } + + /** + * Parse an IAM::Policy resource + */ + private readIdentityPolicyResource(properties: any): Statement[] { + if (properties === undefined) { return []; } + + properties = unCloudFormation(properties); - return statements; + const principals = (properties.Groups || []).concat(properties.Users || []).concat(properties.Roles || []); + return flatMap(principals, (principal: string) => { + const ref = 'AWS:' + principal; + return defaultPrincipal(ref, parseStatements(properties.PolicyDocument.Statement)); + }); } private readResourceStatements(policy: any, logicalId: string): Statement[] { - // Stringify CloudFormation intrinsics so that the IAM parser has an easier domain to work in, - // then parse. - const statements = parseStatements(unCloudFormation(policy.Statement)); - - // Since this is a ResourcePolicy, the meaning of the '*' in a Resource field - // actually scoped to the resource it's on, so replace * resources with what - // they're scoped to. - // - // We replace with the ARN of the resource, exemplified by a "GetAtt" - // expression. This might not strictly work in CFN for all resource types - // (because {X.Arn} might not exist, or it might have to be {Ref}), and for - // example for Buckets a * implies both the bucket and the resources in the - // bucket, but for human consumpion this is sufficient to be intelligible. - const rep = '${' + logicalId + '.Arn}'; - statements.forEach(s => s.resources.replaceStar(rep)); + if (policy === undefined) { return []; } - return statements; + const appliesToResource = '${' + logicalId + '.Arn}'; + return defaultResource(appliesToResource, parseStatements(unCloudFormation(policy.Statement))); + } + + /** + * Parse an AWS::*::{Bucket,Topic,Queue}policy + */ + private readResourcePolicyResource(properties: any): Statement[] { + if (properties === undefined) { return []; } + + properties = unCloudFormation(properties); + + const policyKeys = Object.keys(properties).filter(key => key.indexOf('Policy') > -1); + + // Find the key that identifies the resource(s) this policy applies to + const resourceKeys = Object.keys(properties).filter(key => !policyKeys.includes(key) && !key.endsWith('Name')); + let resources = resourceKeys.length === 1 ? properties[resourceKeys[0]] : ['???']; + + // For some resources, this is a singleton string, for some it's an array + if (!Array.isArray(resources)) { + resources = [resources]; + } + + return flatMap(resources, (resource: string) => { + return defaultResource(resource, parseStatements(properties[policyKeys[0]].Statement)); + }); } private readManagedPolicies(policyArns: string[] | undefined, logicalId: string): ManagedPolicyAttachment[] { @@ -162,57 +249,36 @@ export class IamChanges { } /** - * Render into a summary table cell + * Set an undefined or wildcarded principal on these statements */ -function renderTargets(targets: Targets): string { - if (targets.not) { - return targets.values.map(s => `NOT ${s}`).join('\n'); - } - return targets.values.join('\n'); +function defaultPrincipal(principal: string, statements: Statement[]) { + statements.forEach(s => s.principals.replaceEmpty(principal)); + statements.forEach(s => s.principals.replaceStar(principal)); + return statements; } /** - * Render the Condition column + * Set an undefined or wildcarded resource on these statements */ -function renderCondition(condition: any) { - if (!condition || Object.keys(condition).length === 0) { return ''; } - const jsonRepresentation = JSON.stringify(condition, undefined, 2); - - // The JSON representation looks like this: - // - // { - // "ArnLike": { - // "AWS:SourceArn": "${MyTopic86869434}" - // } - // } - // - // We can make it more compact without losing information by getting rid of the outermost braces - // and the indentation. - const lines = jsonRepresentation.split('\n'); - return lines.slice(1, lines.length - 1).map(s => s.substr(2)).join('\n'); +function defaultResource(resource: string, statements: Statement[]) { + statements.forEach(s => s.resources.replaceEmpty(resource)); + statements.forEach(s => s.resources.replaceStar(resource)); + return statements; } /** - * Turn a (multi-key) extraction function into a comparator for use in Array.sort() + * Render into a summary table cell */ -export function makeComparator(keyFn: (x: T) => U[]) { - return (a: T, b: T) => { - const keyA = keyFn(a); - const keyB = keyFn(b); - const len = Math.min(keyA.length, keyB.length); - - for (let i = 0; i < len; i++) { - const c = compare(keyA[i], keyB[i]); - if (c !== 0) { return c; } - } - - // Arrays are the same up to the min length -- shorter array sorts first - return keyA.length - keyB.length; - }; +function renderTargets(targets: Targets): string { + if (targets.not) { + return targets.values.map(s => `NOT ${s}`).join('\n'); + } + return targets.values.join('\n'); } -function compare(a: T, b: T) { - if (a < b) { return -1; } - if (b < a) { return 1; } - return 0; +export interface IamChangesJson { + statementAdditions?: StatementJson[]; + statementRemovals?: StatementJson[]; + managedPolicyAdditions?: ManagedPolicyJson[]; + managedPolicyRemovals?: ManagedPolicyJson[]; } \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts index 3f1e63b8d8a1a..121b2eae1efd3 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts @@ -6,6 +6,15 @@ export class ManagedPolicyAttachment { return this.identityArn === other.identityArn && this.managedPolicyArn === other.managedPolicyArn; } + + public toJson() { + return { identityArn: this.identityArn, managedPolicyArn: this.managedPolicyArn }; + } +} + +export interface ManagedPolicyJson { + identityArn: string; + managedPolicyArn: string; } export function parseManagedPolicies(identityArn: string, arns: string[]): ManagedPolicyAttachment[] { diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts index 62bb750876c7a..b6dc42fdc988a 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts @@ -1,4 +1,5 @@ import deepEqual = require('fast-deep-equal'); +import { deepRemoveUndefined } from '../util'; export class Statement { /** @@ -51,6 +52,42 @@ export class Statement { && this.principals.equal(other.principals) && deepEqual(this.condition, other.condition)); } + + public toJson(): StatementJson { + return deepRemoveUndefined({ + sid: this.sid, + effect: this.effect, + resources: this.resources.toJson(), + principals: this.principals.toJson(), + actions: this.actions.toJson(), + condition: this.condition, + }); + } + + /** + * Whether this is a negative statement + * + * A statement is negative if any of its targets are negative, inverted + * if the Effect is Deny. + */ + public get isNegativeStatement(): boolean { + const notTarget = this.actions.not || this.principals.not || this.resources.not; + return this.effect === Effect.Allow ? notTarget : !notTarget; + } +} + +export interface StatementJson { + sid?: string; + effect: string; + resources: TargetsJson; + actions: TargetsJson; + principals: TargetsJson; + condition?: any; +} + +export interface TargetsJson { + not: boolean; + values: string[]; } /** @@ -97,6 +134,10 @@ export function parseLambdaPermission(x: any): Statement { if (statement.Condition === undefined) { statement.Condition = {}; } statement.Condition.StringEquals = { 'AWS:SourceAccount': x.SourceAccount }; } + if (x.EventSourceToken !== undefined) { + if (statement.Condition === undefined) { statement.Condition = {}; } + statement.Condition.StringEquals = { 'lambda:EventSourceToken': x.EventSourceToken }; + } return new Statement(statement); } @@ -157,11 +198,16 @@ export class Targets { } this.values.sort(); } + + public toJson(): TargetsJson { + return { not: this.not, values: this.values }; + } } type UnknownMap = {[key: string]: unknown}; export enum Effect { + Unknown = 'Unknown', Allow = 'Allow', Deny = 'Deny', } @@ -172,7 +218,7 @@ function expectString(x: unknown): string | undefined { function expectEffect(x: unknown): Effect { if (x === Effect.Allow || x === Effect.Deny) { return x as Effect; } - throw new Error(`Unknown effect: ${x}`); + return Effect.Unknown; } function forceListOfStrings(x: unknown): string[] { @@ -180,7 +226,7 @@ function forceListOfStrings(x: unknown): string[] { if (typeof x === 'undefined' || x === null) { return []; } if (Array.isArray(x)) { - return x.map(el => `${el}`); + return x.map(e => forceListOfStrings(e).join(',')); } if (typeof x === 'object' && x !== null) { @@ -192,4 +238,25 @@ function forceListOfStrings(x: unknown): string[] { } return [`${x}`]; -} \ No newline at end of file +} + +/** + * Render the Condition column + */ +export function renderCondition(condition: any) { + if (!condition || Object.keys(condition).length === 0) { return ''; } + const jsonRepresentation = JSON.stringify(condition, undefined, 2); + + // The JSON representation looks like this: + // + // { + // "ArnLike": { + // "AWS:SourceArn": "${MyTopic86869434}" + // } + // } + // + // We can make it more compact without losing information by getting rid of the outermost braces + // and the indentation. + const lines = jsonRepresentation.split('\n'); + return lines.slice(1, lines.length - 1).map(s => s.substr(2)).join('\n'); +} diff --git a/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts index a914a3e882ab9..7ad78fd8672e6 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts @@ -1,9 +1,9 @@ import colors = require('colors/safe'); import { PropertyChange, ResourceChange } from "../diff/types"; import { DiffableCollection } from "../diffable"; -import { makeComparator } from '../iam/iam-changes'; -import { unCloudFormation } from "../iam/uncfn"; -import { SecurityGroupRule } from "./security-group-rule"; +import { unCloudFormation } from "../uncfn"; +import { dropIfEmpty, makeComparator } from '../util'; +import { RuleJson, SecurityGroupRule } from "./security-group-rule"; export interface SecurityGroupChangesProps { ingressRulePropertyChanges: PropertyChange[]; @@ -82,6 +82,15 @@ export class SecurityGroupChanges { return ret; } + public toJson(): SecurityGroupChangesJson { + return { + ingressRuleAdditions: dropIfEmpty(this.ingress.additions.map(s => s.toJson())), + ingressRuleRemovals: dropIfEmpty(this.ingress.removals.map(s => s.toJson())), + egressRuleAdditions: dropIfEmpty(this.egress.additions.map(s => s.toJson())), + egressRuleRemovals: dropIfEmpty(this.egress.removals.map(s => s.toJson())), + }; + } + private readInlineRules(rules: any, logicalId: string): SecurityGroupRule[] { if (!rules) { return []; } @@ -99,3 +108,10 @@ export class SecurityGroupChanges { return [new SecurityGroupRule(unCloudFormation(resource))]; } } + +export interface SecurityGroupChangesJson { + ingressRuleAdditions?: RuleJson[]; + ingressRuleRemovals?: RuleJson[]; + egressRuleAdditions?: RuleJson[]; + egressRuleRemovals?: RuleJson[]; +} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-rule.ts b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-rule.ts index 79af1cf60c237..38385b2fa0ba2 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-rule.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-rule.ts @@ -78,6 +78,16 @@ export class SecurityGroupRule { return '?'; } + + public toJson(): RuleJson { + return { + groupId: this.groupId, + ipProtocol: this.ipProtocol, + fromPort: this.fromPort, + toPort: this.toPort, + peer: this.peer + }; + } } export interface CidrIpPeer { @@ -116,4 +126,12 @@ function findFirst(obj: any, keys: string[], fn: (x: string) => T): T | undef } } return undefined; +} + +export interface RuleJson { + groupId: string; + ipProtocol: string; + fromPort?: number; + toPort?: number; + peer?: RulePeer; } \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/uncfn.ts b/packages/@aws-cdk/cloudformation-diff/lib/uncfn.ts similarity index 100% rename from packages/@aws-cdk/cloudformation-diff/lib/iam/uncfn.ts rename to packages/@aws-cdk/cloudformation-diff/lib/uncfn.ts diff --git a/packages/@aws-cdk/cloudformation-diff/lib/util.ts b/packages/@aws-cdk/cloudformation-diff/lib/util.ts new file mode 100644 index 0000000000000..5fcb761b2a284 --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/lib/util.ts @@ -0,0 +1,49 @@ +/** + * Turn a (multi-key) extraction function into a comparator for use in Array.sort() + */ +export function makeComparator(keyFn: (x: T) => U[]) { + return (a: T, b: T) => { + const keyA = keyFn(a); + const keyB = keyFn(b); + const len = Math.min(keyA.length, keyB.length); + + for (let i = 0; i < len; i++) { + const c = compare(keyA[i], keyB[i]); + if (c !== 0) { return c; } + } + + // Arrays are the same up to the min length -- shorter array sorts first + return keyA.length - keyB.length; + }; +} + +function compare(a: T, b: T) { + if (a < b) { return -1; } + if (b < a) { return 1; } + return 0; +} + +export function dropIfEmpty(xs: T[]): T[] | undefined { + return xs.length > 0 ? xs : undefined; +} + +export function deepRemoveUndefined(x: any): any { + if (typeof x === undefined || x === null) { return x; } + if (Array.isArray(x)) { return x.map(deepRemoveUndefined); } + if (typeof x === 'object') { + for (const [key, value] of Object.entries(x)) { + x[key] = deepRemoveUndefined(value); + if (x[key] === undefined) { delete x[key]; } + } + return x; + } + return x; +} + +export function flatMap(xs: T[], f: (x: T) => U[]): U[] { + const ret = new Array(); + for (const x of xs) { + ret.push(...f(x)); + } + return ret; +} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/package-lock.json b/packages/@aws-cdk/cloudformation-diff/package-lock.json index 912138c9c82a7..7ac316ae5efc2 100644 --- a/packages/@aws-cdk/cloudformation-diff/package-lock.json +++ b/packages/@aws-cdk/cloudformation-diff/package-lock.json @@ -7,7 +7,8 @@ "@types/cli-table": { "version": "0.3.0", "resolved": "https://registry.npmjs.org/@types/cli-table/-/cli-table-0.3.0.tgz", - "integrity": "sha512-QnZUISJJXyhyD6L1e5QwXDV/A5i2W1/gl6D6YMc8u0ncPepbv/B4w3S+izVvtAg60m6h+JP09+Y/0zF2mojlFQ==" + "integrity": "sha512-QnZUISJJXyhyD6L1e5QwXDV/A5i2W1/gl6D6YMc8u0ncPepbv/B4w3S+izVvtAg60m6h+JP09+Y/0zF2mojlFQ==", + "dev": true }, "buffer-from": { "version": "1.1.1", @@ -34,11 +35,42 @@ "resolved": "https://registry.npmjs.org/colors/-/colors-1.3.2.tgz", "integrity": "sha512-rhP0JSBGYvpcNQj4s5AdShMeE5ahMop96cTeDl/v9qQQm2fYClE2QXZRi8wLzc+GmXSxdIqqbOIAhyObEXDbfQ==" }, + "fast-check": { + "version": "1.8.0", + "resolved": "https://registry.npmjs.org/fast-check/-/fast-check-1.8.0.tgz", + "integrity": "sha512-BkXKjgri0+wnqrDNv4kma/2YwrAq9SyPBd/7wzMaYGMAT+s100YHDz0K1y+IQf4yOK40CFLwK5BSh0cyY9F1/A==", + "dev": true, + "requires": { + "lorem-ipsum": "~1.0.6", + "pure-rand": "^1.5.0" + } + }, "fast-deep-equal": { "version": "2.0.1", "resolved": "https://registry.npmjs.org/fast-deep-equal/-/fast-deep-equal-2.0.1.tgz", "integrity": "sha1-ewUhjd+WZ79/Nwv3/bLLFf3Qqkk=" }, + "lorem-ipsum": { + "version": "1.0.6", + "resolved": "https://registry.npmjs.org/lorem-ipsum/-/lorem-ipsum-1.0.6.tgz", + "integrity": "sha512-Rx4XH8X4KSDCKAVvWGYlhAfNqdUP5ZdT4rRyf0jjrvWgtViZimDIlopWNfn/y3lGM5K4uuiAoY28TaD+7YKFrQ==", + "dev": true, + "requires": { + "minimist": "~1.2.0" + } + }, + "minimist": { + "version": "1.2.0", + "resolved": "http://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz", + "integrity": "sha1-o1AIsg9BOD7sH7kU9M1d95omQoQ=", + "dev": true + }, + "pure-rand": { + "version": "1.5.0", + "resolved": "https://registry.npmjs.org/pure-rand/-/pure-rand-1.5.0.tgz", + "integrity": "sha512-acykcCwcBmt5l4w/lPxPUxMeYLb6qwsve3v9A44SNckwOyuXxZgTfP5qAsH/CyZSGkHdUnOQGmckOExOldivOw==", + "dev": true + }, "source-map": { "version": "0.6.1", "resolved": "https://registry.npmjs.org/source-map/-/source-map-0.6.1.tgz", diff --git a/packages/@aws-cdk/cloudformation-diff/package.json b/packages/@aws-cdk/cloudformation-diff/package.json index dc80f4397ac8f..09d9714f3015b 100644 --- a/packages/@aws-cdk/cloudformation-diff/package.json +++ b/packages/@aws-cdk/cloudformation-diff/package.json @@ -33,6 +33,7 @@ "devDependencies": { "@types/cli-table": "^0.3.0", "cdk-build-tools": "^0.18.1", + "fast-check": "^1.8.0", "pkglint": "^0.18.1" }, "repository": { diff --git a/packages/@aws-cdk/cloudformation-diff/test/iam/test.broadening.ts b/packages/@aws-cdk/cloudformation-diff/test/iam/test.broadening.ts new file mode 100644 index 0000000000000..f02bbe86b590c --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/test/iam/test.broadening.ts @@ -0,0 +1,46 @@ +import { Test } from 'nodeunit'; +import { diffTemplate } from '../../lib'; +import { poldoc, resource, template } from '../util'; + +export = { + 'adding of positive statements counts as permission widening'(test: Test) { + // WHEN + const diff = diffTemplate({}, template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue' } ], + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + }) + }) + })); + + // THEN + test.equal(diff.permissionsBroadened, true); + + test.done(); + }, + + 'removal of not-statements counts as permission widening'(test: Test) { + // WHEN + const diff = diffTemplate(template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue' } ], + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + NotPrincipal: { Service: 'sns.amazonaws.com' } + }) + }) + }), {}); + + // THEN + test.equal(diff.permissionsBroadened, true); + + test.done(); + }, + +}; \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts b/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts new file mode 100644 index 0000000000000..d76bbef94143c --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts @@ -0,0 +1,256 @@ +import { Test } from 'nodeunit'; +import { diffTemplate } from '../../lib'; +import { poldoc, policy, resource, role, template } from '../util'; + +export = { + 'shows new AssumeRolePolicyDocument'(test: Test) { + // WHEN + const diff = diffTemplate({}, template({ + MyRole: role({ + AssumeRolePolicyDocument: poldoc({ + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { Service: 'lambda.amazonaws.com' } + }) + }) + })); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + statementAdditions: [ + { + effect: 'Allow', + resources: { not: false, values: [ '${MyRole.Arn}' ] }, + principals: { not: false, values: [ 'Service:lambda.amazonaws.com' ] }, + actions: { not: false, values: [ 'sts:AssumeRole' ] }, + } + ] + }); + + test.done(); + }, + + 'implicitly knows principal of identity policy for all resource types'(test: Test) { + for (const attr of ['Roles', 'Users', 'Groups']) { + // WHEN + const diff = diffTemplate({}, template({ + MyPolicy: policy({ + [attr]: [{ Ref: 'MyRole' }], + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 's3:DoThatThing', + Resource: '*' + }) + }) + })); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + statementAdditions: [ + { + effect: 'Allow', + resources: { not: false, values: [ '*' ] }, + principals: { not: false, values: [ 'AWS:${MyRole}' ] }, + actions: { not: false, values: [ 's3:DoThatThing' ] }, + } + ] + }); + } + + test.done(); + }, + + 'if policy is attached to multiple roles all are shown'(test: Test) { + // WHEN + const diff = diffTemplate({}, template({ + MyPolicy: policy({ + Roles: [{ Ref: 'MyRole' }, { Ref: 'ThyRole' }], + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 's3:DoThatThing', + Resource: '*' + }) + }) + })); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + statementAdditions: [ + { + effect: 'Allow', + resources: { not: false, values: [ '*' ] }, + principals: { not: false, values: [ 'AWS:${MyRole}' ] }, + actions: { not: false, values: [ 's3:DoThatThing' ] }, + }, + { + effect: 'Allow', + resources: { not: false, values: [ '*' ] }, + principals: { not: false, values: [ 'AWS:${ThyRole}' ] }, + actions: { not: false, values: [ 's3:DoThatThing' ] }, + }, + ] + }); + + test.done(); + }, + + 'correctly parses Lambda permissions'(test: Test) { + // WHEN + const diff = diffTemplate({}, template({ + MyPermission: resource('AWS::Lambda::Permission', { + Action: 'lambda:InvokeFunction', + FunctionName: { Ref: 'MyFunction' }, + Principal: 's3.amazonaws.com', + SourceAccount: {Ref: 'AWS::AccountId' }, + SourceArn: {'Fn::GetAtt': ['MyBucketF68F3FF0', 'Arn']}, + }) + })); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + statementAdditions: [ + { + effect: 'Allow', + resources: { not: false, values: [ '${MyFunction}' ] }, + principals: { not: false, values: [ 'Service:s3.amazonaws.com' ] }, + actions: { not: false, values: [ 'lambda:InvokeFunction' ] }, + condition: { + StringEquals: { 'AWS:SourceAccount': '${AWS::AccountId}' }, + ArnLike: { 'AWS:SourceArn': '${MyBucketF68F3FF0.Arn}' } + }, + } + ] + }); + test.done(); + }, + + 'implicitly knows resource of (queue) resource policy even if * given'(test: Test) { + // WHEN + const diff = diffTemplate({}, template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue' } ], + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + }) + }) + })); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + statementAdditions: [ + { + effect: 'Allow', + resources: { not: false, values: [ '${MyQueue}' ] }, + principals: { not: false, values: [ 'Service:sns.amazonaws.com' ] }, + actions: { not: false, values: [ 'sqs:SendMessage' ] }, + } + ] + }); + + test.done(); + }, + + 'finds statement removals'(test: Test) { + // WHEN + const diff = diffTemplate(template({ + BucketPolicy: resource('AWS::S3::BucketPolicy', { + Bucket: { Ref: 'MyBucket' }, + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 's3:PutObject', + Resource: '*', + Principal: { AWS: 'me' } + }) + }) + }), {}); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + statementRemovals: [ + { + effect: 'Allow', + resources: { not: false, values: [ '${MyBucket}' ] }, + principals: { not: false, values: [ 'AWS:me' ] }, + actions: { not: false, values: [ 's3:PutObject' ] }, + } + ] + }); + + test.done(); + }, + + 'finds policy attachments'(test: Test) { + // WHEN + const diff = diffTemplate({}, template({ + SomeRole: resource('AWS::IAM::Role', { + ManagedPolicyArns: ['arn:policy'], + }) + })); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + managedPolicyAdditions: [ + { + identityArn: '${SomeRole.Arn}', + managedPolicyArn: 'arn:policy' + } + ] + }); + + test.done(); + }, + + 'finds policy removals'(test: Test) { + test.done(); + }, + + 'queuepolicy queue change counts as removal+addition'(test: Test) { + // WHEN + const diff = diffTemplate(template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue1' } ], + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + }) + }) + }), template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue2' } ], + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + }) + }) + })); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + statementAdditions: [ + { + effect: 'Allow', + resources: { not: false, values: [ '${MyQueue2}' ] }, + principals: { not: false, values: [ 'Service:sns.amazonaws.com' ] }, + actions: { not: false, values: [ 'sqs:SendMessage' ] }, + } + ], + statementRemovals: [ + { + effect: 'Allow', + resources: { not: false, values: [ '${MyQueue1}' ] }, + principals: { not: false, values: [ 'Service:sns.amazonaws.com' ] }, + actions: { not: false, values: [ 'sqs:SendMessage' ] }, + } + ] + }); + + test.done(); + }, +}; \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/test/iam/test.statement.ts b/packages/@aws-cdk/cloudformation-diff/test/iam/test.statement.ts new file mode 100644 index 0000000000000..1f6bdf4d246ab --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/test/iam/test.statement.ts @@ -0,0 +1,235 @@ +import fc = require('fast-check'); +import { Test } from 'nodeunit'; +import { parseLambdaPermission, renderCondition, Statement } from '../../lib/iam/statement'; + +export = { + 'can parse all positive fields'(test: Test) { + const statement = new Statement({ + Sid: 'Sid', + Effect: 'Allow', + Resource: ['resource'], + Action: ['action'], + Principal: [{AWS: 'arn'}], + Condition: { StringEquals: { 'Amzn-This': 'That' } } + }); + + test.equal(statement.sid, 'Sid'); + test.equal(statement.effect, 'Allow'); + test.deepEqual(statement.resources.values, ['resource']); + test.deepEqual(statement.actions.values, ['action']); + test.deepEqual(statement.principals.values, ['AWS:arn']); + test.deepEqual(statement.condition, { StringEquals: { 'Amzn-This': 'That' } }); + + test.equal(statement.resources.not, false); + test.equal(statement.actions.not, false); + test.equal(statement.principals.not, false); + + test.done(); + }, + + 'parses strings as singleton lists'(test: Test) { + const statement = new Statement({ + Resource: 'resource' + }); + + test.deepEqual(statement.resources.values, ['resource']); + + test.done(); + }, + + 'correctly parses NotFields'(test: Test) { + const statement = new Statement({ + NotResource: ['resource'], + NotAction: ['action'], + NotPrincipal: [{AWS: 'arn'}], + }); + + test.equal(statement.resources.not, true); + test.equal(statement.actions.not, true); + test.equal(statement.principals.not, true); + + test.done(); + }, + + 'parse all LambdaPermission fields'(test: Test) { + const statement = parseLambdaPermission({ + Action: 'lambda:CallMeMaybe', + FunctionName: 'Function', + Principal: '*', + SourceAccount: '123456789012', + SourceArn: 'arn', + }); + + test.deepEqual(statement.actions.values, ['lambda:CallMeMaybe']); + test.deepEqual(statement.resources.values, ['Function']); + test.deepEqual(statement.principals.values, ['*']); + test.deepEqual(statement.condition, { + ArnLike: { 'AWS:SourceArn': 'arn' }, + StringEquals: { 'AWS:SourceAccount': '123456789012', }, + }); + + test.done(); + }, + + 'parse lambda eventsourcetoken'(test: Test) { + const statement = parseLambdaPermission({ + Action: 'lambda:CallMeMaybe', + FunctionName: 'Function', + EventSourceToken: 'token', + Principal: '*', + }); + + test.deepEqual(statement.condition, { + StringEquals: { 'lambda:EventSourceToken': 'token' }, + }); + + test.done(); + }, + + 'stringify complex condition'(test: Test) { + // WHEN + const stringified = renderCondition({ + StringEquals: { 'AWS:SourceAccount': '${AWS::AccountId}' }, + ArnLike: { 'AWS:SourceArn': '${MyBucket.Arn}' } + }).split('\n'); + + // THEN + test.deepEqual(stringified, [ + '"StringEquals": {', + ' "AWS:SourceAccount": "${AWS::AccountId}"', + '},', + '"ArnLike": {', + ' "AWS:SourceArn": "${MyBucket.Arn}"', + '}', + ]); + + test.done(); + }, + + 'an Allow statement with a NotPrincipal is negative'(test: Test) { + // WHEN + const statement = new Statement({ + Effect: 'Allow', + Resource: 'resource', + NotPrincipal: { AWS: 'me' }, + }); + + // THEN + test.equals(statement.isNegativeStatement, true); + + test.done(); + }, + + 'a Deny statement with a NotPrincipal is positive'(test: Test) { + // In effect, this is a roundabout way of saying only the given Principal + // should be allowed ("everyone who's not me can't do this"). + + // WHEN + const statement = new Statement({ + Effect: 'Deny', + Resource: 'resource', + NotPrincipal: { AWS: 'me' }, + }); + + // THEN + test.equals(statement.isNegativeStatement, false); + + test.done(); + }, + + 'equality is reflexive'(test: Test) { + fc.assert(fc.property( + arbitraryStatement, (statement) => { + return new Statement(statement).equal(new Statement(statement)); + } + )); + test.done(); + }, + + 'equality is symmetric'(test: Test) { + fc.assert(fc.property( + twoArbitraryStatements, (s) => { + const a = new Statement(s.statement1); + const b = new Statement(s.statement2); + + fc.pre(a.equal(b)); + return b.equal(a); + } + )); + test.done(); + }, + + // We should be testing transitivity as well but it's too much code to generate + // arbitraries that satisfy the precondition enough times to be useful. +}; + +const arbitraryResource = fc.oneof(fc.constantFrom('*', 'arn:resource')); +const arbitraryAction = fc.constantFrom('*', 's3:*', 's3:GetObject', 's3:PutObject'); +const arbitraryPrincipal = fc.oneof( + fc.constant(undefined), + fc.constant('*'), + fc.record({ AWS: fc.oneof(fc.string(), fc.constant('*')) }), + fc.record({ Service: fc.string() }), + fc.record({ Federated: fc.string() }) +); +const arbitraryCondition = fc.oneof( + fc.constant(undefined), + fc.constant({ StringEquals: { Key: 'Value' }}), + fc.constant({ StringEquals: { Key: 'Value' }, NumberEquals: { Key: 5 }}), +); + +const arbitraryStatement = fc.record({ + Sid: fc.oneof(fc.string(), fc.constant(undefined)), + Effect: fc.constantFrom('Allow', 'Deny'), + Resource: fc.array(arbitraryResource, 0, 2), + NotResource: fc.boolean(), + Action: fc.array(arbitraryAction, 1, 2), + NotAction: fc.boolean(), + Principal: fc.array(arbitraryPrincipal, 0, 2), + NotPrincipal: fc.boolean(), + Condition: arbitraryCondition +}).map(record => { + // This map() that shuffles keys is the easiest way to create variation between Action/NotAction etc. + makeNot(record, 'Resource', 'NotResource'); + makeNot(record, 'Action', 'NotAction'); + makeNot(record, 'Principal', 'NotPrincipal'); + return record; +}); + +function makeNot(obj: any, key: string, notKey: string) { + if (obj[notKey]) { + obj[notKey] = obj[key]; + delete obj[key]; + } else { + delete obj[notKey]; + } +} + +/** + * Two statements where one is a modification of the other + * + * This is to generate two statements that have a higher chance of being similar + * than generating two arbitrary statements independently. + */ +const twoArbitraryStatements = fc.record({ + statement1: arbitraryStatement, + statement2: arbitraryStatement, + copySid: fc.boolean(), + copyEffect: fc.boolean(), + copyResource: fc.boolean(), + copyAction: fc.boolean(), + copyPrincipal: fc.boolean(), + copyCondition: fc.boolean(), +}).map(op => { + const original = op.statement1; + const modified = Object.create(original, {}); + + if (op.copySid) { modified.Sid = op.statement2.Sid; } + if (op.copyEffect) { modified.Effect = op.statement2.Effect; } + if (op.copyResource) { modified.Resource = op.statement2.Resource; modified.NotResource = op.statement2.NotResource; } + if (op.copyAction) { modified.Action = op.statement2.Action; modified.NotAction = op.statement2.NotAction; } + if (op.copyPrincipal) { modified.Principal = op.statement2.Principal; modified.NotPrincipal = op.statement2.NotPrincipal; } + if (op.copyCondition) { modified.Condition = op.statement2.Condition; } + + return { statement1: original, statement2: modified }; +}); \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/test/test.uncfn.ts b/packages/@aws-cdk/cloudformation-diff/test/test.uncfn.ts new file mode 100644 index 0000000000000..b21af1e1477f6 --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/test/test.uncfn.ts @@ -0,0 +1,66 @@ +import { Test } from 'nodeunit'; +import { unCloudFormation } from '../lib/uncfn'; + +export = { + 'resolves Ref'(test: Test) { + test.equals( + unCloudFormation({ Ref: 'SomeLogicalId' }), + '${SomeLogicalId}' + ); + test.done(); + }, + + 'resolves Fn::GetAtt'(test: Test) { + test.equals( + unCloudFormation({ 'Fn::GetAtt': ['SomeLogicalId', 'Attribute'] }), + '${SomeLogicalId.Attribute}' + ); + test.done(); + }, + + 'resolves Fn::Join'(test: Test) { + test.equals( + unCloudFormation({ 'Fn::Join': ['/', ['a', 'b', 'c']] }), + 'a/b/c' + ); + + test.done(); + }, + + 'does not resolve Fn::Join if the second argument is not a list literal'(test: Test) { + test.equals( + unCloudFormation({ 'Fn::Join': ['/', { Ref: 'ListParameter' }] }), + '{"Fn::Join":["/","${ListParameter}"]}' + ); + + test.done(); + }, + + 'deep resolves intrinsics in object'(test: Test) { + test.deepEqual( + unCloudFormation({ + Deeper1: { Ref: 'SomeLogicalId' }, + Deeper2: 'Do not replace', + }), + { + Deeper1: '${SomeLogicalId}', + Deeper2: 'Do not replace', + } + ); + test.done(); + }, + + 'deep resolves intrinsics in array'(test: Test) { + test.deepEqual( + unCloudFormation([ + { Ref: 'SomeLogicalId' }, + 'Do not replace', + ]), + [ + '${SomeLogicalId}', + 'Do not replace', + ] + ); + test.done(); + }, +}; diff --git a/packages/@aws-cdk/cloudformation-diff/test/util.ts b/packages/@aws-cdk/cloudformation-diff/test/util.ts new file mode 100644 index 0000000000000..4d2a3bb39f391 --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/test/util.ts @@ -0,0 +1,22 @@ +export function template(resources: {[key: string]: any}) { + return { Resources: resources }; +} + +export function resource(type: string, properties: {[key: string]: any}) { + return { Type: type, Properties: properties }; +} + +export function role(properties: {[key: string]: any}) { + return resource('AWS::IAM::Role', properties); +} + +export function policy(properties: {[key: string]: any}) { + return resource('AWS::IAM::Policy', properties); +} + +export function poldoc(...statements: any[]) { + return { + Version: "2012-10-17", + Statement: statements + }; +} diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index f1454d9390083..5e19c37ea54c7 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -53,10 +53,7 @@ export function printSecurityDiff(oldTemplate: any, newTemplate: cxapi.Synthesiz * us to pick minimum "severities" to alert on. */ function diffHasSecurityImpact(diff: cfnDiff.TemplateDiff) { - return diff.iamChanges.statements.hasAdditions - || diff.iamChanges.managedPolicies.hasAdditions - || diff.securityGroupChanges.ingress.hasAdditions - || diff.securityGroupChanges.egress.hasAdditions; + return diff.permissionsBroadened; } function buildLogicalToPathMap(template: cxapi.SynthesizedStack) { From 68c34d30739f01d8aee6f8dc3eebc795810a4b7d Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 30 Nov 2018 14:00:32 +0100 Subject: [PATCH 13/23] Don't call resourceType when the type has changed --- .../cloudformation-diff/lib/diff/types.ts | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index a72c95792adcb..e775918d0c80d 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -141,7 +141,6 @@ export class TemplateDiff implements ITemplateDiff { if (!resourceChange) { continue; } const commonProps = { - scrutinyType: cfnspec.resourceSpecification(resourceChange.resourceType).ScrutinyType!, oldProperties: resourceChange.oldProperties, newProperties: resourceChange.newProperties, resourceLogicalId @@ -151,14 +150,28 @@ export class TemplateDiff implements ITemplateDiff { if (resourceChange.resourceTypeChanged) { // Treat as DELETE+ADD if (scrutinizableTypes.has(resourceChange.oldResourceType!)) { - ret.push({ ...commonProps, newProperties: undefined, resourceType: resourceChange.oldResourceType! }); + ret.push({ + ...commonProps, + newProperties: undefined, + resourceType: resourceChange.oldResourceType!, + scrutinyType: cfnspec.resourceSpecification(resourceChange.oldResourceType!).ScrutinyType!, + }); } if (scrutinizableTypes.has(resourceChange.newResourceType!)) { - ret.push({ ...commonProps, oldProperties: undefined, resourceType: resourceChange.newResourceType! }); + ret.push({ + ...commonProps, + oldProperties: undefined, + resourceType: resourceChange.newResourceType!, + scrutinyType: cfnspec.resourceSpecification(resourceChange.newResourceType!).ScrutinyType!, + }); } } else { if (scrutinizableTypes.has(resourceChange.resourceType)) { - ret.push({ ...commonProps, resourceType: resourceChange.resourceType }); + ret.push({ + ...commonProps, + resourceType: resourceChange.resourceType, + scrutinyType: cfnspec.resourceSpecification(resourceChange.resourceType).ScrutinyType!, + }); } } } From 7e78061313b2a9e7900915551c18a8627424e0e9 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 30 Nov 2018 14:10:24 +0100 Subject: [PATCH 14/23] Add test on on-role policies --- .../lib/iam/iam-changes.ts | 4 +-- .../test/iam/test.detect-changes.ts | 36 ++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts index bb7991df8f309..23c2bef466b31 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts @@ -181,7 +181,7 @@ export class IamChanges { private readIdentityPolicies(policies: any, logicalId: string): Statement[] { if (policies === undefined) { return []; } - const appliesToPrincipal = '${' + logicalId + '.Arn}'; + const appliesToPrincipal = 'AWS:${' + logicalId + '}'; return flatMap(policies, (policy: any) => { return defaultPrincipal(appliesToPrincipal, parseStatements(unCloudFormation(policy.PolicyDocument.Statement))); @@ -237,7 +237,7 @@ export class IamChanges { private readManagedPolicies(policyArns: string[] | undefined, logicalId: string): ManagedPolicyAttachment[] { if (!policyArns) { return []; } - const rep = '${' + logicalId + '.Arn}'; + const rep = '${' + logicalId + '}'; return parseManagedPolicies(rep, unCloudFormation(policyArns)); } diff --git a/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts b/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts index d76bbef94143c..836d33be5bbc3 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts @@ -60,6 +60,40 @@ export = { test.done(); }, + 'policies on an identity object'(test: Test) { + for (const resourceType of ['Role', 'User', 'Group']) { + // WHEN + const diff = diffTemplate({}, template({ + MyIdentity: resource(`AWS::IAM::${resourceType}`, { + Policies: [ + { + PolicyName: 'Polly', + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 's3:DoThatThing', + Resource: '*' + }) + } + ], + }) + })); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + statementAdditions: [ + { + effect: 'Allow', + resources: { not: false, values: [ '*' ] }, + principals: { not: false, values: [ 'AWS:${MyIdentity}' ] }, + actions: { not: false, values: [ 's3:DoThatThing' ] }, + } + ] + }); + } + + test.done(); + }, + 'if policy is attached to multiple roles all are shown'(test: Test) { // WHEN const diff = diffTemplate({}, template({ @@ -194,7 +228,7 @@ export = { test.deepEqual(diff.iamChanges.toJson(), { managedPolicyAdditions: [ { - identityArn: '${SomeRole.Arn}', + identityArn: '${SomeRole}', managedPolicyArn: 'arn:policy' } ] From f35d076c20a0971a08968200fb6373e5b53f11ba Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 30 Nov 2018 15:12:50 +0100 Subject: [PATCH 15/23] Add tests on security group rules --- .../cloudformation-diff/lib/diff/types.ts | 4 +- .../lib/network/security-group-changes.ts | 11 +- .../test/network/test.detect-changes.ts | 81 ++++++++++++ .../test/network/test.rule.ts | 120 ++++++++++++++++++ 4 files changed, 210 insertions(+), 6 deletions(-) create mode 100644 packages/@aws-cdk/cloudformation-diff/test/network/test.detect-changes.ts create mode 100644 packages/@aws-cdk/cloudformation-diff/test/network/test.rule.ts diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index e775918d0c80d..4afbcf0e2f657 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -94,9 +94,7 @@ export class TemplateDiff implements ITemplateDiff { * Return true if any of the permissions objects involve a broadening of permissions */ public get permissionsBroadened(): boolean { - return this.iamChanges.permissionsBroadened - || this.securityGroupChanges.ingress.hasAdditions - || this.securityGroupChanges.egress.hasAdditions; + return this.iamChanges.permissionsBroadened || this.securityGroupChanges.rulesAdded; } /** diff --git a/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts index 7ad78fd8672e6..a71eda7b345bb 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts @@ -2,7 +2,7 @@ import colors = require('colors/safe'); import { PropertyChange, ResourceChange } from "../diff/types"; import { DiffableCollection } from "../diffable"; import { unCloudFormation } from "../uncfn"; -import { dropIfEmpty, makeComparator } from '../util'; +import { deepRemoveUndefined, dropIfEmpty, makeComparator } from '../util'; import { RuleJson, SecurityGroupRule } from "./security-group-rule"; export interface SecurityGroupChangesProps { @@ -83,12 +83,17 @@ export class SecurityGroupChanges { } public toJson(): SecurityGroupChangesJson { - return { + return deepRemoveUndefined({ ingressRuleAdditions: dropIfEmpty(this.ingress.additions.map(s => s.toJson())), ingressRuleRemovals: dropIfEmpty(this.ingress.removals.map(s => s.toJson())), egressRuleAdditions: dropIfEmpty(this.egress.additions.map(s => s.toJson())), egressRuleRemovals: dropIfEmpty(this.egress.removals.map(s => s.toJson())), - }; + }); + } + + public get rulesAdded(): boolean { + return this.ingress.hasAdditions + || this.egress.hasAdditions; } private readInlineRules(rules: any, logicalId: string): SecurityGroupRule[] { diff --git a/packages/@aws-cdk/cloudformation-diff/test/network/test.detect-changes.ts b/packages/@aws-cdk/cloudformation-diff/test/network/test.detect-changes.ts new file mode 100644 index 0000000000000..c5b9e8cf1e86d --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/test/network/test.detect-changes.ts @@ -0,0 +1,81 @@ +import { Test } from 'nodeunit'; +import { diffTemplate } from '../../lib'; +import { resource, template } from '../util'; + +export = { + 'detect addition of all types of rules'(test: Test) { + // WHEN + const diff = diffTemplate({}, template({ + SG: resource('AWS::EC2::SecurityGroup', { + SecurityGroupIngress: [ + { + CidrIp: '1.2.3.4/8', + FromPort: 80, + ToPort: 80, + IpProtocol: 'tcp', + } + ], + SecurityGroupEgress: [ + { + DestinationSecurityGroupId: { 'Fn::GetAtt': ['ThatOtherGroup', 'GroupId'] }, + FromPort: 80, + ToPort: 80, + IpProtocol: 'tcp', + } + ], + }), + InRule: resource('AWS::EC2::SecurityGroupIngress', { + GroupId: { 'Fn::GetAtt': ['SG', 'GroupId'] }, + FromPort: -1, + ToPort: -1, + IpProtocol: 'icmp', + SourcePrefixListId: 'pl-1234', + }), + OutRule: resource('AWS::EC2::SecurityGroupEgress', { + GroupId: { 'Fn::GetAtt': ['SG', 'GroupId'] }, + FromPort: -1, + ToPort: -1, + IpProtocol: 'udp', + CidrIp: '7.8.9.0/24', + }), + })); + + // THEN + test.deepEqual(diff.securityGroupChanges.toJson(), { + ingressRuleAdditions: [ + { + groupId: '${SG.GroupId}', + ipProtocol: 'tcp', + fromPort: 80, + toPort: 80, + peer: { kind: 'cidr-ip', ip: '1.2.3.4/8' } + }, + { + groupId: '${SG.GroupId}', + ipProtocol: 'icmp', + fromPort: -1, + toPort: -1, + peer: { kind: 'prefix-list', prefixListId: 'pl-1234' } + } + ], + egressRuleAdditions: [ + { + groupId: '${SG.GroupId}', + ipProtocol: 'tcp', + fromPort: 80, + toPort: 80, + peer: { kind: 'security-group', securityGroupId: '${ThatOtherGroup.GroupId}' } + }, + { + groupId: '${SG.GroupId}', + ipProtocol: 'udp', + fromPort: -1, + toPort: -1, + peer: { kind: 'cidr-ip', ip: '7.8.9.0/24' } + } + ] + }); + + test.done(); + } +}; diff --git a/packages/@aws-cdk/cloudformation-diff/test/network/test.rule.ts b/packages/@aws-cdk/cloudformation-diff/test/network/test.rule.ts new file mode 100644 index 0000000000000..c94113f9a5aca --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/test/network/test.rule.ts @@ -0,0 +1,120 @@ +import fc = require('fast-check'); +import { Test } from 'nodeunit'; +import { SecurityGroupRule } from '../../lib/network/security-group-rule'; + +export = { + 'can parse cidr-ip'(test: Test) { + const rule = new SecurityGroupRule({ + GroupId: 'sg-1234', + IpProtocol: 'tcp', + FromPort: 10, + ToPort: 20, + CidrIp: '1.2.3.4/8', + }); + + test.equal(rule.groupId, 'sg-1234'); + test.equal(rule.ipProtocol, 'tcp'); + test.equal(rule.fromPort, 10); + test.equal(rule.toPort, 20); + + const peer = rule.peer!; + if (peer.kind !== 'cidr-ip') { throw new Error('Fail'); } + test.equal(peer.ip, '1.2.3.4/8'); + + test.done(); + }, + + 'can parse cidr-ip 6'(test: Test) { + const rule = new SecurityGroupRule({ + CidrIpv6: '::0/0' + }); + + const peer = rule.peer!; + if (peer.kind !== 'cidr-ip') { throw new Error('Fail'); } + test.equal(peer.ip, '::0/0'); + + test.done(); + }, + + 'can parse securityGroup'(test: Test) { + for (const key of ['DestinationSecurityGroupId', 'SourceSecurityGroupId']) { + const rule = new SecurityGroupRule({ + [key]: 'sg-1234', + }); + + const peer = rule.peer!; + if (peer.kind !== 'security-group') { throw new Error('Fail'); } + test.equal(peer.securityGroupId, 'sg-1234'); + } + + test.done(); + }, + + 'can parse prefixlist'(test: Test) { + for (const key of ['DestinationPrefixListId', 'SourcePrefixListId']) { + const rule = new SecurityGroupRule({ + [key]: 'pl-1', + }); + + const peer = rule.peer!; + if (peer.kind !== 'prefix-list') { throw new Error('Fail'); } + test.equal(peer.prefixListId, 'pl-1'); + } + + test.done(); + }, + + 'equality is reflexive'(test: Test) { + fc.assert(fc.property( + arbitraryRule, (statement) => { + return new SecurityGroupRule(statement).equal(new SecurityGroupRule(statement)); + } + )); + test.done(); + }, + + 'equality is symmetric'(test: Test) { + fc.assert(fc.property( + twoArbitraryRules, (s) => { + const a = new SecurityGroupRule(s.rule1); + const b = new SecurityGroupRule(s.rule2); + + fc.pre(a.equal(b)); + return b.equal(a); + } + )); + test.done(); + }, +}; + +const arbitraryRule = fc.record({ + IpProtocol: fc.constantFrom('tcp', 'udp', 'icmp'), + FromPort: fc.integer(80, 81), + ToPort: fc.integer(81, 82), + CidrIp: fc.constantFrom('0.0.0.0/0', '1.2.3.4/8', undefined, undefined), + DestinationSecurityGroupId: fc.constantFrom('sg-1234', undefined), + DestinationPrefixListId: fc.constantFrom('pl-1', undefined), +}); + +const twoArbitraryRules = fc.record({ + rule1: arbitraryRule, + rule2: arbitraryRule, + copyIp: fc.boolean(), + copyFromPort: fc.boolean(), + copyToPort : fc.boolean(), + copyCidrIp: fc.boolean(), + copySecurityGroupId: fc.boolean(), + copyPrefixListId: fc.boolean(), +}).map(op => { + const original = op.rule1; + const modified = Object.create(original, {}); + + if (op.copyIp) { modified.IpProtocol = op.rule2.IpProtocol; } + if (op.copyFromPort) { modified.FromPort = op.rule2.FromPort; } + if (op.copyToPort) { modified.ToPort = op.rule2.ToPort; } + if (op.copyCidrIp) { modified.CidrIp = op.rule2.CidrIp; } + if (op.copySecurityGroupId) { modified.DestinationSecurityGroupId = op.rule2.DestinationSecurityGroupId; } + if (op.copyPrefixListId) { modified.DestinationPrefixListId = op.rule2.DestinationPrefixListId; } + + return { rule1: original, rule2: modified }; +}); \ No newline at end of file From c3f9557bd614d40003a99078fb187ac5626ade3f Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 30 Nov 2018 15:32:41 +0100 Subject: [PATCH 16/23] Replace tables library in toolkit --- packages/aws-cdk/lib/commands/context.ts | 14 ++++---------- packages/aws-cdk/lib/util/index.ts | 1 + packages/aws-cdk/lib/util/tables.ts | 9 +++++++++ packages/aws-cdk/package.json | 4 ++-- 4 files changed, 16 insertions(+), 12 deletions(-) create mode 100644 packages/aws-cdk/lib/util/tables.ts diff --git a/packages/aws-cdk/lib/commands/context.ts b/packages/aws-cdk/lib/commands/context.ts index e77d0cbf35864..ae6c180644029 100644 --- a/packages/aws-cdk/lib/commands/context.ts +++ b/packages/aws-cdk/lib/commands/context.ts @@ -1,8 +1,8 @@ import colors = require('colors/safe'); -import table = require('table'); import yargs = require('yargs'); -import { print } from '../../lib/logging'; +import { print } from '../logging'; import { Configuration, DEFAULTS } from '../settings'; +import { renderTable } from '../util'; export const command = 'context'; export const describe = 'Manage cached context values'; @@ -57,13 +57,7 @@ function listContext(context: any) { print(`Context found in ${colors.blue(DEFAULTS)}:\n`); - print(table.table(data, { - border: table.getBorderCharacters('norc'), - columns: { - 1: { width: 50, wrapWord: true } as any, - 2: { width: 50, wrapWord: true } as any - } - })); + print(renderTable(data, colWidths: [2, 50, 50])); // tslint:disable-next-line:max-line-length print(`Run ${colors.blue('cdk context --reset KEY_OR_NUMBER')} to remove a context key. It will be refreshed on the next CDK synthesis run.`); @@ -111,4 +105,4 @@ function enumerate1(xs: T[]): Array<[number, T]> { i += 1; } return ret; -} \ No newline at end of file +} diff --git a/packages/aws-cdk/lib/util/index.ts b/packages/aws-cdk/lib/util/index.ts index d3781a0fcc33e..a9a8af258b0f2 100644 --- a/packages/aws-cdk/lib/util/index.ts +++ b/packages/aws-cdk/lib/util/index.ts @@ -1,3 +1,4 @@ export * from './arrays'; export * from './objects'; export * from './types'; +export * from './tables'; diff --git a/packages/aws-cdk/lib/util/tables.ts b/packages/aws-cdk/lib/util/tables.ts new file mode 100644 index 0000000000000..010c626f9457a --- /dev/null +++ b/packages/aws-cdk/lib/util/tables.ts @@ -0,0 +1,9 @@ +import Table = require('cli-table'); + +export function renderTable(cells: string[][], colWidths?: number[]): string { + const head = cells.splice(0, 1)[0]; + + const table = new Table({ head, style: { head: [] }, colWidths }); + table.push(...cells); + return table.toString(); +} diff --git a/packages/aws-cdk/package.json b/packages/aws-cdk/package.json index eea4b095c60c0..ac2c6bfe6b5e5 100644 --- a/packages/aws-cdk/package.json +++ b/packages/aws-cdk/package.json @@ -32,12 +32,12 @@ "license": "Apache-2.0", "devDependencies": { "@types/archiver": "^2.1.2", + "@types/cli-table": "^0.3.0", "@types/fs-extra": "^5.0.4", "@types/minimatch": "^3.0.3", "@types/mockery": "^1.4.29", "@types/request": "^2.47.1", "@types/semver": "^5.5.0", - "@types/table": "^4.0.5", "@types/uuid": "^3.4.3", "@types/yaml": "^1.0.0", "@types/yargs": "^8.0.3", @@ -58,11 +58,11 @@ "json-diff": "^0.3.1", "minimatch": ">=3.0", "promptly": "^0.2.0", + "cli-table": "^0.3.1", "proxy-agent": "^3.0.1", "request": "^2.83.0", "semver": "^5.5.0", "source-map-support": "^0.5.6", - "table": "^5.1.0", "yaml": "^1.0.1", "yargs": "^9.0.1" }, From ec11d7931373beda70a72ffd36e30a3b3c0c2016 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 3 Dec 2018 09:52:39 +0100 Subject: [PATCH 17/23] Fix syntax for renderTable() --- packages/aws-cdk/lib/commands/context.ts | 2 +- packages/aws-cdk/lib/util/tables.ts | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/aws-cdk/lib/commands/context.ts b/packages/aws-cdk/lib/commands/context.ts index ae6c180644029..e60f5a576fcd4 100644 --- a/packages/aws-cdk/lib/commands/context.ts +++ b/packages/aws-cdk/lib/commands/context.ts @@ -57,7 +57,7 @@ function listContext(context: any) { print(`Context found in ${colors.blue(DEFAULTS)}:\n`); - print(renderTable(data, colWidths: [2, 50, 50])); + print(renderTable(data, { colWidths: [2, 50, 50] })); // tslint:disable-next-line:max-line-length print(`Run ${colors.blue('cdk context --reset KEY_OR_NUMBER')} to remove a context key. It will be refreshed on the next CDK synthesis run.`); diff --git a/packages/aws-cdk/lib/util/tables.ts b/packages/aws-cdk/lib/util/tables.ts index 010c626f9457a..56a42f13fb105 100644 --- a/packages/aws-cdk/lib/util/tables.ts +++ b/packages/aws-cdk/lib/util/tables.ts @@ -1,9 +1,13 @@ import Table = require('cli-table'); -export function renderTable(cells: string[][], colWidths?: number[]): string { +export interface RenderTableOptions { + colWidths?: number[]; +} + +export function renderTable(cells: string[][], options: RenderTableOptions = {}): string { const head = cells.splice(0, 1)[0]; - const table = new Table({ head, style: { head: [] }, colWidths }); + const table = new Table({ head, style: { head: [] }, colWidths: options.colWidths }); table.push(...cells); return table.toString(); } From e2cc62a49423ab62356dd8d714734459d3e387bf Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 6 Dec 2018 10:31:24 +0100 Subject: [PATCH 18/23] Add toolkit integ test --- packages/aws-cdk/integ-tests/app/app.js | 14 ++++++++++ packages/aws-cdk/integ-tests/common.bash | 7 ++++- .../integ-tests/test-cdk-deploy-all.sh | 5 ++-- .../aws-cdk/integ-tests/test-cdk-iam-diff.sh | 26 +++++++++++++++++++ packages/aws-cdk/integ-tests/test-cdk-ls.sh | 1 + 5 files changed, 49 insertions(+), 4 deletions(-) create mode 100755 packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh diff --git a/packages/aws-cdk/integ-tests/app/app.js b/packages/aws-cdk/integ-tests/app/app.js index c63e71e9208e6..8da2b8aa6507b 100644 --- a/packages/aws-cdk/integ-tests/app/app.js +++ b/packages/aws-cdk/integ-tests/app/app.js @@ -1,4 +1,5 @@ const cdk = require('@aws-cdk/cdk'); +const iam = require('@aws-cdk/aws-iam'); const sns = require('@aws-cdk/aws-sns'); class MyStack extends cdk.Stack { @@ -19,9 +20,22 @@ class YourStack extends cdk.Stack { } } +class IamStack extends cdk.Stack { + constructor(parent, id) { + super(parent, id); + + new iam.Role(this, 'SomeRole', { + assumedBy: new iam.ServicePrincipal('ec2.amazon.aws.com') + }); + } +} + const app = new cdk.App(); +// Deploy all does a wildcard cdk-toolkit-integration-test-* new MyStack(app, 'cdk-toolkit-integration-test-1'); new YourStack(app, 'cdk-toolkit-integration-test-2'); +// Not included in wildcard +new IamStack(app, 'cdk-toolkit-integration-iam-test'); app.run(); diff --git a/packages/aws-cdk/integ-tests/common.bash b/packages/aws-cdk/integ-tests/common.bash index 9ff7aaf1cd90f..cdb0d0baa4139 100644 --- a/packages/aws-cdk/integ-tests/common.bash +++ b/packages/aws-cdk/integ-tests/common.bash @@ -37,6 +37,7 @@ function setup() { ( cd node_modules/@aws-cdk ln -s ${scriptdir}/../../@aws-cdk/aws-sns + ln -s ${scriptdir}/../../@aws-cdk/aws-iam ln -s ${scriptdir}/../../@aws-cdk/cdk ) } @@ -51,7 +52,7 @@ function assert_diff() { local actual=$2 local expected=$3 - diff ${actual} ${expected} || { + diff -w ${actual} ${expected} || { echo echo "+-----------" echo "| expected:" @@ -95,3 +96,7 @@ function assert_lines() { fail "response has ${lines} lines and we expected ${expected} lines to be returned" fi } + +function strip_color_codes() { + perl -pe 's/\e\[?.*?[\@-~]//g' +} diff --git a/packages/aws-cdk/integ-tests/test-cdk-deploy-all.sh b/packages/aws-cdk/integ-tests/test-cdk-deploy-all.sh index a91c2e1677cd6..dc712453b5594 100755 --- a/packages/aws-cdk/integ-tests/test-cdk-deploy-all.sh +++ b/packages/aws-cdk/integ-tests/test-cdk-deploy-all.sh @@ -6,7 +6,7 @@ source ${scriptdir}/common.bash setup -stack_arns=$(cdk deploy) +stack_arns=$(cdk deploy cdk-toolkit-integration-test-\*) echo "Stack deployed successfully" # verify that we only deployed a single stack (there's a single ARN in the output) @@ -18,7 +18,6 @@ if [ "${lines}" -ne 2 ]; then fail "cdk deploy returned ${lines} arns and we expected 2" fi -cdk destroy -f cdk-toolkit-integration-test-1 -cdk destroy -f cdk-toolkit-integration-test-2 +cdk destroy -f cdk-toolkit-integration-test-\* echo "✅ success" diff --git a/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh b/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh new file mode 100755 index 0000000000000..4246903f079a8 --- /dev/null +++ b/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh @@ -0,0 +1,26 @@ +#!/bin/bash +set -euo pipefail +scriptdir=$(cd $(dirname $0) && pwd) +source ${scriptdir}/common.bash +# ---------------------------------------------------------- + +setup + +function nonfailing_diff() { + ( cdk diff $1 2>&1 || true ) | strip_color_codes +} + +assert "nonfailing_diff cdk-toolkit-integration-iam-test" < Date: Thu, 6 Dec 2018 10:55:56 +0100 Subject: [PATCH 19/23] WIP --- packages/@aws-cdk/cloudformation-diff/lib/format.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index 53d19aad0a734..1118395d42463 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -28,8 +28,7 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp formatter.printSectionFooter(); } - formatter.formatIamChanges(templateDiff.iamChanges); - formatter.formatSecurityGroupChanges(templateDiff.securityGroupChanges); + formatSecurityChangesWithBanner(formatter, templateDiff); formatter.formatSection('Parameters', 'Parameter', templateDiff.parameters); formatter.formatSection('Metadata', 'Metadata', templateDiff.metadata); @@ -47,6 +46,11 @@ export function formatSecurityChanges(stream: NodeJS.WriteStream, templateDiff: const formatter = new Formatter(stream, logicalToPathMap); formatter.readConstructPathsFrom(templateDiff); + formatSecurityChangesWithBanner(formatter, templateDiff); +} + +function formatSecurityChangesWithBanner(formatter: Formatter, templateDiff: TemplateDiff) { + // if (templateDiff.iamChanges. formatter.formatIamChanges(templateDiff.iamChanges); formatter.formatSecurityGroupChanges(templateDiff.securityGroupChanges); } From 86c4287644c369cbb4d345d18da318a3c2fdd894 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 6 Dec 2018 15:34:09 +0100 Subject: [PATCH 20/23] Incorporate review comments --- .../@aws-cdk/cfnspec/build-tools/scrutiny.ts | 4 +- packages/@aws-cdk/cfnspec/lib/index.ts | 2 +- .../cloudformation-diff/lib/diff/types.ts | 7 + .../cloudformation-diff/lib/format.ts | 29 ++- .../lib/iam/iam-changes.ts | 14 +- .../lib/network/security-group-changes.ts | 6 +- .../lib/{uncfn.ts => render-intrinsics.ts} | 10 +- .../test/iam/test.broadening.ts | 233 +++++++++++++++--- .../test/iam/test.detect-changes.ts | 71 +++++- ...est.uncfn.ts => test.render-intrinsics.ts} | 14 +- packages/aws-cdk/bin/cdk.ts | 12 +- packages/aws-cdk/lib/diff.ts | 29 ++- 12 files changed, 348 insertions(+), 83 deletions(-) rename packages/@aws-cdk/cloudformation-diff/lib/{uncfn.ts => render-intrinsics.ts} (88%) rename packages/@aws-cdk/cloudformation-diff/test/{test.uncfn.ts => test.render-intrinsics.ts} (75%) diff --git a/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts b/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts index 9a6bf3df097b3..48cd3102ef528 100644 --- a/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts +++ b/packages/@aws-cdk/cfnspec/build-tools/scrutiny.ts @@ -36,7 +36,7 @@ function detectResourceScrutiny(typeName: string, typeSpec: schema.ResourceType) const properties = Object.entries(typeSpec.Properties || {}); // If this resource is named like *Policy and has a PolicyDocument property - if (typeName.endsWith('Policy') && properties.some(app2(isPolicyDocumentProperty))) { + if (typeName.endsWith('Policy') && properties.some(apply2(isPolicyDocumentProperty))) { typeSpec.ScrutinyType = isIamType(typeName) ? ResourceScrutinyType.IdentityPolicyResource : ResourceScrutinyType.ResourcePolicyResource; return; } @@ -84,6 +84,6 @@ function isPolicyDocumentProperty(propertyName: string, propertySpec: schema.Pro * me to overload this type declaration so we need a different function for * every # of arguments. */ -function app2(fn: (a1: T1, a2: T2) => R): (as: [T1, T2]) => R { +function apply2(fn: (a1: T1, a2: T2) => R): (as: [T1, T2]) => R { return (as) => fn.apply(fn, as); } \ No newline at end of file diff --git a/packages/@aws-cdk/cfnspec/lib/index.ts b/packages/@aws-cdk/cfnspec/lib/index.ts index f114178ff937e..7037cb29c7594 100644 --- a/packages/@aws-cdk/cfnspec/lib/index.ts +++ b/packages/@aws-cdk/cfnspec/lib/index.ts @@ -12,7 +12,7 @@ export function specification(): schema.Specification { /** * Return the resource specification for the given typename * - * Validates that the resource exists. If you don't this validating behavior, read from + * Validates that the resource exists. If you don't want this validating behavior, read from * specification() directly. */ export function resourceSpecification(typeName: string): schema.ResourceType { diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index 4afbcf0e2f657..f222e56171ac3 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -97,6 +97,13 @@ export class TemplateDiff implements ITemplateDiff { return this.iamChanges.permissionsBroadened || this.securityGroupChanges.rulesAdded; } + /** + * Return true if any of the permissions objects have changed + */ + public get permissionsAnyChanges(): boolean { + return this.iamChanges.hasChanges || this.securityGroupChanges.hasChanges; + } + /** * Return all property changes of a given scrutiny type * diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index 1118395d42463..9e5dd702f3d2a 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -16,9 +16,7 @@ import { SecurityGroupChanges } from './network/security-group-changes'; * case there is no aws:cdk:path metadata in the template. */ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: TemplateDiff, logicalToPathMap: { [logicalId: string]: string } = { }) { - const formatter = new Formatter(stream, logicalToPathMap); - - formatter.readConstructPathsFrom(templateDiff); + const formatter = new Formatter(stream, logicalToPathMap, templateDiff); if (templateDiff.awsTemplateFormatVersion || templateDiff.transform || templateDiff.description) { formatter.printSectionHeader('Template'); @@ -43,16 +41,18 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp * Renders a diff of security changes to the given stream */ export function formatSecurityChanges(stream: NodeJS.WriteStream, templateDiff: TemplateDiff, logicalToPathMap: {[logicalId: string]: string} = {}) { - const formatter = new Formatter(stream, logicalToPathMap); - formatter.readConstructPathsFrom(templateDiff); + const formatter = new Formatter(stream, logicalToPathMap, templateDiff); formatSecurityChangesWithBanner(formatter, templateDiff); } function formatSecurityChangesWithBanner(formatter: Formatter, templateDiff: TemplateDiff) { - // if (templateDiff.iamChanges. + if (!templateDiff.iamChanges.hasChanges && !templateDiff.securityGroupChanges.hasChanges) { return; } 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)`); + formatter.printSectionFooter(); } const ADDITION = colors.green('[+]'); @@ -60,13 +60,21 @@ const UPDATE = colors.yellow('[~]'); const REMOVAL = colors.red('[-]'); class Formatter { - constructor(private readonly stream: NodeJS.WriteStream, private readonly logicalToPathMap: { [logicalId: string]: string }) { + constructor(private readonly stream: NodeJS.WriteStream, private readonly logicalToPathMap: { [logicalId: string]: string }, diff?: TemplateDiff) { + // Read additional construct paths from the diff if it is supplied + if (diff) { + this.readConstructPathsFrom(diff); + } } public print(fmt: string, ...args: any[]) { this.stream.write(colors.white(format(fmt, ...args)) + '\n'); } + public warning(fmt: string, ...args: any[]) { + this.stream.write(colors.yellow(format(fmt, ...args)) + '\n'); + } + public formatSection>( title: string, entryType: string, @@ -327,16 +335,13 @@ class Formatter { this.printSectionHeader('IAM Policy Changes'); this.print(renderTable(this.deepSubstituteBracedLogicalIds(changes.summarizeManagedPolicies()))); } - - this.printSectionFooter(); } public formatSecurityGroupChanges(changes: SecurityGroupChanges) { if (!changes.hasChanges) { return; } - this.printSectionHeader('Summary of Security Group Changes'); + this.printSectionHeader('Security Group Changes'); this.print(renderTable(this.deepSubstituteBracedLogicalIds(changes.summarize()))); - this.printSectionFooter(); } public deepSubstituteBracedLogicalIds(rows: string[][]): string[][] { @@ -363,7 +368,7 @@ function renderTable(cells: string[][]): string { const table = new Table({ head, style: { head: [] } }); table.push(...cells); - return stripHorizontalLines(table.toString()); + return stripHorizontalLines(table.toString()).trimRight(); } /** diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts index 23c2bef466b31..6fd16a9759d80 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts @@ -2,7 +2,7 @@ import cfnspec = require('@aws-cdk/cfnspec'); import colors = require('colors/safe'); import { PropertyChange, PropertyMap, ResourceChange } from "../diff/types"; import { DiffableCollection } from '../diffable'; -import { unCloudFormation } from "../uncfn"; +import { renderIntrinsics } from "../render-intrinsics"; import { deepRemoveUndefined, dropIfEmpty, flatMap, makeComparator } from '../util'; import { ManagedPolicyAttachment, ManagedPolicyJson, parseManagedPolicies } from './managed-policy'; import { parseLambdaPermission, parseStatements, renderCondition, Statement, StatementJson, Targets } from "./statement"; @@ -184,7 +184,7 @@ export class IamChanges { const appliesToPrincipal = 'AWS:${' + logicalId + '}'; return flatMap(policies, (policy: any) => { - return defaultPrincipal(appliesToPrincipal, parseStatements(unCloudFormation(policy.PolicyDocument.Statement))); + return defaultPrincipal(appliesToPrincipal, parseStatements(renderIntrinsics(policy.PolicyDocument.Statement))); }); } @@ -194,7 +194,7 @@ export class IamChanges { private readIdentityPolicyResource(properties: any): Statement[] { if (properties === undefined) { return []; } - properties = unCloudFormation(properties); + properties = renderIntrinsics(properties); const principals = (properties.Groups || []).concat(properties.Users || []).concat(properties.Roles || []); return flatMap(principals, (principal: string) => { @@ -207,7 +207,7 @@ export class IamChanges { if (policy === undefined) { return []; } const appliesToResource = '${' + logicalId + '.Arn}'; - return defaultResource(appliesToResource, parseStatements(unCloudFormation(policy.Statement))); + return defaultResource(appliesToResource, parseStatements(renderIntrinsics(policy.Statement))); } /** @@ -216,7 +216,7 @@ export class IamChanges { private readResourcePolicyResource(properties: any): Statement[] { if (properties === undefined) { return []; } - properties = unCloudFormation(properties); + properties = renderIntrinsics(properties); const policyKeys = Object.keys(properties).filter(key => key.indexOf('Policy') > -1); @@ -238,13 +238,13 @@ export class IamChanges { if (!policyArns) { return []; } const rep = '${' + logicalId + '}'; - return parseManagedPolicies(rep, unCloudFormation(policyArns)); + return parseManagedPolicies(rep, renderIntrinsics(policyArns)); } private readLambdaStatements(properties?: PropertyMap): Statement[] { if (!properties) { return []; } - return [parseLambdaPermission(unCloudFormation(properties))]; + return [parseLambdaPermission(renderIntrinsics(properties))]; } } diff --git a/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts index a71eda7b345bb..dca91de398547 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts @@ -1,7 +1,7 @@ import colors = require('colors/safe'); import { PropertyChange, ResourceChange } from "../diff/types"; import { DiffableCollection } from "../diffable"; -import { unCloudFormation } from "../uncfn"; +import { renderIntrinsics } from "../render-intrinsics"; import { deepRemoveUndefined, dropIfEmpty, makeComparator } from '../util'; import { RuleJson, SecurityGroupRule } from "./security-group-rule"; @@ -102,7 +102,7 @@ export class SecurityGroupChanges { // UnCloudFormation so the parser works in an easier domain const ref = '${' + logicalId + '.GroupId}'; - return rules.map((r: any) => new SecurityGroupRule(unCloudFormation(r), ref)); + return rules.map((r: any) => new SecurityGroupRule(renderIntrinsics(r), ref)); } private readRuleResource(resource: any): SecurityGroupRule[] { @@ -110,7 +110,7 @@ export class SecurityGroupChanges { // UnCloudFormation so the parser works in an easier domain - return [new SecurityGroupRule(unCloudFormation(resource))]; + return [new SecurityGroupRule(renderIntrinsics(resource))]; } } diff --git a/packages/@aws-cdk/cloudformation-diff/lib/uncfn.ts b/packages/@aws-cdk/cloudformation-diff/lib/render-intrinsics.ts similarity index 88% rename from packages/@aws-cdk/cloudformation-diff/lib/uncfn.ts rename to packages/@aws-cdk/cloudformation-diff/lib/render-intrinsics.ts index f8bfa7845c957..ea392d2de8444 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/uncfn.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/render-intrinsics.ts @@ -16,9 +16,9 @@ * For other intrinsics we choose a string representation that CloudFormation * cannot actually parse, but is comprehensible to humans. */ -export function unCloudFormation(x: any): any { +export function renderIntrinsics(x: any): any { if (Array.isArray(x)) { - return x.map(unCloudFormation); + return x.map(renderIntrinsics); } const intrinsic = getIntrinsic(x); @@ -32,7 +32,7 @@ export function unCloudFormation(x: any): any { if (typeof x === 'object' && x !== null) { const ret: any = {}; for (const [key, value] of Object.entries(x)) { - ret[key] = unCloudFormation(value); + ret[key] = renderIntrinsics(value); } return ret; } @@ -41,13 +41,13 @@ export function unCloudFormation(x: any): any { function unCloudFormationFnJoin(separator: string, args: any) { if (Array.isArray(args)) { - return args.map(unCloudFormation).join(separator); + return args.map(renderIntrinsics).join(separator); } return stringifyIntrinsic('Fn::Join', [separator, args]); } function stringifyIntrinsic(fn: string, args: any) { - return JSON.stringify({ [fn]: unCloudFormation(args) }); + return JSON.stringify({ [fn]: renderIntrinsics(args) }); } function getIntrinsic(x: any): Intrinsic | undefined { diff --git a/packages/@aws-cdk/cloudformation-diff/test/iam/test.broadening.ts b/packages/@aws-cdk/cloudformation-diff/test/iam/test.broadening.ts index f02bbe86b590c..27b45f54e7651 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/iam/test.broadening.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/iam/test.broadening.ts @@ -3,44 +3,209 @@ import { diffTemplate } from '../../lib'; import { poldoc, resource, template } from '../util'; export = { - 'adding of positive statements counts as permission widening'(test: Test) { - // WHEN - const diff = diffTemplate({}, template({ - QueuePolicy: resource('AWS::SQS::QueuePolicy', { - Queues: [ { Ref: 'MyQueue' } ], - PolicyDocument: poldoc({ - Effect: 'Allow', - Action: 'sqs:SendMessage', - Resource: '*', - Principal: { Service: 'sns.amazonaws.com' } - }) - }) - })); - - // THEN - test.equal(diff.permissionsBroadened, true); - - test.done(); - }, + 'broadening is': { + 'adding of positive statements'(test: Test) { + // WHEN + const diff = diffTemplate({}, template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue' } ], + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + }) + }) + })); + + // THEN + test.equal(diff.permissionsBroadened, true); + + test.done(); + }, + + 'adding of positive statements to an existing policy'(test: Test) { + // WHEN + const diff = diffTemplate(template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue' } ], + PolicyDocument: poldoc( + { + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + } + ) + }) + }), template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue' } ], + PolicyDocument: poldoc( + { + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + }, + { + Effect: 'Allow', + Action: 'sqs:LookAtMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + } + ) + }) + })); + + // THEN + test.equal(diff.permissionsBroadened, true); + + test.done(); + }, + + 'removal of not-statements'(test: Test) { + // WHEN + const diff = diffTemplate(template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue' } ], + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + NotPrincipal: { Service: 'sns.amazonaws.com' } + }) + }) + }), {}); + + // THEN + test.equal(diff.permissionsBroadened, true); + + test.done(); + }, - 'removal of not-statements counts as permission widening'(test: Test) { - // WHEN - const diff = diffTemplate(template({ - QueuePolicy: resource('AWS::SQS::QueuePolicy', { - Queues: [ { Ref: 'MyQueue' } ], - PolicyDocument: poldoc({ - Effect: 'Allow', - Action: 'sqs:SendMessage', - Resource: '*', - NotPrincipal: { Service: 'sns.amazonaws.com' } + 'changing of resource target'(test: Test) { + // WHEN + const diff = diffTemplate(template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue' } ], + PolicyDocument: poldoc( + { + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + } + ) }) - }) - }), {}); + }), template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyOtherQueue' } ], + PolicyDocument: poldoc( + { + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + } + ) + }) + })); + + // THEN + test.equal(diff.permissionsBroadened, true); - // THEN - test.equal(diff.permissionsBroadened, true); + test.done(); + }, + + 'addition of ingress rules'(test: Test) { + // WHEN + const diff = diffTemplate( + template({ + }), + template({ + SG: resource('AWS::EC2::SecurityGroup', { + SecurityGroupIngress: [ + { + CidrIp: '1.2.3.4/8', + FromPort: 80, + ToPort: 80, + IpProtocol: 'tcp', + } + ], + }) + })); - test.done(); + // THEN + test.equal(diff.permissionsBroadened, true); + + test.done(); + }, + + 'addition of egress rules'(test: Test) { + // WHEN + const diff = diffTemplate( + template({ + }), + template({ + SG: resource('AWS::EC2::SecurityGroup', { + SecurityGroupEgress: [ + { + DestinationSecurityGroupId: { 'Fn::GetAtt': ['ThatOtherGroup', 'GroupId'] }, + FromPort: 80, + ToPort: 80, + IpProtocol: 'tcp', + } + ], + }) + })); + + // THEN + test.equal(diff.permissionsBroadened, true); + + test.done(); + }, }, + 'broadening is not': { + 'removal of positive statements from an existing policy'(test: Test) { + // WHEN + const diff = diffTemplate(template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue' } ], + PolicyDocument: poldoc( + { + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + }, + { + Effect: 'Allow', + Action: 'sqs:LookAtMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + } + ) + }) + }), template({ + QueuePolicy: resource('AWS::SQS::QueuePolicy', { + Queues: [ { Ref: 'MyQueue' } ], + PolicyDocument: poldoc( + { + Effect: 'Allow', + Action: 'sqs:SendMessage', + Resource: '*', + Principal: { Service: 'sns.amazonaws.com' } + } + ) + }) + })); + + // THEN + test.equal(diff.permissionsBroadened, false); + + test.done(); + }, + } }; \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts b/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts index 836d33be5bbc3..5006b470cdb5c 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/iam/test.detect-changes.ts @@ -187,7 +187,7 @@ export = { test.done(); }, - 'finds statement removals'(test: Test) { + 'finds sole statement removals'(test: Test) { // WHEN const diff = diffTemplate(template({ BucketPolicy: resource('AWS::S3::BucketPolicy', { @@ -216,6 +216,52 @@ export = { test.done(); }, + 'finds one of many statement removals'(test: Test) { + // WHEN + const diff = diffTemplate( + template({ + BucketPolicy: resource('AWS::S3::BucketPolicy', { + Bucket: { Ref: 'MyBucket' }, + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 's3:PutObject', + Resource: '*', + Principal: { AWS: 'me' } + }, { + Effect: 'Allow', + Action: 's3:LookAtObject', + Resource: '*', + Principal: { AWS: 'me' } + }) + }) + }), + template({ + BucketPolicy: resource('AWS::S3::BucketPolicy', { + Bucket: { Ref: 'MyBucket' }, + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 's3:LookAtObject', + Resource: '*', + Principal: { AWS: 'me' } + }) + }) + })); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + statementRemovals: [ + { + effect: 'Allow', + resources: { not: false, values: [ '${MyBucket}' ] }, + principals: { not: false, values: [ 'AWS:me' ] }, + actions: { not: false, values: [ 's3:PutObject' ] }, + } + ] + }); + + test.done(); + }, + 'finds policy attachments'(test: Test) { // WHEN const diff = diffTemplate({}, template({ @@ -238,6 +284,29 @@ export = { }, 'finds policy removals'(test: Test) { + // WHEN + const diff = diffTemplate( + template({ + SomeRole: resource('AWS::IAM::Role', { + ManagedPolicyArns: ['arn:policy', 'arn:policy2'], + }) + }), + template({ + SomeRole: resource('AWS::IAM::Role', { + ManagedPolicyArns: ['arn:policy2'], + }) + })); + + // THEN + test.deepEqual(diff.iamChanges.toJson(), { + managedPolicyRemovals: [ + { + identityArn: '${SomeRole}', + managedPolicyArn: 'arn:policy' + } + ] + }); + test.done(); }, diff --git a/packages/@aws-cdk/cloudformation-diff/test/test.uncfn.ts b/packages/@aws-cdk/cloudformation-diff/test/test.render-intrinsics.ts similarity index 75% rename from packages/@aws-cdk/cloudformation-diff/test/test.uncfn.ts rename to packages/@aws-cdk/cloudformation-diff/test/test.render-intrinsics.ts index b21af1e1477f6..f0c012cd8b786 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/test.uncfn.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/test.render-intrinsics.ts @@ -1,10 +1,10 @@ import { Test } from 'nodeunit'; -import { unCloudFormation } from '../lib/uncfn'; +import { renderIntrinsics } from '../lib/render-intrinsics'; export = { 'resolves Ref'(test: Test) { test.equals( - unCloudFormation({ Ref: 'SomeLogicalId' }), + renderIntrinsics({ Ref: 'SomeLogicalId' }), '${SomeLogicalId}' ); test.done(); @@ -12,7 +12,7 @@ export = { 'resolves Fn::GetAtt'(test: Test) { test.equals( - unCloudFormation({ 'Fn::GetAtt': ['SomeLogicalId', 'Attribute'] }), + renderIntrinsics({ 'Fn::GetAtt': ['SomeLogicalId', 'Attribute'] }), '${SomeLogicalId.Attribute}' ); test.done(); @@ -20,7 +20,7 @@ export = { 'resolves Fn::Join'(test: Test) { test.equals( - unCloudFormation({ 'Fn::Join': ['/', ['a', 'b', 'c']] }), + renderIntrinsics({ 'Fn::Join': ['/', ['a', 'b', 'c']] }), 'a/b/c' ); @@ -29,7 +29,7 @@ export = { 'does not resolve Fn::Join if the second argument is not a list literal'(test: Test) { test.equals( - unCloudFormation({ 'Fn::Join': ['/', { Ref: 'ListParameter' }] }), + renderIntrinsics({ 'Fn::Join': ['/', { Ref: 'ListParameter' }] }), '{"Fn::Join":["/","${ListParameter}"]}' ); @@ -38,7 +38,7 @@ export = { 'deep resolves intrinsics in object'(test: Test) { test.deepEqual( - unCloudFormation({ + renderIntrinsics({ Deeper1: { Ref: 'SomeLogicalId' }, Deeper2: 'Do not replace', }), @@ -52,7 +52,7 @@ export = { 'deep resolves intrinsics in array'(test: Test) { test.deepEqual( - unCloudFormation([ + renderIntrinsics([ { Ref: 'SomeLogicalId' }, 'Do not replace', ]), diff --git a/packages/aws-cdk/bin/cdk.ts b/packages/aws-cdk/bin/cdk.ts index 99586cb62b478..57cee2b601cb9 100644 --- a/packages/aws-cdk/bin/cdk.ts +++ b/packages/aws-cdk/bin/cdk.ts @@ -10,7 +10,7 @@ import yargs = require('yargs'); import { bootstrapEnvironment, deployStack, destroyStack, loadToolkitInfo, Mode, SDK } from '../lib'; import { environmentsFromDescriptors, globEnvironmentsFromStacks } from '../lib/api/cxapp/environments'; import { AppStacks, listStackNames } from '../lib/api/cxapp/stacks'; -import { printSecurityDiff, printStackDiff } from '../lib/diff'; +import { printSecurityDiff, printStackDiff, RequireApproval } from '../lib/diff'; import { availableInitLanguages, cliInit, printAvailableTemplates } from '../lib/init'; import { interactive } from '../lib/interactive'; import { data, debug, error, highlight, print, setVerbose, success, warning } from '../lib/logging'; @@ -54,7 +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' }) + .option('require-approval', { type: 'string', choices: [RequireApproval.Never, RequireApproval.AnyChange, RequireApproval.Broadening], default: RequireApproval.Broadening, desc: 'what security-sensitive changes need manual approval' }) .option('toolkit-stack-name', { type: 'string', desc: 'the name of the CDK toolkit stack' })) .command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', yargs => yargs .option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' })) @@ -159,7 +159,7 @@ async function initCommandLine() { return await cliBootstrap(args.ENVIRONMENTS, toolkitStackName, args.roleArn); case 'deploy': - return await cliDeploy(args.STACKS, toolkitStackName, args.roleArn, args.prompt); + return await cliDeploy(args.STACKS, toolkitStackName, args.roleArn, args.requireApproval); case 'destroy': return await cliDestroy(args.STACKS, args.force, args.roleArn); @@ -288,7 +288,7 @@ async function initCommandLine() { return 0; // exit-code } - async function cliDeploy(stackNames: string[], toolkitStackName: string, roleArn: string | undefined, prompt: string) { + async function cliDeploy(stackNames: string[], toolkitStackName: string, roleArn: string | undefined, requireApproval: RequireApproval) { const stacks = await appStacks.selectStacks(...stackNames); renames.validateSelectedStacks(stacks); @@ -301,9 +301,9 @@ async function initCommandLine() { const toolkitInfo = await loadToolkitInfo(stack.environment, aws, toolkitStackName); const deployName = renames.finalName(stack.name); - if (prompt !== 'never') { + if (requireApproval !== RequireApproval.Never) { const currentTemplate = await readCurrentTemplate(stack); - if (printSecurityDiff(currentTemplate, stack)) { + if (printSecurityDiff(currentTemplate, stack, requireApproval)) { const confirmed = await confirm(`Do you wish to deploy these changes (y/n)?`); if (!confirmed) { throw new Error('Aborted by user'); } } diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index 5e19c37ea54c7..2cf86c45a3017 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -33,11 +33,25 @@ export function printStackDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedS return diff.count; } -export function printSecurityDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedStack): boolean { +export enum RequireApproval { + Never = 'never', + + AnyChange = 'any-change', + + Broadening = 'broadening' +} + +/** + * Print the security changes of this diff, if the change is impactful enough according to the approval level + * + * Returns true if the changes are prompt-worthy, false otherwise. + */ +export function printSecurityDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedStack, requireApproval: RequireApproval): boolean { const diff = cfnDiff.diffTemplate(oldTemplate, newTemplate.template); - if (diffHasSecurityImpact(diff)) { - warning(`This deployment will make potentially sensitive changes.`); + if (difRequiresApproval(diff, requireApproval)) { + // tslint:disable-next-line:max-line-length + warning(`This deployment will make potentially sensitive changes according to your current security approval level (--require-approval ${requireApproval}).`); warning(`Please confirm you intend to make the following modifications:\n`); cfnDiff.formatSecurityChanges(process.stderr, diff, buildLogicalToPathMap(newTemplate)); @@ -52,8 +66,13 @@ export function printSecurityDiff(oldTemplate: any, newTemplate: cxapi.Synthesiz * TODO: Filter the security impact determination based off of an enum that allows * us to pick minimum "severities" to alert on. */ -function diffHasSecurityImpact(diff: cfnDiff.TemplateDiff) { - return diff.permissionsBroadened; +function difRequiresApproval(diff: cfnDiff.TemplateDiff, requireApproval: RequireApproval) { + switch (requireApproval) { + case RequireApproval.Never: return false; + case RequireApproval.AnyChange: return diff.permissionsAnyChanges; + case RequireApproval.Broadening: return diff.permissionsBroadened; + default: throw new Error(`Unrecognized approval level: ${requireApproval}`); + } } function buildLogicalToPathMap(template: cxapi.SynthesizedStack) { From 2005ce68d0138ca2b353628a97d517beb7e9546c Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 6 Dec 2018 15:48:49 +0100 Subject: [PATCH 21/23] Make it possible to put requireApproval in cdk.json --- docs/src/tools.rst | 39 +++++++++++++++++++++++++++++++++---- packages/aws-cdk/bin/cdk.ts | 7 +++++-- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/docs/src/tools.rst b/docs/src/tools.rst index 77c60c257b349..bf49b647e53fa 100644 --- a/docs/src/tools.rst +++ b/docs/src/tools.rst @@ -46,7 +46,7 @@ Here are the actions you can take on your CDK app .. code-block:: sh Usage: cdk -a COMMAND - + Commands: list Lists all stacks in the app [aliases: ls] synthesize [STACKS..] Synthesizes and prints the CloudFormation template @@ -64,7 +64,7 @@ Here are the actions you can take on your CDK app used. docs Opens the documentation in a browser[aliases: doc] doctor Check your set-up for potential problems - + Options: --app, -a REQUIRED: Command-line for executing your CDK app (e.g. "node bin/my-app.js") [string] @@ -90,12 +90,43 @@ Here are the actions you can take on your CDK app --role-arn, -r ARN of Role to use when invoking CloudFormation [string] --version Show version number [boolean] --help Show help [boolean] - + If your app has a single stack, there is no need to specify the stack name - + If one of cdk.json or ~/.cdk.json exists, options specified there will be used as defaults. Settings in cdk.json take precedence. +.. _security-changes: + +Security-related changes +======================== + +In order to protect you against unintended changes that affect your security posture, +the CDK toolkit will prompt you to approve security-related changes before deploying +them. + +You change the level of changes that requires approval by specifying: + +.. code-block:: + + cdk deploy --require-approval LEVEL + +Where ``LEVEL`` can be one of: + +* ``never`` - approval is never required. +* ``any-change`` - require approval on any IAM or security-group related change. +* ``broadening`` - require approval when IAM statements or traffic rules are added. Removals + do not require approval. + +The setting also be configured in **cdk.json**: + +.. code-block:: js + + { + "app": "...", + "requireApproval": "never" + } + .. _version-reporting: Version Reporting diff --git a/packages/aws-cdk/bin/cdk.ts b/packages/aws-cdk/bin/cdk.ts index 57cee2b601cb9..fdd95accf1ebd 100644 --- a/packages/aws-cdk/bin/cdk.ts +++ b/packages/aws-cdk/bin/cdk.ts @@ -54,7 +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('require-approval', { type: 'string', choices: [RequireApproval.Never, RequireApproval.AnyChange, RequireApproval.Broadening], default: RequireApproval.Broadening, desc: 'what security-sensitive changes need manual approval' }) + .option('require-approval', { type: 'string', choices: [RequireApproval.Never, RequireApproval.AnyChange, RequireApproval.Broadening], desc: 'what security-sensitive changes need manual approval' }) .option('toolkit-stack-name', { type: 'string', desc: 'the name of the CDK toolkit stack' })) .command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', yargs => yargs .option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' })) @@ -159,7 +159,7 @@ async function initCommandLine() { return await cliBootstrap(args.ENVIRONMENTS, toolkitStackName, args.roleArn); case 'deploy': - return await cliDeploy(args.STACKS, toolkitStackName, args.roleArn, args.requireApproval); + return await cliDeploy(args.STACKS, toolkitStackName, args.roleArn, configuration.combined.get(['requireApproval'])); case 'destroy': return await cliDestroy(args.STACKS, args.force, args.roleArn); @@ -289,6 +289,8 @@ async function initCommandLine() { } async function cliDeploy(stackNames: string[], toolkitStackName: string, roleArn: string | undefined, requireApproval: RequireApproval) { + if (requireApproval === undefined) { requireApproval = RequireApproval.Broadening; } + const stacks = await appStacks.selectStacks(...stackNames); renames.validateSelectedStacks(stacks); @@ -448,6 +450,7 @@ async function initCommandLine() { plugin: argv.plugin, toolkitStackName: argv.toolkitStackName, versionReporting: argv.versionReporting, + requireApproval: argv.requireApproval, pathMetadata: argv.pathMetadata, }); } From fc735aa0886a4b9e924e27354c4575b585c7f79d Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 6 Dec 2018 16:16:19 +0100 Subject: [PATCH 22/23] Update diff with new expectation --- packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh b/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh index 4246903f079a8..b33f52ad3713c 100755 --- a/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh +++ b/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh @@ -17,6 +17,7 @@ IAM Statement Changes ├───┼─────────────────┼────────┼────────────────┼────────────────────────────┼───────────┤ │ + │ \${SomeRole.Arn} │ Allow │ sts:AssumeRole │ Service:ec2.amazon.aws.com │ │ └───┴─────────────────┴────────┴────────────────┴────────────────────────────┴───────────┘ +(This feature is experimental -- it might not be complete. See http://bit.ly/cdk-2EhF7Np) Resources [+] AWS::IAM::Role SomeRole SomeRole6DDC54DD From 92ebd324d8945a96955e01bf516322713aed35a6 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 10 Dec 2018 13:34:23 +0100 Subject: [PATCH 23/23] Update docs --- docs/src/tools.rst | 2 +- packages/@aws-cdk/cloudformation-diff/lib/format.ts | 2 +- packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/src/tools.rst b/docs/src/tools.rst index bf49b647e53fa..c18fba36d43b3 100644 --- a/docs/src/tools.rst +++ b/docs/src/tools.rst @@ -115,7 +115,7 @@ Where ``LEVEL`` can be one of: * ``never`` - approval is never required. * ``any-change`` - require approval on any IAM or security-group related change. -* ``broadening`` - require approval when IAM statements or traffic rules are added. Removals +* ``broadening`` (default) - require approval when IAM statements or traffic rules are added. Removals do not require approval. The setting also be configured in **cdk.json**: diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index 9e5dd702f3d2a..df88d075cc2f6 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -51,7 +51,7 @@ function formatSecurityChangesWithBanner(formatter: Formatter, templateDiff: Tem 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)`); + formatter.warning(`(NOTE: There may be security-related changes not in this list. See http://bit.ly/cdk-2EhF7Np)`); formatter.printSectionFooter(); } diff --git a/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh b/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh index b33f52ad3713c..acc7f9d01320c 100755 --- a/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh +++ b/packages/aws-cdk/integ-tests/test-cdk-iam-diff.sh @@ -17,7 +17,7 @@ IAM Statement Changes ├───┼─────────────────┼────────┼────────────────┼────────────────────────────┼───────────┤ │ + │ \${SomeRole.Arn} │ Allow │ sts:AssumeRole │ Service:ec2.amazon.aws.com │ │ └───┴─────────────────┴────────┴────────────────┴────────────────────────────┴───────────┘ -(This feature is experimental -- it might not be complete. See http://bit.ly/cdk-2EhF7Np) +(NOTE: There may be security-related changes not in this list. See http://bit.ly/cdk-2EhF7Np) Resources [+] AWS::IAM::Role SomeRole SomeRole6DDC54DD