Cfn-python-lint: Support for SourceSecurityGroupId under SecurityGroupEgress in AWS::EC2::SecurityGroup

[Hamitamaru]

cfn-lint version: (cfn-lint --version)
cfn-lint 0.7.3

Description of issue.
For some reason I get:

E3002 Invalid Property Resources/LaServiceEndpointSecurityGroup/Properties/SecurityGroupEgress/0/SourceSecurityGroupId
template.yml:1483:9

for

  LaServiceEndpointSecurityGroup:
    Type: AWS::EC2::SecurityGroup
    Properties:
      GroupName: LaServiceEndpointSecurityGroup
      GroupDescription: Security Group for VPC endpoints in the Lambda subnets
      SecurityGroupEgress:
      - IpProtocol: tcp
        FromPort: 443
        ToPort: 443
        SourceSecurityGroupId:
          Fn::GetAtt: [LambdaCanCallLaServiceSecurityGroup, GroupId]
      SecurityGroupIngress:
      - IpProtocol: tcp
        FromPort: 443
        ToPort: 443
        SourceSecurityGroupId:
          Fn::GetAtt: [LambdaCanCallLaServiceSecurityGroup, GroupId]
      VpcId:
        Ref: VPC
      Tags:
        - Key: Name
          Value: LaServiceEndpointSecurityGroup
        - Key: Stage
          Value: {Ref: Stage}

The odd part is it doesn't complain about SourceSecurityGroupId under SecurityGroupIngress only SecurityGroupEgress

[Hamitamaru]

Actually what is odd is that I should be using DestinationSecurityGroupId for Egress according to : https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-security-group-rule.html

But deployment passes anyways with SourceSecurityGroupId under SecurityGroupEgress >____>

[kddejong]

@cmmeyer can you take this one? Looks like another case of weirdness with the documentation/spec/API.

@Hamitamaru we rely on the CloudFormation spec a lot for this. I cut and pasted the appropriate section below for reference and that property isn't specified.

Looks like the documentation matches the spec.

So the API may take SourceSecurityGroupId. My guess is you should be using DestinationSecurityGroupId but something is still allowing SourceSecurityGroupId which isn't documented?

    "AWS::EC2::SecurityGroup.Egress": {
      "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-security-group-rule.html",
      "Properties": {
        "CidrIp": {... },
        "CidrIpv6": { ... },
        "Description": { ... },
        "DestinationPrefixListId": { ... },
        "DestinationSecurityGroupId": { ... },
        "FromPort": { ... },
        "IpProtocol": { ... },
        "ToPort": { ... }
      }

[cmmeyer]

Hi, @Hamitamaru

It looks like this is behaving as expected. It's weird because there is a generic "EC2 Security Group Rule Property Type" but the valid properties are different based on whether you are using it as an Ingress rule or an Egress rule:

From the CloudFormation doc (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-security-group-rule.html):

DestinationSecurityGroupId (SecurityGroupEgress only)
SourceSecurityGroupOwnerId (SecurityGroupIngress only)

This makes sense, since for an Egress Rule, your "source" would always be the current security group. I'm not sure why your provisioning is working -- are the rules showing up as expected?

[cmmeyer]

Ok, that's so WEIRD -- I ran it myself and it looks like the CloudFormation workflow is treating the SourceSecurityGroupId as a DestinationSecurityGroupId. This kind of makes sense since the underlying API (AuthorizeSecurityGroupEgress) doesn't differentiate and simply has a parameter called "GroupId".

I'm going to cut a ticket with the CloudFormation team. Seems silly to have differentiated properties on the CFN side if they're just going to lump them together on the API side, but I would say in general this is behavior as expected within the linter.

Are you ok with changing your template to use DestinationSecurityGroupId per the docs/spec for now and we'll treat this a CloudFormation issue?

[fatbasstard]

We had the same. And yes, it works. But is actually incorrect, like mentioned in the documentation. Apparantly some magic under the hood causes it to actually work.

This Cloudformation code can be changed without impact BTW (been there, done that)

So the documentation and the specifications (and thus the linter) is factually correct. There's just a "hidden feature" that allows this to actually work.

[Hamitamaru]

I have since changed SourceSecurityGroupOwnerId to DestinationSecurityGroupId under SecurityGroupEgress and deployment worked. I agree that if anything this is a "bug"/"feature" on the AWS CloudFormation side and not a bug of cfn-pthon-lint.

@cmmeyer , will you update this Issue with updates from your ticket with the CloudFormation team?

[cmmeyer]

Will do! It may be a while though since this is not blocking and I honestly never noticed it in 5 years of CloudFormation authoring.

[kddejong]

@cmmeyer is this something we should "patch"? or actually maybe we should close this cause I'm not sure we would want to patch this.

[cmmeyer]

I could patch the spec to enforce the xSecurityGroupOwnerId be correct (Source/Destination) to make templates more readable, but it doesn't impact the workflow engine and could throw errors for templates that actually work.

Perhaps instead of updating the spec we add a warning level error?

[theslloyd]

Experiencing a similar issue for AWS CodeBuild:
I have a codebuild project that uses the filter groups feature like so:
Triggers:
Webhook: true
FilterGroups:
- - Type: EVENT
Pattern: PULL_REQUEST_CREATED, PULL_REQUEST_UPDATED
- Type: BASE_REF
Pattern: ^refs/heads/master$

However the project linting fails for the FilterGroups property. This is despite it being correct syntax. The error is:
E3002 Invalid Property Resources/BuildProject/Properties/Triggers/FilterGroups

This is despite the file being actually valid! Any ideas what can be done about this?

[kddejong]

@theslloyd what version are you using? There have been some back and forth on this property and it’s support in the CloudFormation spec. I believe the most recent version should have this covered though.

[theslloyd]

@kddejong using:
[Container] 2019/02/28 11:59:05 Running command cfn-lint --version
cfn-lint 0.13.0

[theslloyd]

I've found out that the fix was merged 10 days ago, and the latest version release was 8 days ago, so the issue should included with v0.14.1 of cfn-lint. Apologies for the waste of notifications...

[tanisdlj]

@kddejong we can still see this issue:

$ cfn-lint --version
cfn-lint 0.20.1

🤔

[kddejong]

@tanisdlj there are two issues together here?
You talking about this one?

E3002 Invalid Property Resources/LaServiceEndpointSecurityGroup/Properties/SecurityGroupEgress/0/SourceSecurityGroupId
template.yml:1483:9