From 5af7c1f0218bc155de873bd6c8358bb67d580893 Mon Sep 17 00:00:00 2001 From: Nick Lynch <1376292+njlynch@users.noreply.github.com> Date: Tue, 25 Jan 2022 12:06:40 +0000 Subject: [PATCH] feat(certificatemanager): DnsValidatedCertificate DNS record cleanup (#18311) Adds an option to DnsValidatedCertificate to automatically cleanup the related DNS validation records when the Certificate is deleted. This is an opt-in property and discouraged for production use, as there are edge cases that can cause unintended side effects. The most obvious is that if two or more certificates exist with the same domain, the same validation record is used for both. If one certificate is deleted (and deletes the validation record), the second certificate (with the same domain name) will be unable to automatically renew. closes #3333 closes #7063 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../lib/index.js | 148 +++++++++------ .../test/handler.test.js | 168 ++++++++++++++++++ .../lib/dns-validated-certificate.ts | 13 ++ .../test/dns-validated-certificate.test.ts | 4 + 4 files changed, 281 insertions(+), 52 deletions(-) diff --git a/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js b/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js index 528dd55eb460a..672b5762dbc15 100644 --- a/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js +++ b/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js @@ -110,26 +110,14 @@ const requestCertificate = async function (requestId, domainName, subjectAlterna console.log('Waiting for ACM to provide DNS records for validation...'); - let records; - for (let attempt = 0; attempt < maxAttempts && !records; attempt++) { + let records = []; + for (let attempt = 0; attempt < maxAttempts && !records.length; attempt++) { const { Certificate } = await acm.describeCertificate({ CertificateArn: reqCertResponse.CertificateArn }).promise(); - const options = Certificate.DomainValidationOptions || []; - // Ensure all records are ready; there is (at least a theory there's) a chance of a partial response here in rare cases. - if (options.length > 0 && options.every(opt => opt && !!opt.ResourceRecord)) { - // some alternative names will produce the same validation record - // as the main domain (eg. example.com + *.example.com) - // filtering duplicates to avoid errors with adding the same record - // to the route53 zone twice - const unique = options - .map((val) => val.ResourceRecord) - .reduce((acc, cur) => { - acc[cur.Name] = cur; - return acc; - }, {}); - records = Object.keys(unique).sort().map(key => unique[key]); - } else { + + records = getDomainValidationRecords(Certificate); + if (!records.length) { // Exponential backoff with jitter based on 200ms base // component of backoff fixed to ensure minimum total wait time on // slow targets. @@ -137,42 +125,13 @@ const requestCertificate = async function (requestId, domainName, subjectAlterna await sleep(random() * base * 50 + base * 150); } } - if (!records) { + if (!records.length) { throw new Error(`Response from describeCertificate did not contain DomainValidationOptions after ${maxAttempts} attempts.`) } - console.log(`Upserting ${records.length} DNS records into zone ${hostedZoneId}:`); - const changeBatch = await route53.changeResourceRecordSets({ - ChangeBatch: { - Changes: records.map((record) => { - console.log(`${record.Name} ${record.Type} ${record.Value}`) - return { - Action: 'UPSERT', - ResourceRecordSet: { - Name: record.Name, - Type: record.Type, - TTL: 60, - ResourceRecords: [{ - Value: record.Value - }] - } - }; - }), - }, - HostedZoneId: hostedZoneId - }).promise(); - - console.log('Waiting for DNS records to commit...'); - await route53.waitFor('resourceRecordSetsChanged', { - // Wait up to 5 minutes - $waiter: { - delay: 30, - maxAttempts: 10 - }, - Id: changeBatch.ChangeInfo.Id - }).promise(); + await commitRoute53Records(route53, records, hostedZoneId); console.log('Waiting for validation...'); await acm.waitFor('certificateValidated', { @@ -193,40 +152,59 @@ const requestCertificate = async function (requestId, domainName, subjectAlterna * * @param {string} arn The certificate ARN */ -const deleteCertificate = async function (arn, region) { +const deleteCertificate = async function (arn, region, hostedZoneId, route53Endpoint, cleanupRecords) { const acm = new aws.ACM({ region }); + const route53 = route53Endpoint ? new aws.Route53({ endpoint: route53Endpoint }) : new aws.Route53(); + if (waiter) { + // Used by the test suite, since waiters aren't mockable yet + route53.waitFor = acm.waitFor = waiter; + } try { console.log(`Waiting for certificate ${arn} to become unused`); let inUseByResources; + let records = []; for (let attempt = 0; attempt < maxAttempts; attempt++) { const { Certificate } = await acm.describeCertificate({ CertificateArn: arn }).promise(); + if (cleanupRecords) { + records = getDomainValidationRecords(Certificate); + } inUseByResources = Certificate.InUseBy || []; - if (inUseByResources.length) { + if (inUseByResources.length || !records.length) { // Exponential backoff with jitter based on 200ms base // component of backoff fixed to ensure minimum total wait time on // slow targets. const base = Math.pow(2, attempt); await sleep(random() * base * 50 + base * 150); } else { - break + break; } } if (inUseByResources.length) { throw new Error(`Response from describeCertificate did not contain an empty InUseBy list after ${maxAttempts} attempts.`) } + if (cleanupRecords && !records.length) { + throw new Error(`Response from describeCertificate did not contain DomainValidationOptions after ${maxAttempts} attempts.`) + } console.log(`Deleting certificate ${arn}`); await acm.deleteCertificate({ CertificateArn: arn }).promise(); + + if (cleanupRecords) { + console.log(`Deleting ${records.length} DNS records from zone ${hostedZoneId}:`); + + await commitRoute53Records(route53, records, hostedZoneId, 'DELETE'); + } + } catch (err) { if (err.name !== 'ResourceNotFoundException') { throw err; @@ -234,6 +212,66 @@ const deleteCertificate = async function (arn, region) { } }; +/** + * Retrieve the unique domain validation options as records to be upserted (or deleted) from Route53. + * + * Returns an empty array ([]) if the domain validation options is empty or the records are not yet ready. + */ +function getDomainValidationRecords(certificate) { + const options = certificate.DomainValidationOptions || []; + // Ensure all records are ready; there is (at least a theory there's) a chance of a partial response here in rare cases. + if (options.length > 0 && options.every(opt => opt && !!opt.ResourceRecord)) { + // some alternative names will produce the same validation record + // as the main domain (eg. example.com + *.example.com) + // filtering duplicates to avoid errors with adding the same record + // to the route53 zone twice + const unique = options + .map((val) => val.ResourceRecord) + .reduce((acc, cur) => { + acc[cur.Name] = cur; + return acc; + }, {}); + return Object.keys(unique).sort().map(key => unique[key]); + } + return []; +} + +/** + * Execute Route53 ChangeResourceRecordSets for a set of records within a Hosted Zone, + * and wait for the records to commit. Defaults to an 'UPSERT' action. + */ +async function commitRoute53Records(route53, records, hostedZoneId, action = 'UPSERT') { + const changeBatch = await route53.changeResourceRecordSets({ + ChangeBatch: { + Changes: records.map((record) => { + console.log(`${record.Name} ${record.Type} ${record.Value}`); + return { + Action: action, + ResourceRecordSet: { + Name: record.Name, + Type: record.Type, + TTL: 60, + ResourceRecords: [{ + Value: record.Value + }] + } + }; + }), + }, + HostedZoneId: hostedZoneId + }).promise(); + + console.log('Waiting for DNS records to commit...'); + await route53.waitFor('resourceRecordSetsChanged', { + // Wait up to 5 minutes + $waiter: { + delay: 30, + maxAttempts: 10 + }, + Id: changeBatch.ChangeInfo.Id + }).promise(); +} + /** * Main handler, invoked by Lambda */ @@ -262,7 +300,13 @@ exports.certificateRequestHandler = async function (event, context) { // If the resource didn't create correctly, the physical resource ID won't be the // certificate ARN, so don't try to delete it in that case. if (physicalResourceId.startsWith('arn:')) { - await deleteCertificate(physicalResourceId, event.ResourceProperties.Region); + await deleteCertificate( + physicalResourceId, + event.ResourceProperties.Region, + event.ResourceProperties.HostedZoneId, + event.ResourceProperties.Route53Endpoint, + event.ResourceProperties.CleanupRecords === "true", + ); } break; default: diff --git a/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js b/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js index 9f180f20211e9..a6a0632fd5aeb 100644 --- a/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js +++ b/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js @@ -996,4 +996,172 @@ describe('DNS Validated Certificate Handler', () => { expect(request.isDone()).toBe(true); }); }); + + describe('Delete option record cleanup', () => { + let describeCertificateFake; + let deleteCertificateFake; + let changeResourceRecordSetsFake; + + beforeEach(() => { + deleteCertificateFake = sinon.fake.resolves({}); + AWS.mock('ACM', 'deleteCertificate', deleteCertificateFake); + changeResourceRecordSetsFake = sinon.fake.resolves({ + ChangeInfo: { + Id: 'bogus' + } + }); + AWS.mock('Route53', 'changeResourceRecordSets', changeResourceRecordSetsFake); + + describeCertificateFake = sinon.fake.resolves({ + Certificate: { + CertificateArn: testCertificateArn, + DomainValidationOptions: [{ + ValidationStatus: 'SUCCESS', + ResourceRecord: { + Name: testRRName, + Type: 'CNAME', + Value: testRRValue + } + }] + } + }); + AWS.mock('ACM', 'describeCertificate', describeCertificateFake); + }); + + test('ignores records if CleanupRecords is not set', () => { + const request = nock(ResponseURL).put('/', body => { + return body.Status === 'SUCCESS'; + }).reply(200); + + return LambdaTester(handler.certificateRequestHandler) + .event({ + RequestType: 'Delete', + RequestId: testRequestId, + PhysicalResourceId: testCertificateArn, + ResourceProperties: { + Region: 'us-east-1', + HostedZoneId: testHostedZoneId, + } + }) + .expectResolve(() => { + sinon.assert.calledWith(describeCertificateFake, sinon.match({ + CertificateArn: testCertificateArn + })); + sinon.assert.calledWith(deleteCertificateFake, sinon.match({ + CertificateArn: testCertificateArn + })); + sinon.assert.notCalled(changeResourceRecordSetsFake); + expect(request.isDone()).toBe(true); + }); + }); + + test('ignores records if CleanupRecords is not set to "true"', () => { + const request = nock(ResponseURL).put('/', body => { + return body.Status === 'SUCCESS'; + }).reply(200); + + return LambdaTester(handler.certificateRequestHandler) + .event({ + RequestType: 'Delete', + RequestId: testRequestId, + PhysicalResourceId: testCertificateArn, + ResourceProperties: { + Region: 'us-east-1', + HostedZoneId: testHostedZoneId, + CleanupRecords: 'TRUE', // Not "true" + } + }) + .expectResolve(() => { + sinon.assert.calledWith(describeCertificateFake, sinon.match({ + CertificateArn: testCertificateArn + })); + sinon.assert.calledWith(deleteCertificateFake, sinon.match({ + CertificateArn: testCertificateArn + })); + sinon.assert.notCalled(changeResourceRecordSetsFake); + expect(request.isDone()).toBe(true); + }); + }); + + test('deletes records if CleanupRecords is set to true and records are present', () => { + const request = nock(ResponseURL).put('/', body => { + return body.Status === 'SUCCESS'; + }).reply(200); + + AWS.mock('Route53', 'changeResourceRecordSets', changeResourceRecordSetsFake); + + return LambdaTester(handler.certificateRequestHandler) + .event({ + RequestType: 'Delete', + RequestId: testRequestId, + PhysicalResourceId: testCertificateArn, + ResourceProperties: { + Region: 'us-east-1', + HostedZoneId: testHostedZoneId, + CleanupRecords: 'true', + }, + }) + .expectResolve(() => { + sinon.assert.calledWith(describeCertificateFake, sinon.match({ + CertificateArn: testCertificateArn + })); + sinon.assert.calledWith(deleteCertificateFake, sinon.match({ + CertificateArn: testCertificateArn + })); + sinon.assert.calledWith(changeResourceRecordSetsFake, sinon.match({ + ChangeBatch: { + Changes: [{ + Action: 'DELETE', + ResourceRecordSet: { + Name: testRRName, + Type: 'CNAME', + TTL: 60, + ResourceRecords: [{ + Value: testRRValue + }] + } + }] + }, + HostedZoneId: testHostedZoneId + })); + expect(request.isDone()).toBe(true); + }); + }); + + test('fails if CleanupRecords is set to true and records are not present', () => { + describeCertificateFake = sinon.fake.resolves({ + Certificate: { + CertificateArn: testCertificateArn, + } + }); + AWS.remock('ACM', 'describeCertificate', describeCertificateFake); + + const request = nock(ResponseURL).put('/', body => { + return body.Status === 'FAILED' && + body.Reason.startsWith('Response from describeCertificate did not contain DomainValidationOptions'); + }).reply(200); + + AWS.mock('Route53', 'changeResourceRecordSets', changeResourceRecordSetsFake); + + return LambdaTester(handler.certificateRequestHandler) + .event({ + RequestType: 'Delete', + RequestId: testRequestId, + PhysicalResourceId: testCertificateArn, + ResourceProperties: { + Region: 'us-east-1', + HostedZoneId: testHostedZoneId, + CleanupRecords: 'true', + }, + }) + .expectResolve(() => { + sinon.assert.calledWith(describeCertificateFake, sinon.match({ + CertificateArn: testCertificateArn + })); + sinon.assert.notCalled(deleteCertificateFake); + sinon.assert.notCalled(changeResourceRecordSetsFake); + expect(request.isDone()).toBe(true); + }); + }); + }); }); diff --git a/packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts b/packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts index 60c965e21a6ab..db7dc6f1f8663 100644 --- a/packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts +++ b/packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts @@ -47,6 +47,17 @@ export interface DnsValidatedCertificateProps extends CertificateProps { */ readonly customResourceRole?: iam.IRole; + /** + * When set to true, when the DnsValidatedCertificate is deleted, + * the associated Route53 validation records are removed. + * + * CAUTION: If multiple certificates share the same domains (and same validation records), + * this can cause the other certificates to fail renewal and/or not validate. + * Not recommended for production use. + * + * @default false + */ + readonly cleanupRoute53Records?: boolean; } /** @@ -113,6 +124,8 @@ export class DnsValidatedCertificate extends CertificateBase implements ICertifi HostedZoneId: this.hostedZoneId, Region: props.region, Route53Endpoint: props.route53Endpoint, + // Custom resources properties are always converted to strings; might as well be explict here. + CleanupRecords: props.cleanupRoute53Records ? 'true' : undefined, Tags: cdk.Lazy.list({ produce: () => this.tags.renderTags() }), }, }); diff --git a/packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts b/packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts index 935f8680415b4..d6881d61d79ac 100644 --- a/packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts +++ b/packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts @@ -14,10 +14,13 @@ test('creates CloudFormation Custom Resource', () => { new DnsValidatedCertificate(stack, 'Certificate', { domainName: 'test.example.com', hostedZone: exampleDotComZone, + subjectAlternativeNames: ['test2.example.com'], + cleanupRoute53Records: true, }); Template.fromStack(stack).hasResourceProperties('AWS::CloudFormation::CustomResource', { DomainName: 'test.example.com', + SubjectAlternativeNames: ['test2.example.com'], ServiceToken: { 'Fn::GetAtt': [ 'CertificateCertificateRequestorFunction5E845413', @@ -27,6 +30,7 @@ test('creates CloudFormation Custom Resource', () => { HostedZoneId: { Ref: 'ExampleDotCom4D1B83AA', }, + CleanupRecords: 'true', }); Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Function', { Handler: 'index.certificateRequestHandler',