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

AWS_IAM auth does not allow InvokeRole override #923

Closed
theburningmonk opened this issue May 16, 2019 · 22 comments
Closed

AWS_IAM auth does not allow InvokeRole override #923

theburningmonk opened this issue May 16, 2019 · 22 comments

Comments

@theburningmonk
Copy link

Description:

When using the new AWS_IAM auth type, the InvokeRole is always set to CALLER_CREDENTIALS even when I specify an override. The problem here is that, it forces the caller to have both API Gateway's invoke permission as well as lambda:InvokeFunction permission. This breaks the API abstraction and leaks implementation details (that there's a Lambda behind API Gateway, and the name of the function).

Steps to reproduce the issue:

  1. create API with auth type set to AWS_IAM and set InvokeRole to null

image

Observed result:

API endpoints still uses CALLER_CREDENTIALS

image

Expected result:

  • Execution role to be `null
  • Invoke with caller credentials to be disabled
@theburningmonk
Copy link
Author

For now, our workaround is to have a custom macro that is triggered after the initial SAM macro and strips the credentials field.

image

@tavolate
Copy link

We have the same issue

@keetonian
Copy link
Contributor

It looks like we're forcing InvokeRole to be non-null here: https://github.com/awslabs/serverless-application-model/blob/develop/samtranslator/swagger/swagger.py#L192

A simple fix for this would be to allow null or blank values and add a test that enforces this behavior.

@jadhavmanoj
Copy link
Contributor

@keetonian if InvokeRole in None. should we remove credentials key from template?

@keetonian
Copy link
Contributor

That's a good question. @brettstack might know, otherwise I'll investigate more and see what SAM should do in this case.

@brettstack
Copy link
Contributor

@jadhavmanoj that's correct.

@benkehoe
Copy link

benkehoe commented May 28, 2019

Is the plan to change the default to not use caller credentials? I am in favor of such a plan. Using caller credentials isn't the default in API Gateway, so I would consider it unexpected behavior for SAM to change that.

@brettstack
Copy link
Contributor

Unfortunately that ship has sailed. Changing default behavior would be a breaking change. The plan is to add InvokeRole: null (or 'NONE') as originally intended.

@benkehoe
Copy link

Perhaps an additional auth type is warranted, then? AWS_IAM_v2? Requiring this opaque incantation everywhere to get the normal behavior of API Gateway, and that forgetting it will cause IAM failures that are already hard to understand, is going to trip up so many people.

@keetonian
Copy link
Contributor

@benkehoe I submitted a PR for this issue following what was discussed. If AWS_IAM is set as a default authorizer and InvokeRole: null or InvokeRole: NONE is set, it will remove the credentials section of the output template. Do you think that will be sufficient?

@benkehoe
Copy link

benkehoe commented Jun 19, 2019

It makes the desired behavior possible, sure. But it's also setting us up for a lot of new users asking about IAM errors they don't understand and getting told to use this trick, and then copy-and-pasting that for the rest of their days without understanding it. This goes against the mission of SAM, which is to make CloudFormation simpler and more understandable, with better defaults.

If I'm missing something, if there's a reason users should want to use caller credentials from API GW to Lambda by default, or why we should guide them in that direction with this default, I'm all ears.

@jlhood
Copy link
Contributor

jlhood commented Jul 2, 2019

@benkehoe As @brettstack said in his previous comment, changing the default value would be a backwards-incompatible change, unfortunately. So this is the best fix we can do without versioning the SAM transform.

@benkehoe
Copy link

benkehoe commented Jul 2, 2019

You don't have to rev the SAM transform, you could just add an additional authorizer with different behavior (Authorizer: AWS_IAM_v2). AWS IoT did this after they realized their initial implementation of a rule action for DynamoDB wasn't right for their users, they just added another action: https://docs.aws.amazon.com/iot/latest/developerguide/iot-rule-actions.html#dynamodb-v2-rule

@praneetap praneetap added stage/waiting-for-release and removed contributors/good-first-issue Good first issue for a contributor labels Jul 26, 2019
@praneetap
Copy link
Contributor

Pending release v1.14.0

@kosmakoff
Copy link

Sorry for posting to inappropriate thread, but I have a question. Since this branch is already merged, and I see that it has a release 1.14.0 tag on it, does it mean it should already work on AWS, or does this change only apply to sam cli? Sorry for confusing terms, but I have the same exact problem with InvokeRole, and I am using dotnet lambda for development. Please advise.

@keetonian
Copy link
Contributor

We will post on and close this issue when this feature is released. It will be released when 1.14.0 is released; we have cut the release branch and are working on releasing it inside of AWS but it is not yet available.

@ashier
Copy link

ashier commented Aug 20, 2019

Would there be a timeline when are we expecting the 1.14.0 version? Or are there any workarounds (without manually unchecking the box) that we can do while we wait for this to be released?

@praneetap
Copy link
Contributor

@ashier We're working on making our release process more transparent to external customers, but we're not quite where we want to be yet. For now, you can see when we cut release branches, which can give you some indication of when we begin working on a specific release.

I don't know of a workaround to this issue, unfortunately.

@jlhood
Copy link
Contributor

jlhood commented Aug 21, 2019

Had a conversation with @benkehoe today where he reiterated his above comment (#923 (comment)) that we could update the default behavior in a backwards-compatible way by adding a AWS_IAM_v2 authorizer. Commenting to make sure we discuss this at our next PR/Issue review meeting.

@caspian154
Copy link

@ashier there are a few workarounds, but both can be pretty tedious. The first would be a macro (discussed in this article - https://medium.com/hackernoon/aws-sam-cloudformation-macros-a-patch-made-in-heaven-da792bd84508).

The second method - I added as a comment to that article would be to use swagger with api-gateway integrations. The following seems to work

      x-amazon-apigateway-integration:
        credentials: ""

@ashier
Copy link

ashier commented Aug 28, 2019

@ashier there are a few workarounds, but both can be pretty tedious. The first would be a macro (discussed in this article - https://medium.com/hackernoon/aws-sam-cloudformation-macros-a-patch-made-in-heaven-da792bd84508).

The second method - I added as a comment to that article would be to use swagger with api-gateway integrations. The following seems to work

      x-amazon-apigateway-integration:
        credentials: ""

Thank you @caspian154. I'll look it up.

@keetonian
Copy link
Contributor

@ashier @caspian154 this is now released, you can now set InvokeRole: NONE.

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

No branches or pull requests