feat!: add custom egress rules to docker-autoscaler security group

[ikarlashov]

Add custom egress rules to docker-autoscaler security group and remove condition to provision unused docker-machine security group.

Description

1. By default, the module provisions a security group for Docker Autoscaler workers with egress rules that allow ALL traffic. Unlike ingress rules, egress rules are not customizable, which poses a significant security concern. This PR introduces the ability to customize egress rules for the Docker Autoscaler workers' security group by declaring a separate variable for docker-autoscaler egress rules.

2. var.runner_worker_docker_autoscaler_asg becomes bulky and hard to read. Considering the complexity of the ingress rules structure, it makes sense to create a separate variable var.runner_worker_docker_autoscaler_ingress_rules for ingress rules. This way we will follow variable convention for existing security group rules variables, i.e. var.runner_worker_docker_machine_extra_egress_rules. In the result of the change: var.runner_worker_docker_autoscaler_asg.sg_ingresses is removed and its content should be moved to var.runner_worker_docker_autoscaler_ingress_rules.

3. Adding var.runner_networking_ingress_rules to manage Ingress rules for runner-manager SG.

4. Changed runner-manager Egress rules' var name and its spec. var.runner_networking_egress_rules has migrated to var.runner_manager_egress_rules with a new spec.

5. Additionally, PR removes the condition that provisions an unused security group intended solely for Docker Machine setup.

Migrations required

Yes

1. Move all docker-autoscaler ingress rules declaration from var.runner_worker_docker_autoscaler_asg.sg_ingresses to var.runner_worker_docker_autoscaler_ingress_rules.
2. var.runner_networking_egress_rules migrated to var.runner_manager_egress_rules with a new spec.

Verification

runner_worker_docker_autoscaler_ingress_rules =  [
    {
      cidr_block      = "10.0.0.0/8"  # Example CIDR block for a private network in Amazon
      from_port        = 22
      protocol         = "tcp"
      to_port          = 22
      description      = "Allow SSH Ingress traffic for private network."
    }
]

runner_worker_docker_autoscaler_egress_rules = [
     {
      cidr_block    = "0.0.0.0/0"
      from_port     = 443
      protocol        = "tcp"
      to_port          = 443
      description   = "Allow HTTPS egress traffic."
    },
   {
    ipv6_cidr_block = "::/0"
    description         = "Allow HTTPS Egress everywhere"
    from_port           = 443
    protocol              = "tcp"
    to_port                =  443
   }
]

[github-actions]

Hey @ikarlashov! 👋

Thank you for your contribution to the project. Please refer to the contribution rules for a quick overview of the process.

Make sure that this PR clearly explains:

• the problem being solved
• the best way a reviewer and you can test your changes

With submitting this PR you confirm that you hold the rights of the code added and agree that it will published under this LICENSE.

The following ChatOps commands are supported:

• /help: notifies a maintainer to help you out

Simply add a comment with the command in the first line. If you need to pass more information, separate it with a blank line from the command.

This message was generated automatically. You are welcome to improve it.

[kayman-mk]

Thanks for your work to improve this module. I noticed the major change, but I think we can go on as it is easy to handle for the users.

[kayman-mk]

question: any specific reason for this default? As you already wrote, it might introduce a vulnerability.

suggestion: get rid of this default.

[ikarlashov]

I left it for lazy people who don't care about setting specific egresses and just want to have an ability to use the module right away. I.e. to pull docker images from Internet.
I can remove it, no problem.

[ikarlashov]

Maybe as a compromise solution we can set default to "Allow Egress to All for port 443 only"?

[kayman-mk]

Sounds good to me, yes.

[ikarlashov]

It would be nice to allow traffic within runner-manager ASG and Docker-autoscaler workers ASG. Basically add this to security_groups.tf:

resource "aws_vpc_security_group_egress_rule" "runner_manager_to_docker_autoscaler_egress" {
  count = var.runner_worker.type == "docker-autoscaler" ? 1 : 0

  security_group_id            = aws_security_group.runner.id
  from_port                    = 0
  to_port                      = 0
  ip_protocol                  = "-1"
  description                  = "Allow ALL Egress traffic between Runner Manager and Docker-autoscaler workers security group"
  referenced_security_group_id = aws_security_group.docker_autoscaler[0].id
}

