Skip to content

Commit 2a4fc89

Browse files
josecelanojosecelano
committed
docs: add issue specification for #340 
1 parent 00663a2 commit 2a4fc89

1 file changed

Lines changed: 291 additions & 0 deletions

File tree

‎docs/issues/340-validate-ssh-key-file-existence.md‎

Lines changed: 291 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,291 @@
1+
# Validate Command Should Not Check SSH Key File Existence
2+
3+
**Issue**: #340
4+
**Parent Epic**: N/A
5+
**Related**: [Validate Command Documentation](../user-guide/commands/validate.md)
6+
7+
## Overview
8+
9+
The `validate` command currently checks whether SSH key files exist on the filesystem. This is incorrect - the validate command should only perform **syntactic validation** (checking that paths are absolute), not **runtime validation** (checking that files exist).
10+
11+
**Current Behavior**:
12+
13+
- ✅ Correctly rejects relative paths: `fixtures/testing_rsa` → Error: "SSH private key path must be absolute"
14+
- ❌ **Bug**: Rejects absolute paths if files don't exist: `/home/user/.ssh/id_rsa` → Error: "SSH private key file not found"
15+
16+
**Expected Behavior**: The validate command should:
17+
18+
- ✅ Reject relative paths like `fixtures/testing_rsa` (already working)
19+
- ✅ Accept absolute paths like `/home/user/.ssh/id_rsa` **without checking if files exist**
20+
- ⚠️ File existence validation should only happen in `create environment` command
21+
22+
**Rationale**: The validate command performs syntactic validation only - it checks configuration structure and constraints in isolation. File existence is a runtime validation that depends on the current system state and should only be checked when actually creating the environment.
23+
24+
## Goals
25+
26+
- [ ] Remove file existence check from validate command
27+
- [ ] Keep absolute path validation (already working correctly)
28+
- [ ] Ensure file existence check only happens in `create environment` command
29+
- [ ] Add unit tests confirming validation passes for non-existent absolute paths
30+
- [ ] Update E2E tests to verify both behaviors
31+
32+
## 🏗️ Architecture Requirements
33+
34+
**DDD Layer**: Application
35+
**Module Path**: `src/application/command_handlers/validate/` or `src/application/command_handlers/create/config/ssh_credentials_config.rs`
36+
**Pattern**: DTO validation / Command handler
37+
38+
### Module Structure Requirements
39+
40+
- [ ] Follow DDD layer separation (validation may happen in DTO → Domain conversion)
41+
- [ ] Respect dependency flow rules (Application → Domain)
42+
- [ ] Use appropriate error types (see [docs/contributing/error-handling.md](../docs/contributing/error-handling.md))
43+
44+
### Architectural Constraints
45+
46+
- [ ] Validation logic should be testable in isolation
47+
- [ ] Error messages must be actionable and user-friendly (see [docs/development-principles.md](../docs/development-principles.md))
48+
- [ ] No file I/O during validate command (syntactic validation only)
49+
50+
### Anti-Patterns to Avoid
51+
52+
- ❌ Checking file existence in validate command (should only be in create environment)
53+
- ❌ Generic error messages without actionable guidance
54+
- ❌ Mixing syntactic and runtime validation concerns
55+
56+
## Specifications
57+
58+
### Current Validation Behavior
59+
60+
The validate command currently performs TWO checks:
61+
62+
1. ✅ **Path must be absolute** - Working correctly, should be kept
63+
2. ❌ **File must exist** - Bug: This should only happen in create environment command
64+
65+
**Test Results:**
66+
67+
```bash
68+
# Test 1: Relative paths (correctly rejected)
69+
$ cargo run -- validate --env-file envs/bug-validation-invalid-relative-paths.json
70+
❌ Error: SSH private key path must be absolute: "fixtures/testing_rsa"
71+
72+
# Test 2: Absolute paths to non-existent files (incorrectly rejected - this is the bug)
73+
$ cargo run -- validate --env-file envs/bug-validation-valid-absolute-paths.json
74+
❌ Error: SSH private key file not found: /home/user/.ssh/id_rsa
75+
```
76+
77+
### Required Fix
78+
79+
**Remove file existence check from validate command** while keeping the absolute path validation.
80+
81+
**After fix, expected behavior:**
82+
83+
```bash
84+
# Test 1: Relative paths (should still be rejected)
85+
$ cargo run -- validate --env-file envs/bug-validation-invalid-relative-paths.json
86+
❌ Error: SSH private key path must be absolute: "fixtures/testing_rsa"
87+
88+
# Test 2: Absolute paths to non-existent files (should now pass)
89+
$ cargo run -- validate --env-file envs/bug-validation-valid-absolute-paths.json
90+
✅ Configuration file 'envs/bug-validation-valid-absolute-paths.json' is valid
91+
```
92+
93+
### Test Configurations
94+
95+
Two minimal test configurations are provided in `envs/`:
96+
97+
**Valid (absolute paths)**: [`envs/bug-validation-valid-absolute-paths.json`](../../envs/bug-validation-valid-absolute-paths.json)
98+
99+
```json
100+
{
101+
"environment": {
102+
"name": "bug-test-valid"
103+
},
104+
"ssh_credentials": {
105+
"private_key_path": "/home/user/.ssh/id_rsa",
106+
"public_key_path": "/home/user/.ssh/id_rsa.pub"
107+
},
108+
"provider": {
109+
"provider": "lxd",
110+
"profile_name": "torrust-profile-bug-test"
111+
},
112+
"tracker": {
113+
"core": {
114+
"database": {
115+
"driver": "sqlite3",
116+
"database_name": "tracker.db"
117+
},
118+
"private": false
119+
},
120+
"udp_trackers": [],
121+
"http_trackers": [],
122+
"http_api": {
123+
"bind_address": "0.0.0.0:1212",
124+
"admin_token": "MyAccessToken"
125+
},
126+
"health_check_api": {
127+
"bind_address": "127.0.0.1:1313"
128+
}
129+
}
130+
}
131+
```
132+
133+
**Invalid (relative paths)**: [`envs/bug-validation-invalid-relative-paths.json`](../../envs/bug-validation-invalid-relative-paths.json)
134+
135+
```json
136+
{
137+
"environment": {
138+
"name": "bug-test-invalid"
139+
},
140+
"ssh_credentials": {
141+
"private_key_path": "fixtures/testing_rsa",
142+
"public_key_path": "fixtures/testing_rsa.pub"
143+
},
144+
"provider": {
145+
"provider": "lxd",
146+
"profile_name": "torrust-profile-bug-test"
147+
},
148+
"tracker": {
149+
"core": {
150+
"database": {
151+
"driver": "sqlite3",
152+
"database_name": "tracker.db"
153+
},
154+
"private": false
155+
},
156+
"udp_trackers": [],
157+
"http_trackers": [],
158+
"http_api": {
159+
"bind_address": "0.0.0.0:1212",
160+
"admin_token": "MyAccessToken"
161+
},
162+
"health_check_api": {
163+
"bind_address": "127.0.0.1:1313"
164+
}
165+
}
166+
}
167+
```
168+
169+
### How to Reproduce
170+
171+
```bash
172+
# Test 1: Relative paths (correctly rejected)
173+
cargo run --bin torrust-tracker-deployer -- validate --env-file envs/bug-validation-invalid-relative-paths.json
174+
# Expected: ❌ Error: SSH private key path must be absolute: "fixtures/testing_rsa"
175+
# Actual: ❌ Error: SSH private key path must be absolute: "fixtures/testing_rsa" ✓
176+
177+
# Test 2: Absolute paths to non-existent files (BUG - incorrectly rejected)
178+
cargo run --bin torrust-tracker-deployer -- validate --env-file envs/bug-validation-valid-absolute-paths.json
179+
# Expected: ✅ Configuration file is valid
180+
# Actual: ❌ Error: SSH private key file not found: /home/user/.ssh/id_rsa (BUG)
181+
```
182+
183+
### Implementation Location
184+
185+
The file existence check is likely in one of these locations:
186+
187+
1. `src/adapters/ssh/credentials.rs` - SshCredentials construction
188+
2. `src/application/command_handlers/validate/handler.rs` - Validate command handler
189+
3. `src/application/command_handlers/create/config/ssh_credentials_config.rs` - TryFrom conversion
190+
191+
**Strategy**: Add a parameter or flag to skip file existence check when called from validate command, but still check when called from create environment command.
192+
193+
## Implementation Plan
194+
195+
### Phase 1: Locate File Existence Check (15 minutes)
196+
197+
- [ ] Search codebase for file existence validation logic
198+
- [ ] Identify where `SshCredentials` checks if files exist
199+
- [ ] Determine if check is in domain layer, application layer, or adapter layer
200+
- [ ] Document current flow: validate command → ... → file existence check
201+
202+
### Phase 2: Refactor to Skip File Check in Validate (45 minutes)
203+
204+
- [ ] Add parameter/flag to skip file existence check (e.g., `validate_existence: bool`)
205+
- [ ] Update `SshCredentials::new()` or conversion method to accept flag
206+
- [ ] Validate command passes `validate_existence: false`
207+
- [ ] Create environment command passes `validate_existence: true`
208+
- [ ] Ensure absolute path validation still runs in both cases
209+
210+
### Phase 3: Unit Tests (30 minutes)
211+
212+
- [ ] Add test: `it_should_accept_absolute_paths_without_checking_existence`
213+
- [ ] Add test: `it_should_still_reject_relative_paths`
214+
- [ ] Add test: `it_should_check_file_existence_when_flag_is_true` (for create command)
215+
- [ ] Verify tests follow naming conventions from [docs/contributing/testing/unit-testing.md](../docs/contributing/testing/unit-testing.md)
216+
217+
### Phase 4: E2E Tests (30 minutes)
218+
219+
- [ ] Test using `bug-validation-valid-absolute-paths.json` (should now pass)
220+
- [ ] Test using `bug-validation-invalid-relative-paths.json` (should still fail)
221+
- [ ] Verify create environment command still checks file existence
222+
- [ ] Update `tests/e2e/validate_command.rs` with regression tests
223+
224+
### Phase 5: Documentation (15 minutes)
225+
226+
- [ ] Update [docs/user-guide/commands/validate.md](../user-guide/commands/validate.md) to clarify validation scope
227+
- [ ] Document that validate checks path format only, not file existence
228+
- [ ] Add example showing this distinction
229+
- [ ] Update error message examples if needed
230+
231+
## Acceptance Criteria
232+
233+
> **Note for Contributors**: These criteria define what the PR reviewer will check. Use this as your pre-review checklist before submitting the PR to minimize back-and-forth iterations.
234+
235+
**Quality Checks**:
236+
237+
- [ ] Pre-commit checks pass: `./scripts/pre-commit.sh`
238+
239+
**Task-Specific Criteria**:
240+
241+
- [ ] ✅ Validate command accepts absolute SSH key paths without checking file existence
242+
- [ ] ❌ Validate command still rejects relative SSH key paths with clear error
243+
- [ ] ✅ Create environment command still checks if SSH key files exist
244+
- [ ] Unit tests cover both validate (no file check) and create (with file check) scenarios
245+
- [ ] E2E tests verify `bug-validation-valid-absolute-paths.json` now passes validation
246+
- [ ] E2E tests verify `bug-validation-invalid-relative-paths.json` still fails validation
247+
- [ ] Documentation clarifies validation scope (syntactic vs runtime checks)
248+
249+
## Related Documentation
250+
251+
- [Validate Command Documentation](../user-guide/commands/validate.md)
252+
- [Error Handling Guide](../contributing/error-handling.md)
253+
- [Unit Testing Conventions](../contributing/testing/unit-testing.md)
254+
- [Config Validation Feature Questions](../features/config-validation-command/questions.md)
255+
256+
## Notes
257+
258+
### What Was Discovered
259+
260+
Testing revealed that the validate command already correctly validates that SSH paths must be absolute. The bug is that it **also checks file existence**, which it shouldn't do.
261+
262+
**Test Results**:
263+
264+
- `bug-validation-invalid-relative-paths.json` → Correctly fails: "SSH private key path must be absolute"
265+
- `bug-validation-valid-absolute-paths.json` → Incorrectly fails: "SSH private key file not found"
266+
267+
The second case is the bug - it should pass validation because the path is absolute, regardless of whether the file exists.
268+
269+
### Why Not Check File Existence in Validate?
270+
271+
The validate command performs **syntactic validation** only - it checks the configuration structure and field constraints in isolation. File existence is a **runtime validation** that depends on the current system state and should only be checked when actually creating the environment.
272+
273+
This separation aligns with the three levels of validation documented in [docs/features/config-validation-command/questions.md](../features/config-validation-command/questions.md):
274+
275+
1. **Syntactic** ✅ - JSON structure, types, required fields, absolute paths (path format)
276+
2. **Config-intrinsic semantics** ✅ - Cross-field rules (e.g., Grafana requires Prometheus)
277+
3. **State-dependent semantics** ❌ - Depends on current app/system state (file existence, environment name conflicts)
278+
279+
The validate command implements levels 1 and 2, while level 3 is deferred to the create environment command.
280+
281+
### Real-World Use Case
282+
283+
A common workflow is:
284+
285+
1. User writes config on Machine A with SSH keys at `/home/alice/.ssh/id_rsa`
286+
2. User validates config on Machine A (should pass)
287+
3. User commits config to git repository
288+
4. CI/CD pipeline validates config on Machine B where `/home/alice/.ssh/id_rsa` doesn't exist (should still pass syntactic validation)
289+
5. Deployment runs on Machine C where the actual SSH keys exist (file existence checked here)
290+
291+
The current behavior breaks step 4 - CI/CD validation incorrectly fails even though the config syntax is correct.

0 commit comments

Comments
 (0)