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

Add support for referenced custom authorizer lambdas #102

Merged
merged 6 commits into from
Mar 10, 2018

Conversation

aleksdikanski
Copy link

@aleksdikanski aleksdikanski commented Feb 13, 2018

Proposed solution for #101

After digging a bit I found this bit in apiGateway.js

const funcIndex = _.findIndex(uriParts, part => _.has(part, 'Fn::GetAtt'));

which selects the index in the AuthorizerUri. Fn::Join array based on finding the GetAtt , ARN of a referenced lambda function. This won't work if the function is directly referenced using its ARN. In this case the uriPart of the AuthorizerUri will simply be a string with the ARN and funcIndex will be set to -1. Hence ${stageVariables.SERVERLESS_ALIAS} will be added to the head of the array.

I would suggest to add a simple check for strings, e.g.

const funcIndex = _.findIndex(uriParts, part =>
      _.has(part, 'Fn::GetAtt') || _.startsWith(part, 'arn:aws:lambda'));

Concerning the missing FunctionName I would simple use the original one, in case no other aliasName can be found, e.g instead of

permission.Properties.FunctionName = { Ref: aliasName };

use the original

permission.Properties.FunctionName = aliasName ? { Ref: aliasName } : permission.Properties.FunctionName;

Background is, that the function name of the referenced custom authorizer can't generally be extended by the alias, as it lives in a different project and might be deployed with a different alias (or no alias at all)

Concerning the null values of the Permission: in this case, there should be no dependencies as the custom authorizer lambda should already be deployed by another project.

permission.DependsOn = _.compact([ versionName, aliasName ]);

But this might lead to some unwanted behavior, as you mentioned in #83, and needs some testing

@@ -267,7 +268,7 @@ module.exports = function(currentTemplate, aliasStackTemplates, currentAliasStac
const aliasName = _.find(_.keys(aliases), alias => _.startsWith(alias, functionName));

// Adjust references and alias permissions
permission.Properties.FunctionName = { Ref: aliasName };
permission.Properties.FunctionName = aliasName ? { Ref: aliasName } : permission.Properties.FunctionName;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this change is not an appropriate fix. It will affect all permission resources and unconditionally use any existing function name in case something is wrong with the alias.

Using the original function name must only be done if we are really sure that the target is an external function, and not a function deployed by the current service. If that happens for a local function this leads to untraceable errors and lets the permissions be attached to $LATEST instead of the alias silently.

We should first check if the permission targets a service external function and handle that in a separate code branch. This allows for handling configuration errors differently for internal functions and external ones instead of mixing them and swallowing errors.

The _.compact() change below is also affected. It must only be in place for external functions, not internal ones. If permissions that reference internal functions are not dependent on an alias, there is a problem with the service.

@@ -287,7 +288,7 @@ module.exports = function(currentTemplate, aliasStackTemplates, currentAliasStac
}

// Add dependency on function version
permission.DependsOn = [ versionName, aliasName ];
permission.DependsOn = _.compact([ versionName, aliasName ]);
Copy link
Member

Choose a reason for hiding this comment

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

See my comment above.

Copy link
Author

@aleksdikanski aleksdikanski left a comment

Choose a reason for hiding this comment

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

The change was fixing the problem I had. I didn't know all the consequences associated. I made some change to the PR based on the assumption that an externally referenced function of a Permission will have the arn string value instead of a GetAttr object.

@HyperBrain
Copy link
Member

Great! I'll have a look tomorrow.

@HyperBrain HyperBrain mentioned this pull request Feb 19, 2018
@alexb-uk
Copy link

alexb-uk commented Mar 8, 2018

I've just started trying to add serverless-aws-alias to my project with the hopeful goal of using it for API versioning.

However I also came across the linked issue:

Serverless Error ---------------------------------------

An error occurred: AuthApiGatewayAuthorizerdev - Invalid Authorizer URI: :${stageVariables.SERVERLESS_ALIAS}arn:aws:apigateway:eu-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:eu-west-2:xxxxx:function:auth0-authorizer-dev-auth/invocations. Authorizer URI should be a valid API Gateway ARN that represents a Lambda function invocation..

If helpful, and when ready, I can help test this fix on a real deployment?

Thanks

@HyperBrain
Copy link
Member

Hi @alexb-uk,

it would be great if you can give this PR a try and report, if it solves the problem properly.
Use "serverless-aws-alias": "github:aleksdikanski/serverless-aws-alias#master" in your package.json.

@alexb-uk
Copy link

alexb-uk commented Mar 10, 2018

Hi @HyperBrain,

I performed the following and all went very smoothly:

For info here is a snippet from my serverless.yml:

functions:
  api:
    handler: index.handler
    onError: arn:aws:sns:eu-west-2:xxxxxxxxxx:sysops-admins
    events:
      - http:
          path: "{proxy+}"
          method: any
          cors: true
          authorizer: ${self:custom.authorizer.default}

plugins:
  - serverless-aws-alias
  - serverless-offline

custom:
  authorizer:
    default:
      arn: arn:aws:lambda:eu-west-2:xxxxxxxxxx:function:auth0-authorizer-dev-auth
      resultTtlInSeconds: 300
      identitySource: method.request.header.Authorization
      identityValidationExpression: ^Bearer [-0-9a-zA-Z\._]*$

Thanks a lot both for all your efforts.

Alex

@HyperBrain HyperBrain added this to the 1.7.0 milestone Mar 10, 2018
@HyperBrain
Copy link
Member

@alexb-uk Thanks for testing. I will merge this now, so that it is part of the next release.

@HyperBrain HyperBrain merged commit 09a9cfc into serverless-heaven:master Mar 10, 2018
@aleksdikanski
Copy link
Author

Thanks @alexb-uk for the testing effort

@HyperBrain HyperBrain modified the milestones: 1.7.0, 1.6.1 Mar 16, 2018
defnotrobbie pushed a commit to 1stdibs/serverless-aws-alias that referenced this pull request Nov 14, 2022
Add support for referenced custom authorizer lambdas
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants