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

Unable to add an SQS event source to a Lambda function with imported role #2381

Closed
robertd opened this issue Apr 26, 2019 · 8 comments · Fixed by #2806 or MechanicalRock/tech-radar#14 · May be fixed by MechanicalRock/cdk-constructs#5, MechanicalRock/cdk-constructs#6 or MechanicalRock/cdk-constructs#7
Labels
bug This issue is a bug.

Comments

@robertd
Copy link
Contributor

robertd commented Apr 26, 2019

Describe the bug
I'm unable to connect Lambda function to an SQS event source. Using 0.28.0 and 0.29.0 I'm getting an error message: Value sqs:ChangeMessageVisibilityBatch for parameter ActionName is invalid. Reason: Please refer to the appropriate WSDL for a list of valid actions.. Due to security limitations we have to provide our own pre-created lambda role. Using 0.27.0 everything is working as expected (w/ imported role). It seems that this might be a regression bug introduced in 0.28.0.

To Reproduce
Here is the stack code:

    const queue = new sqs.Queue(this, "Queue", {
      visibilityTimeoutSec: 60,
      retentionPeriodSec: 172800 // 2 days
    });

    // Import existing role
    const role = iam.Role.import(this, "LambdaProcessingRole", {
      roleArn: "arn:aws:iam::1234567890:role/lambda-processing-role"
    });

    const processQueueFn = new lambda.Function(this, "ProcessQueueFunction", {
      runtime: lambda.Runtime.Python36,
      code: lambda.Code.asset("lambda_functions/python/processor"),
      handler: "process_queue.lambda_handler",
      role: role,
      timeout: 60
    });

    processQueueFn.addEventSource(new eventSources.SqsEventSource(queue, {
      batchSize: 1
    }));

And here is the policy that's attached to lambda-processing-role.

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Action": [
                "sqs:Get*",
                "sqs:List*",
                "sqs:SendMessage",
                "sqs:SendMessageBatch",
                "sqs:ReceiveMessage",
                "sqs:SetQueueAttributes",
                "sqs:TagQueue",
                "sqs:UntagQueue",
                "sqs:CreateQueue",
                "sqs:PurgeQueue",
                "sqs:DeleteMessage",
                "sqs:DeleteMessageBatch",
                "sqs:DeleteQueue",
                "sqs:AddPermission",
                "sqs:RemovePermission",
                "sqs:ChangeMessageVisibility",
                "sqs:ChangeMessageVisibilityBatch"
            ],
            "Resource": "*",
            "Effect": "Allow",
            "Sid": "SQSPolicy"
        }
    ]
}

If I run cdk deploy command using 0.27.0 everything is working as expected. However, when I use 0.28.0 and 0.29.0 with the imported lambda role this is what I'm getting back:

