aws-cloudformation / cfn-lint

CloudFormation Linter
MIT No Attribution
2.45k stars 592 forks source link

Resources created through serverless transforms use hardcoded partitions #1522

Closed dontirun closed 4 years ago

dontirun commented 4 years ago

*cfn-lint version: v0.30.1

Resources (such as Lambda Roles and Permissions) created by resources using AWS::Serverless use hardcoded Partitions

Example Resource

  rUploadRepo:
    Type: AWS::Serverless::Function
    Properties:
      Description: >-
        Used in CloudFormation to zip up and upload the repository to the
        Specified S3 Bucket
      Handler: uploader.lambda_handler
      MemorySize: 128
      Policies:
        - Version: '2012-10-17'
          Statement:
            - Effect: Allow
              Action:
                - s3:PutObject
              Resource:
                - !Sub 'arn:${AWS::Partition}:s3:::${pDeploymentAssetsBucket}/*'
            - Effect: Allow
              Action:
                - logs:CreateLogGroup
                - logs:CreateLogStream
                - logs:PutLogEvents
              Resource: "*"
      Runtime: python3.8
      CodeUri: CodeCommit/Lambda/Code/Uploader
      Layers:
        - !Ref rCodeCommitCustomResourceLayer
      Timeout: 30
      Tags:
        Name: !Sub '${pProjectName}-repo-upload-lambda'

Hardcoded partition in resulting managed policy arn

Properties": {
        "AssumeRolePolicyDocument": {
            "Statement": [
                {
                    "Action": [
                        "sts:AssumeRole"
                    ],
                    "Effect": "Allow",
                    "Principal": {
                        "Service": [
                            "lambda.amazonaws.com"
                        ]
                    }
                }
            ],
            "Version": "2012-10-17"
        },
        "ManagedPolicyArns": [
            "arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
        ],
        "Policies": [
            {
                "PolicyDocument": {
                    "Statement": [
                        {
                            "Action": [
                                "s3:PutObject"
                            ],
                            "Effect": "Allow",
                            "Resource": [
                                {
                                    "Fn::Sub": "arn:${AWS::Partition}:s3:::${pDeploymentAssetsBucket}/*"
                                }
                            ]
                        },
                        {
                            "Action": [
                                "logs:CreateLogGroup",
                                "logs:CreateLogStream",
                                "logs:PutLogEvents"
                            ],
                            "Effect": "Allow",
                            "Resource": "*"
                        }
                    ],
                    "Version": "2012-10-17"
                },
                "PolicyName": "rUploadRepoRolePolicy0"
            }
        ],
        "Tags": [
            {
                "Key": "lambda:createdBy",
                "Value": "SAM"
            },
            {
                "Key": "Name",
                "Value": {
                    "Fn::Sub": "${pProjectName}-repo-upload-lambda"
                }
            }
        ]
    },
    "Type": "AWS::IAM::Role"
}

{
    "Properties": {
        "Action": "lambda:InvokeFunction",
        "FunctionName": {
            "Ref": "rCfnNagNotficationLambda"
        },
        "Principal": "logs.amazonaws.com",
        "SourceArn": {
            "Fn::Sub": [
                "arn:aws:logs:${AWS::Region}:${AWS::AccountId}:log-group:${__LogGroupName__}:*",
                {
                    "__LogGroupName__": {
                        "Fn::GetAtt": [
                            "rDevelopPipeline",
                            "Outputs",
                            "oCfnNagLogGroupName"
                        ]
                    }
                }
            ]
        }
    },
    "Type": "AWS::Lambda::Permission"
}
PatMyron commented 4 years ago

actually been meaning to write a similar rule


think the Linter's just running the AWS::Serverless transform itself though

do you not also experience these hardcoded partitions being generated outside of the Linter?

I'd also check which version of aws-sam-translator you have installed and see if upgrading helps:

pip3 show aws-sam-translator
pip3 install aws-sam-translator --upgrade
dontirun commented 4 years ago

I upgraded

I have generally worked with CloudFormation templates with the SAM Transform directly instead of using the SAM CLI.

actually been meaning to write a similar rule

think the Linter should just be running the AWS::Serverless transform itself though

do you not also experience these hardcoded partitions being generated outside of the Linter?

I'd also check which version of aws-sam-translator you have installed and see if upgrading helps:

pip3 show aws-sam-translator
pip3 install aws-sam-translator --upgrade

I upgraded my version of the aws-sam-translator, but unfortunately that did not solve the issue.

I have generally worked with CloudFormation templates with SAM resources specified directly in those templates packaging and deploying them using the CloudFormation package and deploy CLI commands. The resulting templates from package still have the SAM resources in them, so I assume CloudFormation does the major changes behind the scenes where those resources are generated.

I do know that the SAM spec does work across partitions, so perhaps CFn does the correct resource selection behind the scenes.

Is there a way to run linting before the linter runs the transform? Specifically for checks for harcoded partitions (or perhaps ARN checks in general)?

PatMyron commented 4 years ago

Console's template tab has a View processed template toggle or via the CLI:

aws cloudformation get-template --template-stage Processed --stack-name $NAME


If processed templates contain generated hardcoded partitions, the AWS::Serverless transform itself should fix that

dontirun commented 4 years ago

I checked the processed template and you are correct, this needs to be fixed in the transform itself.

I raised an issue

Is it possible to create a rule that Warns normally, but Errors if the --regions flag is used?

jfuss commented 4 years ago

@PatMyron Why does cfn-lint run on the transformed template? Shouldn't this only be run on the source template? I ask because SAM is partition aware but we (in some cases) explicitly set the partition. Which will create false positives. Not sure I understand why cfn-lint does this validation on the transformed template? Does cfn-lint do this for all transforms?

PatMyron commented 4 years ago

Does cfn-lint do this for all transforms?

currently just the AWS::Serverless Transform

the Linter doesn't require networking or credentials to run, which other transforms require

jfuss commented 4 years ago

Hmm.. So SAM is the only one that is validated based on the transformed template. How do we address that? I really don't think you guys should be validating on what we do under the hood for customers, that will lead to confusion on how a customer can fix some 'issue' with the generated template.

PatMyron commented 4 years ago

This has come up before

Running the AWS::Serverless Transform has some value, but definitely agree that customers shouldn't feel blocked on generated code they can't control

kddejong commented 4 years ago

My ideal solution @jfuss would be to have a SAM CloudFormation spec file we could use to validate the templates. We need to transform for SAM since the transformed template is the only thing that would make sense to the spec files that exist.
Doing this for other Transforms has come up before but there isn’t as easy of a way to do it at scale as there is with SAM.

I agree it does cause some confusion. The other option would be to disable linting on SAM templates (or just lint them as is... which could cause a lot more false positives).

jlhood commented 4 years ago

+1 to @kddejong's preference for SAM to provide a spec cfn-lint can use rather than running the SAM translator library on the template before processing with cfn-lint. There's a SAM issue for this: https://github.com/awslabs/serverless-application-model/issues/1133

jfuss commented 4 years ago

Yup. I know this has come up in (many) discussions and I think we had something small done in this space but I don't think it ever got completed.

This will help many tools out (cfn lint, cdk, sam cli, etc), so there is value to trying to get this prioritized from our side.