From a954b6aaf2a3a344cee990622a632132fc8ac659 Mon Sep 17 00:00:00 2001 From: stevethomas Date: Sat, 23 May 2026 09:42:42 +1000 Subject: [PATCH 1/5] fix: point RDS ingress at the Fargate task SG and remove the dead EC2 SG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The EC2 security group was an EC2/ASG-era leftover: Fargate tasks run in the task SG, so it held no workloads while still opening SSH/22 (to an ipify-derived IP) and HTTP/80. Its only live use was the RDS ingress rule, which authorised 3306 from the EC2 SG — so Fargate tasks were never actually granted DB access. RDS ingress now targets the task SG, applied additively: a tagged rule is ensured and nothing is ever revoked, so any out-of-band access is preserved. The RDS SG step moves to sync:compute (after the task SG it references). Also removes the orphaned launch-template/keyPair helpers in UsesEc2. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Commands/SyncComputeCommand.php | 2 + src/Commands/SyncNetworkCommand.php | 5 +- src/Concerns/UsesEc2.php | 94 -------- src/Enums/SecurityGroup.php | 1 - src/Enums/SecurityGroupRule.php | 3 +- .../Network/SyncEc2SecurityGroupStep.php | 208 ------------------ .../Network/SyncRdsSecurityGroupStep.php | 86 ++++++-- .../Network/SyncRdsSecurityGroupStepTest.php | 166 ++++++++++++++ 8 files changed, 236 insertions(+), 329 deletions(-) delete mode 100644 src/Steps/Network/SyncEc2SecurityGroupStep.php create mode 100644 tests/Unit/Steps/Network/SyncRdsSecurityGroupStepTest.php diff --git a/src/Commands/SyncComputeCommand.php b/src/Commands/SyncComputeCommand.php index 81cf94e..289bb62 100644 --- a/src/Commands/SyncComputeCommand.php +++ b/src/Commands/SyncComputeCommand.php @@ -10,6 +10,8 @@ class SyncComputeCommand extends SyncSteppedCommand Steps\Fargate\SyncEcrRepositoryStep::class, Steps\Fargate\SyncEcsClusterStep::class, Steps\Fargate\SyncTaskSecurityGroupStep::class, + // RDS SG authorises 3306 from the task SG above, so it must run after it. + Steps\Network\SyncRdsSecurityGroupStep::class, Steps\Fargate\SyncLoadBalancerStep::class, Steps\Fargate\SyncTargetGroupStep::class, Steps\Fargate\SyncHttpListenerStep::class, diff --git a/src/Commands/SyncNetworkCommand.php b/src/Commands/SyncNetworkCommand.php index e5cb521..3890194 100644 --- a/src/Commands/SyncNetworkCommand.php +++ b/src/Commands/SyncNetworkCommand.php @@ -25,10 +25,9 @@ class SyncNetworkCommand extends SyncSteppedCommand Steps\Network\SyncDefaultRouteStep::class, Steps\Network\SyncPublicSubnetsAssociationToRouteTableStep::class, - // security groups + // security groups (the RDS SG moved to sync:compute — its ingress source + // is the ECS task SG, which sync:compute provisions) Steps\Network\SyncLoadBalancerSecurityGroupStep::class, - Steps\Network\SyncEc2SecurityGroupStep::class, - Steps\Network\SyncRdsSecurityGroupStep::class, // sns Steps\Network\SyncSnsAlarmTopicStep::class, diff --git a/src/Concerns/UsesEc2.php b/src/Concerns/UsesEc2.php index 07b964e..968f0a2 100644 --- a/src/Concerns/UsesEc2.php +++ b/src/Concerns/UsesEc2.php @@ -6,7 +6,6 @@ use Codinglabs\Yolo\Aws; use Codinglabs\Yolo\Helpers; use Codinglabs\Yolo\Manifest; -use Codinglabs\Yolo\Enums\Iam; use Codinglabs\Yolo\AwsResources; use Codinglabs\Yolo\Enums\PublicSubnets; use Codinglabs\Yolo\Enums\SecurityGroup; @@ -18,22 +17,16 @@ trait UsesEc2 protected static array $internetGateway; - protected static array $launchTemplate; - protected static array $subnets; protected static array $routeTable; protected static array $loadBalancerSecurityGroup; - protected static array $ec2SecurityGroup; - protected static array $ecsTaskSecurityGroup; protected static array $rdsSecurityGroup; - protected static array $keyPair; - public static function availabilityZones(string $region): array { $availabilityZones = Aws::ec2()->describeAvailabilityZones([ @@ -117,20 +110,6 @@ public static function loadBalancerSecurityGroup(): array return static::$loadBalancerSecurityGroup; } - /** - * @throws ResourceDoesNotExistException - */ - public static function ec2SecurityGroup(): array - { - if (isset(static::$ec2SecurityGroup)) { - return static::$ec2SecurityGroup; - } - - static::$ec2SecurityGroup = static::securityGroupByName(Manifest::get('aws.ec2.security-group', SecurityGroup::EC2_SECURITY_GROUP)); - - return static::$ec2SecurityGroup; - } - /** * @throws ResourceDoesNotExistException */ @@ -179,59 +158,6 @@ public static function securityGroupByName(string|BackedEnum $name): array throw new ResourceDoesNotExistException("Could not find Security Group matching name $name"); } - public static function launchTemplate($refresh = false): array - { - if (! $refresh && isset(static::$launchTemplate)) { - return static::$launchTemplate; - } - - $launchTemplates = Aws::ec2()->describeLaunchTemplates([ - 'Filters' => [ - [ - 'Name' => 'launch-template-name', - 'Values' => [Helpers::keyedResourceName(exclusive: false)], - ], - ], - ])['LaunchTemplates']; - - if (count($launchTemplates) === 0) { - throw ResourceDoesNotExistException::make(sprintf('Could not find launch template %s', Helpers::keyedResourceName())) - ->suggest('compute:sync'); - } - - static::$launchTemplate = $launchTemplates[0]; - - return static::$launchTemplate; - } - - public static function launchTemplatePayload(): array - { - return [ - 'LaunchTemplateName' => Helpers::keyedResourceName(exclusive: false), - 'LaunchTemplateData' => [ - 'IamInstanceProfile' => [ - 'Name' => Helpers::keyedResourceName(Iam::INSTANCE_PROFILE, exclusive: false), - ], - 'InstanceType' => Manifest::get('aws.ec2.instance-type'), - 'KeyName' => Manifest::get('aws.ec2.key-pair', Helpers::keyedResourceName(exclusive: false)), - 'SecurityGroupIds' => [ - AwsResources::ec2SecurityGroup()['GroupId'], - ], - 'Monitoring' => [ - 'Enabled' => true, - ], - ], - 'TagSpecifications' => [ - [ - 'ResourceType' => 'launch-template', - ...Aws::tags([ - 'Name' => Helpers::keyedResourceName(), - ]), - ], - ], - ]; - } - public static function vpc(): array { if (isset(static::$vpc)) { @@ -357,24 +283,4 @@ public static function subnetByName(string $name, $relative = true): array throw new ResourceDoesNotExistException("Could not find subnet matching name $fullSubnetName"); } - - public static function keyPair(): array - { - if (isset(static::$keyPair)) { - return static::$keyPair; - } - - $name = Manifest::get('aws.ec2.key-pair', Helpers::keyedResourceName(exclusive: false)); - - foreach (Aws::ec2()->describeKeyPairs()['KeyPairs'] as $keyPair) { - if ($keyPair['KeyName'] === $name) { - static::$keyPair = $keyPair; - - return $keyPair; - } - } - - throw ResourceDoesNotExistException::make("Could not find key pair with name $name") - ->suggest('sync:network'); - } } diff --git a/src/Enums/SecurityGroup.php b/src/Enums/SecurityGroup.php index 1809a65..03632e2 100644 --- a/src/Enums/SecurityGroup.php +++ b/src/Enums/SecurityGroup.php @@ -4,7 +4,6 @@ enum SecurityGroup: string { - case EC2_SECURITY_GROUP = 'ec2-security-group'; case ECS_TASK_SECURITY_GROUP = 'ecs-task-security-group'; case LOAD_BALANCER_SECURITY_GROUP = 'load-balancer-security-group'; case RDS_SECURITY_GROUP = 'rds-security-group'; diff --git a/src/Enums/SecurityGroupRule.php b/src/Enums/SecurityGroupRule.php index 287063c..4b71d0c 100644 --- a/src/Enums/SecurityGroupRule.php +++ b/src/Enums/SecurityGroupRule.php @@ -6,7 +6,6 @@ enum SecurityGroupRule: string { case LOAD_BALANCER_HTTP_RULE = 'load-balancer-http'; case LOAD_BALANCER_HTTPS_RULE = 'load-balancer-https'; - case LOAD_BALANCER_INGRESS_RULE = 'load-balancer-ingress'; case ECS_TASK_LB_INGRESS_RULE = 'ecs-task-lb-ingress'; - case SSH_INGRESS_RULE = 'ssh-ingress'; + case RDS_TASK_INGRESS_RULE = 'rds-task-ingress'; } diff --git a/src/Steps/Network/SyncEc2SecurityGroupStep.php b/src/Steps/Network/SyncEc2SecurityGroupStep.php deleted file mode 100644 index 907ba13..0000000 --- a/src/Steps/Network/SyncEc2SecurityGroupStep.php +++ /dev/null @@ -1,208 +0,0 @@ -value => fn (array $securityGroup) => static::loadBalanceIngressRule($securityGroup), - SecurityGroupRule::SSH_INGRESS_RULE->value => fn (array $securityGroup) => static::sshIngressRule($securityGroup), - ]; - - try { - $securityGroup = AwsResources::ec2SecurityGroup(); - - if (Manifest::has('aws.ec2.security-group')) { - return StepResult::CUSTOM_MANAGED; - } - - foreach ($expectedRules as $tag => $expectedRule) { - $securityGroupRules = Aws::ec2()->describeSecurityGroupRules([ - 'Filters' => [ - [ - 'Name' => 'group-id', - 'Values' => [$securityGroup['GroupId']], - ], - [ - 'Name' => 'tag:yolo:rule-type', - 'Values' => [$tag], - ], - ], - ])['SecurityGroupRules']; - - if (empty($securityGroupRules)) { - // if a rule is missing, add it - if (! Arr::get($options, 'dry-run')) { - Aws::ec2()->authorizeSecurityGroupIngress($expectedRule($securityGroup)); - } else { - return StepResult::OUT_OF_SYNC; - } - } elseif (static::rulesAreDifferent($expectedRule($securityGroup)['IpPermissions'][0], $securityGroupRules[0])) { - if (! Arr::get($options, 'dry-run')) { - $payload = $expectedRule($securityGroup)['IpPermissions'][0]; - - if (isset($payload['UserIdGroupPairs'])) { - $payload['ReferencedGroupId'] = $payload['UserIdGroupPairs'][0]['GroupId']; - $payload['Description'] = $payload['UserIdGroupPairs'][0]['Description']; - - unset($payload['UserIdGroupPairs']); - } - - if (isset($payload['IpRanges'])) { - $payload['CidrIpv4'] = $payload['IpRanges'][0]['CidrIp']; - $payload['Description'] = $payload['IpRanges'][0]['Description']; - - unset($payload['IpRanges']); - } - - Aws::ec2()->modifySecurityGroupRules([ - 'GroupId' => $securityGroup['GroupId'], - 'SecurityGroupRules' => [ - [ - 'SecurityGroupRule' => $payload, - 'SecurityGroupRuleId' => $securityGroupRules[0]['SecurityGroupRuleId'], - ], - ], - ]); - } else { - return StepResult::OUT_OF_SYNC; - } - } - } - - return StepResult::SYNCED; - } catch (ResourceDoesNotExistException) { - if (! Arr::get($options, 'dry-run')) { - if (Manifest::get('aws.ec2.security-group')) { - throw IntegrityCheckException::make('yolo.yml specifies a custom EC2 security group which does not exist'); - } - - $name = Helpers::keyedResourceName(SecurityGroup::EC2_SECURITY_GROUP, exclusive: false); - - Aws::ec2()->createSecurityGroup([ - 'Description' => 'Enable load balancer and SSH traffic', - 'GroupName' => $name, - 'VpcId' => AwsResources::vpc()['VpcId'], - 'TagSpecifications' => [ - [ - 'ResourceType' => 'security-group', - ...Aws::tags([ - 'Name' => $name, - ]), - ], - ], - ]); - - $securityGroup = AwsResources::ec2SecurityGroup(); - - foreach ($expectedRules as $expectedRule) { - Aws::ec2()->authorizeSecurityGroupIngress($expectedRule($securityGroup)); - } - - return StepResult::CREATED; - } - - return Manifest::get('aws.ec2.security-group') - ? StepResult::MANIFEST_INVALID - : StepResult::WOULD_CREATE; - } - } - - protected static function loadBalanceIngressRule(array $securityGroup): array - { - return [ - 'GroupId' => $securityGroup['GroupId'], - 'IpPermissions' => [ - [ - 'IpProtocol' => 'tcp', - 'FromPort' => 80, - 'ToPort' => 80, - 'UserIdGroupPairs' => [ - [ - 'GroupId' => AwsResources::loadBalancerSecurityGroup()['GroupId'], - 'Description' => 'HTTP ingress from the load balancer', - ], - ], - ], - ], - 'TagSpecifications' => [ - [ - 'ResourceType' => 'security-group-rule', - 'Tags' => [ - [ - 'Key' => 'yolo:rule-type', - 'Value' => SecurityGroupRule::LOAD_BALANCER_INGRESS_RULE->value, - ], - ], - ], - ], - ]; - } - - protected static function sshIngressRule(array $securityGroup): array - { - $publicIp = file_get_contents('https://api.ipify.org'); - - return [ - 'GroupId' => $securityGroup['GroupId'], - 'IpPermissions' => [ - [ - 'IpProtocol' => 'tcp', - 'FromPort' => 22, - 'ToPort' => 22, - 'IpRanges' => [ - [ - 'CidrIp' => "$publicIp/32", - 'Description' => 'YOLO-determined public IP during sync. Delete if unused.', - ], - ], - ], - ], - 'TagSpecifications' => [ - [ - 'ResourceType' => 'security-group-rule', - 'Tags' => [ - [ - 'Key' => 'yolo:rule-type', - 'Value' => SecurityGroupRule::SSH_INGRESS_RULE->value, - ], - ], - ], - ], - ]; - } - - protected static function rulesAreDifferent(array $expectedRule, array $rule): bool - { - if (isset($expectedRule['UserIdGroupPairs'])) { - // map to ReferencedGroupInfo - $expectedRule['ReferencedGroupInfo'] = [ - 'GroupId' => $expectedRule['UserIdGroupPairs'][0]['GroupId'], - 'UserId' => $rule['GroupOwnerId'], - ]; - unset($expectedRule['UserIdGroupPairs']); - } - - if (isset($expectedRule['IpRanges'])) { - // map existing IP to expected IP - $expectedRule['CidrIpv4'] = $rule['CidrIpv4']; - unset($expectedRule['IpRanges']); - } - - return Helpers::payloadHasDifferences($expectedRule, $rule); - } -} diff --git a/src/Steps/Network/SyncRdsSecurityGroupStep.php b/src/Steps/Network/SyncRdsSecurityGroupStep.php index ab5c99c..df9afd6 100644 --- a/src/Steps/Network/SyncRdsSecurityGroupStep.php +++ b/src/Steps/Network/SyncRdsSecurityGroupStep.php @@ -4,32 +4,48 @@ use Codinglabs\Yolo\Aws; use Illuminate\Support\Arr; +use Codinglabs\Yolo\Aws\Ec2; use Codinglabs\Yolo\Helpers; use Codinglabs\Yolo\Manifest; use Codinglabs\Yolo\AwsResources; use Codinglabs\Yolo\Contracts\Step; use Codinglabs\Yolo\Enums\StepResult; use Codinglabs\Yolo\Enums\SecurityGroup; +use Codinglabs\Yolo\Enums\SecurityGroupRule; +use Codinglabs\Yolo\Resources\Fargate\EcsTaskSecurityGroup; use Codinglabs\Yolo\Exceptions\ResourceDoesNotExistException; +/** + * Provisions the RDS security group and authorises the Fargate tasks to reach + * the database on 3306. Runs in sync:compute (after SyncTaskSecurityGroupStep) + * rather than sync:network, because the ingress source is the ECS task SG, which + * sync:compute creates — the RDS subnet group stays in sync:network. + * + * The ingress rule is managed purely additively: we ensure a tagged + * "3306 from the task SG" rule exists and never revoke anything. Any rule added + * out of band (e.g. a legacy EC2 SG, a bastion, a hand-granted CIDR) is left + * untouched, so this can't sever existing database access. + */ class SyncRdsSecurityGroupStep implements Step { public function __invoke(array $options): StepResult { try { - AwsResources::rdsSecurityGroup(); + $securityGroup = AwsResources::rdsSecurityGroup(); if (Manifest::has('aws.rds.security-group')) { return StepResult::CUSTOM_MANAGED; } + $this->ensureTaskIngressRule($securityGroup['GroupId'], (bool) Arr::get($options, 'dry-run')); + return StepResult::SYNCED; } catch (ResourceDoesNotExistException) { if (! Arr::get($options, 'dry-run')) { $name = Helpers::keyedResourceName(SecurityGroup::RDS_SECURITY_GROUP, exclusive: false); Aws::ec2()->createSecurityGroup([ - 'Description' => 'Enable EC2 to connect to RDS', + 'Description' => 'Enable Fargate tasks to connect to RDS', 'GroupName' => $name, 'VpcId' => AwsResources::vpc()['VpcId'], 'TagSpecifications' => [ @@ -42,25 +58,7 @@ public function __invoke(array $options): StepResult ], ]); - $securityGroup = AwsResources::rdsSecurityGroup(); - - Aws::ec2()->authorizeSecurityGroupIngress([ - 'GroupId' => $securityGroup['GroupId'], - 'IpPermissions' => [ - [ - // Enable EC2 to connect to RDS - 'IpProtocol' => 'tcp', - 'FromPort' => 3306, - 'ToPort' => 3306, - 'UserIdGroupPairs' => [ - [ - 'GroupId' => AwsResources::ec2SecurityGroup()['GroupId'], - 'Description' => 'Enable EC2 to connect to RDS', - ], - ], - ], - ], - ]); + $this->ensureTaskIngressRule(AwsResources::rdsSecurityGroup()['GroupId'], false); return StepResult::CREATED; } @@ -68,4 +66,50 @@ public function __invoke(array $options): StepResult return StepResult::WOULD_CREATE; } } + + /** + * Additively ensure a tagged 3306-from-task-SG ingress rule. Idempotent (keyed + * off the yolo:rule-type tag), never revokes, and a no-op under --dry-run. + */ + protected function ensureTaskIngressRule(string $groupId, bool $dryRun): void + { + $rules = Ec2::securityGroupRules($groupId, SecurityGroupRule::RDS_TASK_INGRESS_RULE->value); + + if (! empty($rules) || $dryRun) { + return; + } + + $taskSecurityGroup = new EcsTaskSecurityGroup(); + + // sync:compute creates the task SG before this step runs; if it's somehow + // absent there's nothing to authorise against — leave it for a later sync. + if (! $taskSecurityGroup->exists()) { + return; + } + + Aws::ec2()->authorizeSecurityGroupIngress([ + 'GroupId' => $groupId, + 'IpPermissions' => [ + [ + 'IpProtocol' => 'tcp', + 'FromPort' => 3306, + 'ToPort' => 3306, + 'UserIdGroupPairs' => [ + [ + 'GroupId' => $taskSecurityGroup->arn(), + 'Description' => 'Enable Fargate tasks to connect to RDS', + ], + ], + ], + ], + 'TagSpecifications' => [ + [ + 'ResourceType' => 'security-group-rule', + 'Tags' => [ + ['Key' => 'yolo:rule-type', 'Value' => SecurityGroupRule::RDS_TASK_INGRESS_RULE->value], + ], + ], + ], + ]); + } } diff --git a/tests/Unit/Steps/Network/SyncRdsSecurityGroupStepTest.php b/tests/Unit/Steps/Network/SyncRdsSecurityGroupStepTest.php new file mode 100644 index 0000000..695c263 --- /dev/null +++ b/tests/Unit/Steps/Network/SyncRdsSecurityGroupStepTest.php @@ -0,0 +1,166 @@ +> $byCommand + * @param array}> $captured + */ +function bindMockEc2Client(array $byCommand, array &$captured): void +{ + $mock = new class($byCommand, $captured) extends MockHandler + { + /** @var array */ + private array $cursors = []; + + public function __construct(protected array $byCommand, protected array &$captured) {} + + public function __invoke(CommandInterface $cmd, $request) + { + $name = $cmd->getName(); + $this->captured[] = ['name' => $name, 'args' => $cmd->toArray()]; + + $entry = $this->byCommand[$name] ?? new Result(); + + if (is_array($entry)) { + $index = min($this->cursors[$name] ?? 0, count($entry) - 1); + $this->cursors[$name] = $index + 1; + $entry = $entry[$index]; + } + + return Create::promiseFor($entry); + } + }; + + Helpers::app()->instance('ec2', new Ec2Client([ + 'region' => 'ap-southeast-2', + 'version' => 'latest', + 'credentials' => false, + 'handler' => $mock, + ])); +} + +function describeRdsAndTaskGroups(): Result +{ + return new Result([ + 'SecurityGroups' => [ + ['GroupName' => 'yolo-testing-rds-security-group', 'GroupId' => 'sg-rds123'], + ['GroupName' => 'yolo-testing-my-app-ecs-task-security-group', 'GroupId' => 'sg-task456'], + ], + ]); +} + +beforeEach(function () { + writeManifest([ + 'aws' => ['account-id' => '111111111111', 'region' => 'ap-southeast-2'], + ]); +}); + +// This test must run first: it relies on AwsResources::rdsSecurityGroup() not yet +// being memoised, so the absent-SG lookup throws and the create branch runs. +it('creates the RDS security group and adds the task-SG ingress rule when absent', function () { + $captured = []; + + bindMockEc2Client([ + 'DescribeSecurityGroups' => [ + new Result(['SecurityGroups' => []]), // first lookup → not found → create + describeRdsAndTaskGroups(), // re-lookup after create (repeats) + ], + 'DescribeVpcs' => new Result(['Vpcs' => [['VpcId' => 'vpc-1']]]), + 'CreateSecurityGroup' => new Result(['GroupId' => 'sg-rds123']), + 'DescribeSecurityGroupRules' => new Result(['SecurityGroupRules' => []]), + 'AuthorizeSecurityGroupIngress' => new Result(), + ], $captured); + + expect((new SyncRdsSecurityGroupStep())([]))->toBe(StepResult::CREATED); + + $names = array_column($captured, 'name'); + expect($names)->toContain('CreateSecurityGroup'); + expect($names)->toContain('AuthorizeSecurityGroupIngress'); + expect($names)->not->toContain('RevokeSecurityGroupIngress'); +}); + +it('additively authorises 3306 from the task security group on an existing RDS SG', function () { + $captured = []; + + bindMockEc2Client([ + 'DescribeSecurityGroups' => describeRdsAndTaskGroups(), + 'DescribeSecurityGroupRules' => new Result(['SecurityGroupRules' => []]), + 'AuthorizeSecurityGroupIngress' => new Result(), + ], $captured); + + expect((new SyncRdsSecurityGroupStep())([]))->toBe(StepResult::SYNCED); + + $authorise = collect($captured)->firstWhere('name', 'AuthorizeSecurityGroupIngress'); + expect($authorise)->not->toBeNull(); + + $permission = $authorise['args']['IpPermissions'][0]; + expect($permission['FromPort'])->toBe(3306); + expect($permission['ToPort'])->toBe(3306); + expect($permission['UserIdGroupPairs'][0]['GroupId'])->toBe('sg-task456'); + expect($authorise['args']['GroupId'])->toBe('sg-rds123'); + expect($authorise['args']['TagSpecifications'][0]['Tags'][0]['Value'])->toBe('rds-task-ingress'); + + // Purely additive — it must never revoke an existing rule. + expect(array_column($captured, 'name'))->not->toContain('RevokeSecurityGroupIngress'); +}); + +it('does not authorise again when the tagged task-SG rule already exists', function () { + $captured = []; + + bindMockEc2Client([ + 'DescribeSecurityGroups' => describeRdsAndTaskGroups(), + 'DescribeSecurityGroupRules' => new Result(['SecurityGroupRules' => [ + ['SecurityGroupRuleId' => 'sgr-existing'], + ]]), + ], $captured); + + (new SyncRdsSecurityGroupStep())([]); + + expect(array_column($captured, 'name')) + ->not->toContain('AuthorizeSecurityGroupIngress') + ->not->toContain('RevokeSecurityGroupIngress'); +}); + +it('treats a manifest-specified RDS security group as custom-managed', function () { + writeManifest([ + 'aws' => [ + 'account-id' => '111111111111', + 'region' => 'ap-southeast-2', + 'rds' => ['security-group' => 'yolo-testing-rds-security-group'], + ], + ]); + + $captured = []; + + bindMockEc2Client([ + 'DescribeSecurityGroups' => describeRdsAndTaskGroups(), + ], $captured); + + expect((new SyncRdsSecurityGroupStep())([]))->toBe(StepResult::CUSTOM_MANAGED); + expect(array_column($captured, 'name'))->not->toContain('AuthorizeSecurityGroupIngress'); +}); + +it('does not authorise during a dry-run', function () { + $captured = []; + + bindMockEc2Client([ + 'DescribeSecurityGroups' => describeRdsAndTaskGroups(), + 'DescribeSecurityGroupRules' => new Result(['SecurityGroupRules' => []]), + ], $captured); + + (new SyncRdsSecurityGroupStep())(['dry-run' => true]); + + expect(array_column($captured, 'name'))->not->toContain('AuthorizeSecurityGroupIngress'); +}); From 13993ed5a251e45f4618c16365d9bba7ff46bef5 Mon Sep 17 00:00:00 2001 From: stevethomas Date: Sat, 23 May 2026 09:42:50 +1000 Subject: [PATCH 2/5] feat: block public access on the artefact and app S3 buckets The artefact bucket stores .env files but, unlike the asset bucket, had no Block Public Access. Enable all four BPA flags plus versioning (recovery and tamper-evidence for .env), reconciled on every sync. New app buckets get BPA on create only, so existing buckets that legitimately serve public objects aren't broken. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Storage/SyncS3ArtefactBucketStep.php | 36 +++- src/Steps/Storage/SyncS3BucketStep.php | 13 ++ .../Storage/SyncS3BucketHardeningTest.php | 169 ++++++++++++++++++ 3 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 tests/Unit/Steps/Storage/SyncS3BucketHardeningTest.php diff --git a/src/Steps/Storage/SyncS3ArtefactBucketStep.php b/src/Steps/Storage/SyncS3ArtefactBucketStep.php index 595dcba..e7a55ab 100644 --- a/src/Steps/Storage/SyncS3ArtefactBucketStep.php +++ b/src/Steps/Storage/SyncS3ArtefactBucketStep.php @@ -15,13 +15,20 @@ class SyncS3ArtefactBucketStep implements Step public function __invoke(array $options): StepResult { $bucketName = Paths::s3ArtefactsBucket(); + $dryRun = (bool) Arr::get($options, 'dry-run'); try { AwsResources::bucket($bucketName); + // Reconcile the hardening onto the existing bucket — safe to re-apply + // (it's never public and tiny), so older buckets pick it up on sync. + if (! $dryRun) { + $this->harden($bucketName); + } + return StepResult::SYNCED; } catch (ResourceDoesNotExistException $e) { - if (! Arr::get($options, 'dry-run')) { + if (! $dryRun) { Aws::s3()->createBucket([ 'Bucket' => $bucketName, ]); @@ -39,6 +46,8 @@ public function __invoke(array $options): StepResult ], ]); + $this->harden($bucketName); + // todo: this requires the ELB account ID, which is not the same as the account ID. // todo: @see https://docs.aws.amazon.com/elasticloadbalancing/latest/application/enable-access-logging.html // Aws::s3()->putBucketPolicy([ @@ -62,4 +71,29 @@ public function __invoke(array $options): StepResult return StepResult::WOULD_CREATE; } } + + /** + * The artefact bucket holds the application's `.env` files, so it must never + * be publicly reachable and its objects should be recoverable. Lock down all + * four Block Public Access settings and enable versioning (recovery + + * tamper-evidence for a clobbered or malicious `.env`). Both are declarative + * puts — idempotent, safe to re-apply on every sync. + */ + protected function harden(string $bucketName): void + { + Aws::s3()->putPublicAccessBlock([ + 'Bucket' => $bucketName, + 'PublicAccessBlockConfiguration' => [ + 'BlockPublicAcls' => true, + 'IgnorePublicAcls' => true, + 'BlockPublicPolicy' => true, + 'RestrictPublicBuckets' => true, + ], + ]); + + Aws::s3()->putBucketVersioning([ + 'Bucket' => $bucketName, + 'VersioningConfiguration' => ['Status' => 'Enabled'], + ]); + } } diff --git a/src/Steps/Storage/SyncS3BucketStep.php b/src/Steps/Storage/SyncS3BucketStep.php index 11447d5..ac9a1f5 100644 --- a/src/Steps/Storage/SyncS3BucketStep.php +++ b/src/Steps/Storage/SyncS3BucketStep.php @@ -38,6 +38,19 @@ public function __invoke(array $options): StepResult 'Bucket' => $bucketName, ]); + // Secure-by-default for new app buckets. Deliberately NOT reconciled + // onto existing buckets — an app may already serve public objects, and + // flipping Block Public Access under it would break that. + Aws::s3()->putPublicAccessBlock([ + 'Bucket' => $bucketName, + 'PublicAccessBlockConfiguration' => [ + 'BlockPublicAcls' => true, + 'IgnorePublicAcls' => true, + 'BlockPublicPolicy' => true, + 'RestrictPublicBuckets' => true, + ], + ]); + Aws::s3()->putBucketTagging([ 'Bucket' => $bucketName, 'Tagging' => [ diff --git a/tests/Unit/Steps/Storage/SyncS3BucketHardeningTest.php b/tests/Unit/Steps/Storage/SyncS3BucketHardeningTest.php new file mode 100644 index 0000000..8132f97 --- /dev/null +++ b/tests/Unit/Steps/Storage/SyncS3BucketHardeningTest.php @@ -0,0 +1,169 @@ +> $byCommand + * @param array}> $captured + */ +function bindMockS3Client(array $byCommand, array &$captured): void +{ + $mock = new class($byCommand, $captured) extends MockHandler + { + /** @var array */ + private array $cursors = []; + + public function __construct(protected array $byCommand, protected array &$captured) {} + + public function __invoke(CommandInterface $cmd, $request) + { + $name = $cmd->getName(); + $this->captured[] = ['name' => $name, 'args' => $cmd->toArray()]; + + $entry = $this->byCommand[$name] ?? new Result(); + + if (is_array($entry)) { + $index = min($this->cursors[$name] ?? 0, count($entry) - 1); + $this->cursors[$name] = $index + 1; + $entry = $entry[$index]; + } + + return $entry instanceof Throwable + ? Create::rejectionFor($entry) + : Create::promiseFor($entry); + } + }; + + Helpers::app()->instance('s3', new S3Client([ + 'region' => 'ap-southeast-2', + 'version' => 'latest', + 'credentials' => false, + 'handler' => $mock, + ])); +} + +function s3NotFound(): S3Exception +{ + return new S3Exception('Not Found', new Command('HeadBucket'), [ + 'response' => new Response(404), + ]); +} + +beforeEach(function () { + writeManifest([ + 'aws' => ['account-id' => '111111111111', 'region' => 'ap-southeast-2'], + ]); +}); + +it('locks down and versions a newly created artefact bucket', function () { + $captured = []; + + bindMockS3Client([ + // missing on the first check, then present for the BucketExists waiter + // (which matches on a 200 status). + 'HeadBucket' => [s3NotFound(), new Result(['@metadata' => ['statusCode' => 200]])], + 'CreateBucket' => new Result(), + 'PutBucketTagging' => new Result(), + 'PutPublicAccessBlock' => new Result(), + 'PutBucketVersioning' => new Result(), + ], $captured); + + expect((new SyncS3ArtefactBucketStep())([]))->toBe(StepResult::CREATED); + + $names = array_column($captured, 'name'); + expect($names)->toContain('CreateBucket') + ->toContain('PutPublicAccessBlock') + ->toContain('PutBucketVersioning'); + + $blockPublicAccess = collect($captured)->firstWhere('name', 'PutPublicAccessBlock'); + expect($blockPublicAccess['args']['PublicAccessBlockConfiguration'])->toBe([ + 'BlockPublicAcls' => true, + 'IgnorePublicAcls' => true, + 'BlockPublicPolicy' => true, + 'RestrictPublicBuckets' => true, + ]); + + $versioning = collect($captured)->firstWhere('name', 'PutBucketVersioning'); + expect($versioning['args']['VersioningConfiguration']['Status'])->toBe('Enabled'); +}); + +it('reconciles BPA and versioning onto an existing artefact bucket', function () { + $captured = []; + + bindMockS3Client([ + 'HeadBucket' => new Result(), // exists + 'PutPublicAccessBlock' => new Result(), + 'PutBucketVersioning' => new Result(), + ], $captured); + + expect((new SyncS3ArtefactBucketStep())([]))->toBe(StepResult::SYNCED); + + $names = array_column($captured, 'name'); + expect($names)->toContain('PutPublicAccessBlock') + ->toContain('PutBucketVersioning') + ->not->toContain('CreateBucket'); +}); + +it('does not mutate the artefact bucket during a dry-run', function () { + $captured = []; + + bindMockS3Client([ + 'HeadBucket' => new Result(), // exists + ], $captured); + + expect((new SyncS3ArtefactBucketStep())(['dry-run' => true]))->toBe(StepResult::SYNCED); + + $names = array_column($captured, 'name'); + expect($names)->not->toContain('PutPublicAccessBlock') + ->not->toContain('PutBucketVersioning'); +}); + +it('blocks public access on a newly created app bucket', function () { + writeManifest([ + 'aws' => ['account-id' => '111111111111', 'region' => 'ap-southeast-2', 'bucket' => 'my-app-bucket'], + ]); + + $captured = []; + + bindMockS3Client([ + 'HeadBucket' => [s3NotFound(), new Result(['@metadata' => ['statusCode' => 200]])], + 'CreateBucket' => new Result(), + 'PutBucketTagging' => new Result(), + 'PutPublicAccessBlock' => new Result(), + ], $captured); + + expect((new SyncS3BucketStep())([]))->toBe(StepResult::CREATED); + expect(array_column($captured, 'name'))->toContain('PutPublicAccessBlock'); +}); + +it('does not flip public access on an existing app bucket', function () { + writeManifest([ + 'aws' => ['account-id' => '111111111111', 'region' => 'ap-southeast-2', 'bucket' => 'my-app-bucket'], + ]); + + $captured = []; + + bindMockS3Client([ + 'HeadBucket' => new Result(), // exists + ], $captured); + + expect((new SyncS3BucketStep())([]))->toBe(StepResult::SYNCED); + expect(array_column($captured, 'name'))->not->toContain('PutPublicAccessBlock'); +}); From d845a5df0cdce67b4bbae727ab8512c990de26ae Mon Sep 17 00:00:00 2001 From: stevethomas Date: Sat, 23 May 2026 10:54:39 +1000 Subject: [PATCH 3/5] chore: trim the sync step-ordering comments per review Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Commands/SyncComputeCommand.php | 1 - src/Commands/SyncNetworkCommand.php | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Commands/SyncComputeCommand.php b/src/Commands/SyncComputeCommand.php index 289bb62..700f358 100644 --- a/src/Commands/SyncComputeCommand.php +++ b/src/Commands/SyncComputeCommand.php @@ -10,7 +10,6 @@ class SyncComputeCommand extends SyncSteppedCommand Steps\Fargate\SyncEcrRepositoryStep::class, Steps\Fargate\SyncEcsClusterStep::class, Steps\Fargate\SyncTaskSecurityGroupStep::class, - // RDS SG authorises 3306 from the task SG above, so it must run after it. Steps\Network\SyncRdsSecurityGroupStep::class, Steps\Fargate\SyncLoadBalancerStep::class, Steps\Fargate\SyncTargetGroupStep::class, diff --git a/src/Commands/SyncNetworkCommand.php b/src/Commands/SyncNetworkCommand.php index 3890194..53a1b77 100644 --- a/src/Commands/SyncNetworkCommand.php +++ b/src/Commands/SyncNetworkCommand.php @@ -25,8 +25,7 @@ class SyncNetworkCommand extends SyncSteppedCommand Steps\Network\SyncDefaultRouteStep::class, Steps\Network\SyncPublicSubnetsAssociationToRouteTableStep::class, - // security groups (the RDS SG moved to sync:compute — its ingress source - // is the ECS task SG, which sync:compute provisions) + // security groups Steps\Network\SyncLoadBalancerSecurityGroupStep::class, // sns From 0e425be15729629f7244db51cabe8c68610a57ad Mon Sep 17 00:00:00 2001 From: stevethomas Date: Sat, 23 May 2026 11:00:44 +1000 Subject: [PATCH 4/5] refactor: let the task SG name lookup ensure existence, drop the guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The name-based arn() lookup already throws ResourceDoesNotExistException if the task SG is missing — the defensive exists() guard was both redundant (sync:compute provisions it first) and wrong, silently swallowing a real failure instead of surfacing it. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Steps/Network/SyncRdsSecurityGroupStep.php | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/Steps/Network/SyncRdsSecurityGroupStep.php b/src/Steps/Network/SyncRdsSecurityGroupStep.php index df9afd6..8b8b41a 100644 --- a/src/Steps/Network/SyncRdsSecurityGroupStep.php +++ b/src/Steps/Network/SyncRdsSecurityGroupStep.php @@ -79,14 +79,6 @@ protected function ensureTaskIngressRule(string $groupId, bool $dryRun): void return; } - $taskSecurityGroup = new EcsTaskSecurityGroup(); - - // sync:compute creates the task SG before this step runs; if it's somehow - // absent there's nothing to authorise against — leave it for a later sync. - if (! $taskSecurityGroup->exists()) { - return; - } - Aws::ec2()->authorizeSecurityGroupIngress([ 'GroupId' => $groupId, 'IpPermissions' => [ @@ -96,7 +88,9 @@ protected function ensureTaskIngressRule(string $groupId, bool $dryRun): void 'ToPort' => 3306, 'UserIdGroupPairs' => [ [ - 'GroupId' => $taskSecurityGroup->arn(), + // Name lookup throws ResourceDoesNotExistException if the + // task SG is missing — sync:compute provisions it first. + 'GroupId' => (new EcsTaskSecurityGroup())->arn(), 'Description' => 'Enable Fargate tasks to connect to RDS', ], ], From 265efc4351fdd2cd100e912dabac8c046a5cba83 Mon Sep 17 00:00:00 2001 From: stevethomas Date: Sat, 23 May 2026 11:31:41 +1000 Subject: [PATCH 5/5] refactor: drop the rule-type tag, match the RDS ingress rule by content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tag only served to pre-check whether the rule already existed — a guarantee AWS already provides (it rejects duplicate permissions). That made it the same redundant pre-check as the exists() guard, and less robust: an untagged, manually added identical rule slips past the tag filter and then fails on the duplicate. Idempotency is now by content (protocol + port + source SG), which also spots out-of-band duplicates. The tag earns its place only for drift reconciliation (SyncLoadBalancerSecurityGroupStep), which this step doesn't do. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Enums/SecurityGroupRule.php | 1 - .../Network/SyncRdsSecurityGroupStep.php | 43 ++++++++++--------- .../Network/SyncRdsSecurityGroupStepTest.php | 14 ++++-- 3 files changed, 34 insertions(+), 24 deletions(-) diff --git a/src/Enums/SecurityGroupRule.php b/src/Enums/SecurityGroupRule.php index 4b71d0c..aa4f0e2 100644 --- a/src/Enums/SecurityGroupRule.php +++ b/src/Enums/SecurityGroupRule.php @@ -7,5 +7,4 @@ enum SecurityGroupRule: string case LOAD_BALANCER_HTTP_RULE = 'load-balancer-http'; case LOAD_BALANCER_HTTPS_RULE = 'load-balancer-https'; case ECS_TASK_LB_INGRESS_RULE = 'ecs-task-lb-ingress'; - case RDS_TASK_INGRESS_RULE = 'rds-task-ingress'; } diff --git a/src/Steps/Network/SyncRdsSecurityGroupStep.php b/src/Steps/Network/SyncRdsSecurityGroupStep.php index 8b8b41a..7225c97 100644 --- a/src/Steps/Network/SyncRdsSecurityGroupStep.php +++ b/src/Steps/Network/SyncRdsSecurityGroupStep.php @@ -11,7 +11,6 @@ use Codinglabs\Yolo\Contracts\Step; use Codinglabs\Yolo\Enums\StepResult; use Codinglabs\Yolo\Enums\SecurityGroup; -use Codinglabs\Yolo\Enums\SecurityGroupRule; use Codinglabs\Yolo\Resources\Fargate\EcsTaskSecurityGroup; use Codinglabs\Yolo\Exceptions\ResourceDoesNotExistException; @@ -21,10 +20,10 @@ * rather than sync:network, because the ingress source is the ECS task SG, which * sync:compute creates — the RDS subnet group stays in sync:network. * - * The ingress rule is managed purely additively: we ensure a tagged - * "3306 from the task SG" rule exists and never revoke anything. Any rule added - * out of band (e.g. a legacy EC2 SG, a bastion, a hand-granted CIDR) is left - * untouched, so this can't sever existing database access. + * The ingress rule is managed purely additively: we ensure a "3306 from the task + * SG" rule exists and never revoke anything. Any rule added out of band (e.g. a + * legacy EC2 SG, a bastion, a hand-granted CIDR) is left untouched, so this can't + * sever existing database access. */ class SyncRdsSecurityGroupStep implements Step { @@ -68,14 +67,28 @@ public function __invoke(array $options): StepResult } /** - * Additively ensure a tagged 3306-from-task-SG ingress rule. Idempotent (keyed - * off the yolo:rule-type tag), never revokes, and a no-op under --dry-run. + * Additively ensure a 3306-from-task-SG ingress rule exists, identified by its + * content (AWS rejects duplicate permissions anyway, so no marker tag is + * needed). Never revokes, and a no-op under --dry-run. */ protected function ensureTaskIngressRule(string $groupId, bool $dryRun): void { - $rules = Ec2::securityGroupRules($groupId, SecurityGroupRule::RDS_TASK_INGRESS_RULE->value); + if ($dryRun) { + return; + } + + // Name lookup throws ResourceDoesNotExistException if the task SG is + // missing — sync:compute provisions it before this step runs. + $taskSecurityGroupId = (new EcsTaskSecurityGroup())->arn(); + + $alreadyAuthorised = collect(Ec2::securityGroupRules($groupId))->contains( + fn (array $rule) => ! ($rule['IsEgress'] ?? false) + && ($rule['IpProtocol'] ?? null) === 'tcp' + && ($rule['FromPort'] ?? null) === 3306 + && ($rule['ReferencedGroupInfo']['GroupId'] ?? null) === $taskSecurityGroupId + ); - if (! empty($rules) || $dryRun) { + if ($alreadyAuthorised) { return; } @@ -88,22 +101,12 @@ protected function ensureTaskIngressRule(string $groupId, bool $dryRun): void 'ToPort' => 3306, 'UserIdGroupPairs' => [ [ - // Name lookup throws ResourceDoesNotExistException if the - // task SG is missing — sync:compute provisions it first. - 'GroupId' => (new EcsTaskSecurityGroup())->arn(), + 'GroupId' => $taskSecurityGroupId, 'Description' => 'Enable Fargate tasks to connect to RDS', ], ], ], ], - 'TagSpecifications' => [ - [ - 'ResourceType' => 'security-group-rule', - 'Tags' => [ - ['Key' => 'yolo:rule-type', 'Value' => SecurityGroupRule::RDS_TASK_INGRESS_RULE->value], - ], - ], - ], ]); } } diff --git a/tests/Unit/Steps/Network/SyncRdsSecurityGroupStepTest.php b/tests/Unit/Steps/Network/SyncRdsSecurityGroupStepTest.php index 695c263..1bdb376 100644 --- a/tests/Unit/Steps/Network/SyncRdsSecurityGroupStepTest.php +++ b/tests/Unit/Steps/Network/SyncRdsSecurityGroupStepTest.php @@ -110,19 +110,27 @@ function describeRdsAndTaskGroups(): Result expect($permission['ToPort'])->toBe(3306); expect($permission['UserIdGroupPairs'][0]['GroupId'])->toBe('sg-task456'); expect($authorise['args']['GroupId'])->toBe('sg-rds123'); - expect($authorise['args']['TagSpecifications'][0]['Tags'][0]['Value'])->toBe('rds-task-ingress'); // Purely additive — it must never revoke an existing rule. expect(array_column($captured, 'name'))->not->toContain('RevokeSecurityGroupIngress'); }); -it('does not authorise again when the tagged task-SG rule already exists', function () { +it('does not authorise again when a matching task-SG rule already exists', function () { $captured = []; bindMockEc2Client([ 'DescribeSecurityGroups' => describeRdsAndTaskGroups(), + // An existing 3306-from-task-SG rule — note it carries no marker tag, so + // matching by content (not a tag) is what lets us spot it. 'DescribeSecurityGroupRules' => new Result(['SecurityGroupRules' => [ - ['SecurityGroupRuleId' => 'sgr-existing'], + [ + 'SecurityGroupRuleId' => 'sgr-existing', + 'IsEgress' => false, + 'IpProtocol' => 'tcp', + 'FromPort' => 3306, + 'ToPort' => 3306, + 'ReferencedGroupInfo' => ['GroupId' => 'sg-task456'], + ], ]]), ], $captured);