Bug: Creates unnecessary AWSLambdaVPCAccessExecutionRole role

[fade2black]

Description:

SAM seems to add AWSLambdaVPCAccessExecutionRole simply because it detects the VpcConfig without checking the condition.

Steps to reproduce:

I create a simple hello-world lambda function as following:

AWSTemplateFormatVersion: '2010-09-09'
Transform: AWS::Serverless-2016-10-31

Parameters:
  SecurityGroupIds:
    Type: String
    Default: ""
  SubnetIds:
    Type: String
    Default: ""

Conditions:
  AlwaysFalseCondition: !Equals 
    - "123"
    - "345"

Resources:
  WorkerFunction:
    Type: AWS::Serverless::Function
    Properties:
      FunctionName: HelloWorld
      PackageType: Image
      Architectures:
        - x86_64
      MemorySize: 2048
      Timeout: 30
      VpcConfig:
        !If 
          - AlwaysFalseCondition
          -  
            SecurityGroupIds: !Split [ ",", !Ref SecurityGroupIds ] 
            SubnetIds: !Split [ ",", !Ref SubnetIds ] 
          - !Ref AWS::NoValue
    Metadata:
        DockerTag: mytag
        DockerContext: ./lib
        Dockerfile: Dockerfile

and deploy it as following

sam build
sam deploy --config-env dev --resolve-image-repos --no-confirm-changeset --no-fail-on-empty-changeset

Observed result:

When it successfully finishes deployment I go to my lambda function and check Configuration -> VPC. There is no associated security group or subnet. However, when I check the created role it contains two AWS Managed Policies:

• AWSLambdaBasicExecutionRole
• AWSLambdaVPCAccessExecutionRole

Expected result:

Because the condition under the VpcConfig section is always false, it does not associate the lambda with a subnet and a security group as expected. I also expect the sam not to add unnecessary permission AWSLambdaVPCAccessExecutionRole because I don't associate the lambda with any VPC. Nevertheless, it adds AWSLambdaVPCAccessExecutionRole.

While the extra privilege does not harm in this particular case, the intention was to remove the VpcConfig property using If operator and hence prevent adding VPC Access privileges.

Additional environment details:

1. OS: macOS, Sonoma 14.2
2. sam --version: 1.105.0
3. Docker version: 24.0.7
4. AWS region: eu-central-1

Project structure

🗂️lib
⚙︎samconfig.toml
❗️template.yaml

lib folder

Dockerfile

FROM public.ecr.aws/lambda/ruby:3.2
RUN yum install -y amazon-linux-extras ruby-devel make gcc
COPY Gemfile handler.rb ./
ENV GEM_HOME=${LAMBDA_TASK_ROOT}
RUN bundle config without test development
RUN bundle install

CMD ["handler.Handler.process"]

Gemfile

source 'https://rubygems.org'

git_source(:github) { |repo_name| "https://github.com/#{repo_name}" }
gem 'net-sftp', '~> 2.1', '>= 2.1.2'

handler.rb

class Handler
  @logger = Logger.new($stdout)

  class << self
    def process(event:, context:)
      log("Received payload: #{event}")
      log('Done.')
    end

    def log(str_info)
      @logger.info { str_info }
    end
  end
end

samconfig.toml

version = 0.1

[dev]
[dev.deploy]
[dev.deploy.parameters]
stack_name = "hello-world"
s3_bucket = "mybucket124fe5" 
s3_prefix = "dev"
region = "eu-central-1"
capabilities = "CAPABILITY_IAM"
parameter_overrides=[
    "SecurityGroupIds=sg-12345678",
    "SubnetIds=subnet-12345678"
]

[lucashuy]

Thanks for opening this issue. Like you mentioned, it doesn't look like the if is being evaluated here. Transferring to the SAM repo in case the folks there have more insight into this.

[GavinZZ]

Thanks for creating an issue. I can confirm that I can reproduce this issue.

Unfortunately there's not much we can do because SAM does not resolve complex intrinsics and due to backward compatibility issue. A detailed explanation on why SAM doesn't resolve these intrinsics is explained here. AFAIK, resolving these intrinsics typically happens at a later stage after SAM Transform during CloudFormation create changeset step. During SAM transform, there is no way for us to figure out the value of this condition thus deciding to add the policy or not.

If this behaviour is bothering you, I would recommend providing a custom iam role and policies to the lambda function.

[fade2black]

@GavinZZ What do you mean "...there's not much we can do"? 😀 This behaviour simply escalates privileges. It is a bug and you claim "we can't do anything about it"? Isn't it a program after all what ever it does (parses, transforms, etc... one-pass, two-pass, creates abstract syntax tree....)?
Imagine instead of AWSLambdaVPCAccessExecutionRole it adds

“Statement”: [{
        “Effect”: “Allow”,
        “Action”: “iam:PassRole”,
        “Resource”: “*”
    }] }

The rule of the least privileges (AWS's motto) are violated by AWS themselves?
Well, in this particular case it does not bother too much, but I hope we won't see more severe escalations of privileges.

Thank you. 🙂

[GavinZZ]

Hi @fade2black I understand that it is frustrating and I acknowledge it breaks the principle of least privilege. Please allow me to explain the reasoning a bit more in detail. This problem arises from a inconsiderate design issue when this is first implemented. There are are underlying issue that blocked us from fixing it.

As described in my previous comment, SAM does not have the capability to figure out the value of Fn::If intrinsics. Without knowing the exact value, we cannot determine whether or not to add the policy or not. The proper solution here would be not, by default, add the policy (or like the pass role example you provided). I absolutely agree that this should have been the way from the first implementation.

However, we cannot proceed with this change as it could be a potentially backward incompatible changes, causing users' lambda function who rely on this behaviour to add the policy to no longer work. SAM promises to always guarantee backward compatibility and that's why we cannot proceed with this change.

One additional workaround that I just think of is to rely on the use of Transform: AWS::Serverless-2016-10-31 transform before SAM Transform, like the following:

Transform:
  - AWS::LanguageExtensions
  - AWS::Serverless-2016-10-31

This would resolve the intrinsics into a static value and when SAM transform the template, it knows the exact value and would hence not add the policy.

[fade2black]

@GavinZZ Got you. Thanks! I will take this workaround into account.

[GavinZZ]

Closing this issue as it's not in a workable state due to backward compatibility issue. Feel free to open another issue if you have additional questions.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.