Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

feat(route53): support for scoping down domain names in IHostedZone.grantDelegation() #28084

Closed
wants to merge 2 commits into from
Closed

feat(route53): support for scoping down domain names in IHostedZone.grantDelegation() #28084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6136dd
feat(route53): support for scoping down domain names in IHostedZone.g…
marcogrcr Nov 21, 2023
9f6df3b
Merge branch 'main' into delegation-grant-names
kaizencc Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...rk-integ/test/aws-route53/test/integ.cross-account-zone-delegation.js.snapshot/integ.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions ...s-route53/test/integ.cross-account-zone-delegation.js.snapshot/parent-stack.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
"route53:ChangeResourceRecordSetsActions": [
"UPSERT",
"DELETE"
],
"route53:ChangeResourceRecordSetsNormalizedRecordNames": [
"sub.uniqueexample.com"
]
}
},
Expand Down
4 changes: 2 additions & 2 deletions ...-cdk-testing/framework-integ/test/aws-route53/test/integ.cross-account-zone-delegation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class ParentStack extends cdk.Stack {
roleName: delegationRoleName,
assumedBy: new iam.AccountPrincipal(crossAccount),
});
parentZone.grantDelegation(crossAccountRole);
parentZone.grantDelegation(crossAccountRole, route53.DelegationGrantNames.ofEquals(subZoneName));
}
}

Expand Down Expand Up @@ -106,6 +106,6 @@ childStack.addDependency(parentStack);
childOptInStack.addDependency(parentStack);

new IntegTest(app, 'Route53CrossAccountInteg', {
testCases: [childStack, childOptInStack],
testCases: [childStack, childOptInStack, parentStack],
diffAssets: true,
});
19 changes: 19 additions & 0 deletions packages/aws-cdk-lib/aws-route53/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,25 @@ new route53.CrossAccountZoneDelegationRecord(this, 'delegate', {
});
```

To restrict the domain names that can be delegated with the IAM role use the `DelegationGrantNames` class,
which enforces the `route53:ChangeResourceRecordSetsNormalizedRecordNames` condition key.

This allows you to follow the minimum privilege principle:

```ts
const parentZone = new route53.PublicHostedZone(this, 'HostedZone', {
zoneName: 'someexample.com',
});

declare const betaCrossAccountRole: iam.Role;
parentZone.grantDelegation(betaCrossAccountRole, route53.DelegationGrantNames.ofEquals('beta.someexample.com'));

