Only open VPN ports in cloud security groups when enabled

[dguido]

Summary

When ipsec_enabled or wireguard_enabled is false, don't open those ports in cloud provider security groups/firewalls. Previously, ports 500, 4500, and the WireGuard port were always opened regardless of which VPN protocols were actually enabled.

This follows the principle of least privilege - only open ports for services that are actually running.

Changes by Provider

Provider	Approach
EC2	CloudFormation conditions for conditional SecurityGroupIngress resources
Lightsail	Convert to Jinja2 template with conditional port blocks
Azure	Convert to Jinja2 template with conditional security rules
Vultr	Build firewall rules list conditionally using Jinja2
GCE	Build allowed list conditionally using Jinja2
OpenStack	Build security rules list conditionally using Jinja2
CloudStack	Build security rules list conditionally using Jinja2

Notes

• The instance-level iptables firewall already blocked unused ports, so this change improves security posture at the cloud provider level
• Default behavior unchanged - both protocols enabled by default
• Based on approach proposed by @glennschler in the original issue

Fixes #14293

Test plan

• Deploy to EC2 with ipsec_enabled: false - verify ports 500/4500 not in security group
• Deploy to EC2 with wireguard_enabled: false - verify WireGuard port not in security group
• Deploy with both enabled (default) - verify all ports present
• Test at least one other provider (Vultr, GCE, etc.)

🤖 Generated with Claude Code

[claude]

Code Review

This PR implements conditional firewall port opening based on enabled VPN protocols, which is a solid security improvement following the principle of least privilege. The implementation is well-structured across multiple cloud providers with consistent patterns.

✅ Strengths

1. Security Enhancement: Only opening ports for enabled services is a clear security win, reducing attack surface at the cloud provider level (in addition to the existing iptables filtering).

2. Consistent Patterns: The implementation uses appropriate techniques for each provider:

   • CloudFormation conditions for EC2 (native approach)
   • Jinja2 templating for Azure/Lightsail (clean and readable)
   • List concatenation with conditionals for others (functional approach)

3. Backwards Compatible: The default(true) pattern ensures existing deployments aren't affected.

4. Comprehensive Coverage: All major cloud providers are updated (EC2, Lightsail, Azure, GCE, Vultr, OpenStack, CloudStack).

🔍 Code Quality Observations

1. Jinja2 Template Rendering (Azure/Lightsail)

The conversion of static files to .j2 templates is clean. One minor formatting note:

# Azure deployment.json.j2 - good comma placement
}
{% if ipsec_enabled | default(true) | bool %}
,{
  "name": "AllowIPSEC500",

The leading comma approach works but is unconventional. While it solves the "last item" problem in JSON, consider if this might confuse future maintainers. It does work correctly though.

2. EC2 CloudFormation Implementation

# Excellent use of native CloudFormation features
Conditions:
  OpenIpsecPorts: \!Equals ['true', \!Ref IpsecEnabled]
  OpenWireguardPorts: \!Equals ['true', \!Ref WireguardEnabled]

✅ This is the correct way to do conditional resources in CloudFormation. Separating the rules into individual SecurityGroupIngress resources with conditions is cleaner than trying to conditionally include items in an array.

Minor improvement opportunity: The parameter type could be more semantic:

IpsecEnabled:
  Type: String  # Currently
  # Could be more explicit, though String works fine for CloudFormation

3. Complex List Concatenation (GCE, Vultr, etc.)

allowed: >-
  {{
    [{ 'ip_protocol': 'tcp', 'ports': [ssh_port | string] }]
    + ([{ 'ip_protocol': 'udp', 'ports': ['500', '4500'] }] if ipsec_enabled | default(true) | bool else [])
    + ([{ 'ip_protocol': 'udp', 'ports': [wireguard_port | string] }] if wireguard_enabled | default(true) | bool else [])
  }}

This is functionally correct but quite dense. The multi-line formatting helps readability significantly. One consideration:

• Readability vs. Complexity: This pattern is harder to understand at first glance compared to the Jinja2 template approach
• Testing: Harder to test because it's inline YAML/Jinja2 hybrid
• Alternative: Could use set_fact to build the list in multiple steps for clarity

That said, for the number of rules involved, this is acceptable and maintains consistency.

🐛 Potential Issues

1. Missing IPv6 Handling in Some Providers

Looking at the Vultr implementation - it correctly handles both IPv4 and IPv6:

{ 'protocol': 'tcp', 'port': ssh_port, 'ip': 'v4', 'cidr': '0.0.0.0/0' },
{ 'protocol': 'tcp', 'port': ssh_port, 'ip': 'v6', 'cidr': '::/0' }

But GCE doesn't specify IP versions in the diff shown. Double-check that IPv6 rules work correctly across all providers.

2. EC2 Parameter Conversion

IpsecEnabled: "{{ ipsec_enabled | default(true) | string | lower }}"
WireguardEnabled: "{{ wireguard_enabled | default(true) | string | lower }}"

✅ Good catch converting to lowercase strings. CloudFormation's \!Equals is case-sensitive, and Ansible's bool type would render as True/False (capitalized Python).

3. Description Updates

# Before
"description": "Locks inbound down to ssh default port 22.",
# After  
"description": "Allow WireGuard VPN",

✅ Good fix - the old description was clearly copy-pasted and wrong.

🧪 Testing Recommendations

Your test plan in the PR description is good, but I'd suggest adding:

1. Test Protocol Isolation:

   • Deploy with only WireGuard enabled, verify IPsec ports closed
   • Deploy with only IPsec enabled, verify WireGuard port closed
   • Verify VPN still works in each configuration

2. Test Edge Cases:

   • Set both to false (should only open SSH port)
   • Check that existing servers (with both protocols) aren't affected on update

3. Provider-Specific Testing:

   • Test at least one CloudFormation provider (EC2 or Lightsail)
   • Test at least one Jinja2 template provider (Azure)
   • Test at least one list concatenation provider (GCE/Vultr)

4. IPv6 Verification:

   • Ensure IPv6 rules are correct for Vultr and any other dual-stack providers

📝 Documentation Considerations

Consider updating documentation to:

• Mention that disabling a protocol will close its ports at the cloud level
• Note that changing these settings after deployment may require manual security group updates
• Add troubleshooting section for connectivity issues after disabling a protocol

🔒 Security Assessment

✅ No security concerns identified. This change:

• Reduces attack surface (positive)
• Doesn't expose new vulnerabilities
• Maintains defense-in-depth (iptables rules still apply)
• Uses sensible defaults (both protocols enabled)

📊 Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐	Clean, consistent implementation
Security	⭐⭐⭐⭐⭐	Clear security improvement
Testing	⭐⭐⭐⚪	Good plan, but needs execution
Documentation	⭐⭐⭐⚪	Could use updates
Backwards Compat	⭐⭐⭐⭐⭐	Fully compatible

Recommendations

1. Before Merge:

   • Execute the test plan and update PR with results
   • Verify IPv6 behavior across providers
   • Consider adding a note about post-deployment changes to security groups

2. Consider for Future:

   • Add integration tests that verify correct ports are open/closed
   • Update troubleshooting docs with common issues related to disabled protocols

Overall, this is a well-implemented feature that meaningfully improves security posture. The code is clean, the approach is sound, and the backwards compatibility is maintained. Once testing is complete, this should be good to merge.

---

🤖 Generated with Claude Code