-
Notifications
You must be signed in to change notification settings - Fork 0
Add UFW firewall configuration to ConfigureCommand with SSH lockout prevention #101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add ConfigureFirewall variant to ConfigureStep enum - Create configure-firewall.yml.tera template with SSH port variable - Add ssh_port field to InventoryContext for template rendering - Create ConfigureFirewallStep in application layer - Add firewall template rendering to AnsibleTemplateRenderer - Integrate firewall step into ConfigureCommandHandler - Update module exports for new firewall step Co-authored-by: josecelano <[email protected]>
- Fix doc markdown clippy warning in InventoryContext - Apply cargo fmt formatting to all modified files Co-authored-by: josecelano <[email protected]>
PR Review: Configure UFW Firewall Implementation@copilot Thank you for working on this implementation! The PR successfully adds UFW firewall configuration, and all GitHub CI workflows are passing. However, after testing locally with the full E2E test suite, I've identified three critical issues that need to be addressed before merging. 🔍 Issue #1: Template Architecture Violation (Critical)ProblemThe implementation doesn't follow the project's established template architecture pattern. The current implementation adds a generic Current (Incorrect) Implementation// In: src/infrastructure/external_tools/ansible/template/renderer/mod.rs
async fn render_tera_template(
&self,
template_name: &str,
output_name: &str,
context: &InventoryContext, // ❌ Reusing InventoryContext for firewall template
destination_dir: &Path,
) -> Result<(), ConfigurationTemplateError> {
// Ad-hoc inline template rendering
// No type safety, no dedicated context
}Why This is Wrong
Correct Architecture PatternThe project uses a two-layer template architecture for all Tera templates: Layer 1: Template Wrapper (Type-Safe Context)Reference Example: What the Wrapper Does:
Layer 2: Template Renderer (Orchestration)Reference Example: pub struct InventoryTemplateRenderer {
template_manager: Arc<TemplateManager>,
}
impl InventoryTemplateRenderer {
const INVENTORY_TEMPLATE_FILE: &'static str = "inventory.yml.tera";
const INVENTORY_OUTPUT_FILE: &'static str = "inventory.yml";
pub fn render(
&self,
inventory_context: &InventoryContext,
output_dir: &Path,
) -> Result<(), InventoryTemplateError> {
// 1. Load template file
// 2. Create InventoryTemplate with context
// 3. Render to output file
}
}Required Changes for Firewall TemplateYou need to create the same architecture for the firewall playbook. Due to comment length limits, I'll provide the key structure here. Full implementation details are in the PR review file. Required New Files:
Update: Why This Architecture Matters
Documentation References
🐛 Issue #2: Incorrect Ansible Host Pattern (Functional Bug)ProblemThe firewall playbook uses Current (Incorrect)- name: Configure UFW firewall safely
hosts: torrust_servers # ❌ WRONGEvidenceCorrect Implementation- name: Configure UFW firewall safely
hosts: all # ✅ CORRECTReferenceAll other playbooks use
|
Note on Template Architecture ComplexityI recognize that the template architecture described in Issue #1 above may seem overly complex for what is essentially just rendering a simple Ansible playbook with a single variable ( We're aware of this complexity, and it's exactly why we've created issue #19: Refactor Ansible Templates to Variables Pattern. Planned SimplificationIssue #19 proposes refactoring the Ansible template system to use a centralized variables pattern (similar to OpenTofu's
Why This Matters for This PRFor now, please follow the existing architecture pattern as described in Issue #1. This ensures:
Once this PR is merged and issue #18 is closed, we'll tackle issue #19 to simplify the entire template system. The refactoring will eliminate much of the boilerplate while preserving functionality. Timeline
Thank you for your patience with the current architecture! The simplification is coming soon. |
…ttern BREAKING CHANGE: Restructured firewall playbook template rendering - Create proper two-layer template architecture for firewall playbook - Add FirewallPlaybookContext with type-safe SSH port validation - Add FirewallPlaybookTemplate wrapper for template validation - Add FirewallPlaybookTemplateRenderer following inventory pattern - Remove generic render_tera_template method from AnsibleTemplateRenderer - Remove ssh_port field from InventoryContext (use dedicated context) - Fix hosts pattern in configure-firewall.yml.tera (torrust_servers → all) This change ensures consistency with the established template architecture pattern used for inventory.yml.tera, providing better type safety, testability, and maintainability. Addresses all 3 issues from PR review: - Issue #1: Template architecture violation (fixed) - Issue #2: Incorrect Ansible host pattern (fixed) - Issue #3: UFW not active (should be fixed by issue #2) Co-authored-by: josecelano <[email protected]>
All 3 issues have been addressed in commit 91224b5: Issue #1 (Template Architecture): Implemented proper two-layer architecture with Issue #2 (Ansible Host Pattern): Fixed Issue #3 (UFW Not Active): Should be resolved by fixing Issue #2. E2E testing will confirm. The refactoring creates proper separation of concerns with:
All 1062 unit tests passing and all linters pass. |
… VMs The 'Allow SSH service by name' task was using a non-existent UFW application profile 'ssh' which caused the playbook to fail before enabling the firewall. This task was redundant because SSH is already explicitly allowed by port number in the previous task. Fixes Issue #4 identified in PR review - UFW fails with 'Could not find a profile matching ssh'
✅ All Issues Resolved - E2E Tests Pass Successfully@copilot Great work on implementing the three original issues! I've tested your implementation locally with full E2E tests and can confirm:
However, during E2E testing, I discovered a fourth issue that was blocking the firewall from activating. 🐛 Issue 4: UFW Application Profile Not Found (Discovered During E2E Testing)ProblemThe playbook was failing with this error: Root Cause: The task at lines 60-68 in - name: Allow SSH service by name (additional safety measure)
community.general.ufw:
rule: allow
name: ssh # ❌ This UFW app profile doesn't exist in Ubuntu VMsUFW application profiles are defined in Impact
✅ Fix AppliedCommit: Changes: Removed the redundant task (lines 60-68) that was using the non-existent UFW application profile. Rationale: This task was redundant because SSH is already explicitly allowed by port number in the previous task: - name: Allow SSH access on configured port (BEFORE enabling firewall)
community.general.ufw:
rule: allow
port: "{{ssh_port}}"
proto: tcp
comment: "SSH access (configured port {{ssh_port}})"This approach is more robust because:
🧪 Verification ResultsE2E TestsUFW Firewall Status$ lxc exec torrust-tracker-vm-e2e-full -- sudo ufw status verbose
Status: active # ✅ Now active!
Logging: on (low)
Default: deny (incoming), allow (outgoing), deny (routed)
To Action From
-- ------ ----
22/tcp ALLOW IN Anywhere # SSH access (configured port 22)
22/tcp (v6) ALLOW IN Anywhere (v6)SSH Connectivity$ lxc exec torrust-tracker-vm-e2e-full -- echo "SSH works!"
SSH works! # ✅ Perfect!📊 Final Status
All issues are now resolved. The implementation is ready for merge! 🎉 🔍 Testing NotesThese issues demonstrate why local E2E testing with actual LXD VMs is critical for infrastructure code. The UFW app profile issue would not have been caught by:
Only full E2E testing with real Ubuntu VMs revealed the runtime environment issue. |
- Remove iptables permission detection fallback from ConfigureFirewallStep - Add TORRUST_TD_SKIP_FIREWALL_IN_CONTAINER env var to explicitly skip firewall step - Accepts only "true" or "false" (case-sensitive, lowercase) for type safety - E2E config tests automatically set the env var to skip firewall playbook - Update documentation in templates/ansible/README.md UFW/iptables requires kernel capabilities (CAP_NET_ADMIN, CAP_NET_RAW) not available in unprivileged Docker containers. Container-based E2E tests now explicitly skip the firewall configuration step while VM-based tests continue to run it normally.
Testing Results and Fixes ✅I've pulled your changes and completed comprehensive E2E testing. Found two issues that are now fixed and verified working. 🐛 Issues FoundIssue 1: Redundant UFW Task with Non-existent SSH App ProfileProblem: The playbook had a task attempting to allow SSH using UFW's app profile system: - name: Allow SSH service by name
community.general.ufw:
rule: allow
name: sshThis failed because Ubuntu doesn't have a built-in Error: Fix: Removed this redundant task. The playbook already has a task that correctly allows SSH by port number: - name: Allow SSH access
community.general.ufw:
rule: allow
port: '22'
proto: tcpThis approach is more reliable and doesn't depend on app profiles. Issue 2: Container-based E2E Tests Failing with iptables PermissionsProblem: After fixing the template, VM-based tests ( Root Cause: UFW uses iptables internally, which requires kernel capabilities ( Solution: Added environment variable guard to skip firewall configuration in container-based tests:
let skip_firewall = std::env::var("TORRUST_TD_SKIP_FIREWALL_IN_CONTAINER")
.map(|v| v == "true")
.unwrap_or(false);
if skip_firewall {
tracing::warn!("Skipping UFW firewall configuration due to TORRUST_TD_SKIP_FIREWALL_IN_CONTAINER");
} else {
let firewall_step = ConfigureFirewallStep::new(Arc::clone(&ansible_client));
firewall_step.execute(&tracker).await?;
}
std::env::set_var("TORRUST_TD_SKIP_FIREWALL_IN_CONTAINER", "true");✅ Verification ResultsVM-based Tests (e2e-full) - PASSED ✅
Container-based Tests (e2e-config) - PASSED ✅
All Linters - PASSED ✅
📝 Documentation AddedUpdated
🎯 Summary✅ Fixed redundant UFW task using non-existent app profile The firewall configuration is now production-ready with proper handling for both VM and container environments. Great work on the initial implementation! 🚀 |
josecelano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK b8679fe
Configure UFW Firewall - Architecture Refactoring
This PR implements UFW firewall configuration following the correct two-layer template architecture pattern, addressing all feedback from the initial code review.
✅ All Issues Fixed
Issue #1: Template Architecture Violation (CRITICAL) - FIXED
firewall_playbookwrapper withFirewallPlaybookContextandFirewallPlaybookTemplateFirewallPlaybookTemplateRendererfollowing the inventory patternrender_tera_template()methodinventory.yml.teraarchitectureIssue #2: Incorrect Ansible Host Pattern (FUNCTIONAL BUG) - FIXED
hosts: torrust_servers→hosts: allin templateIssue #3: UFW Firewall Not Active - SHOULD BE FIXED
📐 Architecture Implementation
Two-Layer Template Pattern:
Layer 1: Template Wrapper (Type-Safe Context)
Layer 2: Template Renderer (Orchestration)
🔧 Key Changes
New Files Created:
src/infrastructure/external_tools/ansible/template/wrappers/firewall_playbook/mod.rssrc/infrastructure/external_tools/ansible/template/wrappers/firewall_playbook/context.rssrc/infrastructure/external_tools/ansible/template/renderer/firewall_playbook.rsModified Files:
src/infrastructure/external_tools/ansible/template/wrappers/mod.rs- Export new wrappersrc/infrastructure/external_tools/ansible/template/renderer/mod.rs- Use dedicated renderersrc/infrastructure/external_tools/ansible/template/wrappers/inventory/context/mod.rs- Remove ssh_port fieldtemplates/ansible/configure-firewall.yml.tera- Fix hosts patternRemoved:
render_tera_template()method (architecture violation)ssh_portfield fromInventoryContext(wrong abstraction)✅ Quality Checks
🧪 Testing
Unit Tests Added:
FirewallPlaybookContextconstruction and validationFirewallPlaybookTemplaterendering with SSH port substitutionFirewallPlaybookTemplateRendererend-to-end renderingE2E Testing Required:
The firewall will need to be tested in E2E tests to confirm:
ufw statusshows "Status: active"Original prompt
This section details on the original issue you should resolve
<issue_title>Configure UFW Firewall</issue_title>
<issue_description>Implement UFW firewall configuration in the
ConfigureCommandwith comprehensive SSH lockout prevention. This task adds firewall security while ensuring SSH access is preserved through careful configuration sequencing and port management.This is the second phase of system security configuration, following the security updates implementation. It has higher risk due to potential SSH lockout scenarios.
Goals
ConfigureFirewallto theConfigureStepenumSpecifications
Safety-First Implementation
Critical Requirements:
user_inputs.ssh_port(defaults to 22)Domain Integration
Update
ConfigureStepenum insrc/domain/environment/state/configure_failed.rs:New Application Step
Create
src/application/steps/system/configure_firewall.rs:New Ansible Template
Create
templates/ansible/configure-firewall.yml.tera(Tera template for SSH port):{{ ssh_port }}BEFORE enabling firewallImplementation Approach
This is a higher risk implementation that:
Acceptance Criteria
user_inputs.ssh_portvalue correctlyDependencies
Depends On: #17 (Configure Automatic Security Updates)
Parent Epic: #16 - Finish ConfigureCommand - System Security Configuration
Risk Assessment
Medium Risk due to:
Mitigation Strategies:
Estimated Effort: 2-3 days
Full specification: UFW Firewall Documentation</issue_description>
Comments on the Issue (you are @copilot in this section)
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.