Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(apigatewayv2): http api - IAM authorizer support #17519

Merged
merged 55 commits into from
Dec 17, 2021

Conversation

misterjoshua
Copy link
Contributor

@misterjoshua misterjoshua commented Nov 16, 2021

Fixes #15123

See also: @nija-at's comments on grantInvoke, #10534


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

@gitpod-io
Copy link

gitpod-io bot commented Nov 16, 2021

@github-actions github-actions bot added the @aws-cdk/aws-apigatewayv2 Related to Amazon API Gateway v2 label Nov 16, 2021
@misterjoshua misterjoshua marked this pull request as ready for review November 16, 2021 02:21
@pahud
Copy link
Contributor

pahud commented Nov 16, 2021

Hi @ChenKuanSun can you take a look?

@bracki
Copy link
Contributor

bracki commented Nov 16, 2021

Looking at the docs it is unclear to me how to just turn on IAM for the whole API. Also I don't understand why it's not an "authorizer", as in the Rest API gateway. Personally I think that the use case of bringing your own policy/role is more often the case then the "grantInvoke", but that's a gut feeling.

Currently I'm solving this problem with:

    // Enable IAM authentication
    routes.forEach((route) => {
      const r = route.node.defaultChild as CfnRoute;
      r.authorizationType = AuthorizationType.IAM;
    });

Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is starting to look interesting. See my initial comments below.

packages/@aws-cdk/aws-apigatewayv2/lib/http/route.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/http/route.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/http/route.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/http/route.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/http/route.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/http/route.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/test/http/integ.iam.ts Outdated Show resolved Hide resolved
@nija-at nija-at changed the title feat(apigatewayv2): add AWS_IAM authorizer support feat(apigatewayv2): http api - IAM authorizer support Nov 16, 2021
@nija-at nija-at added effort/medium Medium work item – several days of effort p2 labels Nov 16, 2021
@mergify mergify bot dismissed nija-at’s stale review November 16, 2021 18:24

Pull request has been modified.

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Nov 16, 2021

Looking at the docs it is unclear to me how to just turn on IAM for the whole API. Also I don't understand why it's not an "authorizer", as in the Rest API gateway. Personally I think that the use case of bringing your own policy/role is more often the case then the "grantInvoke", but that's a gut feeling.

Currently I'm solving this problem with:

    // Enable IAM authentication
    routes.forEach((route) => {
      const r = route.node.defaultChild as CfnRoute;
      r.authorizationType = AuthorizationType.IAM;
    });

@bracki Good point. Since my initial PR, Nija has suggested adding HttpIamAuthorizer, so now you should be able to turn on IAM for the whole API by specifying a default authorizer in the HttpApi.

@nija-at nija-at assigned otaviomacedo and unassigned nija-at Dec 2, 2021
packages/@aws-cdk/aws-apigatewayv2/README.md Outdated Show resolved Hide resolved
@@ -61,9 +85,14 @@ export class HttpRouteKey {
if (path !== '/' && (!path.startsWith('/') || path.endsWith('/'))) {
throw new Error('A route path must always start with a "/" and not end with a "/"');
}
return new HttpRouteKey(`${method ?? HttpMethod.ANY} ${path}`, path);
const keyMethod = method ?? HttpMethod.ANY;
return new HttpRouteKey(keyMethod, `${keyMethod} ${path}`, path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] Now that key can be derived from the other two attributes, I would remove it from the constructor to make it cleaner and avoid mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now simplified this section so that it's cleaner and easier to avoid mistakes. As far as I can see, HTTP API has only the $default and METHOD /path/here formats for key, so this should work. https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-routes.html

Let me know whether this change works for you.

@mergify mergify bot dismissed otaviomacedo’s stale review December 7, 2021 17:38

Pull request has been modified.

@misterjoshua
Copy link
Contributor Author

@otaviomacedo I got a notification for the following message but I can't find it anywhere to reply to it. I'll respond here in the comments:

I think this is the last point we need to work out. I'd rather have all authorizers in the -authorizers module. What's your idea here? How would you avoid the circular dependency?

Right now I've allowed grantInvoke to configure IAM via authorizer by implementing the authorizer in the main package. But yes, I can see that this makes the IAM authorizer the odd one out.

It seems to me that if the IAM authorizer must be in the other package, the circular dependency arises from adding the authorizer automatically when the user calls grantInvoke.

If we resolve this conflict by removing the route's responsibility to configure IAM authorization, we can relocate the IAM authorizer and there will be no circular dependency problem.

All things being equal, I'd prefer to remove the route's responsibility to configure IAM from grantInvoke as it could simplify the Lazy props.

What do you think?

@otaviomacedo
Copy link
Contributor

If we resolve this conflict by removing the route's responsibility to configure IAM authorization, we can relocate the IAM authorizer and there will be no circular dependency problem.

I agree. So, two questions:

  1. Where would you move that responsibility to?
  2. What would the API look like?

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Dec 16, 2021

1. Where would you move that responsibility to?

@otaviomacedo We'd require that the user specify the authorizer they want in the construct's props as they do now.

2. What would the API look like?

It would look the same as it does now in the README, however, it differs in one case: When the user omits the IAM authorizer and calls grantInvoke, we should probably throw an error. This effectively undoes "If grantInvoke is called, you can set this automatically."

@otaviomacedo
Copy link
Contributor

Yes, the only downside is that, if someone provides a different authorizer, grantInvoke would do nothing (I believe) and they would only find out at runtime. But I think it's a reasonable compromise.

Copy link
Contributor

@otaviomacedo otaviomacedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! Thank you.

@mergify
Copy link
Contributor

mergify bot commented Dec 17, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit fd8e0e3 into aws:master Dec 17, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 17, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@misterjoshua misterjoshua deleted the add-iam-authorizer branch December 17, 2021 16:02
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
Fixes aws#15123

See also: [@nija-at's comments on `grantInvoke`](aws#14853 (comment)), aws#10534

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigatewayv2 Related to Amazon API Gateway v2 effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(apigatewayv2): addRoutes doesn't allow authorizationType: 'AWS_IAM'
7 participants