Skip to content

Commit bc056fa

Browse files
josecelanojosecelano
committed
docs: add issue specification for SSH key path validation
- Create task issue #225 for validating SSH key paths must be absolute - Add detailed specification document with implementation plan - Add 'realpath' to project dictionary for spell checking - Update roadmap issue tracking (standalone improvement task)
1 parent 634ed7c commit bc056fa

File tree

2 files changed

+382
-0
lines changed

2 files changed

+382
-0
lines changed

‎docs/issues/225-validate-ssh-key-paths-must-be-absolute.md‎

Lines changed: 381 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,381 @@
1+
# Validate SSH Key Paths Must Be Absolute
2+
3+
**Issue**: #225
4+
**Parent Epic**: N/A - Standalone improvement task
5+
**Related**: [docs/contributing/roadmap-issues.md](../contributing/roadmap-issues.md)
6+
**Time Estimate**: 2-3 hours
7+
8+
---
9+
10+
## Overview
11+
12+
The `create environment` command currently accepts relative paths for SSH key files (`private_key_path` and `public_key_path`) in the environment JSON configuration. This causes delayed failures when relative paths are used - the command succeeds during environment creation but fails later during the `configure` command when SSH keys cannot be found from a different working directory. This task implements early validation to reject relative paths immediately with a clear error message, following the "fail fast" principle.
13+
14+
## Goals
15+
16+
- [ ] Reject relative SSH key paths during environment creation
17+
- [ ] Provide clear, actionable error messages explaining why absolute paths are required
18+
- [ ] Update environment template generation to clarify absolute path requirement
19+
- [ ] Add comprehensive unit tests for path validation
20+
- [ ] Update documentation to specify absolute path requirement
21+
22+
## 🏗️ Architecture Requirements
23+
24+
**DDD Layer**: Application (Configuration validation)
25+
**Module Path**: `src/application/command_handlers/create/config/ssh_credentials_config.rs`
26+
**Pattern**: Configuration Value Object with validation
27+
28+
### Module Structure Requirements
29+
30+
- [ ] Validation logic stays in `SshCredentialsConfig::to_ssh_credentials()` method
31+
- [ ] New error variant added to `CreateConfigError` enum
32+
- [ ] Error implements `.help()` with actionable troubleshooting guidance
33+
34+
### Architectural Constraints
35+
36+
```rust
37+
// The validation should happen during configuration conversion to domain types
38+
// in the to_ssh_credentials() method where other SSH validations occur
39+
40+
impl SshCredentialsConfig {
41+
pub fn to_ssh_credentials(self) -> Result<SshCredentials, CreateConfigError> {
42+
// 1. Validate paths are absolute (NEW)
43+
// 2. Convert string paths to PathBuf
44+
// 3. Validate files exist
45+
// 4. Convert username to domain type
46+
// 5. Create domain credentials object
47+
}
48+
}
49+
```
50+
51+
**Related Files**:
52+
53+
- `src/application/command_handlers/create/config/ssh_credentials_config.rs` - Add validation
54+
- `src/application/command_handlers/create/config/errors.rs` - Add error variant
55+
- `src/presentation/controllers/create/tests/template.rs` - Update template generation
56+
- `envs/environment-template-*.json` - Update template comments
57+
58+
## Specifications
59+
60+
### Current Behavior (Problem)
61+
62+
```json
63+
{
64+
"ssh_credentials": {
65+
"private_key_path": "fixtures/testing_rsa",
66+
"public_key_path": "fixtures/testing_rsa.pub",
67+
"username": "torrust",
68+
"port": 22
69+
}
70+
}
71+
```
72+
73+
**What happens**:
74+
75+
1. `create environment` command accepts the configuration ✅
76+
2. Environment is created successfully ✅
77+
3. Later, `configure` command fails ❌ when trying to use SSH keys from a different working directory
78+
79+
**Root cause**: Path resolution is context-dependent (relative to current working directory)
80+
81+
### Desired Behavior (Solution)
82+
83+
```json
84+
{
85+
"ssh_credentials": {
86+
"private_key_path": "/absolute/path/to/testing_rsa",
87+
"public_key_path": "/absolute/path/to/testing_rsa.pub",
88+
"username": "torrust",
89+
"port": 22
90+
}
91+
}
92+
```
93+
94+
**What should happen**:
95+
96+
1. `create environment` command validates paths are absolute
97+
2. If relative path detected, reject immediately with clear error:
98+
99+
```text
100+
❌ Create environment command failed: SSH key paths must be absolute
101+
102+
Configuration validation failed: SSH private key path must be absolute
103+
104+
Found relative path: fixtures/testing_rsa
105+
106+
SSH key paths must be absolute to ensure they work correctly across
107+
different working directories and command invocations.
108+
109+
How to fix:
110+
1. Convert relative path to absolute path:
111+
- Current: "fixtures/testing_rsa"
112+
- Correct: "/home/user/project/fixtures/testing_rsa"
113+
114+
2. Get absolute path from command line:
115+
realpath fixtures/testing_rsa
116+
117+
3. Update your configuration file with the absolute path
118+
119+
4. Alternative: Use environment variables or home directory expansion
120+
- ~/.ssh/id_rsa expands to /home/user/.ssh/id_rsa
121+
- $HOME/.ssh/id_rsa expands based on HOME environment variable
122+
123+
Related documentation:
124+
- Environment configuration guide: docs/user-guide/configuration.md
125+
- SSH key setup: docs/user-guide/ssh-keys.md
126+
```
127+
128+
### Validation Logic
129+
130+
```rust
131+
// In src/application/command_handlers/create/config/ssh_credentials_config.rs
132+
133+
impl SshCredentialsConfig {
134+
pub fn to_ssh_credentials(self) -> Result<SshCredentials, CreateConfigError> {
135+
// Convert string username to domain Username type
136+
let username = Username::new(&self.username)?;
137+
138+
// Convert string paths to PathBuf
139+
let private_key_path = PathBuf::from(&self.private_key_path);
140+
let public_key_path = PathBuf::from(&self.public_key_path);
141+
142+
// NEW: Validate paths are absolute
143+
if !private_key_path.is_absolute() {
144+
return Err(CreateConfigError::RelativePrivateKeyPath {
145+
path: private_key_path,
146+
});
147+
}
148+
149+
if !public_key_path.is_absolute() {
150+
return Err(CreateConfigError::RelativePublicKeyPath {
151+
path: public_key_path,
152+
});
153+
}
154+
155+
// Validate SSH key files exist
156+
if !private_key_path.exists() {
157+
return Err(CreateConfigError::PrivateKeyNotFound {
158+
path: private_key_path,
159+
});
160+
}
161+
162+
if !public_key_path.exists() {
163+
return Err(CreateConfigError::PublicKeyNotFound {
164+
path: public_key_path,
165+
});
166+
}
167+
168+
// Create domain credentials object
169+
Ok(SshCredentials::new(
170+
private_key_path,
171+
public_key_path,
172+
username,
173+
))
174+
}
175+
}
176+
```
177+
178+
### Error Type Additions
179+
180+
```rust
181+
// In src/application/command_handlers/create/config/errors.rs
182+
183+
#[derive(Debug, Error)]
184+
pub enum CreateConfigError {
185+
// ... existing variants ...
186+
187+
/// SSH private key path must be absolute
188+
#[error("SSH private key path must be absolute: {path:?}")]
189+
RelativePrivateKeyPath { path: PathBuf },
190+
191+
/// SSH public key path must be absolute
192+
#[error("SSH public key path must be absolute: {path:?}")]
193+
RelativePublicKeyPath { path: PathBuf },
194+
}
195+
196+
impl CreateConfigError {
197+
pub fn help(&self) -> String {
198+
match self {
199+
Self::RelativePrivateKeyPath { path } => format!(
200+
"SSH private key path must be absolute\n\n\
201+
Found relative path: {}\n\n\
202+
SSH key paths must be absolute to ensure they work correctly across\n\
203+
different working directories and command invocations.\n\n\
204+
How to fix:\n\
205+
1. Convert relative path to absolute path using: realpath {}\n\
206+
2. Update configuration file with the absolute path\n\
207+
3. Alternatively, use ~ for home directory (e.g., ~/.ssh/id_rsa)",
208+
path.display(),
209+
path.display()
210+
),
211+
Self::RelativePublicKeyPath { path } => format!(
212+
"SSH public key path must be absolute\n\n\
213+
Found relative path: {}\n\n\
214+
SSH key paths must be absolute to ensure they work correctly across\n\
215+
different working directories and command invocations.\n\n\
216+
How to fix:\n\
217+
1. Convert relative path to absolute path using: realpath {}\n\
218+
2. Update configuration file with the absolute path\n\
219+
3. Alternatively, use ~ for home directory (e.g., ~/.ssh/id_rsa.pub)",
220+
path.display(),
221+
path.display()
222+
),
223+
// ... existing variants ...
224+
}
225+
}
226+
}
227+
```
228+
229+
### Template Updates
230+
231+
Update the generated template files to clarify the absolute path requirement:
232+
233+
```json
234+
{
235+
"ssh_credentials": {
236+
"private_key_path": "REPLACE_WITH_SSH_PRIVATE_KEY_ABSOLUTE_PATH",
237+
"public_key_path": "REPLACE_WITH_SSH_PUBLIC_KEY_ABSOLUTE_PATH",
238+
"username": "torrust",
239+
"port": 22
240+
}
241+
}
242+
```
243+
244+
The placeholder names already indicate "ABSOLUTE_PATH", but we should also update the template generation guidance message.
245+
246+
## Implementation Plan
247+
248+
### Phase 1: Add Validation Logic (30 min)
249+
250+
- [ ] Add `is_absolute()` check in `SshCredentialsConfig::to_ssh_credentials()` before file existence check
251+
- [ ] Check private key path is absolute
252+
- [ ] Check public key path is absolute
253+
- [ ] Return appropriate error if relative path detected
254+
255+
**Acceptance**: Validation rejects relative paths with appropriate error
256+
257+
### Phase 2: Add Error Variants (30 min)
258+
259+
- [ ] Add `RelativePrivateKeyPath` variant to `CreateConfigError`
260+
- [ ] Add `RelativePublicKeyPath` variant to `CreateConfigError`
261+
- [ ] Implement `.help()` methods with actionable guidance
262+
- [ ] Include `realpath` command suggestion in help text
263+
- [ ] Include alternative solutions (~ expansion, environment variables)
264+
265+
**Acceptance**: Errors provide clear, actionable guidance for users
266+
267+
### Phase 3: Update Tests (45 min)
268+
269+
- [ ] Add unit test for relative private key path rejection
270+
- [ ] Add unit test for relative public key path rejection
271+
- [ ] Add unit test for absolute path acceptance
272+
- [ ] Add unit test for home directory (~) expansion if supported
273+
- [ ] Verify existing tests still pass
274+
- [ ] Follow unit test naming conventions: `it_should_{behavior}_when_{condition}` (see [docs/contributing/testing/unit-testing.md](../contributing/testing/unit-testing.md))
275+
276+
**Example test names**:
277+
278+
```rust
279+
#[test]
280+
fn it_should_reject_config_when_private_key_path_is_relative() { /* ... */ }
281+
282+
#[test]
283+
fn it_should_reject_config_when_public_key_path_is_relative() { /* ... */ }
284+
285+
#[test]
286+
fn it_should_accept_config_when_ssh_key_paths_are_absolute() { /* ... */ }
287+
288+
#[test]
289+
fn it_should_return_clear_error_message_when_relative_path_detected() { /* ... */ }
290+
```
291+
292+
**Acceptance**: All test scenarios covered and passing
293+
294+
### Phase 4: Update Documentation (30 min)
295+
296+
- [ ] Update `create template` command output message to emphasize absolute paths
297+
- [ ] Update any user guide documentation mentioning SSH key configuration
298+
- [ ] Add troubleshooting entry for "SSH key not found" errors
299+
300+
**Acceptance**: Documentation clearly specifies absolute path requirement
301+
302+
### Phase 5: Final Verification (15 min)
303+
304+
- [ ] Run pre-commit checks: `./scripts/pre-commit.sh`
305+
- [ ] Manually test with relative path (should fail)
306+
- [ ] Manually test with absolute path (should succeed)
307+
- [ ] Verify error message clarity and actionability
308+
309+
**Acceptance**: All quality checks pass
310+
311+
## Acceptance Criteria
312+
313+
**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.
314+
315+
### Functional Requirements
316+
317+
- [ ] Relative SSH private key paths are rejected with clear error during `create environment`
318+
- [ ] Relative SSH public key paths are rejected with clear error during `create environment`
319+
- [ ] Absolute SSH key paths continue to work without issues
320+
- [ ] Error messages include specific fix instructions (realpath command, alternatives)
321+
- [ ] Validation happens before file existence check (fail fast on relative paths)
322+
323+
### Code Quality
324+
325+
- [ ] Pre-commit checks pass: `./scripts/pre-commit.sh`
326+
- [ ] All unit tests pass (including new path validation tests)
327+
- [ ] Error types follow project error handling conventions ([docs/contributing/error-handling.md](../contributing/error-handling.md))
328+
- [ ] Code follows project architecture patterns ([docs/codebase-architecture.md](../docs/codebase-architecture.md))
329+
- [ ] Module organization follows project conventions ([docs/contributing/module-organization.md](../contributing/module-organization.md))
330+
331+
### User Experience
332+
333+
- [ ] Error messages are clear and actionable (not just "invalid path")
334+
- [ ] Error messages include the problematic relative path value
335+
- [ ] Error messages explain why absolute paths are required
336+
- [ ] Error messages provide specific command to fix (realpath)
337+
- [ ] Error messages suggest alternatives (~ expansion)
338+
339+
### Documentation
340+
341+
- [ ] Template generation clarifies absolute path requirement
342+
- [ ] Any affected user guides updated
343+
- [ ] Troubleshooting guide includes this error scenario
344+
345+
## Related Documentation
346+
347+
- [Configuration Error Handling](../contributing/error-handling.md) - Error message conventions
348+
- [DDD Layer Placement](../contributing/ddd-layer-placement.md) - Where validation belongs
349+
- [Module Organization](../contributing/module-organization.md) - Code organization conventions
350+
- [Development Principles](../development-principles.md) - Observability and user friendliness
351+
- [Unit Testing Conventions](../contributing/testing/unit-testing.md) - Test naming and structure
352+
353+
## Notes
354+
355+
### Why Absolute Paths?
356+
357+
1. **Working Directory Independence**: Commands may be run from different directories
358+
2. **State Persistence**: Environment state stores paths that must remain valid
359+
3. **Multi-command Workflows**: `create` → `provision` → `configure` sequence
360+
4. **Clear Intent**: Absolute paths eliminate ambiguity
361+
362+
### Implementation Considerations
363+
364+
- The validation should occur in `to_ssh_credentials()` where other SSH validations happen
365+
- Check `is_absolute()` **before** file existence to fail fast on incorrect path format
366+
- Home directory expansion (`~`) may require special handling if supported
367+
- Consider adding a helper function if path validation logic becomes complex
368+
369+
### Test Strategy
370+
371+
- Unit tests for validation logic in `ssh_credentials_config.rs`
372+
- Existing E2E tests should continue to work (they use absolute paths)
373+
- Manual testing with relative paths to verify user experience
374+
- Consider adding E2E test for error case if valuable
375+
376+
### Future Enhancements (Out of Scope)
377+
378+
- Auto-conversion of relative to absolute paths (could hide user intent)
379+
- Home directory (`~`) expansion support
380+
- Environment variable expansion in paths
381+
- Path validation for other configuration fields

‎project-words.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,7 @@ println
168168
publickey
169169
pytest
170170
readlink
171+
realpath
171172
reentrancy
172173
reprioritize
173174
reprovision

0 commit comments

Comments
 (0)