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: http api - iam authorizer #14853

Closed
wants to merge 6 commits into from
Closed

feat: http api - iam authorizer #14853

wants to merge 6 commits into from

Conversation

iRoachie
Copy link
Contributor

@iRoachie iRoachie commented May 24, 2021

Last outstanding authorizer for http apis.

resolves #10534

Not totally sure if we should include the bit inside integ.iam.signature but I added it as a way to test the integ stack.


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

Last outstanding authorizer for http apis.

resolves #10534
@gitpod-io
Copy link

gitpod-io bot commented May 24, 2021

@iRoachie
Copy link
Contributor Author

cc @nija-at How should I go forward with the testing bit? The build fails right now because of the linting done on all package.json files

@peterwoodworth peterwoodworth added the @aws-cdk/aws-apigatewayv2-authorizers Related to aws-apigatewayv2-authorizers package label Jun 3, 2021
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.

See my comment below.

@@ -192,3 +193,25 @@ api.addRoutes({
authorizer,
});
```

## IAM Authorizers
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @iRoachie -

This does not feel like the right approach to solving IAM authorizers. Just having an HttpIamAuthorizer class to set the AuthorizationType and then leaving it up to the user to correctly configure the IAM policy feels sub-optimal.

I would suggest going with an alternate approach. Here's my proposal written in the form of what the user experience will be -

const api = new HttpApi(stack, 'HttpApi');
const routes: Route[] = api.addRoutes({ ... });
routes[0].grantInvoke(principal, { // principal is of type iam.IPrincipal
  httpMethod: 'GET'
});

This should, under the hood, automatically set the AuthorizationType to AWS_IAM and attach the relevant IAM permissions to the principal.

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 was going based on usage and what was requested in #10534.

As far as I understand the use case for IAM Authorization is to set policies on individual users to grant access to all/some routes.

I see where you are going with your suggestion however I think this can be an add-on to what is currently in this PR. If we only change the authorizationType when grantInvoke is called on a route then we automatically rule out scenarios where user permissions are managed outside of cdk/cloudformation (which is pretty common for physical users).

So just to be clear they're two things here:

  1. Setting route to use IAM Authorization
  2. Allowing an easy way to assign the relevant policies to a user/principal so that it can invoke that route.

@mergify mergify bot dismissed nija-at’s stale review July 7, 2021 05:46

Pull request has been modified.

@nija-at nija-at added effort/small Small work item – less than a day of effort p2 labels Jul 15, 2021
@nija-at nija-at removed their assignment Jul 15, 2021
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@misterjoshua
Copy link
Contributor

Hey all. What's needed to move this PR forward?

@iRoachie
Copy link
Contributor Author

@misterjoshua i’m a bit unsure how to go through with integration tests. I added an example which generates an AWS signature to attach to the request to simulate a real environment.

this fails the lint because of the additional package.json file.

@misterjoshua
Copy link
Contributor

@misterjoshua i’m a bit unsure how to go through with integration tests. I added an example which generates an AWS signature to attach to the request to simulate a real environment.

this fails the lint because of the additional package.json file.

@iRoachie I have taken a crack at fixing the integration tests. My branch is here: https://github.com/misterjoshua/aws-cdk/tree/ft/http-api-iam-authorizer

Some notes:

  • I used python instead of node so that I could work around that package.json complaint.
  • Instead of running the signing locally, I ran it in lambdas.
  • I also switched the unit tests over to use @aws-cdk/assertions

Let me know your thoughts.

mergify bot pushed a commit that referenced this pull request Dec 17, 2021
Fixes #15123

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

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@iRoachie
Copy link
Contributor Author

@misterjoshua Awesome job. Apologize I didn't have the time.

@iRoachie iRoachie closed this Dec 21, 2021
@misterjoshua
Copy link
Contributor

@misterjoshua Awesome job. Apologize I didn't have the time.

No worries. Thanks for getting the boulder rolling!

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-authorizers Related to aws-apigatewayv2-authorizers package effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[@aws-cdk/aws-apigatewayv2] support authorizers for HTTP API
5 participants