Repository: bridgecrewio/checkov
Pull request: Exclude NLBs from CKV_AWS_2 HTTPS check
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Closes #424 

CloudFormation config may need to be verified too - I haven't worked with it before (I'm more of a TF dude).

I wanted to add protocols to exclude from the check, as opposed to only checking for `HTTP` so that we don't miss any new protocols supported by the ALB. If new protocols supported by ALB are added then we would miss them in this check.

Problem statement:
Cloudformation NLB Listener Rules are incorrectly detected as ALB rules and fail the HTTPS expectation
**Describe the bug**
NLB listeners are evaluated as ALB listeners, and fail because they don't use HTTPS

**To Reproduce**
1. Create a Cloudformation template with NLB listeners (e.g. TCP)
2. Run default tests
3. Tests will fail with `"AWS::ElasticLoadBalancingV2::Listener.<listenername>\" failed in check \"Ensure ALB protocol is HTTPS\"

**Expected behavior**
NLB rules should pass. 

**Screenshots**


**Desktop (please complete the following information):**
 - OS: Ubuntu 20.04
 - Checkov Version: 1.0.423

**Additional context**
This resource:
```
    "ConsulDnsListener": {                                          
      "Type": "AWS::ElasticLoadBalancingV2::Listener",              
      "Properties": {                                               
        "DefaultActions": [                                         
          {                                                         
            "TargetGroupArn": {                                     
              "Ref": "ConsulDnsTargetGroup"                         
            },                                                      
            "Type": "forward"                                       
          }                                                         
        ],                                                          
        "LoadBalancerArn": {                                        
          "Ref": "LoadBalancerArn"                                  
        },                                                          
        "Port": 53,                                                 
        "Protocol": "TCP_UDP"                                       
      }                                                             
    },     
