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: Add a built-in AWS_IAM authorizer for HTTP APIs #1924

Merged

Conversation

harrisonhjones
Copy link
Contributor

@harrisonhjones harrisonhjones commented Feb 11, 2021

Issue #, if available: #1731

Description of changes:

This is another approach for adding AWS_IAM support to HTTP APIs created by SAM. Other approaches can be found in #1876 and #1878. This approach is based on a conversation with @jfuss.

In this version a new property is added to the Serverless HTTP API object called EnableIamAuthorizer. When set to true an AWS_IAM authorizer is automatically added to the HTTP API's (internal) list of authorizers under the key "AWS_IAM". This authorizer is setup to us IAM auth. If enabled (It is not enabled by default) it can be set as the HTTP API's default authorizer and set as the authorizer for any Serverless functions that use the API.

Description of how you validated changes:

I created the following test template:

AWSTemplateFormatVersion: "2010-09-09"
Description: AWS SAM template with a simple API definition
Transform: AWS::Serverless-2016-10-31
Globals:
  HttpApi:
    Auth:
      EnableIamAuthorizer: true
Resources:
  #######
  # Serverless functions that use the automatically-created AWS::Serverless::HttpApi called "ServerlessHttpApi".
  #######
  # Should have no auth set because there is no default authorizer defined on the API.
  HttpApiFunctionDefaultApiDefaultAuth:
    Type: AWS::Serverless::Function
    Properties:
      Events:
        ApiEvent:
          Type: HttpApi
          Properties:
            Path: /default-api/default-auth
            Method: GET
      Handler: index.handler
      InlineCode:
        "def handler(event, context):\n    return {'body':
        'HttpApiFunctionDefaultApiDefaultAuth', 'statusCode': 200}\n"
      Runtime: python3.8
  # Should have IAM auth set because the IAM authorizer is enabled (see Globals) and set.
  HttpApiFunctionDefaultApiIAMAuth:
    Type: AWS::Serverless::Function
    Properties:
      Events:
        ApiEvent:
          Type: HttpApi
          Properties:
            Path: /default-api/iam-auth
            Method: GET
            Auth:
              Authorizer: AWS_IAM
      Handler: index.handler
      InlineCode:
        "def handler(event, context):\n    return {'body':
        'HttpApiFunctionDefaultApiIAMAuth', 'statusCode': 200}\n"
      Runtime: python3.8
  #######
  # Serverless functions that use a manually-created AWS::Serverless::HttpApi with the IAM authorizer enable and the default auth set to the IAM authorizer.
  #######
  # Should have IAM auth set because the default authorizer is the IAM authorizer.
  HttpApiFunctionCustomApiWithDefaultIamAuthDefaultAuth:
    Type: AWS::Serverless::Function
    Properties:
      Events:
        ApiEvent:
          Type: HttpApi
          Properties:
            Path: /custom-api-with-default-iam-auth/default-auth
            Method: GET
            ApiId: !Ref CustomServerlessHttpApiWithDefaultIamAuth
      Handler: index.handler
      InlineCode:
        "def handler(event, context):\n    return {'body':
        'HttpApiFunctionCustomApiWithDefaultIamAuthDefaultAuth', 'statusCode':
        200}\n"
      Runtime: python3.8
  # Should have no auth (override the default authorizer).
  HttpApiFunctionCustomApiWithDefaultIamAuthNoneAuth:
    Type: AWS::Serverless::Function
    Properties:
      Events:
        ApiEvent:
          Type: HttpApi
          Properties:
            Path: /custom-api-with-default-iam-auth/none-auth
            Method: GET
            ApiId: !Ref CustomServerlessHttpApiWithDefaultIamAuth
            Auth:
              Authorizer: NONE
      Handler: index.handler
      InlineCode:
        "def handler(event, context):\n    return {'body':
        'HttpApiFunctionCustomApiWithDefaultIamAuthNoneAuth', 'statusCode':
        200}\n"
      Runtime: python3.8
  # Should have IAM auth set because the authorizer is set to the IAM authorizer.
  HttpApiFunctionCustomApiWithDefaultIamAuthIamAuth:
    Type: AWS::Serverless::Function
    Properties:
      Events:
        ApiEvent:
          Type: HttpApi
          Properties:
            Path: /custom-api-with-default-iam-auth/iam-auth
            Method: GET
            ApiId: !Ref CustomServerlessHttpApiWithDefaultIamAuth
            Auth:
              Authorizer: AWS_IAM
      Handler: index.handler
      InlineCode:
        "def handler(event, context):\n    return {'body':
        'HttpApiFunctionCustomApiWithDefaultIamAuthIamAuth', 'statusCode':
        200}\n"
      Runtime: python3.8
  # Serverless Http Api with an IAM authorizer (default) and a None authorizer.
  CustomServerlessHttpApiWithDefaultIamAuth:
    Type: AWS::Serverless::HttpApi
    Properties:
      FailOnWarnings: true
      Auth:
        EnableIamAuthorizer: true
        DefaultAuthorizer: AWS_IAM

  #######
  # Serverless functions that use a manually-created AWS::Serverless::HttpApi with the IAm authorizer enabled but the default auth unset.
  #######
  # Should have no auth set because the default authorizer is not set.
  HttpApiFunctionCustomApiWithNoDefaultAuthDefaultAuth:
    Type: AWS::Serverless::Function
    Properties:
      Events:
        ApiEvent:
          Type: HttpApi
          Properties:
            Path: /custom-api-with-no-default-auth/default-auth
            Method: GET
            ApiId: !Ref CustomServerlessHttpApiWithNoDefault
      Handler: index.handler
      InlineCode:
        "def handler(event, context):\n    return {'body':
        'HttpApiFunctionCustomApiWithNoDefaultAuthDefaultAuth', 'statusCode':
        200}\n"
      Runtime: python3.8
  # Should have IAM auth set because the authorizer is set to the IAM authorizer.
  HttpApiFunctionCustomApiWithNoDefaultIamAuth:
    Type: AWS::Serverless::Function
    Properties:
      Events:
        ApiEvent:
          Type: HttpApi
          Properties:
            Path: /custom-api-with-no-default-auth/iam-auth
            Method: GET
            ApiId: !Ref CustomServerlessHttpApiWithNoDefault
            Auth:
              Authorizer: AWS_IAM
      Handler: index.handler
      InlineCode:
        "def handler(event, context):\n    return {'body':
        'HttpApiFunctionCustomApiWithNoDefaultIamAuth', 'statusCode': 200}\n"
      Runtime: python3.8
  # Serverless Http Api with the IAM authorizer enabled but no default authorizer set.
  CustomServerlessHttpApiWithNoDefault:
    Type: AWS::Serverless::HttpApi
    Properties:
      FailOnWarnings: true
      Auth:
        EnableIamAuthorizer: true

