From dfe2ac8d2267726d17bc285da4abfa141b53d200 Mon Sep 17 00:00:00 2001 From: Nitzan Raz Date: Thu, 12 Jan 2023 12:58:31 +0200 Subject: [PATCH] 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 --- ecs/awsResources.go | 31 +++++++++- ecs/cloudformation.go | 38 +++++++++++-- .../simple-cloudformation-conversion.golden | 56 +++++++++++++++---- ...y-complex-cloudformation-conversion.golden | 56 +++++++++++++++---- 4 files changed, 152 insertions(+), 29 deletions(-) diff --git a/ecs/awsResources.go b/ecs/awsResources.go index 7abb7cf94..97d79ee96 100644 --- 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 index 8b673510b..88cc70478 100644 --- 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 index 7f82eb5f0..daa007407 100644 --- 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 index e8d6366e6..57aab2444 100644 --- 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