Make hosted_zone_id optional, code update

[lorchda]

Feature or Bugfix

• Bugfix

Detail

• Make hosted_zone_id optional, code update

Relates

• Make DNS alias record creation toggleable / optional #797

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

• Does this PR introduce or modify any input fields or queries - this includes
  fetching data from storage outside the application (e.g. a database, an S3 bucket)? N/A
  • Is the input sanitized? N/A
  • What precautions are you taking before deserializing the data you consume? N/A
  • Is injection prevented by parametrizing queries? N/A
  • Have you ensured no eval or similar functions are used? N/A
• Does this PR introduce any functionality or component that requires authorization? N/A
  • How have you ensured it respects the existing AuthN/AuthZ mechanisms? N/A
  • Are you logging failed auth attempts? N/A
• Are you using or adding any cryptographic features? N/A
  • Do you use a standard proven implementations? N/A
  • Are the used keys controlled by the customer? Where are they stored? N/A
• Are you introducing any new policies/roles/users? N/A
  • Have you used the least-privilege principle? How? N/A

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

Description

Make hosted_zone_id optional and provide HostedZoneId and DNSName in CloudFormation Stack Output, so users can create their own Route53 AliasTarget.

Following validation checks in ecs_patterns.ApplicationLoadBalancedFargateService were considered:

• frontend_alternate_domain and userguide_alternate_domain have to be None when the hosted_zone is None, see checks in multiple-target-groups-service-base.ts#L463, or else a A Route53 hosted domain zone name is required to configure the specified domain name error is raised
• for a HTTPS ALB listener, only the certificate is ultimately required, and not the domainName or domainZone, as per evaluation logic in application-load-balanced-service-base.ts#L509

[lorchda]

This is what the documentation says for the parameters to ecs_patterns.ApplicationLoadBalancedFargateService:

• domain_name (Optional[str]) – The domain name for the service, e.g. “api.example.com.”.
  Default: - No domain name.
• domain_zone (Optional[IHostedZone]) – The Route53 hosted zone for the domain, e.g. “example.com.”.
  Default: - No Route53 hosted domain zone.

[dlpzx]

Hi @lorchda, I am looking into this PR today. The albfront stack will only be deployed for VPC-facing frontend. In our documentation we specify that the custom_domain MUST be specified for this type of deployment. The reason behind this is the ALB integration with ACM.

From the code I think there is going to be an issue in lines 149-159:

        if custom_domain and custom_domain.get('certificate_arn'):
            certificate = acm.Certificate.from_certificate_arn(self, "CustomDomainCertificate",
                                                               custom_domain.get('certificate_arn'))
        else:
            certificate = acm.Certificate(
                self,
                'CustomDomainCertificate',
                domain_name=custom_domain['hosted_zone_name'],
                subject_alternative_names=[f'*.{custom_domain["hosted_zone_name"]}'],
                validation=acm.CertificateValidation.from_dns(hosted_zone=hosted_zone),
            )

If the ACM certificate is not provided, data.all tries to create a certificate. In the case no custom_domain is provided the hosted_zone is left as None and my guess is that the certificate creation is going to fail. For this PR to be merged we need an additional condition on the creation of the certificate to create it only when there is a custom domain. In addition we need to update the docs to clearly communicate that no certificate is created, which is a downgrade in security.

What are your thoughts? Also @noah-paige I would like your opinion on this

[lorchda]

@dlpzx yes noticed that as well. My thinking was that custom_domain['hosted_zone_name'] would raise a not very descriptive KeyError along with a stack trace, but would otherwise abort as desired. This could be improved by providing a user-friendly error message that checks for the presence of custom_domain['hosted_zone_name'].

However, this is an unrelated issue that already existed before this PR. This PR is related only to hosted_zoned_id, and the semantics around hosted_zone_name (which must be provided) remains the same.

Example configuration:

    "DeploymentEnvironments": [
      {
        "internet_facing": false,
        "custom_domain": {
          "hosted_zone_name": "dataall.example.internal",
          "certificate_arn": "arn:aws:acm:REGION:ACCOUNT_ID:certificate/UID"
        }
        ...
      }
    ]

[lorchda]

@dlpzx I am pretty sure that custom_domain['hosted_zone_name'] will abort with a KeyError and stacktrace, and not simply continue with deployment as you suggested. This reduces the problem to a cosmetic issue, where a more user-friendly error message could be provided in the absence of custom_domain['hosted_zone_name'], but the desired functionality is there. To be validated.

[lorchda]

custom_domain['hosted_zone_name']
raises a KeyError when the key does not exist

custom_domain.get('hosted_zone_name')
returns None if the key does not exist

[dlpzx]

Hi @lorchda, thanks for the quick response. It is true that the certificate problem is more of a cosmetic issue, but based on our experience with customers we really need to provide an error message to users for them to understand the issue. This problem was not there before because custom_domain['hosted_zone_name'] was mandatory (at least in the documentation) for an internet_facing=false.

It is still unclear to me what your final architecture looks like. The target is to not provide any route53 zone or certificate and have the ALB defined without those optional parameters right? In your cdk.json the parameter "custom_domain" does not exist right?

[lorchda]

@dlpzx I added a fix for this (unrelated) issue to check for custom_domain['hosted_zone_name']

[dlpzx]

Thank you @lorchda, the PR looks good. I will do a final test by deploying it to AWS and we can merge. I will test multiple scenarios to ensure backwards compatibility:

Scenario 1: pre-existing deployment. VPC-facing, with hosted zone ID

"internet_facing": false,
        "custom_domain": {
          "hosted_zone_name": "somedomain.dataall.com",
          "hosted_zone_id": "XXXXXXXXXXXXX",
          "certificate_arn": "arn:aws:acm:us-east-1:111111111111:certificate/xxxxxxxxxxxxxxx"
        },

• tested, CICD and deployment is unaffected by the PR changes

Scenario 2: new deployment. VPC-facing, without hosted zone name

"internet_facing": false,
        "custom_domain": {
          "hosted_zone_id": "XXXXXXXXXXXXX",
          "certificate_arn": "arn:aws:acm:us-east-1:111111111111:certificate/xxxxxxxxxxxxxxx"
        },

• It should fail at synthesis - It fails, but in a previously synthesized stack. Either way it fails, which is what we want.

Scenario 3: existing deployment. VPC-facing, without hosted zone ID

"internet_facing": false,
        "custom_domain": {
          "hosted_zone_name": "somedomain.dataall.com",
          "certificate_arn": "arn:aws:acm:us-east-1:111111111111:certificate/xxxxxxxxxxxxxxx"
        },

• CICD and deployment successful (see the screenshot below with the albfront stack updated
• Application working as expected

All good, @lorchda approving and merging! Thanks again