I then compiled it using:

python ./bin/sam-translate.py --template-file=../project/template.yml
I then manually deployed it using CloudFormation in my personal AWS account.

Once deployed I validated all the routes had the expected auth type:

Default API

  1. https://ptwccx1qv4.execute-api.us-east-1.amazonaws.com/default-api/default-auth - has no auth
  2. https://ptwccx1qv4.execute-api.us-east-1.amazonaws.com/default-api/iam-auth - has IAM auth

Custom API with no default auth

  1. https://warmewfp00.execute-api.us-east-1.amazonaws.com/custom-api-with-no-default-auth/default-auth - has no auth
  2. https://warmewfp00.execute-api.us-east-1.amazonaws.com/custom-api-with-no-default-auth/iam-auth - has IAM auth

Custom API with default IAM auth

  1. https://fj5vzsu863.execute-api.us-east-1.amazonaws.com/custom-api-with-default-iam-auth/default-auth - has IAM auth
  2. https://fj5vzsu863.execute-api.us-east-1.amazonaws.com/custom-api-with-default-iam-auth/iam-auth - has IAM auth
  3. https://fj5vzsu863.execute-api.us-east-1.amazonaws.com/custom-api-with-default-iam-auth/none-auth - has no auth

Checklist:

  • Write/update tests
  • make pr passes
  • Update documentation
  • Verify transformed template deploys and application functions as expected

Examples?

Please reach out in the comments, if you want to add an example. Examples will be
added to sam init through https://github.com/awslabs/aws-sam-cli-app-templates/

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

@harrisonhjones harrisonhjones changed the title Add a built-in- AWS_IAM authorizer for HTTP APIs Add a built-in AWS_IAM authorizer for HTTP APIs Feb 11, 2021
@jfuss
Copy link
Contributor

jfuss commented Feb 17, 2021

@harrisonhjones I think this is probably the best option from the 3 you have proposed. I didn't do a deep review of the code, but I think the setup you have is good. I am having another member of my team (@hawflau) look at this as well, just so we have some agreement on the approach.

@hawflau
Copy link
Contributor

hawflau commented Feb 17, 2021

@harrisonhjones This option looks good to me.
I compared with the other two options you provided, but it seems they require altering things that customers might currently rely on. We should avoid this.
The code implementation in this option also looks fine. We just probably need more tests. And when we roll this out, we need docs and examples to let customers know how to use IAM auth

@harrisonhjones
Copy link
Contributor Author

@harrisonhjones This option looks good to me.

👍

