Skip to content

Commit

Permalink
refactor(route53): favor grant method over precreated role (aws#23083)
Browse files Browse the repository at this point in the history
`PublicHostedZone` allows creating exactly one role for cross-account delegation.

Because of limitations of trust policy size, some teams inside Amazon are creating a role per opt-in region, in which case the "one precreated role" feature doesn't help at all.

Instead, add a `grantDelegation()` helper which works for both cases.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored and Brennan Ho committed Feb 22, 2023
1 parent 36260c0 commit 33af87b
Show file tree
Hide file tree
Showing 6 changed files with 185 additions and 17 deletions.
54 changes: 51 additions & 3 deletions packages/@aws-cdk/aws-iam/lib/grant.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ export interface CommonGrantOptions {
* The resource ARNs to grant to
*/
readonly resourceArns: string[];

/**
* Any conditions to attach to the grant
*
* @default - No conditions
*/
readonly conditions?: Record<string, Record<string, unknown>>;
}

/**
Expand Down Expand Up @@ -160,6 +167,7 @@ export class Grant implements IDependable {
const statement = new PolicyStatement({
actions: options.actions,
resources: options.resourceArns,
conditions: options.conditions,
});

const addedToPrincipal = options.grantee.grantPrincipal.addToPrincipalPolicy(statement);
Expand Down Expand Up @@ -224,17 +232,27 @@ export class Grant implements IDependable {
/**
* The statement that was added to the principal's policy
*
* Can be accessed to (e.g.) add additional conditions to the statement.
* @deprecated Use `principalStatements` instead
*/
public readonly principalStatement?: PolicyStatement;

/**
* The statements that were added to the principal's policy
*/
public readonly principalStatements = new Array<PolicyStatement>();

/**
* The statement that was added to the resource policy
*
* Can be accessed to (e.g.) add additional conditions to the statement.
* @deprecated Use `resourceStatements` instead
*/
public readonly resourceStatement?: PolicyStatement;

/**
* The statements that were added to the principal's policy
*/
public readonly resourceStatements = new Array<PolicyStatement>();

/**
* The options originally used to set this result
*
Expand All @@ -243,14 +261,26 @@ export class Grant implements IDependable {
*/
private readonly options: CommonGrantOptions;

private readonly dependables = new Array<IDependable>();

private constructor(props: GrantProps) {
this.options = props.options;
this.principalStatement = props.principalStatement;
this.resourceStatement = props.resourceStatement;
if (this.principalStatement) {
this.principalStatements.push(this.principalStatement);
}
if (this.resourceStatement) {
this.resourceStatements.push(this.resourceStatement);
}
if (props.policyDependable) {
this.dependables.push(props.policyDependable);
}

const self = this;
Dependable.implement(this, {
get dependencyRoots() {
return props.policyDependable ? Dependable.of(props.policyDependable).dependencyRoots : [];
return Array.from(new Set(self.dependables.flatMap(d => Dependable.of(d).dependencyRoots)));
},
});
}
Expand Down Expand Up @@ -282,6 +312,24 @@ export class Grant implements IDependable {
construct.node.addDependency(this);
}
}

/**
* Combine two grants into a new one
*/
public combine(rhs: Grant) {
const combinedPrinc = [...this.principalStatements, ...rhs.principalStatements];
const combinedRes = [...this.resourceStatements, ...rhs.resourceStatements];

const ret = new Grant({
options: this.options,
principalStatement: combinedPrinc[0],
resourceStatement: combinedRes[0],
});
ret.principalStatements.splice(0, ret.principalStatements.length, ...combinedPrinc);
ret.resourceStatements.splice(0, ret.resourceStatements.length, ...combinedRes);
ret.dependables.push(...this.dependables, ...rhs.dependables);
return ret;
}
}

function describeGrant(options: CommonGrantOptions) {
Expand Down
34 changes: 31 additions & 3 deletions packages/@aws-cdk/aws-iam/test/grant.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Template } from '@aws-cdk/assertions';
import { Template, Match } from '@aws-cdk/assertions';
import { CfnResource, Resource, Stack } from '@aws-cdk/core';
import { Construct } from 'constructs';
import * as iam from '../lib';
Expand Down Expand Up @@ -97,6 +97,34 @@ describe('Grant dependencies', () => {
expectDependencyOn('RoleDefaultPolicy5FFB7DAB');
expectDependencyOn('BuckarooPolicy74174DA4');
});

