Skip to content
New issue

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

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

Already on GitHub? # to your account

(appsync): Lambda authorizer permission is not scoped to appsync api arn #31550

Closed
1 task
vruffer opened this issue Sep 25, 2024 · 6 comments · Fixed by #31567 or softwaremill/tapir#4137 · May be fixed by NOUIY/aws-solutions-constructs#135 or NOUIY/aws-solutions-constructs#136
Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. p1

Comments

@vruffer
Copy link

vruffer commented Sep 25, 2024

Describe the bug

When using a lambda authorizer with a GraphqlAPI, the cdk automatically creates the AWS::Lambda::Permission required for the AppSync API to invoke the lambda authorizer. It does not however add a SourceArn.

This conflicts with the control tower policy [CT.LAMBDA.PR.2], and it is in general good practice to scope permissions.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

I expected that the AWS::Lambda::Permission was scoped to only allow the "parent" AppSync API to invoke the authorizer.

Current Behavior

The CDK outputs a AWS:Lambda::Permission without a SourceArn property.

Reproduction Steps

  1. Add a graphql api
  2. Add a lambda function
  3. Set defaultAuthorization (or additionalAuthorizationTypes it doesn't matter) to aws_appsync.AuthorizationType.LAMBDA and point to the created lambda function
  4. Inspect the created AWS::Lambda::Permission and notice that SourceArn is missing.
const authorizer = new aws_lambda_nodejs.NodejsFunction(
  this,
  'authorizerLambda',
  {},
);

new aws_appsync.GraphqlApi(this, 'api', {
  name: 'api',
  authorizationConfig: {
    defaultAuthorization: {
      authorizationType: aws_appsync.AuthorizationType.LAMBDA,
      lambdaAuthorizerConfig: {
        handler: authorizer,
      },
    },
  },
});

Possible Solution

Inspecting the aws_appsync package reveals that appsync adds this permission when discovering an authorizationType === AuthorizationType.LAMBDA.

The fix should be as simple as adding sourceArn: this.arn, but I don't have the time currently to setup local development and create a pull request.

Additional Information/Context

A workaround for now is to modify the permission with an Aspect:

export class AppsyncAuthorizerFixer implements IAspect {
  appsyncArn: string;
  constructor(appsyncArn: string) {
    this.appsyncArn = appsyncArn;
  }

  public visit(node: IConstruct): void {
    if (
      node instanceof aws_lambda.CfnPermission &&
      node.principal === 'appsync.amazonaws.com' &&
      !node.sourceArn
    ) {
      // The metadata is the only non-resolved value I could find that
      // contains information about which lambda function the permission is tied to
      const cdkPath = node.getMetadata('aws:cdk:path') as unknown;
      if (!cdkPath) {
        Annotations.of(node).addError('Permission metadata not present');
        return;
      }
      if (
        cdkPath.includes(`${idOfLambdaAuthorizer}/${idOfAppsyncApi}-appsync`)
      ) {
        console.log(
          `Found 'AWS::Lambda::Permission' for appsync authorizer with principal = 'appsync.amazonaws.com' and no sourceArn. Adding appsync arn...`,
          cdkPath,
        );

        node.addPropertyOverride('SourceArn', this.appsyncArn);
      }
    }
  }
}

CDK CLI Version

2.159.1 (build c66f4e3)

Framework Version

No response

Node.js Version

v20.13.1

OS

MacOS 14.6.1 (Sonoma)

Language

TypeScript

Language Version

Typescript (5.6.2)

Other information

No response

@vruffer vruffer added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 25, 2024
@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Sep 25, 2024
@pahud
Copy link
Contributor

pahud commented Sep 25, 2024

Agree. When addPermission(), it is always a best practice to specify sourceArn top scope the permission.

config?.handler.addPermission(`${id}-appsync`, {
principal: new ServicePrincipal('appsync.amazonaws.com'),
action: 'lambda:InvokeFunction',

Making it a p1.

@pahud pahud added p1 and removed needs-triage This issue or PR still needs to be triaged. labels Sep 25, 2024
@pahud
Copy link
Contributor

pahud commented Sep 25, 2024

Bump to p0 now. We are confirming some details before the fix.

@pahud pahud added p0 and removed p1 labels Sep 25, 2024
@QuantumNeuralCoder QuantumNeuralCoder added p1 and removed p0 labels Sep 25, 2024
@QuantumNeuralCoder
Copy link
Contributor

Reducing the severity. We will incorporate the best practice as a fix for this issue. However current assessment does not make it clear that it is a security risk.

Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

1 similar comment
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2024
@paulhcsun
Copy link
Contributor

Fix is behind a feature flag: APPSYNC_GRAPHQLAPI_SCOPE_LAMBDA_FUNCTION_PERMISSION

{
  "context": {
    "@aws-cdk/aws-ec2:appSyncGraphQLAPIScopeLambdaPermission": true
  }
}

Please note that this fix adds a sourceArn which will cause a REPLACEMENT of your AWS::Lambda::Permissionresource and not an Update.

# for free to subscribe to this conversation on GitHub. Already have an account? #.