But if I add it, terraform doesn't detect any changes for runner's SG.

[kayman-mk]

It would be nice to allow traffic within runner-manager ASG and Docker-autoscaler workers ASG. Basically add this to security_groups.tf:

resource "aws_vpc_security_group_egress_rule" "runner_manager_to_docker_autoscaler_egress" {
  count = var.runner_worker.type == "docker-autoscaler" ? 1 : 0

  security_group_id            = aws_security_group.runner.id
  from_port                    = 0
  to_port                      = 0
  ip_protocol                  = "-1"
  description                  = "Allow ALL Egress traffic between Runner Manager and Docker-autoscaler workers security group"
  referenced_security_group_id = aws_security_group.docker_autoscaler[0].id
}

But if I add it, terraform doesn't detect any changes for runner's SG.

Perhaps it is already in place?

[ikarlashov]

It would be nice to allow traffic within runner-manager ASG and Docker-autoscaler workers ASG. Basically add this to security_groups.tf:

resource "aws_vpc_security_group_egress_rule" "runner_manager_to_docker_autoscaler_egress" {
  count = var.runner_worker.type == "docker-autoscaler" ? 1 : 0

  security_group_id            = aws_security_group.runner.id
  from_port                    = 0
  to_port                      = 0
  ip_protocol                  = "-1"
  description                  = "Allow ALL Egress traffic between Runner Manager and Docker-autoscaler workers security group"
  referenced_security_group_id = aws_security_group.docker_autoscaler[0].id
}

But if I add it, terraform doesn't detect any changes for runner's SG.

Perhaps it is already in place?

If you don't set var.runner_networking_egress_rules, then the runner manager SG won't have any outbound rules. As a result, the runner manager won't be able to connect to the docker-autoscaler workers. This PR already includes an ingress rule in the docker-autoscaler workers SG to allow traffic between the runner manager and the docker-autoscaler workers SG. But that's not enough. Specifying an egress rule to "allow SSH" in var.runner_networking_egress_rules is still required.

[kayman-mk]

Shouldn't we add this egress rule automatically? Otherwise the applied result doesn't work.

[ikarlashov]

If I add:

resource "aws_security_group_rule" "runner_manager_to_docker_autoscaler_egress" {
  count = var.runner_worker.type == "docker-autoscaler" ? 1 : 0

  type                     = "egress"
  from_port                = 0
  to_port                  = 0
  protocol                 = "-1"
  security_group_id        = aws_security_group.runner.id
  source_security_group_id = aws_security_group.docker_autoscaler[0].id

  description = "Allow ALL Egress traffic between Runner Manager and Docker-autoscaler workers security group"
}

the rule will be added to SG upon the first run and removed upon the 2nd. And so on. I've tested multiple times.

This won't work at all:

 resource "aws_vpc_security_group_egress_rule" "runner_manager_to_docker_autoscaler_egress" {
   count = var.runner_worker.type == "docker-autoscaler" ? 1 : 0

   security_group_id            = aws_security_group.runner.id
   from_port                    = 0
   to_port                      = 0
   ip_protocol                  = "-1"
   description                  = "Allow ALL Egress traffic between Runner Manager and Docker-autoscaler workers security group"
   referenced_security_group_id = aws_security_group.docker_autoscaler[0].id
 }

If we leave the default egress values of runner_networking_egress_rules, which is "ALLOW ALL", then user-defined egress rules in runner_networking_egress_rules will override "ALLOW ALL" and we still won't get an Egress connection via port 22.

So the only way to add the rule to runner-manager's SG is to refactor security-group resource by removing all dynamic blocks.

And that will be another breaking change. What do you think?

[kayman-mk]

Sounds good to me as these inline egress is not best practice. Using separate resources is much better.

[ikarlashov]

Alright. I think it's ready for another round of review (: