feat(cloudfront): load external pem keys for Trusted Key Groups

[robertd]

@njlynch Here is my first take on this... Constructive criticism is more than welcome. ✌️

---

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

[gitpod-io]

[robertd]

@njlynch I've been following your guidance from #12743 (comment) . Here are the few things that I'm trying to figure out:

• I've been getting the following error: [awslint:construct-ctor-props-optional:@aws-cdk/aws-cloudfront.PublicKey] construct "props" must be optional since all props are optional. In this MR all the PublicKeyProps params are now optional, thus awslint suggesting that PublicKey needs to be optional as well in KeyGroup. This leads me to believe that we need to create something similar to lambda.Code.fromInline and lambda.Code.fromAsset. But in this case our "asset" would not be uploaded to S3 bucket... it would just be loaded from the filesystem, unless that S3 upload is desired feature. WDYT?
• Current implementation is working... but something that I'm not really sure about is the approach of handling encodedKeyPath for fs.readFileSync implementation. I've noticed that while writing tests I had to use relative paths and that looked weird to me. This kinda pushed me even more to think about using something similar to lambda.Code.fromInline and lambda.Code.fromAsset approach.
• PublicKey class now has bunch of if checks... is there a better way to approach this? I can think of few more checks (is it pem file, etc).
• What encoding fs.readFileSync should use when reading the .pem key in? utf-8?

Do you have any design guidance we should pursue instead?

[robertd]

@njlynch

• I've been getting the following error: [awslint:construct-ctor-props-optional:@aws-cdk/aws-cloudfront.PublicKey] construct "props" must be optional since all props are optional. In this MR all the PublicKeyProps params are now optional, thus awslint suggesting that PublicKey needs to be optional as well in KeyGroup. This leads me to believe that we need to create something similar to lambda.Code.fromInline and lambda.Code.fromAsset. But in this case our "asset" would not be uploaded to S3 bucket... it would just be loaded from the filesystem, unless that S3 upload is desired feature. WDYT?
• Current implementation is working... but something that I'm not really sure about is the approach of handling encodedKeyPath for fs.readFileSync implementation. I've noticed that while writing tests I had to use relative paths and that looked weird to me. This kinda pushed me even more to think about using something similar to lambda.Code.fromInline and lambda.Code.fromAsset approach.

These are two bullet points are kinda obsolete now after 41c1a0a. I've borrowed heavily from lambda.Code. Let me know what you think of the overall approach.

//inline
new PublicKey(stack, 'MyPublicKey', {
  encodedKey: Key.fromInline(''),
});

//from file
new PublicKey(stack, 'MyPublicKey', {
  encodedKey: Key.fromFile(path.join(__dirname, 'pem/pubkey-good.test.pem')),
})

• PublicKey class now has bunch of if checks... is there a better way to approach this? I can think of few more checks (is it pem file, etc).

InlineKey class has a majority of these checks now. However, let me know what else we can check on. I'll gladly add more and update tests.

On a side note... can you please explain what's the purpose of !Token.isUnresolved(key) (

aws-cdk/packages/@aws-cdk/aws-cloudfront/lib/public-key.ts

Line 143 in 41c1a0a

if (!Token.isUnresolved(key) && !/^-----BEGIN PUBLIC KEY-----/.test(key)) {

) and what kind of error handling we would need to put in place for a standalone check? I was thinking of separating -----BEGIN----- and -----END----- checks from Token check. Thoughts?

• What encoding fs.readFileSync should use when reading the .pem key in? utf-8?

In addition to this... any guidelines on error handling when reading files? Should I put this in try/catch block? i.e. check if pem file is empty, check if it has .pem extension, etc.

Do you have any design guidance we should pursue instead?

Let me know... If this makes sense from design perspective and if we need to go in different direction.

P.S. I think we should merge this PR first and then merge #12847. That way we can update/rebase #12847 once this one lands on master and we won't have breaking API changes right off the bat. Thoughts?

I'll convert #12847 to draft in the meantime.

[robertd]

Hmm... my latest builds are failing... the only thing that changed is that 1.88.0 came out in the meantime....

Working build on 1.87.1

Determining baseline version...
current version: 1.87.1
current version: 1.87.1
  Current version is 1.87.1.