test('can combine two grants', () => {
// GIVEN
const role1 = new iam.Role(stack, 'Role1', {
assumedBy: new iam.ServicePrincipal('bla.amazonaws.com'),
});
const role2 = new iam.Role(stack, 'Role2', {
assumedBy: new iam.ServicePrincipal('bla.amazonaws.com'),
});

// WHEN
const g1 = iam.Grant.addToPrincipal({
actions: ['service:DoAThing'],
grantee: role1,
resourceArns: ['*'],
});
const g2 = iam.Grant.addToPrincipal({
actions: ['service:DoAThing'],
grantee: role2,
resourceArns: ['*'],
});

(g1.combine(g2)).applyBefore(resource);

// THEN
expectDependencyOn('Role1DefaultPolicyD3EF4D0A');
expectDependencyOn('Role2DefaultPolicy3A7A0A1B');
});
});

function applyGrantWithDependencyTo(principal: iam.IPrincipal) {
Expand All @@ -108,8 +136,8 @@ function applyGrantWithDependencyTo(principal: iam.IPrincipal) {
}

function expectDependencyOn(id: string) {
Template.fromStack(stack).hasResource('CDK::Test::SomeResource', (props: any) => {
return (props?.DependsOn ?? []).includes(id);
Template.fromStack(stack).hasResource('CDK::Test::SomeResource', {
DependsOn: Match.arrayWith([id]),
});
}

Expand Down
13 changes: 10 additions & 3 deletions packages/@aws-cdk/aws-route53/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -152,16 +152,23 @@ new route53.ARecord(this, 'ARecord', {

### Cross Account Zone Delegation

To add a NS record to a HostedZone in different account you can do the following:
If you want to have your root domain hosted zone in one account and your subdomain hosted
zone in a diferent one, you can use `CrossAccountZoneDelegationRecord` to set up delegation
between them.

In the account containing the parent hosted zone:

```ts
const parentZone = new route53.PublicHostedZone(this, 'HostedZone', {
zoneName: 'someexample.com',
crossAccountZoneDelegationPrincipal: new iam.AccountPrincipal('12345678901'),
crossAccountZoneDelegationRoleName: 'MyDelegationRole',
});
const crossAccountRole = new iam.Role(this, 'CrossAccountRole', {
// The role name must be predictable
roleName: 'MyDelegationRole',
// The other account
assumedBy: new iam.AccountPrincipal('12345678901'),
});
parentZone.grantDelegation(crossAccountRole);
```

In the account containing the child zone to be delegated:
Expand Down
26 changes: 26 additions & 0 deletions packages/@aws-cdk/aws-route53/lib/hosted-zone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -213,13 +213,15 @@ export interface PublicHostedZoneProps extends CommonHostedZoneProps {
* with appropriate permissions for every opt-in region instead.
*
* @default - No delegation configuration
* @deprecated Create the Role yourself and call `hostedZone.grantDelegation()`.
*/
readonly crossAccountZoneDelegationPrincipal?: iam.IPrincipal;

/**
* The name of the role created for cross account delegation
*
* @default - A role name is generated automatically
* @deprecated Create the Role yourself and call `hostedZone.grantDelegation()`.
*/
readonly crossAccountZoneDelegationRoleName?: string;
}
Expand Down Expand Up @@ -342,6 +344,30 @@ export class PublicHostedZone extends HostedZone implements IPublicHostedZone {
ttl: opts.ttl,
});
}

/**
* Grant permissions to add delegation records to this zone
*/
public grantDelegation(grantee: iam.IGrantable) {
const g1 = iam.Grant.addToPrincipal({
grantee,
actions: ['route53:ChangeResourceRecordSets'],
resourceArns: [this.hostedZoneArn],
conditions: {
'ForAllValues:StringEquals': {
'route53:ChangeResourceRecordSetsRecordTypes': ['NS'],
'route53:ChangeResourceRecordSetsActions': ['UPSERT', 'DELETE'],
},
},
});
const g2 = iam.Grant.addToPrincipal({
grantee,
actions: ['route53:ListHostedZonesByName'],
resourceArns: ['*'],
});

return g1.combine(g2);
}
}

/**
Expand Down
62 changes: 60 additions & 2 deletions packages/@aws-cdk/aws-route53/test/hosted-zone.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Match, Template } from '@aws-cdk/assertions';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import { testDeprecated } from '@aws-cdk/cdk-build-tools';
import * as cdk from '@aws-cdk/core';
import { HostedZone, PrivateHostedZone, PublicHostedZone } from '../lib';

Expand Down Expand Up @@ -59,7 +60,7 @@ describe('hosted zone', () => {
});
});

test('with crossAccountZoneDelegationPrincipal', () => {
testDeprecated('with crossAccountZoneDelegationPrincipal', () => {
// GIVEN
const stack = new cdk.Stack(undefined, 'TestStack', {
env: { account: '123456789012', region: 'us-east-1' },
Expand Down Expand Up @@ -154,7 +155,7 @@ describe('hosted zone', () => {
});
});

test('with crossAccountZoneDelegationPrincipal, throws if name provided without principal', () => {
testDeprecated('with crossAccountZoneDelegationPrincipal, throws if name provided without principal', () => {
// GIVEN
const stack = new cdk.Stack(undefined, 'TestStack', {
env: { account: '123456789012', region: 'us-east-1' },
Expand Down Expand Up @@ -229,3 +230,60 @@ describe('Vpc', () => {
});
});
});

test('grantDelegation', () => {
// GIVEN
const stack = new cdk.Stack(undefined, 'TestStack', {
env: { account: '123456789012', region: 'us-east-1' },
});

const role = new iam.Role(stack, 'Role', {
assumedBy: new iam.AccountPrincipal('22222222222222'),
});

const zone = new PublicHostedZone(stack, 'Zone', {
zoneName: 'banana.com',
});

// WHEN
zone.grantDelegation(role);

// THEN
const template = Template.fromStack(stack);
template.hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Action: 'route53:ChangeResourceRecordSets',
Effect: 'Allow',
Resource: {
'Fn::Join': [
'',
[
'arn:',
{
Ref: 'AWS::Partition',
},
':route53:::hostedzone/',
{
Ref: 'ZoneA5DE4B68',
},
],
],
},
Condition: {
'ForAllValues:StringEquals': {
'route53:ChangeResourceRecordSetsRecordTypes': ['NS'],
'route53:ChangeResourceRecordSetsActions': ['UPSERT', 'DELETE'],
},
},
},
{
Action: 'route53:ListHostedZonesByName',
Effect: 'Allow',
Resource: '*',
},
],
},
});
});
13 changes: 7 additions & 6 deletions packages/@aws-cdk/aws-route53/test/record-set.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Template } from '@aws-cdk/assertions';
import * as iam from '@aws-cdk/aws-iam';
import { testDeprecated } from '@aws-cdk/cdk-build-tools';
import { Duration, RemovalPolicy, Stack } from '@aws-cdk/core';
import * as route53 from '../lib';

Expand Down Expand Up @@ -578,7 +579,7 @@ describe('record set', () => {
});
});

test('Cross account zone delegation record with parentHostedZoneId', () => {
testDeprecated('Cross account zone delegation record with parentHostedZoneId', () => {
// GIVEN
const stack = new Stack();
const parentZone = new route53.PublicHostedZone(stack, 'ParentHostedZone', {
Expand Down Expand Up @@ -630,7 +631,7 @@ describe('record set', () => {
});
});

test('Cross account zone delegation record with parentHostedZoneName', () => {
testDeprecated('Cross account zone delegation record with parentHostedZoneName', () => {
// GIVEN
const stack = new Stack();
const parentZone = new route53.PublicHostedZone(stack, 'ParentHostedZone', {
Expand Down Expand Up @@ -675,7 +676,7 @@ describe('record set', () => {
});
});

test('Cross account zone delegation record throws when parent id and name both/nither are supplied', () => {
testDeprecated('Cross account zone delegation record throws when parent id and name both/nither are supplied', () => {
// GIVEN
const stack = new Stack();
const parentZone = new route53.PublicHostedZone(stack, 'ParentHostedZone', {
Expand Down Expand Up @@ -707,7 +708,7 @@ describe('record set', () => {
}).toThrow(/Only one of parentHostedZoneName and parentHostedZoneId is supported/);
});

test('Multiple cross account zone delegation records', () => {
testDeprecated('Multiple cross account zone delegation records', () => {
// GIVEN
const stack = new Stack();
const parentZone = new route53.PublicHostedZone(stack, 'ParentHostedZone', {
Expand Down Expand Up @@ -773,7 +774,7 @@ describe('record set', () => {
}
});

test('Cross account zone delegation policies', () => {
testDeprecated('Cross account zone delegation policies', () => {
// GIVEN
const stack = new Stack();
const parentZone = new route53.PublicHostedZone(stack, 'ParentHostedZone', {
Expand Down Expand Up @@ -845,7 +846,7 @@ describe('record set', () => {
}
});

test('Cross account zone context flag', () => {
testDeprecated('Cross account zone context flag', () => {
// GIVEN
const stack = new Stack();
stack.node.setContext('@aws-cdk/aws-route53:useRegionalStsEndpoint', true);
Expand Down

0 comments on commit 33af87b

Please sign in to comment.