Skip to content

Commit dd567d0

Browse files
committed
Merge #101: Add UFW firewall configuration to ConfigureCommand with SSH lockout prevention
b8679fe fix: [#18] skip firewall configuration in container-based E2E tests (Jose Celano) d51b156 fix: [#18] remove redundant UFW app profile task that fails on Ubuntu VMs (Jose Celano) 91224b5 fix: refactor firewall template to follow established architecture pattern (copilot-swe-agent[bot]) 7d91d18 fix: apply code formatting and fix clippy warnings (copilot-swe-agent[bot]) c2829c2 feat: add UFW firewall configuration step (copilot-swe-agent[bot]) 3653e1c Initial plan (copilot-swe-agent[bot]) Pull request description: ## 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 - ✅ Created proper `firewall_playbook` wrapper with `FirewallPlaybookContext` and `FirewallPlaybookTemplate` - ✅ Created dedicated `FirewallPlaybookTemplateRenderer` following the inventory pattern - ✅ Removed generic `render_tera_template()` method - ✅ Type-safe SSH port validation at context construction - ✅ Consistent with `inventory.yml.tera` architecture #### Issue #2: Incorrect Ansible Host Pattern (FUNCTIONAL BUG) - FIXED - ✅ Changed `hosts: torrust_servers` → `hosts: all` in template - ✅ Matches pattern used by all other playbooks #### Issue #3: UFW Firewall Not Active - SHOULD BE FIXED - ✅ Fixed by correcting host pattern (Issue #2) - ⏳ Requires E2E testing to confirm UFW is active ### 📐 Architecture Implementation **Two-Layer Template Pattern:** #### Layer 1: Template Wrapper (Type-Safe Context) ``` src/infrastructure/external_tools/ansible/template/wrappers/firewall_playbook/ ├── mod.rs # FirewallPlaybookTemplate wrapper └── context.rs # FirewallPlaybookContext with type-safe SSH port ``` #### Layer 2: Template Renderer (Orchestration) ``` src/infrastructure/external_tools/ansible/template/renderer/ ├── mod.rs # Main renderer (updated) └── firewall_playbook.rs # FirewallPlaybookTemplateRenderer ``` ### 🔧 Key Changes 1. **New Files Created:** - `src/infrastructure/external_tools/ansible/template/wrappers/firewall_playbook/mod.rs` - `src/infrastructure/external_tools/ansible/template/wrappers/firewall_playbook/context.rs` - `src/infrastructure/external_tools/ansible/template/renderer/firewall_playbook.rs` 2. **Modified Files:** - `src/infrastructure/external_tools/ansible/template/wrappers/mod.rs` - Export new wrapper - `src/infrastructure/external_tools/ansible/template/renderer/mod.rs` - Use dedicated renderer - `src/infrastructure/external_tools/ansible/template/wrappers/inventory/context/mod.rs` - Remove ssh_port field - `templates/ansible/configure-firewall.yml.tera` - Fix hosts pattern 3. **Removed:** - Generic `render_tera_template()` method (architecture violation) - `ssh_port` field from `InventoryContext` (wrong abstraction) ### ✅ Quality Checks - **Build**: ✅ Compiles without warnings - **Tests**: ✅ All 1062 tests passing - **Linters**: ✅ All linters passing (markdown, yaml, toml, cspell, clippy, rustfmt, shellcheck) ### 🧪 Testing **Unit Tests Added:** - `FirewallPlaybookContext` construction and validation - `FirewallPlaybookTemplate` rendering with SSH port substitution - `FirewallPlaybookTemplateRenderer` end-to-end rendering - Error handling for invalid SSH ports and template syntax **E2E Testing Required:** The firewall will need to be tested in E2E tests to confirm: - Playbook runs without "no hosts matched" warning - UFW is active: `ufw status` shows "Status: active" - SSH port 22 is allowed - SSH connection works after firewall enable <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> ---- *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 `ConfigureCommand` with 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 - [ ] **UFW Firewall Active**: Configure UFW with restrictive default policies - [ ] **SSH Access Preserved**: Maintain SSH connectivity throughout configuration - [ ] **Configurable SSH Port**: Support custom SSH ports from user configuration - [ ] **New Domain Step**: Add `ConfigureFirewall` to the `ConfigureStep` enum - [ ] **Ansible Integration**: Create Tera template for SSH port resolution - [ ] **Safety First**: Comprehensive SSH lockout prevention measures - [ ] **Testing**: Extensive E2E testing with SSH connectivity validation ## Specifications ### Safety-First Implementation **Critical Requirements:** 1. **Allow SSH BEFORE enabling firewall** - Never enable UFW before SSH rules are in place 2. **Use configured SSH port** - Support `user_inputs.ssh_port` (defaults to 22) 3. **Dual SSH protection** - Allow both port number and SSH service name 4. **Verify SSH access** - Confirm SSH rules are active before completing ### Domain Integration Update `ConfigureStep` enum in `src/domain/environment/state/configure_failed.rs`: ```rust /// Steps in the configure workflow #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub enum ConfigureStep { /// Installing Docker InstallDocker, /// Installing Docker Compose InstallDockerCompose, /// Configuring automatic security updates ConfigureSecurityUpdates, /// Configuring UFW firewall ConfigureFirewall, // <- NEW } ``` ### New Application Step Create `src/application/steps/system/configure_firewall.rs`: - Implements ConfigureFirewallStep with SSH port variable resolution - Uses AnsibleClient with Tera template for dynamic SSH port - Comprehensive error handling for SSH lockout scenarios - Follows established patterns while handling template variables ### New Ansible Template Create `templates/ansible/configure-firewall.yml.tera` (Tera template for SSH port): - Reset UFW to clean state - Set restrictive default policies (deny incoming, allow outgoing) - **CRITICALLY**: Allow SSH on `{{ ssh_port }}` BEFORE enabling firewall - Allow SSH service by name as additional safety measure - Enable UFW only after SSH rules are confirmed - Verify SSH access is preserved ## Implementation Approach This is a **higher risk** implementation that: - Requires Tera template for SSH port variable resolution - Must prevent SSH lockout through careful sequencing - Needs comprehensive testing with SSH connectivity validation - Follows the template pattern similar to inventory.yml.tera ## Acceptance Criteria - [ ] **UFW Firewall Enabled**: UFW is active with restrictive default policies - [ ] **SSH Access Maintained**: SSH connectivity preserved on configured port - [ ] **Port Configuration**: Uses `user_inputs.ssh_port` value correctly - [ ] **Domain Integration**: ConfigureFirewall step properly integrated - [ ] **SSH Lockout Prevention**: No scenario causes SSH access loss - [ ] **Template Resolution**: SSH port variable correctly resolved in Tera template - [ ] **Error Handling**: Clear, actionable error messages for firewall failures - [ ] **Tests Pass**: All existing tests continue to pass - [ ] **E2E Validation**: E2E tests confirm firewall is active AND SSH works - [ ] **Safety Verification**: SSH rules verified before firewall activation ## Dependencies **Depends On**: #17 (Configure Automatic Security Updates) **Parent Epic**: #16 - Finish ConfigureCommand - System Security Configuration ## Risk Assessment **Medium Risk** due to: - Potential SSH lockout if improperly sequenced - Network-level changes affecting remote access - Tera template complexity for SSH port resolution **Mitigation Strategies**: - Comprehensive E2E testing with SSH verification - Careful implementation sequencing (SSH rules before firewall activation) - Detailed error handling for SSH connectivity issues - Multiple safety checks and verification steps **Estimated Effort**: 2-3 days Full specification: [UFW Firewall Documentation](https://github.com/torrust/torrust-tracker-deployer/blob/main/docs/issues/18-configure-ufw-firewall.md)</issue_description> ## Comments on the Issue (you are @copilot in this section) <comments> </comments> </details> - Fixes #18 <!-- START COPILOT CODING AGENT TIPS --> --- 💡 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](https://gh.io/copilot-coding-agent-tips) in the docs. ACKs for top commit: josecelano: ACK b8679fe Tree-SHA512: 87ff241a7203b01e4d8405ba59ce4820b97771cfcc7be93b7cebd449f5f08bc63a84edae6d5d2530379eaf2e74db54a68d9fa3645ac690b4a8b4a81502e1f4c7
2 parents b8e1024 + b8679fe commit dd567d0

File tree

13 files changed

+1046
-4
lines changed

13 files changed

+1046
-4
lines changed

src/application/command_handlers/configure/handler.rs

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ use super::errors::ConfigureCommandHandlerError;
88
use crate::adapters::ansible::AnsibleClient;
99
use crate::application::command_handlers::common::StepResult;
1010
use crate::application::steps::{
11-
ConfigureSecurityUpdatesStep, InstallDockerComposeStep, InstallDockerStep,
11+
ConfigureFirewallStep, ConfigureSecurityUpdatesStep, InstallDockerComposeStep,
12+
InstallDockerStep,
1213
};
1314
use crate::domain::environment::repository::{EnvironmentRepository, TypedEnvironmentRepository};
1415
use crate::domain::environment::state::{ConfigureFailureContext, ConfigureStep};
@@ -24,6 +25,7 @@ use crate::shared::error::Traceable;
2425
/// 1. Install Docker
2526
/// 2. Install Docker Compose
2627
/// 3. Configure automatic security updates
28+
/// 4. Configure UFW firewall
2729
///
2830
/// # State Management
2931
///
@@ -161,6 +163,27 @@ impl ConfigureCommandHandler {
161163
.execute()
162164
.map_err(|e| (e.into(), current_step))?;
163165

166+
let current_step = ConfigureStep::ConfigureFirewall;
167+
// Allow tests or CI to explicitly skip the firewall configuration step
168+
// (useful for container-based test runs where iptables/ufw require
169+
// elevated kernel capabilities not available in unprivileged containers).
170+
let skip_firewall = std::env::var("TORRUST_TD_SKIP_FIREWALL_IN_CONTAINER")
171+
.map(|v| v == "true")
172+
.unwrap_or(false);
173+
174+
if skip_firewall {
175+
info!(
176+
command = "configure",
177+
step = "configure_firewall",
178+
status = "skipped",
179+
"Skipping UFW firewall configuration due to TORRUST_TD_SKIP_FIREWALL_IN_CONTAINER"
180+
);
181+
} else {
182+
ConfigureFirewallStep::new(Arc::clone(&self.ansible_client))
183+
.execute()
184+
.map_err(|e| (e.into(), current_step))?;
185+
}
186+
164187
// Transition to Configured state
165188
let configured = environment.clone().configured();
166189

src/application/steps/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ pub use rendering::{
3636
RenderAnsibleTemplatesError, RenderAnsibleTemplatesStep, RenderOpenTofuTemplatesStep,
3737
};
3838
pub use software::{InstallDockerComposeStep, InstallDockerStep};
39-
pub use system::{ConfigureSecurityUpdatesStep, WaitForCloudInitStep};
39+
pub use system::{ConfigureFirewallStep, ConfigureSecurityUpdatesStep, WaitForCloudInitStep};
4040
pub use validation::{
4141
ValidateCloudInitCompletionStep, ValidateDockerComposeInstallationStep,
4242
ValidateDockerInstallationStep,
Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
//! UFW firewall configuration step
2+
//!
3+
//! This module provides the `ConfigureFirewallStep` which handles configuration
4+
//! of UFW (Uncomplicated Firewall) on remote hosts via Ansible playbooks.
5+
//! This step ensures that the firewall is configured with restrictive default
6+
//! policies while maintaining SSH access to prevent lockout.
7+
//!
8+
//! ## Key Features
9+
//!
10+
//! - Configures UFW with restrictive default policies (deny incoming, allow outgoing)
11+
//! - Preserves SSH access on the configured port
12+
//! - Uses Tera template for dynamic SSH port resolution
13+
//! - Comprehensive SSH lockout prevention measures
14+
//! - Verification steps to ensure firewall is active and SSH is accessible
15+
//!
16+
//! ## Configuration Process
17+
//!
18+
//! The step executes the "configure-firewall" Ansible playbook which handles:
19+
//! - UFW installation and setup
20+
//! - Reset UFW to clean state
21+
//! - Set restrictive default policies
22+
//! - Allow SSH access BEFORE enabling firewall (critical for preventing lockout)
23+
//! - Enable UFW firewall
24+
//! - Verify firewall status and SSH access
25+
//!
26+
//! ## SSH Lockout Prevention
27+
//!
28+
//! This is a **high-risk operation** that could result in SSH lockout if not
29+
//! handled correctly. Safety measures include:
30+
//!
31+
//! 1. **Correct Sequencing**: SSH rules are added BEFORE enabling firewall
32+
//! 2. **Dual SSH Protection**: Both port-specific and service-name rules
33+
//! 3. **Port Configuration**: Uses actual SSH port from user configuration
34+
//! 4. **Verification Steps**: Ansible tasks verify SSH access is preserved
35+
//! 5. **Comprehensive Logging**: Detailed logging of each firewall step
36+
37+
use std::sync::Arc;
38+
use tracing::{info, instrument, warn};
39+
40+
use crate::adapters::ansible::AnsibleClient;
41+
use crate::shared::command::CommandError;
42+
43+
/// Step that configures UFW firewall on a remote host via Ansible
44+
///
45+
/// This step configures a restrictive UFW firewall policy while ensuring
46+
/// SSH access is maintained. The SSH port is resolved during template rendering
47+
/// and embedded in the final Ansible playbook. The configuration follows the
48+
/// principle of "allow SSH BEFORE enabling firewall" to prevent lockout.
49+
pub struct ConfigureFirewallStep {
50+
ansible_client: Arc<AnsibleClient>,
51+
}
52+
53+
impl ConfigureFirewallStep {
54+
/// Create a new firewall configuration step
55+
///
56+
/// # Arguments
57+
///
58+
/// * `ansible_client` - Ansible client for running playbooks
59+
///
60+
/// # Note
61+
///
62+
/// SSH port configuration is resolved during template rendering phase,
63+
/// not at step execution time. The rendered playbook contains the
64+
/// resolved SSH port value.
65+
#[must_use]
66+
pub fn new(ansible_client: Arc<AnsibleClient>) -> Self {
67+
Self { ansible_client }
68+
}
69+
70+
/// Execute the firewall configuration
71+
///
72+
/// # Safety
73+
///
74+
/// This method is designed to prevent SSH lockout by:
75+
/// 1. Resetting UFW to clean state
76+
/// 2. Allowing SSH access BEFORE enabling firewall
77+
/// 3. Using the correct SSH port from user configuration
78+
///
79+
/// The SSH port is resolved during template rendering and embedded in the
80+
/// playbook, so this method executes a playbook with pre-configured values.
81+
///
82+
/// # Errors
83+
///
84+
/// Returns `CommandError` if:
85+
/// - Ansible playbook execution fails
86+
/// - UFW commands fail
87+
/// - SSH rules cannot be applied
88+
/// - Firewall verification fails
89+
#[instrument(
90+
name = "configure_firewall",
91+
skip_all,
92+
fields(step_type = "system", component = "firewall", method = "ansible")
93+
)]
94+
pub fn execute(&self) -> Result<(), CommandError> {
95+
warn!(
96+
step = "configure_firewall",
97+
action = "configure_ufw",
98+
"Configuring UFW firewall - CRITICAL: SSH access will be restricted to configured port"
99+
);
100+
101+
// Run Ansible playbook (SSH port already resolved during template rendering)
102+
match self.ansible_client.run_playbook("configure-firewall") {
103+
Ok(_) => {
104+
info!(
105+
step = "configure_firewall",
106+
status = "success",
107+
"UFW firewall configured successfully with SSH access preserved"
108+
);
109+
Ok(())
110+
}
111+
Err(e) => {
112+
// Propagate errors to the caller. Tests that run in container environments
113+
// should explicitly opt-out of running this step (for example via an
114+
// environment variable) instead of relying on runtime error detection.
115+
Err(e)
116+
}
117+
}
118+
}
119+
}
120+
121+
#[cfg(test)]
122+
mod tests {
123+
use std::path::PathBuf;
124+
use std::sync::Arc;
125+
126+
use super::*;
127+
128+
#[test]
129+
fn it_should_create_configure_firewall_step() {
130+
let ansible_client = Arc::new(AnsibleClient::new(PathBuf::from("test_inventory.yml")));
131+
let step = ConfigureFirewallStep::new(ansible_client);
132+
133+
// Test that the step can be created successfully
134+
assert_eq!(
135+
std::mem::size_of_val(&step),
136+
std::mem::size_of::<Arc<AnsibleClient>>()
137+
);
138+
}
139+
}

src/application/steps/system/mod.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,18 @@
77
* Current steps:
88
* - Cloud-init completion waiting
99
* - Automatic security updates configuration
10+
* - UFW firewall configuration
1011
*
1112
* Future steps may include:
1213
* - User account setup and management
13-
* - Firewall configuration
1414
* - Log rotation configuration
1515
* - System service management
1616
*/
1717

18+
pub mod configure_firewall;
1819
pub mod configure_security_updates;
1920
pub mod wait_cloud_init;
2021

22+
pub use configure_firewall::ConfigureFirewallStep;
2123
pub use configure_security_updates::ConfigureSecurityUpdatesStep;
2224
pub use wait_cloud_init::WaitForCloudInitStep;

src/bin/e2e_config_tests.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,10 @@ struct CliArgs {
112112
pub async fn main() -> Result<()> {
113113
let cli = CliArgs::parse();
114114

115+
// Set environment variable to skip firewall configuration in container-based tests
116+
// UFW/iptables requires kernel capabilities not available in unprivileged containers
117+
std::env::set_var("TORRUST_TD_SKIP_FIREWALL_IN_CONTAINER", "true");
118+
115119
// Initialize logging with production log location for E2E tests using the builder pattern
116120
LoggingBuilder::new(std::path::Path::new("./data/logs"))
117121
.with_format(cli.log_format.clone())

src/domain/environment/state/configure_failed.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ pub enum ConfigureStep {
4747
InstallDockerCompose,
4848
/// Configuring automatic security updates
4949
ConfigureSecurityUpdates,
50+
/// Configuring UFW firewall
51+
ConfigureFirewall,
5052
}
5153

5254
/// Error state - Application configuration failed

0 commit comments

Comments
 (0)