fix(apigateway): consolidate lambda permissions when reused for multiple operations

[cogwirrel]

Issue # (if applicable)

Fixes #9327
Fixes #19535

Reason for this change

The maximum Lambda permission policy size could be exceeded for APIs which reused the same Lambda function for multiple operations, as the integration added a new permission for each operation, scoped down to the specific operation.

Description of changes

This change updates both the REST and HTTP API lambda integrations to consolidate permissions when more than 10 permissions would be added for the same handler, creating a permission scoped to the entire API rather than the operation. The behaviour remains the same where individual lambdas are used for operations.

Note that we search for permissions within the route's parent stack for HTTP APIs, or within the API for REST APIs, and so it won't prevent the policy size being exceeded if the same lambda is reused cross-stack.

Describe any new or updated permissions being added

Permission for API Gateway to invoke the lambda is scoped to any resource/method/stage when a lambda is reused for multiple operations.

Description of how you validated changes

Unit tests, Integration tests

Added an integration test for both REST and HTTP (integ.lambda-permission-consolidation).

There are a lot of integration tests that now have updated snapshots since I've changed the logical ID for lambda permissions to include both the API and Handler IDs so that they can be identified for consolidation.

I wasn't able to get the following 2 integration tests to run:

• aws-route53-targets/test/integ.api-gateway-domain-name.ts - there's a comment in here that mentions it doesn't work due to the reliance on a hardcoded domain name
• aws-apigatewayv2-integrations/test/http/integ.lambda-permission-consolidation.ts - fails since the lambda permission tries to deploy prior to the imported lambda function - I'm not sure if this worked previously - can look into fixing this integration test if required

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[aws-cdk-automation]

(This review is outdated)

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[Abogical]

Can you try recreating the integ test snapshots that are currently conflicting with the main branch atm? If a test fails, you can run the integ test with --dry-run and let me know which tests don't pass.

Pull request has been modified.

[cogwirrel]

Thanks @Abogical ! :) I ran the integ tests and the only one that failed was aws-route53-targets/test/integ.api-gateway-domain-name.ts - this failed with the following error:

  FAILED     aws-route53-targets/test/integ.api-gateway-domain-name-apigateway-domain-name/DefaultTest (undefined/us-east-1) 50.814s
      Integration test failed: ToolkitError: ❌  aws-cdk-apigw-alias-integ failed: ToolkitError: The stack named aws-cdk-apigw-alias-integ failed creation, it may need to be manually deleted from the AWS console: ROLLBACK_COMPLETE: Resource handler returned message: "The domain name you provided already exists. (Service: ApiGateway, Status Code: 400, Request ID: 55e00a5d-0f44-4a7a-8217-80a5f0eb9764) (SDK Attempt Count: 1)" (RequestToken: 0276c41e-32e9-9381-0cb8-8e16227f325a, HandlerErrorCode: AlreadyExists)

I've run that with --dry-run and committed the updated snapshots.

[cogwirrel]

Looks like the security guardian check is failing on integ.prop-injectors.ts - I think this is unrelated to my change as it's flagging kms permissions. By the looks of things security guardian runs for any changed integ tests and since this change has updated the logical ids of lambda permissions, integ.prop-injectors.ts has some changes.

[Abogical]

@cogwirrel The security guardian is not a required workflow to pass atm.

[Abogical]

Thanks for the change! The integration tests are now passing. I've asked for a security review of this PR.

[Abogical]

Hi @cogwirrel , thank you for your contribution to this. However, we think that the approach you take for this PR is not a good idea from a security perspective. Consolidating lambda permissions to a permission that exposes the entire API is too permissive. This would only be acceptable if you can detect that ALL the methods use the same lambda integration, not just if its used more than 10 times.

I'm discussing possible alternative solutions to the issue with the team. I'll let you know once I have an update.

[Abogical]

Hi @cogwirrel !

An alternative fix we can accept to this is to add an opt-in configuration property to the integration class. This property if set will have the integration add a permission per API instead of per route. This should fix the issue you mentioned while making it clear that the user intends to have the lambda permissions more permissive.

[cogwirrel]

Hi @cogwirrel !

An alternative fix we can accept to this is to add an opt-in configuration property to the integration class. This property if set will have the integration add a permission per API instead of per route. This should fix the issue you mentioned while making it clear that the user intends to have the lambda permissions more permissive.

Thanks for reviewing! I've raised a separate PR which implements this suggestion here: #36021

[Abogical]

Closing in favor of #36021.

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.