I compared with the other two options you provided, but it seems they require altering things that customers might currently rely on. We should avoid this.

I agree.

The code implementation in this option also looks fine. We just probably need more tests.

Got it. Will add tests.

And when we roll this out, we need docs and examples to let customers know how to use IAM auth

Where do I add that? In this package or elsewhere? I see an "Update Documentation" checkbox but after a quick scan of the repo I don't see how documentation is supposed to be updated.

@harrisonhjones
Copy link
Contributor Author

2/18/2021 Update:

  • ➕ Added tests
  • make pr passes
  • ✅ integration test: pytest --no-cov integration/single/test_function_with_http_api_and_auth.py passes
  • ✅existing integration test: pytest --no-cov integration/single/test_basic_http_api.py passes
  • ⚠️ confirmed I don't like writing in Python.

@codecov-io
Copy link

codecov-io commented Feb 19, 2021

Codecov Report

Merging #1924 (8accf8a) into develop (16aefe7) will decrease coverage by 0.03%.
The diff coverage is 87.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1924      +/-   ##
===========================================
- Coverage    93.86%   93.82%   -0.04%     
===========================================
  Files           89       89              
  Lines         5900     5913      +13     
  Branches      1208     1214       +6     
===========================================
+ Hits          5538     5548      +10     
- Misses         166      168       +2     
- Partials       196      197       +1     
Impacted Files Coverage Δ
samtranslator/model/eventsources/push.py 90.54% <70.00%> (-0.47%) ⬇️
samtranslator/model/api/http_api_generator.py 91.30% <100.00%> (+0.11%) ⬆️
samtranslator/model/apigatewayv2.py 95.16% <100.00%> (+0.20%) ⬆️
samtranslator/open_api/open_api.py 92.64% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16aefe7...8accf8a. Read the comment docs.

samtranslator/model/api/http_api_generator.py Show resolved Hide resolved
samtranslator/model/api/http_api_generator.py Outdated Show resolved Hide resolved
samtranslator/model/eventsources/push.py Outdated Show resolved Hide resolved
tests/model/api/test_http_api_generator.py Show resolved Hide resolved
@jfuss
Copy link
Contributor

jfuss commented Feb 25, 2021

@hawflau or @mgrandis Could one of take a look through this as well?

@jfuss
Copy link
Contributor

jfuss commented Feb 25, 2021

@harrisonhjones

Where do I add that? In this package or elsewhere? I see an "Update Documentation" checkbox but after a quick scan of the repo I don't see how documentation is supposed to be updated.

I would need to double check, but I believe we generate docs based on https://github.com/aws/serverless-application-model/blob/develop/versions/2016-10-31.md. I will handle the communication with our doc writer about getting these things added, but updating that file will help in our process.

@mgrandis
Copy link
Contributor

Sorry about the late review @harrisonhjones!

I have mixed feelings about adding a new property EnableIamAuthorizer to Auth rather than a new value for Authorizers.

Couldn't we add a new IamAuthorizer with that EnableIamAuthorizer property in it instead?
It would be future proof if we were to add a new option/property to configure the IamAuthorizer in the future.

@jfuss
Copy link
Contributor

jfuss commented Mar 30, 2021

@mgrandis The main reason @harrisonhjones and I landed on this design was because the IAM Auth is a static configuration. Forcing a customer to specify Authorizers seemed like unnecessary set. Authorizers is also a set a different objects, which can be confusing. So we elected to push this up to an Enable Property under Auth. We played with just setting the DefaultAuthorizer: AWS_IAM or Auth.Authorizer: AWS_IAM but forcing an Property to set what Authorizers are available made checking for Auth in other tools (like SAM CLI or CFN Lint) easier and more explicit.

Do you still have concerns in this approach?

Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

The looks good to me.

@mgrandis
Copy link
Contributor

All right, let's move on with this approach.

@jfuss jfuss marked this pull request as ready for review April 8, 2021 18:18
@AndersAsa
Copy link

Any timeline for this feature being merged?

@mgrandis mgrandis changed the title Add a built-in AWS_IAM authorizer for HTTP APIs feat: Add a built-in AWS_IAM authorizer for HTTP APIs Jul 27, 2021
@mgrandis
Copy link
Contributor

@harrisonhjones It looks like there are only a few minor updates to do, do you have time to work on them?
Thanks!

@wolfeidau
Copy link

@mgrandis looks like the author isn't able to address this feedback, can we merge this and follow up with another PR?

I would love to see this merged as it has been an issue for a while.

@CrayTexas2018
Copy link

Any timeline on when this PR will be merged?

@harrisonhjones
Copy link
Contributor Author

@mgrandis looks like the author isn't able to address this feedback, can we merge this and follow up with another PR?

