ECS: Creating special security groups for ingress, instead of adding …

…the ingress rule to other security groups Solves #1783 Previously, the ECS stack included an ingress rule to allow LB to reach the tasks. However, it added this ingress rule toe very Docker network security group, meaning other tasks on the same Docker network, possibly sensitive, were accessible externally. We now create a new security group for port assignments for every service that has ports, and attach that security group only to that service. This prevents other tasks in the same Docker networks are not accessible externally. Signed-off-by: Nitzan Raz <[email protected]>
docker-archive · Jan 11, 2023 · dfe2ac8 · dfe2ac8
1 parent fdf4ebf
commit dfe2ac8
Show file tree

Hide file tree

Showing 4 changed files with 152 additions and 29 deletions.
diff --git a/ecs/awsResources.go b/ecs/awsResources.go
@@ -51,6 +51,9 @@ func (r *awsResources) serviceSecurityGroups(service types.ServiceConfig) []stri
 	for net := range service.Networks {
 		groups = append(groups, r.securityGroups[net])
 	}
+	if len(service.Ports) > 0 {
+		groups = append(groups, r.securityGroups[serviceIngressSecGroupName(service.Name, false)])
+	}
 	return groups
 }
 
@@ -330,6 +333,7 @@ func (b *ecsAPIService) ensureCluster(r *awsResources, project *types.Project, t
 
 func (b *ecsAPIService) ensureNetworks(r *awsResources, project *types.Project, template *cloudformation.Template) {
 	if r.securityGroups == nil {
+		// TODO NITZ change the size hint?
 		r.securityGroups = make(map[string]string, len(project.Networks))
 	}
 	for name, net := range project.Networks {
@@ -353,6 +357,27 @@ func (b *ecsAPIService) ensureNetworks(r *awsResources, project *types.Project,
 
 		r.securityGroups[name] = cloudformation.Ref(securityGroup)
 	}
+
+	for _, service := range project.Services {
+		if len(service.Ports) == 0 {
+			continue
+		}
+		lbSecurityGroup := serviceIngressSecGroupName(service.Name, true)
+		template.Resources[lbSecurityGroup] = &ec2.SecurityGroup{
+			GroupDescription: fmt.Sprintf("%s Security Group for service %s ingress, LB side", project.Name, service.Name),
+			VpcId:            r.vpc,
+			Tags:             serviceTags(project, service),
+		}
+		r.securityGroups[lbSecurityGroup] = cloudformation.Ref(lbSecurityGroup)
+
+		serviceSecurityGroup := serviceIngressSecGroupName(service.Name, false)
+		template.Resources[serviceSecurityGroup] = &ec2.SecurityGroup{
+			GroupDescription: fmt.Sprintf("%s Security Group for service %s ingress, Service side", project.Name, service.Name),
+			VpcId:            r.vpc,
+			Tags:             serviceTags(project, service),
+		}
+		r.securityGroups[serviceSecurityGroup] = cloudformation.Ref(serviceSecurityGroup)
+	}
 }
 
 func (b *ecsAPIService) ensureVolumes(r *awsResources, project *types.Project, template *cloudformation.Template) error {
@@ -465,9 +490,9 @@ func (b *ecsAPIService) ensureLoadBalancer(r *awsResources, project *types.Proje
 
 func (r *awsResources) getLoadBalancerSecurityGroups(project *types.Project) []string {
 	securityGroups := []string{}
-	for name, network := range project.Networks {
-		if !network.Internal {
-			securityGroups = append(securityGroups, r.securityGroups[name])
+	for _, service := range project.Services {
+		if len(service.Ports) > 0 {
+			securityGroups = append(securityGroups, r.securityGroups[serviceIngressSecGroupName(service.Name, true)])
 		}
 	}
 	return securityGroups

diff --git a/ecs/cloudformation.go b/ecs/cloudformation.go
@@ -197,11 +197,15 @@ func (b *ecsAPIService) createService(project *types.Project, service types.Serv
 		dependsOn []string
 		serviceLB []ecs.Service_LoadBalancer
 	)
-	for _, port := range service.Ports {
-		for net := range service.Networks {
-			b.createIngress(service, net, port, template, resources)
-		}
 
+	for _, port := range service.Ports {
+		lbSecurityGroupName := serviceIngressSecGroupName(service.Name, true)
+		serviceSecurityGroupName := serviceIngressSecGroupName(service.Name, false)
+		// outside to ingress
+		b.createIngress(service, lbSecurityGroupName, port, template, resources)
+		// ingress to service
+		b.createCrossGroupIngress(service, lbSecurityGroupName, serviceSecurityGroupName, port, template, resources)
+		// TODO attach new secgroup to this service
 		protocol := strings.ToUpper(port.Protocol)
 		if resources.loadBalancerType == elbv2.LoadBalancerTypeEnumApplication {
 			// we don't set Https as a certificate must be specified for HTTPS listeners
@@ -293,6 +297,22 @@ func (b *ecsAPIService) createIngress(service types.ServiceConfig, net string, p
 	}
 }
 
+func (b *ecsAPIService) createCrossGroupIngress(service types.ServiceConfig, source string, dest string, port types.ServicePortConfig, template *cloudformation.Template, resources awsResources) {
+	protocol := strings.ToUpper(port.Protocol)
+	if protocol == "" {
+		protocol = allProtocols
+	}
+	ingress := fmt.Sprintf("%s%d%sIngress", normalizeResourceName(source), port.Target, normalizeResourceName(dest))
+	template.Resources[ingress] = &ec2.SecurityGroupIngress{
+		SourceSecurityGroupId: resources.securityGroups[source],
+		Description:           fmt.Sprintf("LB connectivity on %d", port.Target),
+		GroupId:               resources.securityGroups[dest],
+		FromPort:              int(port.Target),
+		IpProtocol:            protocol,
+		ToPort:                int(port.Target),
+	}
+}
+
 func (b *ecsAPIService) createSecret(project *types.Project, name string, s types.SecretConfig, template *cloudformation.Template) error {
 	if s.External.External {
 		return nil
@@ -534,6 +554,16 @@ func networkResourceName(network string) string {
 	return fmt.Sprintf("%sNetwork", normalizeResourceName(network))
 }
 
+func serviceIngressSecGroupName(service string, isLBSide bool) string {
+	var header string
+	if isLBSide {
+		header = "LB"
+	} else {
+		header = "Service"
+	}
+	return fmt.Sprintf("%s%sIngressSecurityGroup", normalizeResourceName(service), header)
+}
+
 func serviceResourceName(service string) string {
 	return fmt.Sprintf("%sService", normalizeResourceName(service))
 }

diff --git a/ecs/testdata/simple-cloudformation-conversion.golden b/ecs/testdata/simple-cloudformation-conversion.golden
@@ -13,16 +13,6 @@ Resources:
       - Key: com.docker.compose.project
         Value: TestSimpleConvert
     Type: AWS::ECS::Cluster
-  Default80Ingress:
-    Properties:
-      CidrIp: 0.0.0.0/0
-      Description: simple:80/tcp on default network
-      FromPort: 80
-      GroupId:
-        Ref: DefaultNetwork
-      IpProtocol: TCP
-      ToPort: 80
-    Type: AWS::EC2::SecurityGroupIngress
   DefaultNetwork:
     Properties:
       GroupDescription: TestSimpleConvert Security Group for default network
@@ -46,7 +36,7 @@ Resources:
     Properties:
       Scheme: internet-facing
       SecurityGroups:
-      - Ref: DefaultNetwork
+      - Ref: SimpleLBIngressSecurityGroup
       Subnets:
       - subnet1
       - subnet2
@@ -59,6 +49,38 @@ Resources:
     Properties:
       LogGroupName: /docker-compose/TestSimpleConvert
     Type: AWS::Logs::LogGroup
+  SimpleLBIngressSecurityGroup:
+    Properties:
+      GroupDescription: TestSimpleConvert Security Group for service simple ingress,
+        LB side
+      Tags:
+      - Key: com.docker.compose.project
+        Value: TestSimpleConvert
+      - Key: com.docker.compose.service
+        Value: simple
+      VpcId: vpc-123
+    Type: AWS::EC2::SecurityGroup
+  SimpleLBIngressSecurityGroup80Ingress:
+    Properties:
+      CidrIp: 0.0.0.0/0
+      Description: simple:80/tcp on SimpleLBIngressSecurityGroup network
+      FromPort: 80
+      GroupId:
+        Ref: SimpleLBIngressSecurityGroup
+      IpProtocol: TCP
+      ToPort: 80
+    Type: AWS::EC2::SecurityGroupIngress
+  SimpleLBIngressSecurityGroup80SimpleServiceIngressSecurityGroupIngress:
+    Properties:
+      Description: LB connectivity on 80
+      FromPort: 80
+      GroupId:
+        Ref: SimpleServiceIngressSecurityGroup
+      IpProtocol: TCP
+      SourceSecurityGroupId:
+        Ref: SimpleLBIngressSecurityGroup
+      ToPort: 80
+    Type: AWS::EC2::SecurityGroupIngress
   SimpleService:
     DependsOn:
     - SimpleTCP80Listener
@@ -84,6 +106,7 @@ Resources:
           AssignPublicIp: ENABLED
           SecurityGroups:
           - Ref: DefaultNetwork
+          - Ref: SimpleServiceIngressSecurityGroup
           Subnets:
           - subnet1
           - subnet2
@@ -117,6 +140,17 @@ Resources:
       NamespaceId:
         Ref: CloudMap
     Type: AWS::ServiceDiscovery::Service
+  SimpleServiceIngressSecurityGroup:
+    Properties:
+      GroupDescription: TestSimpleConvert Security Group for service simple ingress,
+        Service side
+      Tags:
+      - Key: com.docker.compose.project
+        Value: TestSimpleConvert
+      - Key: com.docker.compose.service
+        Value: simple
+      VpcId: vpc-123
+    Type: AWS::EC2::SecurityGroup
   SimpleTCP80Listener:
     Properties:
       DefaultActions:

diff --git a/ecs/testdata/slightly-complex-cloudformation-conversion.golden b/ecs/testdata/slightly-complex-cloudformation-conversion.golden
@@ -13,16 +13,6 @@ Resources:
       - Key: com.docker.compose.project
         Value: TestSlightlyComplexConvert
     Type: AWS::ECS::Cluster
-  Default80Ingress:
-    Properties:
-      CidrIp: 0.0.0.0/0
-      Description: entrance:80/tcp on default network
-      FromPort: 80
-      GroupId:
-        Ref: DefaultNetwork
-      IpProtocol: TCP
-      ToPort: 80
-    Type: AWS::EC2::SecurityGroupIngress
   DefaultNetwork:
     Properties:
       GroupDescription: TestSlightlyComplexConvert Security Group for default network
@@ -42,6 +32,38 @@ Resources:
       SourceSecurityGroupId:
         Ref: DefaultNetwork
     Type: AWS::EC2::SecurityGroupIngress
+  EntranceLBIngressSecurityGroup:
+    Properties:
+      GroupDescription: TestSlightlyComplexConvert Security Group for service entrance
+        ingress, LB side
+      Tags:
+      - Key: com.docker.compose.project
+        Value: TestSlightlyComplexConvert
+      - Key: com.docker.compose.service
+        Value: entrance
+      VpcId: vpc-123
+    Type: AWS::EC2::SecurityGroup
+  EntranceLBIngressSecurityGroup80EntranceServiceIngressSecurityGroupIngress:
+    Properties:
+      Description: LB connectivity on 80
+      FromPort: 80
+      GroupId:
+        Ref: EntranceServiceIngressSecurityGroup
+      IpProtocol: TCP
+      SourceSecurityGroupId:
+        Ref: EntranceLBIngressSecurityGroup
+      ToPort: 80
+    Type: AWS::EC2::SecurityGroupIngress
+  EntranceLBIngressSecurityGroup80Ingress:
+    Properties:
+      CidrIp: 0.0.0.0/0
+      Description: entrance:80/tcp on EntranceLBIngressSecurityGroup network
+      FromPort: 80
+      GroupId:
+        Ref: EntranceLBIngressSecurityGroup
+      IpProtocol: TCP
+      ToPort: 80
+    Type: AWS::EC2::SecurityGroupIngress
   EntranceService:
     DependsOn:
     - EntranceTCP80Listener
@@ -67,6 +89,7 @@ Resources:
           AssignPublicIp: ENABLED
           SecurityGroups:
           - Ref: DefaultNetwork
+          - Ref: EntranceServiceIngressSecurityGroup
           Subnets:
           - subnet1
           - subnet2
@@ -100,6 +123,17 @@ Resources:
       NamespaceId:
         Ref: CloudMap
     Type: AWS::ServiceDiscovery::Service
+  EntranceServiceIngressSecurityGroup:
+    Properties:
+      GroupDescription: TestSlightlyComplexConvert Security Group for service entrance
+        ingress, Service side
+      Tags:
+      - Key: com.docker.compose.project
+        Value: TestSlightlyComplexConvert
+      - Key: com.docker.compose.service
+        Value: entrance
+      VpcId: vpc-123
+    Type: AWS::EC2::SecurityGroup
   EntranceTCP80Listener:
     Properties:
       DefaultActions:
@@ -192,7 +226,7 @@ Resources:
     Properties:
       Scheme: internet-facing
       SecurityGroups:
-      - Ref: DefaultNetwork
+      - Ref: EntranceLBIngressSecurityGroup
       Subnets:
       - subnet1
       - subnet2