FormidableLabs / terraform-aws-serverless

Infrastructure support for Serverless framework apps, done the right way
https://registry.terraform.io/modules/FormidableLabs/serverless/aws
MIT License
144 stars 19 forks source link

admin policy needs s3:PutEncryptionConfiguration permission #33

Closed shrugs closed 5 years ago

shrugs commented 5 years ago

It seems like the admin role needs the s3:PutEncryptionConfiguration permission as well.

Here is the full list of permissions a serverless-deploying agent might need: https://gist.github.com/ServerlessBot/7618156b8671840a539f405dea2704c8 — might be worth mentioning that your serverless agent may need way more permissions than this module provides by default (depending on which resources are managed with serverless and which are managed with terraform)

from the serverless cli:

  An error occurred: ServerlessDeploymentBucket - API: s3:SetBucketEncryption Access Denied.

note: SetBucketEncryption is actually PutEncryptionConfiguration


A workaround is to simply add that permission to your user/group/policy/whatever ad-hoc.

resource "aws_iam_group_policy" "additional_admin_policy" {
  name  = "tf-${var.service_name}-${var.stage}-additional-admin-policy"
  group = "tf-${var.service_name}-${var.stage}-admin"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "s3:PutEncryptionConfiguration"
      ],
      "Effect": "Allow",
      "Resource": "*"
    }
  ]
}
EOF
}
ryan-roemer commented 5 years ago

Do you have a reproduction for what serverless.yml or AWS-native configuration causes this?

Our long term plan is enhancements like xray for specific scenarios that a minimal sls app doesn't have. Or simple flags in root module / default support depending on how wide the permission is...

shrugs commented 5 years ago

here's my serverless.yml (random details redacted). perhaps it's the ssm environment injection?

service: sls-${self:custom.service}

package:
  excludeDevDependencies: true
  exclude:
    - "**/node_modules/.cache/**"
    - "**/node_modules/aws-sdk/**" # included on Lambda.
    - "**/node_modules/.yarn-integrity"

plugins:
  - serverless-plugin-typescript

provider:
  name: aws
  runtime: nodejs8.10
  profile: sls-${self:custom.service}-${self:custom.stage}
  stage: ${self:custom.stage}
  region: ${self:custom.region}
  timeout: 30 # seconds (`300` max)
  memorySize: 128 # MB value (`1024` default)
  vpc:
    securityGroupIds:
      - sg-1234567890
    subnetIds:
      - subnet-1234567890
      - subnet-1234567890
      - subnet-1234567890
  iamRoleStatements:
    - Effect: Allow
      Action:
        - dynamodb:*
      Resource:
        - "arn:aws:dynamodb:us-east-1:1234567890:table/issues"
        - "arn:aws:dynamodb:us-east-1:1234567890:table/receipts"
    - Effect: Allow
      Action:
        - rds-data:*
      Resource:
        - "arn:aws:rds:us-east-1:1234567890:cluster:*"
  environment:
    STAGE: ${self:custom.stage}
    SERVICE_NAME: ${self:custom.service}
    DB_HOSTNAME: ${ssm:DFT_DB_HOSTNAME}
    DB_USERNAME: ${ssm:DFT_DB_USERNAME}
    DB_PASSWORD: ${ssm:DFT_DB_PASSWORD}
    DB_PORT: ${ssm:DFT_DB_PORT}

functions:
  my_function:
    handler: src/my_function.handler

custom:
  service: ${env:SERVICE_NAME}
  region: ${opt:region, env:AWS_REGION}
  stage: ${opt:stage, env:STAGE}
ryan-roemer commented 5 years ago

Cool! I'll dig in when I get a moment (juggling a handful of OSS things right now on top of the 'ol day job), and I'll try to work up a scenario for: https://github.com/FormidableLabs/aws-lambda-serverless-reference/blob/master/serverless.yml#L48-L59 like I have for the root module and xray...

SSM does sound like it might be behind it, and SSM support is something we'd definitely like to support as well :)

shrugs commented 5 years ago

cheers!

Actually, I also had to add the ec2:DescribeSecurityGroups permission to my agent as well and ended up just adding that full list of "minimal permissions" from serverless to my addon policy to remove all doubt.