I would love to see this merged as it has been an issue for a while.

I can try to take a look at fixing this next week. Sorry for the delay getting back to y'all.

@AndersAsa
Copy link

@mgrandis looks like the author isn't able to address this feedback, can we merge this and follow up with another PR?
I would love to see this merged as it has been an issue for a while.

I can try to take a look at fixing this next week. Sorry for the delay getting back to y'all.

Thank you!!!! This will be super helpful for me

@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2022

Codecov Report

Merging #1924 (6eb4ae9) into develop (e7a1496) will increase coverage by 0.91%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1924      +/-   ##
===========================================
+ Coverage    93.58%   94.50%   +0.91%     
===========================================
  Files           90       97       +7     
  Lines         6124     7073     +949     
  Branches      1260     1441     +181     
===========================================
+ Hits          5731     6684     +953     
+ Misses         183      177       -6     
- Partials       210      212       +2     
Impacted Files Coverage Δ
samtranslator/__init__.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/dialup.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/feature_toggle.py 100.00% <100.00%> (+12.16%) ⬆️
samtranslator/intrinsics/actions.py 98.78% <100.00%> (+0.06%) ⬆️
samtranslator/intrinsics/resource_refs.py 95.83% <100.00%> (-0.17%) ⬇️
samtranslator/metrics/method_decorator.py 100.00% <100.00%> (ø)
samtranslator/region_configuration.py 77.77% <0.00%> (-22.23%) ⬇️
samtranslator/validator/validator.py 91.80% <0.00%> (-3.85%) ⬇️
samtranslator/model/exceptions.py 97.67% <0.00%> (-2.33%) ⬇️
samtranslator/model/s3_utils/uri_parser.py 68.42% <0.00%> (-0.81%) ⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c575b54...6eb4ae9. Read the comment docs.

@CrayTexas2018
Copy link

Thank you! Hopefully this gets merged! Really appreciate the effort.

Copy link
Contributor Author

@harrisonhjones harrisonhjones left a comment

Choose a reason for hiding this comment

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

Three small comments; two for more experienced Python folks.

samtranslator/model/api/http_api_generator.py Outdated Show resolved Hide resolved
samtranslator/model/api/http_api_generator.py Outdated Show resolved Hide resolved
tests/model/api/test_http_api_generator.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

Just a couple small things.

samtranslator/model/eventsources/push.py Outdated Show resolved Hide resolved
@harrisonhjones
Copy link
Contributor Author

harrisonhjones commented Feb 1, 2022

Addressed PR feedback; added five new tests, removed an unnecessary error test, and updated and existing multi-auth test to include the new iam authorizer.

make pr - passes

Copy link
Contributor

@CoshUS CoshUS left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! One small question and everything else looks good to me.

samtranslator/model/apigatewayv2.py Show resolved Hide resolved
samtranslator/model/api/http_api_generator.py Show resolved Hide resolved
Copy link
Contributor

@CoshUS CoshUS left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for addressing the comments!

@harrisonhjones
Copy link
Contributor Author

harrisonhjones commented Feb 15, 2022

@CoshUS you are welcome :) What are the next steps to getting this merged? Anything I need to do? It looks like a few tests are failing now that you merged in cb5ded2

- Added tests
- Added test-fast target
- Updated documentation
@harrisonhjones harrisonhjones force-pushed the feature-iam-authorizer-http-api-redux-2 branch from 56e8bd2 to 8e83fdb Compare February 15, 2022 17:49
@harrisonhjones
Copy link
Contributor Author

Rebased off of develop, fixed merge conflicts, updated new util function that was breaking tests, squashed into a single commit, ran make pr successfully, and force-pushed.

@CoshUS CoshUS requested a review from jfuss February 16, 2022 17:02
"components": {
"securitySchemes": {
"AWS_IAM": {
"type": "oauth2",
Copy link
Contributor

Choose a reason for hiding this comment

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

IAM auth is oauth2 type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the description in tests/translator/input/http_api_custom_iam_auth.yaml:

# This test-case tests what happens when an AWS_IAM authorizer is defined on an HttpApi but not enabled anywhere else.
# While the defined authorizer isn't really a true Iam authorizer (it's just a poorly-named OAuth authorizer) this shouldn't cause an error.

@jfuss
Copy link
Contributor

jfuss commented Mar 3, 2022

There are some things we need to handle internally (docs for example). I need to kick off that processes before merging.

@ksd-mx
Copy link

ksd-mx commented Mar 11, 2022

great work guys this is fantastic. any estimates as to when it's merged/released? I have a bit of a life/death situation here that would greatly benefit from this one

@brysontyrrell
Copy link
Contributor

I was just talking to my TAM about this. 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet