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

fix(acm) enabled validation of certificates on the zone name #2133

Merged
merged 7 commits into from
Apr 11, 2019
Merged

fix(acm) enabled validation of certificates on the zone name #2133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a8e30c4
Enabled validation of certificates on the zone name
McDoit Apr 1, 2019
35b9ce6
Added test for certificate on rootdomain
McDoit Apr 7, 2019
b0896ff
Updated test with better validation
McDoit Apr 7, 2019
d30eaad
Fix for trailing whitespce
McDoit Apr 7, 2019
b880ffa
Updated the test case
McDoit Apr 7, 2019
30a732f
Removed duplicate domain name
McDoit Apr 8, 2019
815351d
Replaced quotation
McDoit Apr 8, 2019
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
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export class DnsValidatedCertificate extends cdk.Construct implements ICertifica
protected validate(): string[] {
const errors: string[] = [];
// Ensure the zone name is a parent zone of the certificate domain name
if (!this.domainName.endsWith('.' + this.normalizedZoneName)) {
if (this.domainName !== this.normalizedZoneName && !this.domainName.endsWith('.' + this.normalizedZoneName)) {
errors.push(`DNS zone ${this.normalizedZoneName} is not authoritative for certificate domain name ${this.domainName}`);
}
return errors;
Expand Down
27 changes: 27 additions & 0 deletions packages/@aws-cdk/aws-certificatemanager/test/test.dns-validated-certificate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,31 @@ export = {
test.throws(() => expect(stack), /DNS zone hello.com is not authoritative for certificate domain name example.com/);
test.done();
},

Copy link
Contributor

Choose a reason for hiding this comment

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

This test doesn't seem to verify the new behavior (we need to verify that there's an error thrown when you synthesize)

Copy link
Contributor Author

@McDoit McDoit Apr 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry @eladb but i don't think i follow you, i built the test like the previous ones on this test suite, but i am totally inexperienced with writing TS and TS tests so might have missed something?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, my bad. Looks good.

'test root certificate'(test: Test) {
const stack = new Stack();

const exampleDotComZone = new PublicHostedZone(stack, 'ExampleDotCom', {
zoneName: 'example.com'
});

new DnsValidatedCertificate(stack, 'Cert', {
domainName: 'example.com',
hostedZone: exampleDotComZone,
});

expect(stack).to(haveResource('AWS::CloudFormation::CustomResource', {
ServiceToken: {
'Fn::GetAtt': [
'CertCertificateRequestorFunction98FDF273',
'Arn'
]
},
DomainName: 'example.com',
HostedZoneId: {
Ref: 'ExampleDotCom4D1B83AA'
}
}));
test.done();
},
};