```
Produces this error:
```
{
  "type": "failure",
  "message": "Resource \"AWS::ElasticLoadBalancingV2::Listener.ConsulDnsListener\" failed in check \"Ensure ALB protocol is HTTPS\""
}```

Hints from issue discussion:
Hi @luckymike !
Please note the configuration does not distinguish NLBs and ALBs. If this is not a duplicate of #375 and I believe the reasoning is the same. Let me know your thoughts there, and feel free to reopen #375 if you disagree!
@nimrodkor I guess the confusion here is that the rule inaccurately identifies the listener as an ALB rule:
https://github.com/bridgecrewio/checkov/blob/master/checkov/cloudformation/checks/resource/aws/ALBListenerHTTPS.py#L23

`conf['Properties']['Protocol'] == 'TLS'` is an NLB listener, not an ALB listener.

IMO, this check is overly broad and naive about the actual supported listener scenarios. For example, `UDP` or `TCP_UDP` are valid NLB listener types which don't support load balancer terminated TLS. 

At a minimum, this check should probably be reworded to correctly reflect the scope as being wider than ALB, but a better approach would likely be to split out ALB and NLB checks, b/c enforcing `HTTPS` for all ALBs is easy to justify as a standard, while NLBs have much broader use-cases. 

I agree with @luckymike  – we run our own TLS terminators, fronted by NLBs. As it is, the test is both worded wrong (claims an ALB when it is an NLB) and flags valid use cases (non-TCP listeners, or cases where the connection is end-to-end encrypted).

#375 describes the solution, this issue describes the problem. It appears that I cannot reopen an issue that was opened by someone else?
Oh also, for the record – the same applies to the [Terraform equivalent of this rule](https://github.com/bridgecrewio/checkov/blob/master/checkov/terraform/checks/resource/aws/ALBListenerHTTPS.py) (I missed that this issue was originally about CFN)
This is preventing me from merging my Terraform code in - is there a recommended workaround? Should we be excluding this check in the meantime?
Also just plain protocol = "TCP" for a basic network load balancer should be ignored.

Although easily avoided, by setting the ignore, a listener which redirects from HTTP to HTTPS could be considered safe as well (but is not).

```
resource "aws_lb_listener" "http-to-https" {
  load_balancer_arn = aws_lb.example.arn
  port              = "80"
  protocol          = "HTTP"

  default_action {
    type = "redirect"

    redirect {
      port        = 443
      protocol    = "HTTPS"
      status_code = "HTTP_301"
    }
  }
}
```

Commit message: Exclude NLBs from CKV_AWS_2 HTTPS check

Patch to review:
diff --git a/checkov/cloudformation/checks/resource/aws/ALBListenerHTTPS.py b/checkov/cloudformation/checks/resource/aws/ALBListenerHTTPS.py
index a26144506c..1034c9cfe4 100644
--- a/checkov/cloudformation/checks/resource/aws/ALBListenerHTTPS.py
+++ b/checkov/cloudformation/checks/resource/aws/ALBListenerHTTPS.py
@@ -20,7 +20,7 @@ def scan_resource_conf(self, conf):
 
         if 'Properties' in conf.keys():
             if 'Protocol' in conf['Properties'].keys():
-                if conf['Properties']['Protocol'] in ('HTTPS', 'TLS'):
+                if conf['Properties']['Protocol'] in ('HTTPS', 'TLS', 'TCP', 'UDP', 'TCP_UDP'):
                     return CheckResult.PASSED
                 else:
                     if (
diff --git a/checkov/terraform/checks/resource/aws/ALBListenerHTTPS.py b/checkov/terraform/checks/resource/aws/ALBListenerHTTPS.py
index 1399252ceb..1f33dfb1e2 100644
--- a/checkov/terraform/checks/resource/aws/ALBListenerHTTPS.py
+++ b/checkov/terraform/checks/resource/aws/ALBListenerHTTPS.py
@@ -23,6 +23,12 @@ def scan_resource_conf(self, conf):
                 conf[key] == ["HTTPS"]
                 or
                 conf[key] == ["TLS"]
+                or
+                conf[key] == ["TCP"]
+                or
+                conf[key] == ["UDP"]
+                or
+                conf[key] == ["TCP_UDP"]
             ):
                 return CheckResult.PASSED
             elif conf[key] == ["HTTP"]:
diff --git a/tests/cloudformation/checks/resource/aws/example_ALBListener/ALBListenerHTTPS-PASSED-TCP.yaml b/tests/cloudformation/checks/resource/aws/example_ALBListener/ALBListenerHTTPS-PASSED-TCP.yaml
new file mode 100644
index 0000000000..3cff7f6134
--- /dev/null
+++ b/tests/cloudformation/checks/resource/aws/example_ALBListener/ALBListenerHTTPS-PASSED-TCP.yaml
@@ -0,0 +1,67 @@
+Description: >
+  This template deploys an NLB with TLS termination
+Parameters:
+  VPC:
+    Type: AWS::EC2::VPC::Id
+    Description: Choose which VPC the Application Load Balancer should be deployed to
+
+  Subnets:
+    Description: Choose a minimum of 2 subnets the ALB should be deployed to
+    Type: List<AWS::EC2::Subnet::Id>
+
+Resources:
+  LoadBalancer:
+    Type: AWS::ElasticLoadBalancingV2::LoadBalancer
+    Properties:
+      Name: CheckovTest
+      Type: 'network'
+      Subnets: !Ref Subnets
+  
+  ListenerHTTPS:
+    Type: AWS::ElasticLoadBalancingV2::Listener
+    Properties:
+      Certificates:
+        - CertificateArn: !Ref Certificate
+      LoadBalancerArn: !Ref LoadBalancer
+      Port: 22
+      Protocol: TCP
+      DefaultActions:
+        - Type: forward
+          TargetGroupArn: !Ref DefaultTargetGroup
+
+  ### NOTE - Stack will remain in CREATE_IN_PROGRESS until you validate the certificate!
+  Certificate:
+    Type: AWS::CertificateManager::Certificate
+    Properties:
+      DomainName: example.com
+      DomainValidationOptions:
+        - DomainName: example.com
+          ValidationDomain: example.com
+
+  ListenerCert:
+    Type: AWS::ElasticLoadBalancingV2::ListenerCertificate
+    Properties: 
+      Certificates: 
+        - CertificateArn: !Ref Certificate
+      ListenerArn: !Ref ListenerHTTPS
+
+  # We define a default target group here, as this is a mandatory Parameters
+  # when creating an Application Load Balancer Listener. This is not used, instead
+  # a target group is created per-service in each service template (../services/*)
+  DefaultTargetGroup:
+    Type: AWS::ElasticLoadBalancingV2::TargetGroup
+    Properties:
+      Name: checkov-default
+      VpcId: !Ref VPC
+      Port: 22
+      Protocol: TCP
+
+Outputs:
+  LoadBalancer:
+    Description: A reference to the Application Load Balancer
+    Value: !Ref LoadBalancer
+
+  LoadBalancerUrl:
+    Description: The URL of the ALB
+    Value: !GetAtt LoadBalancer.DNSName
+
diff --git a/tests/cloudformation/checks/resource/aws/example_ALBListener/ALBListenerHTTPS-PASSED-TCP_UDP.yaml b/tests/cloudformation/checks/resource/aws/example_ALBListener/ALBListenerHTTPS-PASSED-TCP_UDP.yaml
new file mode 100644
index 0000000000..5d33b7b29f
--- /dev/null
+++ b/tests/cloudformation/checks/resource/aws/example_ALBListener/ALBListenerHTTPS-PASSED-TCP_UDP.yaml
@@ -0,0 +1,67 @@
+Description: >
+  This template deploys an NLB with TLS termination
+Parameters:
+  VPC:
+    Type: AWS::EC2::VPC::Id
+    Description: Choose which VPC the Application Load Balancer should be deployed to
+
+  Subnets:
+    Description: Choose a minimum of 2 subnets the ALB should be deployed to
+    Type: List<AWS::EC2::Subnet::Id>
+
+Resources:
+  LoadBalancer:
+    Type: AWS::ElasticLoadBalancingV2::LoadBalancer
+    Properties:
+      Name: CheckovTest
+      Type: 'network'
+      Subnets: !Ref Subnets
+  
+  ListenerHTTPS:
+    Type: AWS::ElasticLoadBalancingV2::Listener
+    Properties:
+      Certificates:
+        - CertificateArn: !Ref Certificate
+      LoadBalancerArn: !Ref LoadBalancer
+      Port: 53
+      Protocol: TCP_UDP
+      DefaultActions:
+        - Type: forward
+          TargetGroupArn: !Ref DefaultTargetGroup
+
+  ### NOTE - Stack will remain in CREATE_IN_PROGRESS until you validate the certificate!
+  Certificate:
+    Type: AWS::CertificateManager::Certificate
+    Properties:
+      DomainName: example.com
+      DomainValidationOptions:
+        - DomainName: example.com
+          ValidationDomain: example.com
+
+  ListenerCert:
+    Type: AWS::ElasticLoadBalancingV2::ListenerCertificate
+    Properties: 
+      Certificates: 
+        - CertificateArn: !Ref Certificate
+      ListenerArn: !Ref ListenerHTTPS
+
+  # We define a default target group here, as this is a mandatory Parameters
+  # when creating an Application Load Balancer Listener. This is not used, instead
+  # a target group is created per-service in each service template (../services/*)
+  DefaultTargetGroup:
+    Type: AWS::ElasticLoadBalancingV2::TargetGroup
+    Properties:
+      Name: checkov-default
+      VpcId: !Ref VPC
+      Port: 53
+      Protocol: TCP_UDP
+
+Outputs:
+  LoadBalancer:
+    Description: A reference to the Application Load Balancer
+    Value: !Ref LoadBalancer
+
+  LoadBalancerUrl:
+    Description: The URL of the ALB
+    Value: !GetAtt LoadBalancer.DNSName
+
diff --git a/tests/cloudformation/checks/resource/aws/example_ALBListener/ALBListenerHTTPS-PASSED-UDP.yaml b/tests/cloudformation/checks/resource/aws/example_ALBListener/ALBListenerHTTPS-PASSED-UDP.yaml
new file mode 100644
index 0000000000..94c5322ccf
--- /dev/null
+++ b/tests/cloudformation/checks/resource/aws/example_ALBListener/ALBListenerHTTPS-PASSED-UDP.yaml
@@ -0,0 +1,67 @@
+Description: >
+  This template deploys an NLB with TLS termination
+Parameters:
+  VPC:
+    Type: AWS::EC2::VPC::Id
+    Description: Choose which VPC the Application Load Balancer should be deployed to
+
+  Subnets:
+    Description: Choose a minimum of 2 subnets the ALB should be deployed to
+    Type: List<AWS::EC2::Subnet::Id>
+
+Resources:
+  LoadBalancer:
+    Type: AWS::ElasticLoadBalancingV2::LoadBalancer
+    Properties:
+      Name: CheckovTest
+      Type: 'network'
+      Subnets: !Ref Subnets
+  
+  ListenerHTTPS:
+    Type: AWS::ElasticLoadBalancingV2::Listener
+    Properties:
+      Certificates:
+        - CertificateArn: !Ref Certificate
+      LoadBalancerArn: !Ref LoadBalancer
+      Port: 53
+      Protocol: UDP
+      DefaultActions:
+        - Type: forward
+          TargetGroupArn: !Ref DefaultTargetGroup
+
+  ### NOTE - Stack will remain in CREATE_IN_PROGRESS until you validate the certificate!
+  Certificate:
+    Type: AWS::CertificateManager::Certificate
+    Properties:
+      DomainName: example.com
+      DomainValidationOptions:
+        - DomainName: example.com
+          ValidationDomain: example.com
+
+  ListenerCert:
+    Type: AWS::ElasticLoadBalancingV2::ListenerCertificate
+    Properties: 
+      Certificates: 
+        - CertificateArn: !Ref Certificate
+      ListenerArn: !Ref ListenerHTTPS
+
+  # We define a default target group here, as this is a mandatory Parameters
+  # when creating an Application Load Balancer Listener. This is not used, instead
+  # a target group is created per-service in each service template (../services/*)
+  DefaultTargetGroup:
+    Type: AWS::ElasticLoadBalancingV2::TargetGroup
+    Properties:
+      Name: checkov-default
+      VpcId: !Ref VPC
+      Port: 53
+      Protocol: UDP
+
+Outputs:
+  LoadBalancer:
+    Description: A reference to the Application Load Balancer
+    Value: !Ref LoadBalancer
+
+  LoadBalancerUrl:
+    Description: The URL of the ALB
+    Value: !GetAtt LoadBalancer.DNSName
+
diff --git a/tests/cloudformation/checks/resource/aws/test_ALBListenerHTTPS.py b/tests/cloudformation/checks/resource/aws/test_ALBListenerHTTPS.py
index d3d6b3ca35..0165fe4630 100644
--- a/tests/cloudformation/checks/resource/aws/test_ALBListenerHTTPS.py
+++ b/tests/cloudformation/checks/resource/aws/test_ALBListenerHTTPS.py
@@ -16,7 +16,7 @@ def test_summary(self):
         report = runner.run(root_folder=test_files_dir,runner_filter=RunnerFilter(checks=[check.id]))
         summary = report.get_summary()
 
-        self.assertEqual(summary['passed'], 4)
+        self.assertEqual(summary['passed'], 7)
         self.assertEqual(summary['failed'], 1)
         self.assertEqual(summary['skipped'], 0)
         self.assertEqual(summary['parsing_errors'], 0)
diff --git a/tests/terraform/checks/resource/aws/test_ALBListenerHTTPS.py b/tests/terraform/checks/resource/aws/test_ALBListenerHTTPS.py
index 6c1d32bffc..5364f42d51 100644
--- a/tests/terraform/checks/resource/aws/test_ALBListenerHTTPS.py
+++ b/tests/terraform/checks/resource/aws/test_ALBListenerHTTPS.py
@@ -20,6 +20,24 @@ def test_success(self):
         scan_result = check.scan_resource_conf(conf=resource_conf)
         self.assertEqual(CheckResult.PASSED, scan_result)
 
+    def test_nlb_tcp_success(self):
+        resource_conf = {'load_balancer_arn': ['${aws_lb.front_end.arn}'], 'port': ['22'], 'protocol': ['TCP']}
+
+        scan_result = check.scan_resource_conf(conf=resource_conf)
+        self.assertEqual(CheckResult.PASSED, scan_result)
+
+    def test_nlb_udp_success(self):
+        resource_conf = {'load_balancer_arn': ['${aws_lb.front_end.arn}'], 'port': ['53'], 'protocol': ['UDP']}
+
+        scan_result = check.scan_resource_conf(conf=resource_conf)
+        self.assertEqual(CheckResult.PASSED, scan_result)
+
+    def test_nlb_tcp_udp_success(self):
+        resource_conf = {'load_balancer_arn': ['${aws_lb.front_end.arn}'], 'port': ['53'], 'protocol': ['TCP_UDP']}
+
+        scan_result = check.scan_resource_conf(conf=resource_conf)
+        self.assertEqual(CheckResult.PASSED, scan_result)
+
     def test_failure(self):
         resource_conf = {'load_balancer_arn': ['${aws_lb.front_end.arn}'], 'port': ['80'], 'protocol': ['HTTP']}
         scan_result = check.scan_resource_conf(conf=resource_conf)


Code review comments:
diff_hunk: @@ -23,6 +23,12 @@ def scan_resource_conf(self, conf):
                 conf[key] == ["HTTPS"]
                 or
                 conf[key] == ["TLS"]
+                or
+                conf[key] == ["TCP"]
+                or
+                conf[key] == ["UDP"]
+                or
+                conf[key] == ["TCP_UDP"]
path: checkov/terraform/checks/resource/aws/ALBListenerHTTPS.py
text: @user1:
@author Might be more readable to replace this prolonged condition as it is now to:
`conf[key] in ("HTTPS", "TLS", "TCP"...)`

@author:
Fair suggestion! I've made the change now 👍