aws-cloudformation / cfn-lint

CloudFormation Linter
MIT No Attribution
2.43k stars 586 forks source link

Warning on condition keys that are not applicable to given actions #1489

Open imrehg opened 4 years ago

imrehg commented 4 years ago

cfn-lint version: (cfn-lint 0.29.6)

Description of issue.

When writing IAM rules, not all actions accept all conditions. In case a non-acceptable condition is added to an action, it is just silently ignored, as much as I can tell. Thus can result in a while goose chase: "why my rule did not apply?"

For example, the S3 object-level rules in general do not accept s3:prefix-based conditions. Thus this template (arguably contrived, there could be better example) would result in a policy that is listed except the condition ignored (and thus applied at all cases):

Resources:
  WarehouseBucket:
    Type: AWS::S3::Bucket
    Properties:
      BucketName: "test-bucket"
  WarehouseBucketPolicy:
    Type: AWS::S3::BucketPolicy
    Properties:
      Bucket: !Ref WarehouseBucket
      PolicyDocument:
        Version: 2012-10-17
        Statement:
          - Effect: Deny
            Principal: "*"
            Action: s3:GetObject
            Resource: !Join ["", [!GetAtt [WarehouseBucket, Arn], "/*"]]
            Condition:
              StringLike:
                s3:prefix: "hq/"

cfn-lint happily accepts it, though. It would be nice to at least warn, that the condition is not applicable.

For the applicable rules, I've checked https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_actions-resources-contextkeys.html and https://iam.cloudonaut.io/

kddejong commented 4 years ago

We did start working on something for bucket policies specifically. https://github.com/aws-cloudformation/cfn-python-lint/pull/914

It doesn't do conditions but we are looking for object based commands, bucket level commands so we may be able to validate the condition while we do that.

kddejong commented 1 month ago

Was re-looking at this and it appears that the permissions file we are using doesn't separate out the actions and conditions for S3 like this. I think in theory we could do this but it wouldn't catch the issue you provided because our data source doesn't separate it out.