feat(cloudfront): verify acm certificate is in us-east-1 #3485

nmussy · 2019-07-30T18:46:55Z

Fixes #3464

Please read the contribution guidelines and follow the pull-request checklist.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

packages/@aws-cdk/aws-cloudfront/test/test.basic.ts

nmussy · 2019-08-07T07:17:06Z

@eladb Thanks for the verification, but do you have an insight into this issue?

…hrow-invalid-acm-region

nmussy · 2019-08-12T14:20:17Z

packages/@aws-cdk/aws-cloudfront/test/test.basic.ts

+    const certificate = new certificatemanager.DnsValidatedCertificate(stack, 'TestCertificate', {
+      domainName: 'www.example.com',
+      hostedZone,
+      region: 'eu-west-3',


I should be able to resolve that region, right? It's currently resolved as ${Token[Default.Arn.132]}

DnsValidatedCertificate.certificateArn returns an Fn::GetAtt token which is only resolved at deploy time. This is why you are seeing a token. The only situation in which you will be able to resolve to a concrete region value is if you using fromCertificateArn.

That makes this PR kind of useless though. The main use was to not request an unusable certificate. If we already have the ARN for it, it's too late.

Would you be open to deprecating acmCertRef for a acmCert: Certificate property, to be able to access the region property?

I just noticed that this will cause a typing issue. acmCertRef is currently non-optional in the AliasConfiguration. If I put them both in AliasConfiguration, I will have to make them optional.

Are you also okay with deprecating aliasConfiguration instead, and allowing to keep both acmCertRef and acmCert required? If yes, I'd a appreciate a suggestion for the new alias property. 👍

nmussy · 2019-08-13T08:52:00Z

@eladb Any insight into this issue?

…hrow-invalid-acm-region # Conflicts: # packages/@aws-cdk/aws-cloudfront/package.json

* deprecate acmCertArn -> acmCert * deprecate aliasConfiguration -> aliasConfig

nmussy · 2019-08-15T10:26:27Z

@eladb I finally got this feature where I wanted it to be, but this PR is getting fairly big for a minor UX inconvenience. I'd be fine with aborting it

eladb · 2019-08-18T17:09:23Z

Hey, I am going on holiday until 8/26 @rix0rrr can you please follow up on this?

My general approach is the the cloudfront library needs an overhaul anyway. It's one of the only libraries we haven't refactored to adhere to our API standards.

nmussy · 2019-08-18T17:11:10Z

Got it. And enjoy your holidays Elad!

rix0rrr · 2019-08-19T13:04:15Z

packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts

+      if (props.aliasConfig) {
+        const {acmCert} = props.aliasConfig;
+
+        const isDnsCert = (cert: certificatemanager.ICertificate): cert is certificatemanager.DnsValidatedCertificate =>


I would rather have some static helper functions in the ACM module to get the region from an ICertificate. One module grubbing around in the internals of another one without a well-defined public interface to do so seems like a bad idea.

The simplest would be:

class CertificateUtils { public static certificateRegion(cert: ICertificate): string | undefined { // Put all that logic here } }

rix0rrr · 2019-08-19T13:06:56Z

packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts

+  /**
+   * AWS Certificate Manager (ACM) certificate.
+   */
+  readonly acmCert: certificatemanager.ICertificate;


No abbreviations. acmCertificate. I think we might even just do certificate, no?

rix0rrr · 2019-08-19T13:07:54Z

@aws-cdk/aws-cloudfront: ERROR: /home/travis/build/aws/aws-cdk/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-security-policy.ts:2:1 - Import sources within a group must be alphabetized.

mergify · 2019-08-19T13:08:03Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

mergify · 2019-08-19T13:08:03Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

mergify · 2019-08-19T13:08:06Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-08-19T13:08:06Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

rix0rrr

See previous comments.

mergify · 2019-09-13T00:18:34Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

rix0rrr · 2019-09-16T07:56:17Z

Looks like interest in this PR has died out. To clean up our queue, I'm going to close it. Feel free to reopen if someone wants to revive it.

aletheia · 2019-09-20T18:36:18Z

Any update about this? I am using

     const certificate = new certificateManager.DnsValidatedCertificate(this, 'Certificate', {
                        domainName: domain,
                        hostedZone: zone,
                        region: region,
                    });

with eu-west-1 as region and it's failing certificate validation

nmussy · 2019-09-20T18:40:17Z

@aletheia That sounds unrelated to this PR. This is just a user experience improvement, preventing the deployment of a stack containing a CloudFront distribution with a certificate that isn't located in us-east-1. It wouldn't affect certificate validation.

Feel free to open another issue with additional details if you can't figure it out though!

aletheia · 2019-09-20T18:43:08Z

The issue already exists and it's open since Jul 29th #3464
Unfortunately still no answers on that.

BTW this PR could have saved me almost 2 hrs before I figured out attribute "region" is not honored

rix0rrr · 2019-09-25T13:54:01Z

What do you mean the region attribute is not being honored?

aletheia · 2019-09-25T13:57:48Z

If I specify a different region than us-east-1 certificate validation stay stuck until stack timeout Il giorno mer 25 set 2019 alle 15:54 Rico Huijbers <notifications@github.com> ha scritto:

What do you mean the region attribute is not being honored? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3485?email_source=notifications&email_token=AAE7T7ZNTGEGNIG62MZFFG3QLNUQBA5CNFSM4IH7H2AKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7R7VAI#issuecomment-535034497>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE7T72TOZEOGFWQ5ZCZ2J3QLNUQBANCNFSM4IH7H2AA> .

-- Luca Bianchi Mobile: +39 328 6867731 Mail: bianchi.luca@gmail.com

nmussy added 3 commits July 30, 2019 20:45

feat(cloudfront): verify acmCertificateArn region

94e75ed

fix(cloudfront): strict error checking, quotes

9a82955

fix(cloudfront): check isUnresolved

d9f63e6

nmussy commented Jul 30, 2019

View reviewed changes

packages/@aws-cdk/aws-cloudfront/test/test.basic.ts Show resolved Hide resolved

NGL321 assigned RomainMuller, skinny85, rix0rrr, eladb and shivlaks Aug 1, 2019

eladb unassigned RomainMuller, skinny85, rix0rrr, eladb and shivlaks Aug 5, 2019

Merge branch 'master' into feat-cloudfront-throw-invalid-acm-region

72fb7b0

NGL321 assigned eladb Aug 6, 2019

eladb previously approved these changes Aug 7, 2019

View reviewed changes

Merge branch 'master' into feat-cloudfront-throw-invalid-acm-region

add34c4

eladb changed the title ~~feat(cloudfront): verify acmCertificateArn region~~ feat(cloudfront): verify acm certificate is in us-east-1 Aug 7, 2019

Jimmy Gaussen added 2 commits August 12, 2019 15:38

Merge remote-tracking branch 'upstream/master' into feat-cloudfront-t…

845f2e8

…hrow-invalid-acm-region

fix: stack region lookup, additional test cases

9e03612

nmussy dismissed eladb’s stale review via 9e03612 August 12, 2019 14:19

nmussy commented Aug 12, 2019

View reviewed changes

Elad Ben-Israel added 2 commits August 12, 2019 22:45

Merge branch 'master' into feat-cloudfront-throw-invalid-acm-region

773bddf

Merge branch 'master' into feat-cloudfront-throw-invalid-acm-region

f139b94

eladb previously approved these changes Aug 13, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feat-cloudfront-t…

98c22b6

…hrow-invalid-acm-region # Conflicts: # packages/@aws-cdk/aws-cloudfront/package.json

nmussy dismissed eladb’s stale review via 98c22b6 August 14, 2019 07:35

feat: DnsValidatedCertificate region check

c83b27d

* deprecate acmCertArn -> acmCert * deprecate aliasConfiguration -> aliasConfig

eladb assigned rix0rrr and unassigned eladb Aug 18, 2019

rix0rrr reviewed Aug 19, 2019

View reviewed changes

mergify bot added the automerge-enabled label Aug 19, 2019

rix0rrr requested changes Aug 19, 2019

View reviewed changes

NGL321 removed the automerge-enabled label Sep 13, 2019

mergify bot added the automerge-enabled label Sep 13, 2019

NGL321 removed the automerge-enabled label Sep 13, 2019

rix0rrr closed this Sep 16, 2019

nmussy mentioned this pull request Oct 18, 2019

feat(cloudfront): complete viewerCertificate support #4579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloudfront): verify acm certificate is in us-east-1 #3485

feat(cloudfront): verify acm certificate is in us-east-1 #3485

nmussy commented Jul 30, 2019

nmussy commented Aug 7, 2019 •

edited

Loading

nmussy Aug 12, 2019

eladb Aug 13, 2019

nmussy Aug 13, 2019

eladb Aug 14, 2019

nmussy Aug 14, 2019

nmussy commented Aug 13, 2019

nmussy commented Aug 15, 2019

eladb commented Aug 18, 2019

nmussy commented Aug 18, 2019

rix0rrr Aug 19, 2019

rix0rrr Aug 19, 2019

rix0rrr commented Aug 19, 2019

mergify bot commented Aug 19, 2019

mergify bot commented Aug 19, 2019

mergify bot commented Aug 19, 2019

mergify bot commented Aug 19, 2019

rix0rrr left a comment

mergify bot commented Sep 13, 2019

rix0rrr commented Sep 16, 2019

aletheia commented Sep 20, 2019

nmussy commented Sep 20, 2019

aletheia commented Sep 20, 2019

rix0rrr commented Sep 25, 2019

aletheia commented Sep 25, 2019 via email

feat(cloudfront): verify acm certificate is in us-east-1 #3485

feat(cloudfront): verify acm certificate is in us-east-1 #3485

Conversation

nmussy commented Jul 30, 2019

nmussy commented Aug 7, 2019 • edited Loading

nmussy Aug 12, 2019

Choose a reason for hiding this comment

eladb Aug 13, 2019

Choose a reason for hiding this comment

nmussy Aug 13, 2019

Choose a reason for hiding this comment

eladb Aug 14, 2019

Choose a reason for hiding this comment

nmussy Aug 14, 2019

Choose a reason for hiding this comment

nmussy commented Aug 13, 2019

nmussy commented Aug 15, 2019

eladb commented Aug 18, 2019

nmussy commented Aug 18, 2019

rix0rrr Aug 19, 2019

Choose a reason for hiding this comment

rix0rrr Aug 19, 2019

Choose a reason for hiding this comment

rix0rrr commented Aug 19, 2019

mergify bot commented Aug 19, 2019

Pull Request Checklist

mergify bot commented Aug 19, 2019

Pull Request Checklist

mergify bot commented Aug 19, 2019

mergify bot commented Aug 19, 2019

rix0rrr left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 13, 2019

rix0rrr commented Sep 16, 2019

aletheia commented Sep 20, 2019

nmussy commented Sep 20, 2019

aletheia commented Sep 20, 2019

rix0rrr commented Sep 25, 2019

aletheia commented Sep 25, 2019 via email

nmussy commented Aug 7, 2019 •

edited

Loading