Add WAF parameter to GuEc2App

[aug24]

What does this change?

It provides the capability to automatically create an SSM Param for the Arn to be protected by WAF, using a standard form.

This is achieved simply by adding waf:true to GuEc2App.

How to test

Tests have been added to base.test.ts. These can be validated by synthing a cdk template for a stack which uses WAF; removing the WAF Arn param block and adding waf:true to the stack declaration.

How can we measure success?

A standard form Aws::SSM:Parameter is created.

Have we considered potential risks?

Risks are minimised as the default is to not create this param.

Checklist

• I have listed any breaking changes, along with a migration path ¹
• I have updated the documentation as required for the described changes ²

Footnotes

1. Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs. ↩

2. If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid? ↩

[changeset-bot]

⚠️ No Changeset found

Latest commit: f1a0fce

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[kelvin-chappell]

Perhaps add the prerequisites for this setting (eg. account needs to have waf set up) and a link to the waf configuration doc.

[jacobwinch]

I wonder if calling this exposeArnForWaf (or similar) would make it clearer¹ that this boolean alone isn't sufficient to get WAF protection?

I don't feel strongly about this though so feel free to leave it as is if you think that's too verbose.

Footnotes

1. To someone in a rush who doesn't read the typedocs! ↩

[akash1810]

I think this repo tends to name these as enableX?

[jacobwinch]

Thinking about this gotcha some more, I think the problem is likely to be:

1. In the original CFN template, the LogicalIdA resource defines a AWS::SSM::Parameter with name /infosec/waf/services/TEST/my-app-alb-arn.
2. Setting the new waf boolean to true adds the LogicalIdB resource; this is another AWS::SSM::Parameter with name /infosec/waf/services/TEST/my-app-alb-arn.

We cannot deploy a CFN stack which contains both LogicalIdA and LogicalIdB because a parameter name must be unique per account (per region?) - and we want to point at different values!

Even if we delete LogicalIdA in the same PR (CFN update) that provisions LogicalIdB, the update will fail because CFN applies creates before deletes, so LogicalIdA is still 'reserving' the magic parameter name at the crucial moment that we try to provision LogicalIdB.

IIUC this is what forces us to currently use 2 PRs (CFN updates) - one to delete LogicalIdA and one to provision LogicalIdB.

We could also consider allowing users to retain the original logical id (LogicalIdA) so that they can roll the change out in a single PR (CFN update).

In order to achieve this, the prop would need to be something like:

waf?: {
  enabled: boolean;
  existingParamId?: string;
}

And then in our implementation below, we'd have something like:

if (waf.enabled) {
  const stage = scope.stage;
  const wafParam = new StringParameter(this, "AlbSsmParam", {
    parameterName: `/infosec/waf/services/${stage}/${app}-alb-arn`,
    description: `The ARN of the ALB for ${stage}-${app}.`,
    simpleName: false,
    stringValue: loadBalancer.loadBalancerArn,
    tier: ParameterTier.STANDARD,
    dataType: ParameterDataType.TEXT,
  });
  if (waf.existingParamId) { 
    this.overrideLogicalId(wafParam, {
      logicalId: "LogicalIdA",
      reason: "Retaining original parameter's logical ID to avoid deployment issues"
    });
  }
}

This would allows users to delete the LogicalIdA resource from their YAML template in the same PR as enabling the WAF param via the pattern.

From a CloudFormation perspective, this means that the update just looks like a change to the value of the LogicalIdA AWS::SSM::Parameter (i.e. we update the exact same parameter but starting pointing it at the new ALB).

[jacobwinch]

Although that allows a single deployment to work, it's still pretty fiddly, so if you are happy with the 2-step deployment then I think it's fine to leave it as is!

[jacobwinch]

Thanks for adding the helpful docs here.

In order to generate a new library release as part of this PR, you'll need to follow these instructions for adding a changeset. I'd suggest copy/pasting most of these details into the release notes that you add as part of that too.

[jacobwinch]

Thanks for improving this; I've made some small suggestions but none of them are blocking.

Feel free to merge this in once you've added the changeset. This will trigger some automation which raises a PR similar to this one. Feel free to check/merge that PR yourself too.

Once the relevant Release package updates PR is merged, a new GuCDK version containing this feature should be available via npm.

[kelvin-chappell]

This is a great idea! 👍

[adamnfish-gu]

As mentioned in chat today I think this may be better as an object, even if it still just contains a single boolean flag. That would let us expose additional WAF configuration in the future without a breaking change for teams, and this is particularly relevant if we decide to change the implementation from an SSM param (as now) to a directly provisioned WAF.

This would be in line with other parts of our CDK offering, for example the Google Auth property of GuEC2App:

googleAuth: {
  enabled: true,
  ... other properties
}

I think it might be good to do this up front so we don't need another major breaking change in future, even though we aren't demanding this flexibility today?