Using version '1.87.1' as the baseline...
Installing from NPM...
npx: installed 254 in 2.999s
npm WARN deprecated @aws-cdk/cdk-assets-schema@1.87.1: merged into @aws-cdk/cloud-assembly-schema
npm WARN deprecated @aws-cdk/aws-eks-legacy@1.87.1: Use the @aws-cdk/aws-eks module instead
npm WARN deprecated @aws-cdk/aws-dynamodb-global@1.87.1: This module has been deprecated. Use @aws-cdk/aws-dynamodb.Table with replicationRegions instead
npm WARN deprecated @aws-cdk/app-delivery@1.87.1: Use the @aws-cdk/pipelines module instead

added 184 packages, and audited 223 packages in 22s

found 0 vulnerabilities
Checking compatibility...
monocdk... OK.
@aws-cdk/alexa-ask... OK.
@aws-cdk/app-delivery... OK.
@aws-cdk/assets... OK.
@aws-cdk/aws-accessanalyzer... OK.
@aws-cdk/aws-acmpca... OK.
@aws-cdk/aws-amazonmq... OK.
@aws-cdk/aws-amplify... OK.
@aws-cdk/aws-apigateway... OK.
@aws-cdk/aws-apigatewayv2-integrations... OK.
@aws-cdk/aws-apigatewayv2... OK.
@aws-cdk/aws-appconfig... OK.
@aws-cdk/aws-appflow... OK.
@aws-cdk/aws-applicationautoscaling... OK.
@aws-cdk/aws-applicationinsights... OK.
@aws-cdk/aws-appmesh... OK.
@aws-cdk/aws-appstream... OK.
@aws-cdk/aws-appsync... OK.
@aws-cdk/aws-athena... OK.
@aws-cdk/aws-auditmanager... OK.
@aws-cdk/aws-autoscaling-common... OK.
@aws-cdk/aws-autoscaling-hooktargets... OK.
@aws-cdk/aws-autoscaling... OK.
@aws-cdk/aws-autoscalingplans... OK.
@aws-cdk/aws-backup... OK.
@aws-cdk/aws-batch... OK.
@aws-cdk/aws-budgets... OK.
@aws-cdk/aws-cassandra... OK.
@aws-cdk/aws-ce... OK.
@aws-cdk/aws-certificatemanager... OK.
@aws-cdk/aws-chatbot... OK.
@aws-cdk/aws-cloud9... OK.
@aws-cdk/aws-cloudformation... OK.
@aws-cdk/aws-cloudfront-origins... OK.
@aws-cdk/aws-cloudfront... OK.
...

Non-working build on 1.88.0

Determining baseline version...
current version: 1.88.0
current version: 1.88.0
  Current version is 1.88.0.
Using version '1.88.0' as the baseline...
Installing from NPM...
npx: installed 254 in 2.952s
npm WARN deprecated @aws-cdk/cdk-assets-schema@1.88.0: merged into @aws-cdk/cloud-assembly-schema
npm WARN deprecated @aws-cdk/aws-dynamodb-global@1.88.0: This module has been deprecated. Use @aws-cdk/aws-dynamodb.Table with replicationRegions instead
npm WARN deprecated @aws-cdk/aws-eks-legacy@1.88.0: Use the @aws-cdk/aws-eks module instead
npm WARN deprecated @aws-cdk/app-delivery@1.88.0: Use the @aws-cdk/pipelines module instead

added 184 packages, and audited 223 packages in 18s

found 0 vulnerabilities
Checking compatibility...
monocdk... OK.
@aws-cdk/alexa-ask... OK.
@aws-cdk/app-delivery... OK.
@aws-cdk/assets... OK.
@aws-cdk/aws-accessanalyzer... OK.
@aws-cdk/aws-acmpca... OK.
@aws-cdk/aws-amazonmq... OK.
@aws-cdk/aws-amplify... OK.
@aws-cdk/aws-apigateway... OK.
@aws-cdk/aws-apigatewayv2-integrations... OK.
@aws-cdk/aws-apigatewayv2... OK.
@aws-cdk/aws-appconfig... OK.
@aws-cdk/aws-appflow... OK.
@aws-cdk/aws-applicationautoscaling... OK.
@aws-cdk/aws-applicationinsights... OK.
@aws-cdk/aws-appmesh... OK.
@aws-cdk/aws-appstream... OK.
@aws-cdk/aws-appsync... OK.
@aws-cdk/aws-athena... OK.
@aws-cdk/aws-auditmanager... OK.
@aws-cdk/aws-autoscaling-common... OK.
@aws-cdk/aws-autoscaling-hooktargets... OK.
@aws-cdk/aws-autoscaling... OK.
@aws-cdk/aws-autoscalingplans... OK.
@aws-cdk/aws-backup... OK.
@aws-cdk/aws-batch... OK.
@aws-cdk/aws-budgets... OK.
@aws-cdk/aws-cassandra... OK.
@aws-cdk/aws-ce... OK.
@aws-cdk/aws-certificatemanager... OK.
@aws-cdk/aws-chatbot... OK.
@aws-cdk/aws-cloud9... OK.
@aws-cdk/aws-cloudformation... OK.
@aws-cdk/aws-cloudfront-origins... OK.
@aws-cdk/aws-cloudfront... CHANGES.
Original assembly: @aws-cdk/aws-cloudfront@1.88.0
Updated assembly:  @aws-cdk/aws-cloudfront@0.0.0
API elements with incompatible changes:
err  - IFACE @aws-cdk/aws-cloudfront.PublicKeyProps: property encodedKey, string is not assignable to @aws-cdk/aws-cloudfront.Key: input to @aws-cdk/aws-cloudfront.PublicKey.<initializer> [strengthened:@aws-cdk/aws-cloudfront.PublicKeyProps]
...

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: a500b5d
• Result: FAILED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[robertd]

