aws-cloudformation / cfn-lint

CloudFormation Linter
MIT No Attribution
2.4k stars 576 forks source link

v1: Bug for E2031, E3002, E3512 #3325

Closed egut closed 2 weeks ago

egut commented 2 weeks ago

CloudFormation Lint Version

v1.3.0

What operating system are you using?

Ubuntu

Describe the bug

Sorry for multiple bug in same issue, but all come from the exact same line.

For only some s3-bucket we share with other accounts and therefore do we have a check if we should ignore the parameter or not. cfn-lint 1.3.0 seams to be very confused by this 0.87.7 had no problem resolving this correct.

**E3002** '*' was expected
**E3512** {'AWS': {'Ref': 'CrossAccountRead'}} is not valid under any of the given schemas
**W2031** '' does not match '^((arn:(aws|aws-cn|aws-us-gov):iam::\\d{12}:(?:root|user|group|role)|\\*)|\\d{12})' when 'Ref' is resolved

Expected behavior

That cfn-lint also follow the condition and do not give error to the code.

Reproduction template

---
AWSTemplateFormatVersion: 2010-09-09
Description: 'Bug for E2031, E3002, E3512'
Parameters:
  CrossAccountRead:
    Type: 'CommaDelimitedList'
    Description: >-
      List of account ids that are allowed to read this bucket
    Default: ''

Conditions:
  HaveCrossAccountRead: !Not
    - !Equals
      - !Join
        - ""
        - !Ref 'CrossAccountRead'
      - ''
Resources:

  S3KmsKey:
    Type: 'AWS::KMS::Key'
    Properties:
      EnableKeyRotation: true
      KeyPolicy:
        Version: 2012-10-17
        Statement:
          - Sid: 'AdministrativeAccess'
            Effect: 'Allow'
            Principal:
              AWS: !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:root'
            Action:
              - 'kms:*'
            Resource: '*'

          - !If
            - 'HaveCrossAccountRead'
            - Sid: 'AllowReadRoles'
              Effect: 'Allow'
              Principal:
                AWS: !Ref 'CrossAccountRead'

              Resource: "*"
              Action:
                - 'kms:Decrypt*'
                - 'kms:Describe*'
              Condition:
                StringEquals:
                  "kms:ViaService": !Sub "s3.${AWS::Region}.amazonaws.com"
            - !Ref 'AWS::NoValue'
karpovkos commented 2 weeks ago

CloudFormation Lint Version

What operating system are you using?

Describe the bug cfn-lint highlighting the line s3:x-amz-server-side-encryption: and shows an error:

Reproduction template

S3ArtifactBucketArfPolicy:
    Type: AWS::S3::BucketPolicy
    Properties:
      Bucket: !Ref S3ArtifactBucket
      PolicyDocument:
        Id: S3ArtifactBucketArfPolicy
        Version: '2012-10-17'
        Statement:
          - Sid: DenyIncorrectEncryptionHeader
            Effect: Deny
            Action:
              - s3:PutObject
            Resource: !Sub ${S3ArtifactBucket.Arn}/*
            Principal:
              AWS:
                - '*'
            Condition:
              'Null':
                s3:x-amz-server-side-encryption:
                  - false
              StringNotEquals:
                s3:x-amz-server-side-encryption:
                  - "aws:kms"

Expected behavior

kddejong commented 2 weeks ago

@karpovkos not related to the above issue but your issue is fixed in #3349

isuftin commented 2 weeks ago

I'm using cfn-lint 1.3.3 and did run cfn-lint -u..

I have a template which includes the resource:

  DataSyncLocationEFS:
    Type: AWS::DataSync::LocationEFS
    Properties:
      Ec2Config:
        SecurityGroupArns:
          - !Sub arn:${AWS::Partition}:ec2:${AWS::Region}:${AWS::AccountId}:security-group/${SelfReferencingSecurityGroup}
        SubnetArn: !Ref SubnetAArn
      EfsFilesystemArn: !GetAtt 'FileSystem.Arn'
      FileSystemAccessRoleArn: !GetAtt 'EFSDataSyncRole.Arn'
      InTransitEncryption: TLS1_2

And the parameter:

  SubnetAArn:
    Description: The ARN of the "A" subnet
    Type: String

The parameter value ends up being of the format...arn:aws:ec2:us-west-2:1234567890:subnet/subnet-0987654321abc

cfn-lint comes back withW2031 {'Ref': 'SubnetAArn'} does not match '^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b):ec2:[a-z\\-0-9]*:[0-9]{12}:subnet/.*$' when 'Ref' is resolved

Note that this seems to have begun in cfn-lint 1.3.1 and persists in 1.3.3

kddejong commented 2 weeks ago

@isuftin I'm not able to replicate this issue with the latest version.

I can replicate it if I do this. Anything else in this template (modules, transform, conditions) that I should know about?

SubnetAArn:
    Description: The ARN of the "A" subnet
    Type: String
    Default: ""
isuftin commented 2 weeks ago

@kddejong - I think my YAML did cut off too soon and yes there's a default in the param ("-"). So that's a bad on my end. this did use to work. I can remove the default of course, but could you shed light on in which version this may have changed? The last successful run we had with this was in version 0.87.4. I'm trying to track down where that change may have happened via https://github.com/aws-cloudformation/cfn-lint/compare/v1.3.3...v0.87.4 but am not seeing it.

kddejong commented 2 weeks ago

Should be in the 1.3.0 change which would have the first v1 version available. Blog. In this version we will test Default and AllowedValues against the schemas in which they are used.

I believe 1.3.1 change had a change to apply better logic if you used a condition. For instance

Parameters:
  SubnetAArn:
    Type: String
    Default: "-"
Conditions:
  UseSubnetAArn: !Not [!Equals [!Ref SubnetAArn, "-"]]
Resources:
  DataSyncLocationEFS:
    Type: AWS::DataSync::LocationEFS
    Properties:
      Ec2Config:
        SecurityGroupArns:
          - !Sub arn:${AWS::Partition}:ec2:${AWS::Region}:${AWS::AccountId}:security-group/${SelfReferencingSecurityGroup}
        SubnetArn: !If [UseSubnetAArn, !Ref SubnetAArn, !Ref AWS::NoValue]
      EfsFilesystemArn: !GetAtt 'FileSystem.Arn'
      FileSystemAccessRoleArn: !GetAtt 'EFSDataSyncRole.Arn'
      InTransitEncryption: TLS1_2

Wouldn't result in an error as if the value was "-" then we aren't using it as an ARN.

isuftin commented 2 weeks ago

@kddejong - I much appreciate the information. We've updated our cfn template and are moving forward without error. Thank you again.