declare const prodCrossAccountRole: iam.Role;
parentZone.grantDelegation(prodCrossAccountRole, route53.DelegationGrantNames.ofEquals('prod.someexample.com'));
```

marcogrcr marked this conversation as resolved.
Show resolved Hide resolved
> Visit [Using IAM policy conditions for fine-grained access control to manage resource record sets](https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/specifying-rrset-conditions.html) for more details.

### Add Trailing Dot to Domain Names

In order to continue managing existing domain names with trailing dots using CDK, you can set `addTrailingDot: false` to prevent the Construct from adding a dot at the end of the domain name.
Expand Down
45 changes: 45 additions & 0 deletions packages/aws-cdk-lib/aws-route53/lib/delegation-grant-names.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/**
* Limit the delegation grant to a set of domain names using the IAM
* `route53:ChangeResourceRecordSetsNormalizedRecordNames` context key.
*/
export abstract class DelegationGrantNames {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have to do this pattern? Feels like it would be easier to just go with

parentZone.grantDelegation(prodCrossAccountRole, {
  nameLike: 'beta.someexample.com',
  // or nameEquals, mutually exclusive
});

Copy link
Author

@marcogrcr marcogrcr Jan 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We don't have to. I just followed what I've observed in other instances of aws-cdk (e.g. aws-cdk-lib » aws_dynamodb » Billing). Thought I could maintain consistency that way. I haven't personally come across this type pattern:

type GrantDelegationProps =
  {
    readonly nameEquals string[];
  }
  | {
    readonly nameLike:: string[];
  };

Happy to change it though if you consider it's necessary for getting the PR approved.

Copy link
Contributor

Choose a reason for hiding this comment

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

the pattern you are emulating, enum-like classes, i don't think is too relevant here. @marcogrcr, I responded in the other comment thread

Copy link
Contributor

Choose a reason for hiding this comment

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

So, I'm not sure that I 100% agree with this assessment. I think that it was good to treat this like an enum like class. We should be enforcing in the contract that you can't input both rather than adding a synth time validation. I think, though, I'd actually take this in a slightly different direction.

Instead of altering the current functions, why don't we create a new function that is grantScopedDelegation (or something similar) and have those functions take in GrantDelegationProps that look something like:

interface GrantDelegationProps {
  readonly grantee: IGrantable;
  readonly scope: DelegationScope;
}

DelegationScope would then have basically the two functions you created, but I would rename them nameEquals and nameLike.

The above is assuming that these two inputs really should be mutually exclusive as @kaizencc notes in another comment. If they are not, that changes my assessment here.

/**
* Match the domain names using the IAM `StringEquals` condition.
*
* @param names List of allowed record names.
*/
public static ofEquals(...names: string[]): DelegationGrantNames {
return new (class extends DelegationGrantNames {
public _equals() {
return names;
}
})();
}

/**
* Match the domain names using the IAM `StringLike` condition.
*
* @param names List of allowed record names.
*/
public static ofLike(...names: string[]): DelegationGrantNames {
return new (class extends DelegationGrantNames {
public _like() {
return names;
}
})();
}

/**
* @internal
*/
public _equals(): string[] | null {
return null;
}

/**
* @internal
*/
public _like(): string[] | null {
return null;
}
}
6 changes: 5 additions & 1 deletion packages/aws-cdk-lib/aws-route53/lib/hosted-zone-ref.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { DelegationGrantNames } from './delegation-grant-names';
import * as iam from '../../aws-iam';
import { IResource } from '../../core';

Expand Down Expand Up @@ -36,8 +37,11 @@ export interface IHostedZone extends IResource {

/**
* Grant permissions to add delegation records to this zone
*
* @param grantee grantee to receive the permissions
* @param names specify to restrict the delegation to a specific set of names
*/
grantDelegation(grantee: iam.IGrantable): iam.Grant;
grantDelegation(grantee: iam.IGrantable, names?: DelegationGrantNames): iam.Grant;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use a property bag. We only have once chance to add an optional prop to a public API with backwards compatibility, so lets keep the door open for future additions too. It has the additional benefit of forcing users to supply the prop name with their input, which makes things clearer.

grantDelegation(grantee: iam.IGrantable, { nameLike?: string, nameEquals?: string }): iam.Grant;

Copy link
Author

@marcogrcr marcogrcr Jan 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Acknowledged. Does this look good to you?

interface GrantDelegationProps {
  readonly name?:
    {
      readonly equals: string[];
    }
    | {
      readonly like: string[];
    };
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ideally i want

interface GrantDelegationProps {
  readonly nameEquals?: string[];
  readonly nameLike?: string[];
}

And then somewhere else make sure that nameEquals and nameLike are not both set at once.

}

/**
Expand Down
25 changes: 13 additions & 12 deletions packages/aws-cdk-lib/aws-route53/lib/hosted-zone.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Construct } from 'constructs';
import { DelegationGrantNames } from './delegation-grant-names';
import { HostedZoneProviderProps } from './hosted-zone-provider';
import { HostedZoneAttributes, IHostedZone, PublicHostedZoneAttributes } from './hosted-zone-ref';
import { CaaAmazonRecord, ZoneDelegationRecord } from './record-set';
Expand Down Expand Up @@ -84,8 +85,8 @@ export class HostedZone extends Resource implements IHostedZone {
public get hostedZoneArn(): string {
return makeHostedZoneArn(this, this.hostedZoneId);
}
public grantDelegation(grantee: iam.IGrantable): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn);
public grantDelegation(grantee: iam.IGrantable, names?: DelegationGrantNames): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn, names);
}
}

Expand All @@ -108,8 +109,8 @@ export class HostedZone extends Resource implements IHostedZone {
public get hostedZoneArn(): string {
return makeHostedZoneArn(this, this.hostedZoneId);
}
public grantDelegation(grantee: iam.IGrantable): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn);
public grantDelegation(grantee: iam.IGrantable, names?: DelegationGrantNames): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn, names);
}
}

Expand Down Expand Up @@ -199,8 +200,8 @@ export class HostedZone extends Resource implements IHostedZone {
this.vpcs.push({ vpcId: vpc.vpcId, vpcRegion: vpc.env.region ?? Stack.of(vpc).region });
}

public grantDelegation(grantee: iam.IGrantable): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn);
public grantDelegation(grantee: iam.IGrantable, names?: DelegationGrantNames): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn, names);
}
}

Expand Down Expand Up @@ -274,8 +275,8 @@ export class PublicHostedZone extends HostedZone implements IPublicHostedZone {
public get hostedZoneArn(): string {
return makeHostedZoneArn(this, this.hostedZoneId);
}
public grantDelegation(grantee: iam.IGrantable): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn);
public grantDelegation(grantee: iam.IGrantable, names?: DelegationGrantNames): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn, names);
}
}
return new Import(scope, id);
Expand All @@ -297,8 +298,8 @@ export class PublicHostedZone extends HostedZone implements IPublicHostedZone {
public get hostedZoneArn(): string {
return makeHostedZoneArn(this, this.hostedZoneId);
}
public grantDelegation(grantee: iam.IGrantable): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn);
public grantDelegation(grantee: iam.IGrantable, names?: DelegationGrantNames): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn, names);
}
}
return new Import(scope, id);
Expand Down Expand Up @@ -435,8 +436,8 @@ export class PrivateHostedZone extends HostedZone implements IPrivateHostedZone
public get hostedZoneArn(): string {
return makeHostedZoneArn(this, this.hostedZoneId);
}
public grantDelegation(grantee: iam.IGrantable): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn);
public grantDelegation(grantee: iam.IGrantable, names?: DelegationGrantNames): iam.Grant {
return makeGrantDelegation(grantee, this.hostedZoneArn, names);
}
}
return new Import(scope, id);
Expand Down
1 change: 1 addition & 0 deletions packages/aws-cdk-lib/aws-route53/lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export * from './alias-record-target';
export * from './delegation-grant-names';
export * from './hosted-zone';
export * from './hosted-zone-provider';
export * from './hosted-zone-ref';
Expand Down
11 changes: 10 additions & 1 deletion packages/aws-cdk-lib/aws-route53/lib/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Construct } from 'constructs';
import { DelegationGrantNames } from './delegation-grant-names';
import { IHostedZone } from './hosted-zone-ref';
import * as iam from '../../aws-iam';
import { Stack } from '../../core';
Expand Down Expand Up @@ -71,7 +72,7 @@ export function makeHostedZoneArn(construct: Construct, hostedZoneId: string): s
});
}

export function makeGrantDelegation(grantee: iam.IGrantable, hostedZoneArn: string): iam.Grant {
export function makeGrantDelegation(grantee: iam.IGrantable, hostedZoneArn: string, names?: DelegationGrantNames): iam.Grant {
const g1 = iam.Grant.addToPrincipal({
grantee,
actions: ['route53:ChangeResourceRecordSets'],
Expand All @@ -80,7 +81,15 @@ export function makeGrantDelegation(grantee: iam.IGrantable, hostedZoneArn: stri
'ForAllValues:StringEquals': {
'route53:ChangeResourceRecordSetsRecordTypes': ['NS'],
'route53:ChangeResourceRecordSetsActions': ['UPSERT', 'DELETE'],
...(names?._equals() ? {
'route53:ChangeResourceRecordSetsNormalizedRecordNames': names._equals(),
} : {}),
},
...(names?._like() ? {
'ForAllValues:StringLike': {
'route53:ChangeResourceRecordSetsNormalizedRecordNames': names._like(),
},
} : {}),
},
});
const g2 = iam.Grant.addToPrincipal({
Expand Down
23 changes: 23 additions & 0 deletions packages/aws-cdk-lib/aws-route53/test/delegation-grant-names.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { DelegationGrantNames } from '../lib/delegation-grant-names';

describe('delegation-grant-names', () => {
const NAMES = ['name-1', 'name-2'];

test('ofEquals() creates instance whose _equals() is not null', () => {
// WHEN
const actual = DelegationGrantNames.ofEquals(...NAMES);

// THEN
expect(actual._equals()).toStrictEqual(NAMES);
expect(actual._like()).toBeNull();
});

test('ofLike() creates instance whose _like() is not null', () => {
// WHEN
const actual = DelegationGrantNames.ofLike(...NAMES);

// THEN
expect(actual._equals()).toBeNull();
expect(actual._like()).toStrictEqual(NAMES);
});
});
63 changes: 63 additions & 0 deletions packages/aws-cdk-lib/aws-route53/test/util.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import * as iam from '../../aws-iam';
import * as cdk from '../../core';
import { HostedZone } from '../lib';
import { DelegationGrantNames } from '../lib/delegation-grant-names';
import * as util from '../lib/util';

describe('util', () => {
Expand Down Expand Up @@ -67,4 +69,65 @@ describe('util', () => {
// THEN
expect(qualified).toEqual('test.domain.com.');
});

test('grant delegation without names returns ChangeResourceRecordSets statement with only two condition keys', () => {
// GIVEN
const stack = new cdk.Stack();
const grantee = new iam.User(stack, 'Grantee');

// WHEN
const actual = util.makeGrantDelegation(grantee, 'hosted-zone');

// WHEN
const statement = actual.principalStatements.find(x => x.actions.includes('route53:ChangeResourceRecordSets'));
expect(statement).not.toBeUndefined();
expect(statement?.conditions).toEqual({
'ForAllValues:StringEquals': {
'route53:ChangeResourceRecordSetsRecordTypes': ['NS'],
'route53:ChangeResourceRecordSetsActions': ['UPSERT', 'DELETE'],
},
});
});

test('grant delegation with equals names returns ChangeResourceRecordSets statement with normalized record names condition', () => {
// GIVEN
const stack = new cdk.Stack();
const grantee = new iam.User(stack, 'Grantee');

// WHEN
const actual = util.makeGrantDelegation(grantee, 'hosted-zone', DelegationGrantNames.ofEquals('name-1', 'name-2'));

// WHEN
const statement = actual.principalStatements.find(x => x.actions.includes('route53:ChangeResourceRecordSets'));
expect(statement).not.toBeUndefined();
expect(statement?.conditions).toEqual({
'ForAllValues:StringEquals': {
'route53:ChangeResourceRecordSetsRecordTypes': ['NS'],
'route53:ChangeResourceRecordSetsActions': ['UPSERT', 'DELETE'],
'route53:ChangeResourceRecordSetsNormalizedRecordNames': ['name-1', 'name-2'],
},
});
});

test('grant delegation with like names returns ChangeResourceRecordSets statement with normalized record names condition', () => {
// GIVEN
const stack = new cdk.Stack();
const grantee = new iam.User(stack, 'Grantee');

// WHEN
const actual = util.makeGrantDelegation(grantee, 'hosted-zone', DelegationGrantNames.ofLike('name-1', 'name-2'));

// WHEN
const statement = actual.principalStatements.find(x => x.actions.includes('route53:ChangeResourceRecordSets'));
expect(statement).not.toBeUndefined();
expect(statement?.conditions).toEqual({
'ForAllValues:StringEquals': {
'route53:ChangeResourceRecordSetsRecordTypes': ['NS'],
'route53:ChangeResourceRecordSetsActions': ['UPSERT', 'DELETE'],
},
'ForAllValues:StringLike': {
'route53:ChangeResourceRecordSetsNormalizedRecordNames': ['name-1', 'name-2'],
},
});
});
});