feat(apigateway): resource policy configuration for private API #32719

badmintoncryer · 2025-01-03T01:50:17Z

Issue # (if applicable)

Reason for this change

The same PR is closed during maintainer's review. (#31692)

To create a Private API Gateway, we need to attach a resource policy that allows access only from specific Interface VPC Endpoints, as shown below.

new apigateway.RestApi(this, 'PrivateRestApi', {
      endpointTypes: [apigateway.EndpointType.PRIVATE],
      handler: fn,
      policy: new iam.PolicyDocument({
        statements: [
          new iam.PolicyStatement({
            principals: [new iam.AnyPrincipal],
            actions: ['execute-api:Invoke'],
            resources: ['execute-api:/*'],
            effect: iam.Effect.DENY,
            conditions: {
              StringNotEquals: {
                "aws:SourceVpce": vpcEndpoint.vpcEndpointId
              }
            }
          }),
          new iam.PolicyStatement({
            principals: [new iam.AnyPrincipal],
            actions: ['execute-api:Invoke'],
            resources: ['execute-api:/*'],
            effect: iam.Effect.ALLOW
          })
        ]
      })
    })

This is a bit troublesome.

Description of changes

Define IRestApi.addToResourcePolicy()
Implement addToResourcePolicy() at RestApi, SpecApi, and imported RestApi class
Implement RestApiBase.grantInvokeToVpcEndpoint()

In the grantInvokeToVpcEndpoint method, it was necessary to set a resource policy, and since a policy already existed in RestApiProps, I implemented it so that both can be used simultaneously.

Describe any new or updated permissions being added

Add 2 functions which modify resource policies.

Description of how you validated changes

Add both unit and integ tests.

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

codecov · 2025-01-07T12:55:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.38%. Comparing base (3fa5b23) to head (ef9521e).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32719   +/-   ##
=======================================
  Coverage   81.38%   81.38%           
=======================================
  Files         222      222           
  Lines       13698    13698           
  Branches     2413     2413           
=======================================
  Hits        11148    11148           
  Misses       2271     2271           
  Partials      279      279

Flag	Coverage Δ
suite.unit	`81.38% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.69% <ø> (ø)`
packages/aws-cdk-lib/core	`82.10% <ø> (ø)`

go-to-k · 2025-01-08T05:02:58Z

packages/aws-cdk-lib/aws-apigateway/lib/restapi.ts

+   *
+   * @param statement the policy statement to add
+   */
+  addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult;


This adds something non-optional (a method that should be implemented compulsorily) to the interface. However, if a user has already created a custom construct that has implemented this IRestApi, this change will cause the CDK application to stop working. (It may be a Construct made in community, or it may be a Construct made within a company.)

The interface is a foundation of Construct and the medium used for communication between Constructs. So I'm wondering if we should avoid this sort of change, what do you think?

Thank you for your review! A valid point.
As an alternative, how about having the RestApiBase class implement the IResourceWithPolicy interface?