Once I'm happy with my infra (currently converting to terraform) I'll do some testing and see which permissions were actually required.

ryan-roemer commented 5 years ago

Yeah, would love to get a honed, narrowly scoped permission for this specific scenario.

That mega list with the wide-open * is definitely way, way more than is needed (and the lack of more nuanced lists existing is the main reason we embarked on the research leading up to this project -- we manually test and scoped down everything to the narrowest resources possible, doing deploy, after deploy, after deploy... 😬 ).

shrugs commented 5 years ago

Cool, I've confirmed with some manual testing that:

1: The admin agent needs s3: PutEncryptionConfiguration regardless of the presence of ssm: templates in the serverless.yml

The developer account doesn't need s3: PutEncryptionConfiguration permission, but, because I'm reading parameters at build-time, does need the AmazonSSMReadOnlyAccess policy attached:

resource "aws_iam_group_policy_attachment" "developers_read_ssm" {
  # allow developers to read SSM documents when deploying lambdas
  group      = "tf-${var.service_name}-${var.stage}-developer"
  policy_arn = "arn:aws:iam::aws:policy/AmazonSSMReadOnlyAccess"
}

2: my developer account wants the lambda tag permissions as well, though this only errors in cloudformation, not sls cli

image

the tags seem to be cloudformation specific, though I'm not sure how the STAGE tag gets there, since I only inject that into the environment (cf template confirms this)

image

perhaps the cd-lambdas group should have those tag permissions by default?

¯\_(ツ)_/¯

3: I would probably add the iam:PutRolePolicy permission to the admin policy for completion's sake, since I'm guessing it needs that permission to update serverless.yml's iamRoleStatements on deploy. Speaking of, should developers be able to manage that policy, since it's managed by serverless.yml? I expect only allowing admins to manage

4: I don't actually need the apigateway:* permissions on my developer role, since my serverless doesn't expose any routes (I'm using appsync behind the scenes). perhaps that's a good candidate for another module (although most people will probably want it by default so again ¯\_(ツ)_/¯)

5: The admin account, beyond needing s3: PutEncryptionConfiguration, also needs ec2:DescribeSecurityGroups, ec2:DescribeSubnets, ec2:DescribeVpcs, and ec2:DescribeNetworkInterfaces (perhaps because of the vpc.securityGroupIds and vpc.subnetIds configuration in my serverless.yml?)

resource "aws_iam_group_policy" "additional_admin_policy" {
  name  = "tf-${var.service_name}-${var.stage}-additional-admin-policy"
  group = "tf-${var.service_name}-${var.stage}-admin"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "ec2:DescribeSecurityGroups",
        "ec2:DescribeSubnets",
        "ec2:DescribeVpcs",
        "ec2:DescribeNetworkInterfaces",
        "s3:PutEncryptionConfiguration"
      ],
      "Effect": "Allow",
      "Resource": "*"
    }
  ]
}
EOF
}
ryan-roemer commented 5 years ago

Good stuff! As you've got various issues, I've split some out. Here's where we are:

  1. Thanks. Part of this ticket.
  2. Split out to https://github.com/FormidableLabs/terraform-aws-serverless/issues/36
  3. Can you explain this more with an example of something that's failing in maybe a different ticket? So far, we've only see that needed here: https://github.com/FormidableLabs/terraform-aws-serverless/blob/c8d61ab0a0d44da67cd0df67f9344c081af8b0b1/policy-cd-lambdas.tf#L40-L48
  4. Split out to https://github.com/FormidableLabs/terraform-aws-serverless/issues/37
  5. That's all the VPC stuff. We have a full ticket for that support already: https://github.com/FormidableLabs/terraform-aws-serverless/issues/10 (I've also updated with our internal CloudFormation IAM stuff that I'm porting over to Terraform as part of ongoing, already-working-in-CloudFormation work).
ryan-roemer commented 5 years ago

Verified bug in https://github.com/FormidableLabs/aws-lambda-serverless-reference with a serverless package upgrade. It looks like:

Researching more and working on the fix.

ryan-roemer commented 5 years ago

@shrugs -- ICYMI we've got VPC IAM support submodule + reference app example (with full VPC instance) at:

if you wanted to try it out and see if it works with your existing VPC setup...