IAM Statement Changes
┌───┬──────────────────────────┬────────┬────────────────────────────────────────────────────────────────┬────────────────────────────────────────────────────────────────┬───────────┐
│   │ Resource                 │ Effect │ Action                                                         │ Principal                                                      │ Condition │
├───┼──────────────────────────┼────────┼────────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────┼───────────┤
│ + │ ${Queue.Arn}             │ Allow  │ sqs:ChangeMessageVisibility                                    │ AWS:arn:aws:iam::123456789:role/lambda-processing-role         │           │
│   │                          │        │ sqs:ChangeMessageVisibilityBatch                               │                                                                │           │
│   │                          │        │ sqs:DeleteMessage                                              │                                                                │           │
│   │                          │        │ sqs:DeleteMessageBatch                                         │                                                                │           │
│   │                          │        │ sqs:GetQueueAttributes                                         │                                                                │           │
│   │                          │        │ sqs:GetQueueUrl                                                │                                                                │           │
│   │                          │        │ sqs:ReceiveMessage                                             │                                                                │           │
└───┴──────────────────────────┴────────┴────────────────────────────────────────────────────────────────┴────────────────────────────────────────────────────────────────┴───────────┘
(NOTE: There may be security-related changes not in this list. See http://bit.ly/cdk-2EhF7Np)

Do you wish to deploy these changes (y/n)? y
Stack: deploying...
Stack: creating CloudFormation changeset...
 0/3 | 5:08:12 PM | CREATE_IN_PROGRESS   | AWS::SQS::QueuePolicy           | Queue/Policy (QueuePolicyD47E3C93)
 1/3 | 5:08:13 PM | CREATE_FAILED        | AWS::SQS::QueuePolicy           | Queue/Policy (QueuePolicyD47E3C93) Value sqs:ChangeMessageVisibilityBatch for parameter ActionName is invalid. Reason: Please refer to the appropriate WSDL for a list of valid actions. (Service: AmazonSQS; Status Code: 400; Error Code: InvalidParameterValue; Request ID: 06f3148d-36d8-5eec-836d-41b176c59a7e)

Expected behavior
I should be able to attach Lambda function to SQS events.

Version:

  • OS: Mac 10.13.6 High Sierra
  • Node: 12.0.0
  • Programming Language: TypeScript
  • CDK Version: 0.28.0 and 0.29.0
@robertd robertd added the bug This issue is a bug. label Apr 26, 2019
@robertd robertd changed the title Not able to add event source to Lambda function with imported role Unable to add an SQS event source to a Lambda function with imported role Apr 26, 2019
@robertd
Copy link
Contributor Author

robertd commented May 27, 2019

I've tested this in 0.32.0 and it's still an issue.

  5/18 | 10:48:42 PM | CREATE_FAILED        | AWS::SQS::QueuePolicy           | ElevationMRFDeadLetterQueue/Policy (ElevationMRFDeadLetterQueuePolicyF20502D0) Value sqs:ChangeMessageVisibilityBatch for parameter ActionName is invalid. Reason: Please refer to the appropriate WSDL for a list of valid actions. (Service: AmazonSQS; Status Code: 400; Error Code: InvalidParameterValue; Request ID: 48d9ec52-c7ca-5dbd-ade3-b937a9a0461f)

@eladb
Copy link
Contributor

eladb commented May 27, 2019

Confirming this is a regression.

Minimal repro (v0.32.0):

const queue = new sqs.Queue(this, 'q');
const role = iam.Role.fromRoleArn(this, 'r', 'arn:aws:iam::585695036304:role/ReproRole2381');

queue.grantConsumeMessages(role);

I think we have a few issues here:

  1. Since the role is external to the stack, the permissions are added to the queue policy instead of attached to the role. I am wondering why this is the behavior for imported roles. We can technically define an iam.Policy and attach it to the imported role. I've tested this approach and it works and seems more robust. @rix0rrr any insight on why we prefer to use a resource policy instead of an attached policy for imported roles?

  2. I am not 100% sure why it is not possible to specify the sqs:ChangeMessageVisibilityBatch action in a queue policy. I filed an issue with the SQS team to inquire (internal reference: TT0193007159).

@robertd
Copy link
Contributor Author

robertd commented May 27, 2019

Thanks for confirming this is a regression @eladb. My current workaround is to comment out step where I’m attaching a lambda function to trigger on an sqs event, and then manually hook them up in aws console. Not ideal, but I’m hoping this can get fixed in the future. Especially since we’re going to end up with more than dozen of queues and triggers in our processing pipeline. Again, thanks again for confirming.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 4, 2019

  1. Since the role is external to the stack, the permissions are added to the queue policy instead of attached to the role. I am wondering why this is the behavior for imported roles.

You're right, it shouldn't be. The permissions should be added to the resource policy if the principal is in another account, and only then. I guess the API is still a little wonky, which makes this mistake too easy to make. My apologies, my understanding of IAM is progressing as this project progresses :)

Actually, @shivlaks used 2 different terms recently which are a lot more illuminating to think about (than "policy" or "permission"), that I wonder whether we should use more widely:

  • Permissions: the statements in IAM identity policy.
  • Trust: the statements in a resource policy.

I'm thinking we might need to do something similar to what we do for outbound security groups (where the default is "allow all outbound traffic", but that can be disabled for fine-grained configuration):

  • On every resource with a trust policy, have a trustOwnAccount?: boolean (defaults to true).
  • On every Grant, add both a permission and a trust, unless the resource has trustOwnAccount=true.

@robertd
Copy link
Contributor Author

robertd commented Jun 4, 2019

@rix0rrr No worries at all. Thanks for providing detailed insight in what's going on. Much appreciated.

rix0rrr added a commit that referenced this issue Jun 10, 2019
Now create a Policy and attach it to imported roles as well.

This will only work for imported roles in the same account. If you
need to reference roles in other accounts without trying to add
these policy statements, use an `AwsPrincipal`.

Relates to #2381, #2651, #2652, #2662.
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 10, 2019

I imagine the error might be caused because the IAM permission sqs:ChangeMessageVisibility would give permission to call both sqs:ChangeMessageVisibility and sqs:ChangeMessageVisibilityBatch.

SQS can and will validate the queue policy, but it does not see and so cannot validate the IAM policy. So actually sqs:ChangeMessageVisibilityBatch is probably incorrect.

Just surmising here.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 10, 2019

rix0rrr added a commit that referenced this issue Jun 10, 2019
Batch permissions are automatically implied when given regular API call
permissions. For example, giving IAM permissions to `sqs:SendMessage`
gives permission to call both `SendMessage` and `SendMessageBatch`.

Fixes #2381.
rix0rrr added a commit that referenced this issue Jun 11, 2019
Batch permissions are automatically implied when given regular API call
permissions. For example, giving IAM permissions to `sqs:SendMessage`
gives permission to call both `SendMessage` and `SendMessageBatch`.

Fixes #2381.
rix0rrr added a commit that referenced this issue Jun 17, 2019
Now create a Policy and attach it to imported roles as well.

This will only work for imported roles in the same account. If you
need to reference roles in other accounts without trying to add
these policy statements, use an `AwsPrincipal`.

Relates to #2381, #2651, #2652, #2662.
@robertd
Copy link
Contributor Author

robertd commented Jun 20, 2019

@eladb @rix0rrr I'm still unable to use imported role with 0.35.0 (see screenshot). Policy that's attached to this role is very permissive (see original post) and contains all actions listed in the screenshot.

image

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