- export abstract class RestApiBase extends Resource implements IRestApi { + export abstract class RestApiBase extends Resource implements IRestApi, IResourceWithPolicy {

That's maybe good!

However, RestApiBase has been exported, so there may be a slight impact. But since you've created a PR, let's just make it up and leave the rest to the maintainer's discretion!

Because I don't think many users will extend RestApiBase instead of the interface :)

I haven't seen most of the code yet, so please give me a shout so I can look at it when you fix it.
(I haven't submitted change request yet, so if another community reviewers see it first, I'll leave it to them! So let's not resolve these comments for now, please.)

Thanks!

I'll update my code and ping you later.

badmintoncryer · 2025-01-09T14:51:06Z

@go-to-k I've modified the RestApiBase class to implement the IResourceWithPolicy interface!

go-to-k · 2025-01-10T11:10:50Z

packages/aws-cdk-lib/aws-apigateway/lib/restapi.ts

+   *
+   * @param vpcEndpoint the interface VPC endpoint to grant access to
+   */
+  public grantInvokeToVpcEndpoint(vpcEndpoint: ec2.IVpcEndpoint): void {


This is a method to invoke from (via) a VPC endpoint, but is the name grantInvokeToVpcEndpoint as it is supposed to be? Do you mean grant the policy to a VPCEndpoint? (Sounds like VPCEndpoint is the principal...?)

Also, Method class has a method called grantExecute, is it correct to not use the word "Execute" instead of "Invoke" for the grantInvokeToVpcEndpoint?

This is not a suggestion, just a question :)

I've really thought about what to call this function... Therefore, I had asked the maintainer and he suggested grantInvokeToVpcEndpoint.

#31692 (comment)

go-to-k · 2025-01-10T11:33:36Z

This is just another question, but is there a reason you created this method in the Rest API instead of Method Construct?

go-to-k · 2025-01-10T11:47:04Z

packages/aws-cdk-lib/aws-apigateway/lib/restapi.ts

+    this.addToResourcePolicy(new iam.PolicyStatement({
+      principals: [new iam.AnyPrincipal()],
+      actions: ['execute-api:Invoke'],
+      resources: ['execute-api:/*'],
+      effect: iam.Effect.ALLOW,
+    }));


Related: #32719 (comment)

If you don't want to "grant specific permissions" but rather "deny access from non-VPCEndpoints", do we need this permission policy?

However, I know that in this case it would be a strange method to just delete this policy because it is different from the normal Grant method. Anyway, take this as just another question.

I think if we remove this ALLOW policy, the IAM policy implicitly denies all actions. Therefore, we cannot access to the APIGW via the correct endpoint.

I think it is enough to allow the access from the correct vpc endpoint, I used these policies which is suggested in the official document.
https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-resource-policies-examples.html#apigateway-resource-policies-source-vpc-example

badmintoncryer · 2025-01-10T12:22:36Z

This is just another question, but is there a reason you created this method in the Rest API instead of Method Construct?

The purpose of associating this function with the Method is to define private and public ones for each Method, is that correct?
I believe that a REST API can only be created as either private or public, and it is not possible to have both private and public Methods coexist within the same REST API.

go-to-k

I see. It is sufficient to have answers to these questions. I wanted it. I'm okay and approve it.

To a maintainer: please see the comment. This is about a behavior for users who customize their own constructs.

badmintoncryer · 2025-01-10T12:44:19Z

Thanks @go-to-k !!

aws-cdk-automation · 2025-01-13T11:36:45Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: ef9521e
Result: SUCCEEDED
Build Logs (available for 30 days)

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

badmintoncryer added 14 commits October 8, 2024 14:38

addToResourcePolicy and grantInvoke

aa6f826

add integ test

a3a2d54

update readme

b599740

lint

53f5970

update integ file

c13188f

add trailing comma

24479bb

Merge branch 'main' into 31660-grantInvoke

e03a901

fix readme

6eb176f

Merge branch 'main' into 31660-grantInvoke

841c596

Merge branch 'main' into 31660-grantInvoke

dc2de52

rename to grantInvokeToVpcEndpoint

15da511

Merge branch 'main' into 31660-grantInvoke

24df0ca

Merge branch 'main' into 31660-grantInvoke

de49a41

Merge branch 'main' into 31660-grantInvoke

4e21a14

aws-cdk-automation requested a review from a team January 3, 2025 01:50

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Jan 3, 2025

badmintoncryer added 7 commits January 3, 2025 11:57

Merge remote-tracking branch 'origin/main' into 31660-grantInvoke

3ae6fab

update integ

7112b7e

Merge branch 'main' into 31660-grantInvoke

6a01cb5

Merge branch 'main' into 31660-grantInvoke

b8bda4f

Merge branch 'main' into 31660-grantInvoke

17e3069

Merge branch 'main' into 31660-grantInvoke

8ab7ebc

fix

60520ba

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 7, 2025

go-to-k reviewed Jan 8, 2025

View reviewed changes

refactor: implement IResourceWithPolicy

77118ec

badmintoncryer added 2 commits January 9, 2025 09:51

fix

804b028

fix

571709a

Merge branch 'main' into 31660-grantInvoke

1f91465

go-to-k reviewed Jan 10, 2025

View reviewed changes

go-to-k approved these changes Jan 10, 2025

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jan 10, 2025

badmintoncryer added 2 commits January 10, 2025 21:50

Merge branch 'main' into 31660-grantInvoke

0dd2dca

Merge branch 'main' into 31660-grantInvoke

ef9521e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apigateway): resource policy configuration for private API #32719

feat(apigateway): resource policy configuration for private API #32719

badmintoncryer commented Jan 3, 2025 •

edited

Loading

codecov bot commented Jan 7, 2025 •

edited

Loading

go-to-k Jan 8, 2025 •

edited

Loading

badmintoncryer Jan 8, 2025 •

edited

Loading

go-to-k Jan 8, 2025

go-to-k Jan 8, 2025 •

edited

Loading

badmintoncryer Jan 8, 2025

badmintoncryer commented Jan 9, 2025

go-to-k Jan 10, 2025 •

edited

Loading

go-to-k Jan 10, 2025

badmintoncryer Jan 10, 2025

go-to-k commented Jan 10, 2025

go-to-k Jan 10, 2025 •

edited

Loading

go-to-k Jan 10, 2025 •

edited

Loading

badmintoncryer Jan 10, 2025

badmintoncryer commented Jan 10, 2025

go-to-k left a comment

badmintoncryer commented Jan 10, 2025

aws-cdk-automation commented Jan 13, 2025

feat(apigateway): resource policy configuration for private API #32719

Are you sure you want to change the base?

feat(apigateway): resource policy configuration for private API #32719

Conversation

badmintoncryer commented Jan 3, 2025 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

codecov bot commented Jan 7, 2025 • edited Loading

Codecov Report

go-to-k Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

badmintoncryer Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

go-to-k Jan 8, 2025

Choose a reason for hiding this comment

go-to-k Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

badmintoncryer Jan 8, 2025

Choose a reason for hiding this comment

badmintoncryer commented Jan 9, 2025

go-to-k Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

go-to-k Jan 10, 2025

Choose a reason for hiding this comment

badmintoncryer Jan 10, 2025

Choose a reason for hiding this comment

go-to-k commented Jan 10, 2025

go-to-k Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

go-to-k Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

badmintoncryer Jan 10, 2025

Choose a reason for hiding this comment

badmintoncryer commented Jan 10, 2025

go-to-k left a comment

Choose a reason for hiding this comment

badmintoncryer commented Jan 10, 2025

aws-cdk-automation commented Jan 13, 2025

AWS CodeBuild CI Report

badmintoncryer commented Jan 3, 2025 •

edited

Loading

codecov bot commented Jan 7, 2025 •

edited

Loading

go-to-k Jan 8, 2025 •

edited

Loading

badmintoncryer Jan 8, 2025 •

edited

Loading

go-to-k Jan 8, 2025 •

edited

Loading

go-to-k Jan 10, 2025 •

edited

Loading

go-to-k Jan 10, 2025 •

edited

Loading

go-to-k Jan 10, 2025 •

edited

Loading