@njlynch Do you happen to know why package.json is full of 0.0.0s? Is it possible that’s why builds started failing all of a sudden with the release of 1.88.0?

Edit: I saw explanation about zeroes here https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#full-clean-build

Still trying to figure out why builds are failing.

[njlynch]

Still trying to figure out why builds are failing.
err - IFACE @aws-cdk/aws-cloudfront.PublicKeyProps: property encodedKey, string is not assignable to @aws-cdk/aws-cloudfront.Key: input to @aws-cdk/aws-cloudfront.PublicKey.<initializer> [strengthened:@aws-cdk/aws-cloudfront.PublicKeyProps]

The build is failing because we can't make backwards-incompatible changes to (non-experimental) classes (in non-experimental modules). As the PublicKey L2 was released, it now is a contract that we can't break subsequently by changing the type of one of the properties.

If we want to add this functionality now, the options are:

1. Add the new property for file name/path, make both fields optional, suppress the linter warnings, and add checks in the constructor that confirm that exactly one of the key fields has been provided. I think this is similar to your first approach here (and what I mentioned in feat(cloudfront): add PublicKey and KeyGroup L2 constructs  #12743). The downside, of course, to this approach is that the compiler isn't forcing that one of these fields is present, so we're pushing validation to 'runtime' checks rather than having the compiler help us do the right thing.
2. A mix of the above and what you've done here. We can create a new field with the new type, deprecate the existing string-based one, and make both optional. We still suffer from 'runtime' checks, but can remove the old field in our v2 launch.
3. Do nothing for a bit. This construct was just released. Wait for some feedback from the community on what makes sense. Maybe folks actually want to feed in a public key from SSM, or Secrets Manager, or something else, rather than local files. Maybe some helper construct that generates both private & public key, provides the public key to CloudFront and stores the private in Secrets Manager. Once we have a slightly better picture of what users need the most, we can attack this with more data.

On a side note... can you please explain what's the purpose of !Token.isUnresolved(key)

Sure. https://docs.aws.amazon.com/cdk/latest/guide/tokens.html is a handy primer. Short version -- any string (or number, or list) in the CDK might actually be a Token which references some other value from some other construct/resource. So rather than the string being ----BEGIN PUBLIC KEY----...., it might be ${TOKEN[SecretsManager.Value.1234]}, making "normal" string comparison/manipulation useless. In those cases, we usually opt to trust the input and skip whatever verification check we're performing.

[robertd]

@njlynch Thanks again for explaining all this. I figured it was probably a breaking contract. Makes total sense.

For the future reference, lesson learned on my part, maybe I should’ve created these two L2 constructs as experimental first and let the dust settle a bit... before changing the Implementation :)

In our use case (multiple teams using same AWS acct) we wanted to create bunch of trusted key groups (and pub keys) using factory pattern from file system and that guided my design of this PR. But I like the ideas you mentioned in option 3 so let’s wait for it then.

You can go ahead and review/merge #12847 so we can at least use new constructs in Distribution.

I’ll leave this PR as draft and come back to it after we get some feedback from the community.

[njlynch]

@robertd - Would you be okay just closing out this PR until we've received some feedback and ready to implement something like this? I keep seeing this in my "queue" and thinking I need to review it. :) We can always re-open it later.

[robertd]

@njlynch 👍