Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

ECS: Redid security group generation for LBs #2215

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BackSlasher
Copy link

@BackSlasher BackSlasher commented Jan 11, 2023

What I did
The ECS stack includes an ingress rule to allow LB to reach the tasks.
However, the ingress was added inside the Docker network security group, exposing all containers to requests on this port from 0.0.0.0/0.
This means tasks that don't have exposed ports on the compose spec, and possibly contain unprotected sensitive endpoints, were exposed to outside access (I personally had a NodeJS Express server getting /.env hit by random scrapers).

We now do the following:

  1. Do not touch the network security group. It only contains one rule, allowing cross-task communication inside the network.
  2. Create a per-service "LB" security group, attached to the load balancer, exposing the published ports for outside access
  3. Create a per-service "Service" security group, attaching it to the services' tasks, allowing access from the LB on the specified ports

Why is this better

  1. Other sensitive tasks in the same network as port-exposing ones remain unaccessible, except for other tasks on the network
  2. The port-exposing tasks are only accessible by the LB (not outside access)
  3. The LB is exposed to the outside, and in turn can only access the port-exposing tasks (and not other tasks on the same networks)

Related issue

Solves #1783

Manual testing
A bit long:

Modify the slightly-complex stack to have a real VPC and subnets, then create it:

set -eu
cd ecs
go test . -update
perl -pe 's/vpc-123/vpc-XXXXXXX/ ; s/subnet1/subnet-YYYYY/ ; s/subnet2/subnet-ZZZZZ' -i testdata/slightly-complex-cloudformation-conversion.golden
aws us-east-1 cloudformation delete-stack --stack-name slightly-complex
aws cloudformation create-stack --stack-name slighlty-complex --template-body file://$(realpath testdata/slightly-complex-cloudformation-conversion.golden) --capabilities CAPABILITY_IAM

Reminder, the Compose looks like this:

services:
  entrance:
    image: nginx
    ports:
      - "80:80"

  sensitive:
    image: httpd

Both tasks have a public IP, but they're not accessible from the outside:
image

~$ curl --connect-timeout 5 52.201.211.172
curl: (28) Connection timeout after 5001 ms

image

$ curl --connect-timeout 5 54.160.87.90
curl: (28) Connection timeout after 5001 ms

However, the LB is accessible and responding:

$ curl sligh-LoadB-MSADFI5M9Z1P-479736060.us-east-1.elb.amazonaws.com --head
HTTP/1.1 200 OK
...

(not mandatory) A picture of a cute animal, if possible in relation with what you did
Claro
image

Summary:
Can be used to test service-service interaction, and stacks with more than one service

Test Plan:
This is a unit test

Signed-off-by: Nitzan Raz <[email protected]>
…the ingress rule to other security groups

Solves docker-archive#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]>
@github-actions github-actions bot added the ecs label Jan 11, 2023
@BackSlasher BackSlasher force-pushed the ingress-secgroup branch 2 times, most recently from b295152 to dfe2ac8 Compare January 12, 2023 10:58
@BackSlasher BackSlasher marked this pull request as ready for review January 12, 2023 10:59
@BackSlasher BackSlasher changed the title Ingress secgroup ECS: Redid security group generation for LBs Jan 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant