(route53): CrossAccountZoneDelegationRecord does not remove old NS records when the zoneName changes

[moltar]

Describe the bug

When changing the zoneName value of the PublicHostedZone the old NS entries in the delegated zone are not removed.

I think this is because when the logical ID did not change, the custom resource event type is still UPDATE and judging by the handler code, it does not handle this edge case.

aws-cdk/packages/@aws-cdk/aws-route53/lib/cross-account-zone-delegation-handler/index.ts

Lines 25 to 51 in cea1039

	async function cfnEventHandler(props: ResourceProperties, isDeleteEvent: boolean) {
	const { AssumeRoleArn, ParentZoneId, ParentZoneName, DelegatedZoneName, DelegatedZoneNameServers, TTL } = props;
	if (!ParentZoneId && !ParentZoneName) {
	throw Error('One of ParentZoneId or ParentZoneName must be specified');
	}
	const credentials = await getCrossAccountCredentials(AssumeRoleArn);
	const route53 = new Route53({ credentials });
	const parentZoneId = ParentZoneId ?? await getHostedZoneIdByName(ParentZoneName!, route53);
	await route53.changeResourceRecordSets({
	HostedZoneId: parentZoneId,
	ChangeBatch: {
	Changes: [{
	Action: isDeleteEvent ? 'DELETE' : 'UPSERT',
	ResourceRecordSet: {
	Name: DelegatedZoneName,
	Type: 'NS',
	TTL,
	ResourceRecords: DelegatedZoneNameServers.map(ns => ({ Value: ns })),
	},
	}],
	},
	}).promise();
	}

Expected Behavior

Old NS records to be removed.

Current Behavior

NS records remain in the delegated (parent) zone.

Reproduction Steps

Deploy:

    const hostedZone = new PublicHostedZone(this, 'PublicZone', {
      zoneName: 'abc.example.com',
    })

    new CrossAccountZoneDelegationRecord(this, 'Delegate', {
      delegationRole: 'arn...',
      delegatedZone: hostedZone,
      parentHostedZoneId: 'xyz',
    })

Then change zoneName value to abc2.example.com and deploy again.

Observe that NS records for abc.example.com remain.

Possible Solution

Compare zoneName values with the existing resource and delete it when the name chagnes.

Additional Information/Context

No response

CDK CLI Version

2.29.1 (build c42e961)

Framework Version

TS

Node.js Version

v14.19.3

OS

macOS

Language

Typescript

Language Version

TS 4.7.4

Other information

No response

[moltar]

Ping @ayush987goyal, @phoefflin

[ayush987goyal]

Yeah seems like a bug to me. We might need a separate handler for the UPDATE case perhaps and some corresponding refactoring. One callout is that the current implementation should technically work since it would still add the nameservers for the new hosted zone name.

[garciparedes]

Hi! I would like to come this issue alive again as I'm also being affected by the same issue.

As @moltar described, if the zoneName changes the current implementation will insert the new NS record on the parent Hosted Zone but will keep the old one so it leaves a dangling NS record which MUST be removed manually. If it is not removed, it may allow an attacker to control the subdomain and serve content in your name.

[QwerTech]

Looks like the issue is still present, and it's currently reproducing for me with CDK 2.40.0 (build 56ba2ab).
Checked the PR #25285, and it seems it was closed without merging. I hope @garciparedes you can republish your PR to finally fix the issue.

[garciparedes]

I've just republished the #27523 pull request adding the missing integration tests from previous one.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.