Skip to content

Commit ccb1955

Browse files
authored
fix(ecs_patterns): openListener should be false when custom sg is provided (#35297)
### Issue # (if applicable) Closes #35292. ### Reason for this change This change addresses a security vulnerability in the ECS patterns where providing a custom load balancer would still result in the creation of overly permissive internet access rules, potentially bypassing user-intended security controls. **The Security Issue:** The `openListener` property controls whether CDK automatically creates security group ingress rules allowing internet traffic to reach the load balancer: - **`openListener: true` (current default)**: CDK automatically creates an `AWS::EC2::SecurityGroupIngress` rule with `CidrIp: '0.0.0.0/0'`, allowing **anyone on the internet** to access the load balancer on the listener port - **`openListener: false`**: CDK does **not** create automatic ingress rules, leaving security group management entirely to the user **The Problem:** Currently, `openListener` always defaults to `true`, regardless of whether users have provided custom load balancers with their own security groups. This creates a security gap where: 1. Users carefully configure custom load balancers with restrictive security groups 2. CDK still automatically adds `0.0.0.0/0` ingress rules, potentially bypassing the user's intended access controls 3. The load balancer becomes accessible from the internet even when users intended to restrict access **Why This Matters:** When users provide custom load balancers, they typically want to manage security group rules themselves. The automatic creation of `0.0.0.0/0` rules can unintentionally expose services to the internet, creating a security vulnerability that users may not immediately notice. ### Description of changes This implementation adds a smart default mechanism for the `openListener` property in `ApplicationLoadBalancedServiceBase` that detects when users provide custom load balancers (which typically have custom security groups) and automatically defaults `openListener` to `false` for improved security. **Key changes made:** - **Smart Default Logic**: Added detection for custom load balancers in `ApplicationLoadBalancedServiceBase.ts` - **Feature Flag Protection**: Implemented `@aws-cdk/aws-ecs-patterns:smartDefaultOpenListener` feature flag to ensure backward compatibility - **Secure Default Behavior**: When feature flag is enabled and custom load balancer is detected, `openListener` defaults to `false` instead of `true` - **Override Capability**: Users can still explicitly set `openListener: true` to override the smart default if needed - **Comprehensive Testing**: Added 5 new test cases covering all scenarios including backward compatibility **Technical implementation details:** - The smart default logic checks if the feature flag is enabled and if a custom load balancer is provided - When both conditions are met, `openListener` defaults to `false`, preventing automatic creation of `AWS::EC2::SecurityGroupIngress` rules with `CidrIp: '0.0.0.0/0'` - When feature flag is disabled or no custom load balancer is provided, behavior remains unchanged (defaults to `true`) - The implementation uses existing CDK patterns and requires no new dependencies **CloudFormation Impact:** - **Before (or with `openListener: true`)**: CDK generates `AWS::EC2::SecurityGroupIngress` resources allowing internet access (`CidrIp: '0.0.0.0/0'`) - **After (with smart default `openListener: false`)**: No automatic ingress rules are created, users maintain full control over security group configuration **Design decisions made:** - **Feature Flag Approach**: Chose feature flag implementation to ensure zero breaking changes for existing users - **Custom Load Balancer Detection**: Used `props.loadBalancer !== undefined` as the detection mechanism since custom load balancers typically indicate custom security group management - **Explicit Override Support**: Maintained ability for users to explicitly set `openListener: true` to override smart defaults when needed - **Conservative Approach**: Only applies smart default when feature flag is explicitly enabled, ensuring opt-in behavior **Alternatives considered and rejected:** - **Always-on behavior**: Rejected due to potential breaking changes for existing users - **Security group inspection**: Rejected due to complexity and potential for false positives - **Warning-only approach**: Rejected as it doesn't actually fix the security vulnerability **Problem Description:** ```mermaid flowchart TD A[User Creates ECS Service] --> B[User Provides Custom Load Balancer<br/>with Custom Security Groups] B --> C[User Configures Restrictive Rules<br/>e.g., only VPC access] C --> D[CDK Always Defaults openListener = true] D --> E[CDK Creates 0.0.0.0/0 Ingress Rule<br/>⚠️ BYPASSES User's Security Intent] E --> F[🚨 Unintended Internet Exposure<br/>Security Vulnerability] style E fill:#ffebee style F fill:#ffcdd2 ``` **Smart Default Logic (Solution):** ```mermaid flowchart TD A[ECS Service Created] --> B{Feature Flag Enabled?} B -->|No| C[Legacy: openListener = true<br/>Creates 0.0.0.0/0 ingress rules] B -->|Yes| D{Custom Load Balancer Provided?} D -->|No| E[Default: openListener = true<br/>🌐 Creates 0.0.0.0/0 ingress rules] D -->|Yes| F[Smart Default: openListener = false<br/>🔒 No automatic ingress rules] style F fill:#e8f5e8 style E fill:#fff3e0 style C fill:#f5f5f5 ``` ### Describe any new or updated permissions being added N/A - This change does not introduce new IAM permissions or modify existing permission requirements. The change only affects the default behavior of security group rule creation, using existing ECS and ELB permissions. ### Description of how you validated changes **Unit tests**: Added comprehensive test coverage with 5 new test cases: - Smart default with custom load balancer (feature flag enabled) - verifies `openListener` defaults to `false` - Smart default without custom load balancer (feature flag enabled) - verifies `openListener` defaults to `true` - Explicit override with `openListener: true` (feature flag enabled) - verifies explicit values override smart defaults - Redirect listener behavior with smart defaults - verifies redirect functionality works with smart defaults - Backward compatibility test (feature flag disabled) - verifies no behavior change when feature flag is disabled **Integration tests**: Created comprehensive integration test (`integ.alb-fargate-service-smart-defaults.ts`) that: - Deploys 4 different ECS service configurations to validate real AWS behavior - Uses AWS SDK calls to verify actual security group configurations - Confirms that custom security groups do not contain 0.0.0.0/0 ingress rules when smart defaults are applied - Validates that the feature works correctly in real AWS environments - **Integration test results**: Successfully deployed and all assertions passed **Manual validation**: - Verified CloudFormation template generation with and without feature flag enabled - Confirmed that SecurityGroupIngress resources are not created when custom load balancers are used with smart defaults - Tested explicit `openListener: true` override functionality - Validated that existing behavior is unchanged when feature flag is disabled **Regression testing**: - All existing unit tests continue to pass (226/226 tests passed) - Existing integration tests verified to ensure no functionality is broken - Confirmed backward compatibility by testing with feature flag disabled **Performance testing**: - No performance impact - only adds a simple conditional check during construct creation - Build times and synthesis times remain unchanged ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 86638f6 commit ccb1955

File tree

17 files changed

+35701
-3
lines changed

17 files changed

+35701
-3
lines changed

packages/@aws-cdk-testing/framework-integ/test/aws-ecs-patterns/test/fargate/integ.alb-fargate-service-smart-defaults.js.snapshot/albFargateServiceSmartDefaultsTestDefaultTestDeployAssert0E39EDDC.assets.json

Lines changed: 33 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)