diff --git a/packages/@aws-cdk/aws-ecr/jest.config.js b/packages/@aws-cdk/aws-ecr/jest.config.js index f4f042f9d2c29..09efb83e68e72 100644 --- a/packages/@aws-cdk/aws-ecr/jest.config.js +++ b/packages/@aws-cdk/aws-ecr/jest.config.js @@ -5,6 +5,7 @@ module.exports = { global: { ...baseConfig.coverageThreshold.global, branches: 70, + statements: 78, }, }, }; diff --git a/packages/@aws-cdk/aws-ecr/lib/repository.ts b/packages/@aws-cdk/aws-ecr/lib/repository.ts index 1955a3447e18b..d32f3430e3c62 100644 --- a/packages/@aws-cdk/aws-ecr/lib/repository.ts +++ b/packages/@aws-cdk/aws-ecr/lib/repository.ts @@ -2,7 +2,7 @@ import { EOL } from 'os'; import * as events from '@aws-cdk/aws-events'; import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; -import { ArnFormat, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core'; +import { ArnFormat, IResource, Lazy, RemovalPolicy, Resource, Stack, Tags, Token, TokenComparison } from '@aws-cdk/core'; import { IConstruct, Construct } from 'constructs'; import { CfnRepository } from './ecr.generated'; import { LifecycleRule, TagStatus } from './lifecycle'; @@ -279,17 +279,49 @@ export abstract class RepositoryBase extends Resource implements IRepository { rule.addTarget(options.target); return rule; } + /** * Grant the given principal identity permissions to perform the actions on this repository */ public grant(grantee: iam.IGrantable, ...actions: string[]) { - return iam.Grant.addToPrincipalOrResource({ - grantee, - actions, - resourceArns: [this.repositoryArn], - resourceSelfArns: [], - resource: this, - }); + const crossAccountPrincipal = this.unsafeCrossAccountResourcePolicyPrincipal(grantee); + if (crossAccountPrincipal) { + // If the principal is from a different account, + // that means addToPrincipalOrResource() will update the Resource Policy of this repo to trust that principal. + // However, ECR verifies that the principal used in the Policy exists, + // and will error out if it doesn't. + // Because of that, if the principal is a newly created resource, + // and there is not a dependency relationship between the Stacks of this repo and the principal, + // trust the entire account of the principal instead + // (otherwise, deploying this repo will fail). + // To scope down the permissions as much as possible, + // only trust principals from that account with a specific tag + const crossAccountPrincipalStack = Stack.of(crossAccountPrincipal); + const roleTag = `${crossAccountPrincipalStack.stackName}_${crossAccountPrincipal.node.addr}`; + Tags.of(crossAccountPrincipal).add('aws-cdk:id', roleTag); + this.addToResourcePolicy(new iam.PolicyStatement({ + actions, + principals: [new iam.AccountPrincipal(crossAccountPrincipalStack.account)], + conditions: { + StringEquals: { 'aws:PrincipalTag/aws-cdk:id': roleTag }, + }, + })); + + return iam.Grant.addToPrincipal({ + grantee, + actions, + resourceArns: [this.repositoryArn], + scope: this, + }); + } else { + return iam.Grant.addToPrincipalOrResource({ + grantee, + actions, + resourceArns: [this.repositoryArn], + resourceSelfArns: [], + resource: this, + }); + } } /** @@ -319,6 +351,43 @@ export abstract class RepositoryBase extends Resource implements IRepository { 'ecr:UploadLayerPart', 'ecr:CompleteLayerUpload'); } + + /** + * Returns the resource that backs the given IAM grantee if we cannot put a direct reference + * to the grantee in the resource policy of this ECR repository, + * and 'undefined' in case we can. + */ + private unsafeCrossAccountResourcePolicyPrincipal(grantee: iam.IGrantable): IConstruct | undefined { + // A principal cannot be safely added to the Resource Policy of this ECR repository, if: + // 1. The principal is from a different account, and + // 2. The principal is a new resource (meaning, not just referenced), and + // 3. The Stack this repo belongs to doesn't depend on the Stack the principal belongs to. + + // condition #1 + const principal = grantee.grantPrincipal; + const principalAccount = principal.principalAccount; + if (!principalAccount) { + return undefined; + } + const repoAndPrincipalAccountCompare = Token.compareStrings(this.env.account, principalAccount); + if (repoAndPrincipalAccountCompare === TokenComparison.BOTH_UNRESOLVED || + repoAndPrincipalAccountCompare === TokenComparison.SAME) { + return undefined; + } + + // condition #2 + if (!iam.principalIsOwnedResource(principal)) { + return undefined; + } + + // condition #3 + const principalStack = Stack.of(principal); + if (this.stack.dependencies.includes(principalStack)) { + return undefined; + } + + return principal; + } } /** @@ -491,7 +560,6 @@ export class Repository extends RepositoryBase { }); } - private static validateRepositoryName(physicalName: string) { const repositoryName = physicalName; if (!repositoryName || Token.isUnresolved(repositoryName)) { diff --git a/packages/@aws-cdk/aws-ecr/test/repository.test.ts b/packages/@aws-cdk/aws-ecr/test/repository.test.ts index 432ab41a2e8c1..84ea9d1a9930f 100644 --- a/packages/@aws-cdk/aws-ecr/test/repository.test.ts +++ b/packages/@aws-cdk/aws-ecr/test/repository.test.ts @@ -444,6 +444,36 @@ describe('repository', () => { }).toThrow('encryptionKey is specified, so \'encryption\' must be set to KMS (value: AES256)'); }); + test('removal policy is "Retain" by default', () => { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + new ecr.Repository(stack, 'Repo'); + + // THEN + Template.fromStack(stack).hasResource('AWS::ECR::Repository', { + 'Type': 'AWS::ECR::Repository', + 'DeletionPolicy': 'Retain', + }); + }); + + test('"Delete" removal policy can be set explicitly', () => { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + new ecr.Repository(stack, 'Repo', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + }); + + // THEN + Template.fromStack(stack).hasResource('AWS::ECR::Repository', { + 'Type': 'AWS::ECR::Repository', + 'DeletionPolicy': 'Delete', + }); + }); + describe('events', () => { test('onImagePushed without imageTag creates the correct event', () => { const stack = new cdk.Stack(); diff --git a/packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts b/packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts index 9ff3023b1e965..5f09864f5f3c7 100644 --- a/packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts +++ b/packages/@aws-cdk/aws-ecs/test/images/tag-parameter-container-image.test.ts @@ -77,51 +77,52 @@ describe('tag parameter container image', () => { 'Fn::Join': ['', [ 'arn:', { Ref: 'AWS::Partition' }, - ':iam::service-account:role/servicestackionexecutionrolee7e2d9a783a54eb795f4', + ':iam::service-account:root', ]], }, }, + Condition: { + StringEquals: { + 'aws:PrincipalTag/aws-cdk:id': 'ServiceStack_c8a38b9d3ed0e8d960dd0d679c0bab1612dafa96f5', + }, + }, }], }, }); - Template.fromStack(serviceStack).hasResourceProperties('AWS::IAM::Role', { - RoleName: 'servicestackionexecutionrolee7e2d9a783a54eb795f4', + + Template.fromStack(serviceStack).hasResourceProperties('AWS::IAM::Policy', { + PolicyDocument: Match.objectLike({ + Statement: Match.arrayWith([ + Match.objectLike({ + Action: [ + 'ecr:BatchCheckLayerAvailability', + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + ], + Effect: 'Allow', + Resource: { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + `:ecr:us-west-1:pipeline-account:repository/${repositoryName}`, + ]], + }, + }), + Match.objectLike({ + Action: 'ecr:GetAuthorizationToken', + Effect: 'Allow', + Resource: '*', + }), + ]), + }), }); - Template.fromStack(serviceStack).hasResourceProperties('AWS::ECS::TaskDefinition', { - ContainerDefinitions: [ - Match.objectLike({ - Image: { - 'Fn::Join': ['', [ - { - 'Fn::Select': [4, { - 'Fn::Split': [':', { - 'Fn::Join': ['', [ - 'arn:', - { Ref: 'AWS::Partition' }, - `:ecr:us-west-1:pipeline-account:repository/${repositoryName}`, - ]], - }], - }], - }, - '.dkr.ecr.', - { - 'Fn::Select': [3, { - 'Fn::Split': [':', { - 'Fn::Join': ['', [ - 'arn:', - { Ref: 'AWS::Partition' }, - `:ecr:us-west-1:pipeline-account:repository/${repositoryName}`, - ]], - }], - }], - }, - '.', - { Ref: 'AWS::URLSuffix' }, - `/${repositoryName}:`, - { Ref: 'ServiceTaskDefinitionContainerImageTagParamCEC9D0BA' }, - ]], - }, - }), + + Template.fromStack(serviceStack).hasResourceProperties('AWS::IAM::Role', { + Tags: [ + { + Key: 'aws-cdk:id', + Value: 'ServiceStack_c8a38b9d3ed0e8d960dd0d679c0bab1612dafa96f5', + }, ], }); }); diff --git a/packages/@aws-cdk/aws-iam/lib/group.ts b/packages/@aws-cdk/aws-iam/lib/group.ts index ea8784524c5c1..0b81c6c572158 100644 --- a/packages/@aws-cdk/aws-iam/lib/group.ts +++ b/packages/@aws-cdk/aws-iam/lib/group.ts @@ -6,8 +6,8 @@ import { IManagedPolicy } from './managed-policy'; import { Policy } from './policy'; import { PolicyStatement } from './policy-statement'; import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; +import { AttachedPolicies } from './private/util'; import { IUser } from './user'; -import { AttachedPolicies } from './util'; /** * Represents an IAM Group. diff --git a/packages/@aws-cdk/aws-iam/lib/index.ts b/packages/@aws-cdk/aws-iam/lib/index.ts index 7b13245b49f39..bf402b8037830 100644 --- a/packages/@aws-cdk/aws-iam/lib/index.ts +++ b/packages/@aws-cdk/aws-iam/lib/index.ts @@ -14,6 +14,7 @@ export * from './oidc-provider'; export * from './permissions-boundary'; export * from './saml-provider'; export * from './access-key'; +export * from './utils'; // AWS::IAM CloudFormation Resources: export * from './iam.generated'; diff --git a/packages/@aws-cdk/aws-iam/lib/managed-policy.ts b/packages/@aws-cdk/aws-iam/lib/managed-policy.ts index 4aab13017d20b..0b6285cd56323 100644 --- a/packages/@aws-cdk/aws-iam/lib/managed-policy.ts +++ b/packages/@aws-cdk/aws-iam/lib/managed-policy.ts @@ -5,9 +5,9 @@ import { IGroup } from './group'; import { CfnManagedPolicy } from './iam.generated'; import { PolicyDocument } from './policy-document'; import { PolicyStatement } from './policy-statement'; +import { undefinedIfEmpty } from './private/util'; import { IRole } from './role'; import { IUser } from './user'; -import { undefinedIfEmpty } from './util'; /** * A managed policy diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts index 53eeede5a0e9b..ebba378b92e3a 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts @@ -6,7 +6,7 @@ import { FederatedPrincipal, IPrincipal, PrincipalBase, PrincipalPolicyFragment, ServicePrincipal, ServicePrincipalOpts, validateConditionObject, } from './principals'; import { normalizeStatement } from './private/postprocess-policy-document'; -import { LITERAL_STRING_KEY, mergePrincipal, sum } from './util'; +import { LITERAL_STRING_KEY, mergePrincipal, sum } from './private/util'; const ensureArrayOrUndefined = (field: any) => { if (field === undefined) { diff --git a/packages/@aws-cdk/aws-iam/lib/policy.ts b/packages/@aws-cdk/aws-iam/lib/policy.ts index 2de04f0990b73..f46afbc151ec3 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy.ts @@ -4,9 +4,9 @@ import { IGroup } from './group'; import { CfnPolicy } from './iam.generated'; import { PolicyDocument } from './policy-document'; import { PolicyStatement } from './policy-statement'; +import { generatePolicyName, undefinedIfEmpty } from './private/util'; import { IRole } from './role'; import { IUser } from './user'; -import { generatePolicyName, undefinedIfEmpty } from './util'; /** * Represents an IAM Policy diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts index d782355902d04..bfdc8a2369644 100644 --- a/packages/@aws-cdk/aws-iam/lib/principals.ts +++ b/packages/@aws-cdk/aws-iam/lib/principals.ts @@ -6,8 +6,8 @@ import { IOpenIdConnectProvider } from './oidc-provider'; import { PolicyDocument } from './policy-document'; import { Condition, Conditions, PolicyStatement } from './policy-statement'; import { defaultAddPrincipalToAssumeRole } from './private/assume-role-policy'; +import { LITERAL_STRING_KEY, mergePrincipal } from './private/util'; import { ISamlProvider } from './saml-provider'; -import { LITERAL_STRING_KEY, mergePrincipal } from './util'; /** * Any object that has an associated principal that a permission can be granted to diff --git a/packages/@aws-cdk/aws-iam/lib/private/util.ts b/packages/@aws-cdk/aws-iam/lib/private/util.ts new file mode 100644 index 0000000000000..aa4bb0a1288a4 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/private/util.ts @@ -0,0 +1,143 @@ +import { captureStackTrace, DefaultTokenResolver, IPostProcessor, IResolvable, IResolveContext, Lazy, StringConcat, Token, Tokenization } from '@aws-cdk/core'; +import { IConstruct } from 'constructs'; +import { IPolicy } from '../policy'; + +const MAX_POLICY_NAME_LEN = 128; + +export const LITERAL_STRING_KEY = 'LiteralString'; + +export function undefinedIfEmpty(f: () => string[]): string[] { + return Lazy.list({ + produce: () => { + const array = f(); + return (array && array.length > 0) ? array : undefined; + }, + }); +} + +/** + * Used to generate a unique policy name based on the policy resource construct. + * The logical ID of the resource is a great candidate as long as it doesn't exceed + * 128 characters, so we take the last 128 characters (in order to make sure the hash + * is there). + */ +export function generatePolicyName(scope: IConstruct, logicalId: string): string { + // as logicalId is itself a Token, resolve it first + const resolvedLogicalId = Tokenization.resolve(logicalId, { + scope, + resolver: new DefaultTokenResolver(new StringConcat()), + }); + return lastNCharacters(resolvedLogicalId, MAX_POLICY_NAME_LEN); +} + +/** + * Returns a string composed of the last n characters of str. + * If str is shorter than n, returns str. + * + * @param str the string to return the last n characters of + * @param n how many characters to return + */ +function lastNCharacters(str: string, n: number) { + const startIndex = Math.max(str.length - n, 0); + return str.substring(startIndex, str.length); +} + +/** + * Helper class that maintains the set of attached policies for a principal. + */ +export class AttachedPolicies { + private policies = new Array(); + + /** + * Adds a policy to the list of attached policies. + * + * If this policy is already, attached, returns false. + * If there is another policy attached with the same name, throws an exception. + */ + public attach(policy: IPolicy) { + if (this.policies.find(p => p === policy)) { + return; // already attached + } + + if (this.policies.find(p => p.policyName === policy.policyName)) { + throw new Error(`A policy named "${policy.policyName}" is already attached`); + } + + this.policies.push(policy); + } +} + +/** + * Merge two dictionaries that represent IAM principals + * + * Does an in-place merge. + */ +export function mergePrincipal(target: { [key: string]: string[] }, source: { [key: string]: string[] }) { + // If one represents a literal string, the other one must be empty + const sourceKeys = Object.keys(source); + const targetKeys = Object.keys(target); + + if ((LITERAL_STRING_KEY in source && targetKeys.some(k => k !== LITERAL_STRING_KEY)) || + (LITERAL_STRING_KEY in target && sourceKeys.some(k => k !== LITERAL_STRING_KEY))) { + throw new Error(`Cannot merge principals ${JSON.stringify(target)} and ${JSON.stringify(source)}; if one uses a literal principal string the other one must be empty`); + } + + for (const key of sourceKeys) { + target[key] = target[key] ?? []; + + let value = source[key]; + if (!Array.isArray(value)) { + value = [value]; + } + + target[key].push(...value); + } + + return target; +} + +/** + * Lazy string set token that dedupes entries + * + * Needs to operate post-resolve, because the inputs could be + * `[ '${Token[TOKEN.9]}', '${Token[TOKEN.10]}', '${Token[TOKEN.20]}' ]`, which + * still all resolve to the same string value. + * + * Needs to JSON.stringify() results because strings could resolve to literal + * strings but could also resolve to `{ Fn::Join: [...] }`. + */ +export class UniqueStringSet implements IResolvable, IPostProcessor { + public static from(fn: () => string[]) { + return Token.asList(new UniqueStringSet(fn)); + } + + public readonly creationStack: string[]; + + private constructor(private readonly fn: () => string[]) { + this.creationStack = captureStackTrace(); + } + + public resolve(context: IResolveContext) { + context.registerPostProcessor(this); + return this.fn(); + } + + public postProcess(input: any, _context: IResolveContext) { + if (!Array.isArray(input)) { return input; } + if (input.length === 0) { return undefined; } + + const uniq: Record = {}; + for (const el of input) { + uniq[JSON.stringify(el)] = el; + } + return Object.values(uniq); + } + + public toString(): string { + return Token.asString(this); + } +} + +export function sum(xs: number[]) { + return xs.reduce((a, b) => a + b, 0); +} diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index c2c0f4c68598e..ff83e13e611ad 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -14,7 +14,7 @@ import { ImmutableRole } from './private/immutable-role'; import { ImportedRole } from './private/imported-role'; import { MutatingPolicyDocumentAdapter } from './private/policydoc-adapter'; import { PrecreatedRole } from './private/precreated-role'; -import { AttachedPolicies, UniqueStringSet } from './util'; +import { AttachedPolicies, UniqueStringSet } from './private/util'; const MAX_INLINE_SIZE = 10000; const MAX_MANAGEDPOL_SIZE = 6000; diff --git a/packages/@aws-cdk/aws-iam/lib/user.ts b/packages/@aws-cdk/aws-iam/lib/user.ts index ece9c206a8c56..7909fd3d92404 100644 --- a/packages/@aws-cdk/aws-iam/lib/user.ts +++ b/packages/@aws-cdk/aws-iam/lib/user.ts @@ -7,7 +7,7 @@ import { IManagedPolicy } from './managed-policy'; import { Policy } from './policy'; import { PolicyStatement } from './policy-statement'; import { AddToPrincipalPolicyResult, ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; -import { AttachedPolicies, undefinedIfEmpty } from './util'; +import { AttachedPolicies, undefinedIfEmpty } from './private/util'; /** * Represents an IAM user diff --git a/packages/@aws-cdk/aws-iam/lib/utils.ts b/packages/@aws-cdk/aws-iam/lib/utils.ts new file mode 100644 index 0000000000000..45ae5754c8fdc --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/utils.ts @@ -0,0 +1,38 @@ +import { Resource } from '@aws-cdk/core'; +import { Construct, IConstruct } from 'constructs'; +import { IPrincipal } from './principals'; + +/** + * Determines whether the given Principal is a newly created resource managed by the CDK, + * or if it's a referenced existing resource. + * + * @param principal the Principal to check + * @returns true if the Principal is a newly created resource, false otherwise. + * Additionally, the type of the principal will now also be IConstruct + * (because a newly created resource must be a construct) + */ +export function principalIsOwnedResource(principal: IPrincipal): principal is IPrincipal & IConstruct { + // a newly created resource will for sure be a construct + if (!isConstruct(principal)) { + return false; + } + + return Resource.isOwnedResource(principal); +} + +/** + * Whether the given object is a Construct + * + * Normally we'd do `x instanceof Construct`, but that is not robust against + * multiple copies of the `constructs` library on disk. This can happen + * when upgrading and downgrading between v2 and v1, and in the use of CDK + * Pipelines is going to an error that says "Can't use Pipeline/Pipeline/Role in + * a cross-environment fashion", which is very confusing. + */ +function isConstruct(x: any): x is IConstruct { + const sym = Symbol.for('constructs.Construct.node'); + return (typeof x === 'object' && x && + (x instanceof Construct // happy fast case + || !!(x as any).node // constructs v10 + || !!(x as any)[sym])); // constructs v3 +} diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index 0dc523fb73c37..2eb86115dd534 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -203,12 +203,9 @@ abstract class KeyBase extends Resource implements IKey { */ private granteeStackDependsOnKeyStack(grantee: iam.IGrantable): string | undefined { const grantPrincipal = grantee.grantPrincipal; - if (!isConstruct(grantPrincipal)) { - return undefined; - } // this logic should only apply to newly created // (= not imported) resources - if (!Resource.isOwnedResource(grantPrincipal)) { + if (!iam.principalIsOwnedResource(grantPrincipal)) { return undefined; } const keyStack = Stack.of(this); @@ -223,20 +220,20 @@ abstract class KeyBase extends Resource implements IKey { } private isGranteeFromAnotherRegion(grantee: iam.IGrantable): boolean { - if (!isConstruct(grantee)) { + if (!iam.principalIsOwnedResource(grantee.grantPrincipal)) { return false; } const bucketStack = Stack.of(this); - const identityStack = Stack.of(grantee); + const identityStack = Stack.of(grantee.grantPrincipal); return bucketStack.region !== identityStack.region; } private isGranteeFromAnotherAccount(grantee: iam.IGrantable): boolean { - if (!isConstruct(grantee)) { + if (!iam.principalIsOwnedResource(grantee.grantPrincipal)) { return false; } const bucketStack = Stack.of(this); - const identityStack = Stack.of(grantee); + const identityStack = Stack.of(grantee.grantPrincipal); return bucketStack.account !== identityStack.account; } } @@ -725,20 +722,3 @@ export class Key extends KeyBase { })); } } - -/** - * Whether the given object is a Construct - * - * Normally we'd do `x instanceof Construct`, but that is not robust against - * multiple copies of the `constructs` library on disk. This can happen - * when upgrading and downgrading between v2 and v1, and in the use of CDK - * Pipelines is going to an error that says "Can't use Pipeline/Pipeline/Role in - * a cross-environment fashion", which is very confusing. - */ -function isConstruct(x: any): x is Construct { - const sym = Symbol.for('constructs.Construct.node'); - return (typeof x === 'object' && x && - (x instanceof Construct // happy fast case - || !!(x as any).node // constructs v10 - || !!(x as any)[sym])); // constructs v3 -} \ No newline at end of file