aws-cloudformation / cfn-lint

CloudFormation Linter
MIT No Attribution
2.41k stars 580 forks source link

Malformed IAM principal does not get caught #2412

Closed ericzbeard closed 1 week ago

ericzbeard commented 1 year ago

CloudFormation Lint Version

cfn-lint 0.64.1

What operating system are you using?

Mac

Describe the bug

Received no warnings from cfn-lint. Deploying the template gives this error:

Invalid principal in policy (Service: Amazon S3; Status Code: 400; Error Code: MalformedPolicy

The template YAML snippet from the policy:

Principal:
  AWS: !Sub "arn:aws:iam::${BetaAccountId}/role/cep-publish-role" 

Which should be:

Principal:
  AWS: !Sub "arn:aws:iam::${BetaAccountId}:role/cep-publish-role" 

Expected behavior

Expected cfn-lint to fail with a warning about a malformed principal.

Reproduction template

Parameters:

  BetaAccountId:
    Type: String
    Description: AccountId for the beta account, which pushes builds to the PublishBuildBucket 

Resources:

  PublishBuildBucket:
    Type: AWS::S3::Bucket

  PublishBuildBucketPolicy:
    Type: AWS::S3::BucketPolicy
    Properties:
      Bucket: !Ref PublishBuildBucket
      PolicyDocument:
        Version: 2012-10-17
        Statement:
          - Sid: BetaAccountPut
            Effect: Allow
            Principal:
              AWS: !Sub "arn:aws:iam::${BetaAccountId}/role/cep-publish-role" 
            Action: s3:PutObject
            Resource: !Ref PublishBuildBucket 
PatMyron commented 1 year ago

related: https://github.com/aws-cloudformation/cfn-lint/issues/1985

Since all resource types have different ARN syntax formats, the simplest general ARN check I can think of would be ensuring at least 5 colons:

https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html#arns-syntax

or more thoroughly generally checking everything non-resource type specific: arn:partition:service:region:account-id:


Potentially noisy once you're substituting like that though. No way for cfn-lint to definitively know whether references like that contain colons

kddejong commented 1 year ago

Possible extension of https://github.com/aws-cloudformation/cfn-lint/blob/main/src/cfnlint/rules/resources/iam/Policy.py since we are talking about IAM policy syntax it is tricky (with Cfn functions). If it is a sub or straight string we could check some of the valid options against it.

Some items that could be possible: [ ] 5 colons [ ] if IAM is the service we could check the first slash value or if its root [ ] validate no region is provided

kddejong commented 1 month ago

v1 gets us closer to this level of validation but only if something like

Parameters:

  BetaAccountId:
    Type: String
    Description: AccountId for the beta account, which pushes builds to the PublishBuildBucket 
    Default: '123456789012'

or AllowedValues is used.

E1019 {'Fn::Sub': 'arn:aws:iam::${BetaAccountId}/role/cep-publish-role'} does not match '^((arn:(aws|aws-cn|aws-us-gov):iam::\\d{12}:(?:root|user|group|role)|\\*)|\\d{12})' when 'Fn::Sub' is resolved
kddejong commented 1 week ago

This is now caught in v1. Going to close this and we can add more validation as needed.