fix: make reset-ubuntu script wrk with username/password - #62
Conversation
Code Review: Password-based SSH Authentication SupportThank you for this PR! I have reviewed the changes to add username/password authentication support to the deployment scripts. Strengths
CRITICAL Security Concerns1. StrictHostKeyChecking Disabled (Line 81 in run-on-remotes.sh) The PR automatically disables host key verification when using password auth: This makes connections vulnerable to MITM attacks - a significant security regression. Recommendation: Remove this automatic disabling, add a warning log message, or use 2. Password Exposure Risk Storing passwords in environment variables (SSH_PASSWORD, SSHPASS) is less secure:
3. No Credential Validation The script does not validate SSH_PASSWORD values before use. Potential Issues1. Missing Dependency Check The code assumes sshpass is available but does not verify installation. Add a check: if [[ -n "${SSHPASS:-}" ]]; then
if ! command -v sshpass &> /dev/null; then
log_error "sshpass required but not installed"
exit 1
fi
fi2. Host Parsing Logic Lines 100-105 handle both user@host and bare host formats correctly, but this is implicit. Add a comment explaining the logic. Code Quality Issues1. Missing Documentation
2. Testing No automated tests exist for these scripts. Consider:
SummaryThis PR should NOT be merged as-is due to security implications. Required Changes:
Recommended Changes:
Next Steps
Let me know if you would like help with these changes! |
There was a problem hiding this comment.
Pull request overview
Updates the remote execution tooling used by the scripts/reset-ubuntu.Dockerfile workflow to support password-based SSH (via sshpass) and optional username overrides when deploying/running scripts on remote hosts.
Changes:
- Add optional password-based SSH/SCP support to
run-on-remotes.shusingsshpasswhenSSH_PASSWORD/SSHPASSis set. - Add
SSH_USERoverride support to rewriteuser@hosttargets when reading the hosts file. - Install
sshpassand add placeholder SSH credential env vars inreset-ubuntu.Dockerfile.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/run-on-remotes.sh | Adds ssh/scp command wrapping for password auth + user override while iterating hosts |
| scripts/reset-ubuntu.Dockerfile | Installs sshpass and exposes optional env vars for the container entrypoint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ -n "${SSHPASS:-}" ]]; then | ||
| log_info "Password-based SSH authentication enabled" | ||
| SSH_OPTS="$SSH_OPTS -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null" | ||
| SCP_CMD="sshpass -e scp" | ||
| SSH_CMD="sshpass -e ssh" | ||
| fi |
There was a problem hiding this comment.
If SSHPASS/SSH_PASSWORD is provided but sshpass is not installed on the machine running this script, the first scp/ssh attempt will fail with a generic copy/exec error. Add an early dependency check (e.g., command -v sshpass) when enabling password-based mode and emit a clear error before processing hosts.
| # Build SSH/SCP command prefixes based on env vars | ||
| SSH_OPTS="-o ConnectTimeout=5" | ||
| SCP_CMD="scp" | ||
| SSH_CMD="ssh" | ||
|
|
||
| # Map SSH_PASSWORD to SSHPASS (what sshpass -e reads) | ||
| if [[ -n "${SSH_PASSWORD:-}" ]] && [[ -z "${SSHPASS:-}" ]]; then | ||
| export SSHPASS="$SSH_PASSWORD" | ||
| fi |
There was a problem hiding this comment.
This change introduces new environment variables (SSH_USER, SSH_PASSWORD/SSHPASS) that alter how hosts are targeted and how authentication works, but the script header/usage text still implies only user@host entries in the hosts file. Please document these env vars (and their precedence/behavior) in the script’s usage comments or scripts/README.md so users know how to invoke password mode safely.
Code ReviewThank you for this contribution! I've reviewed the changes to add username/password authentication support to the deployment scripts. Here's my feedback: Security Concerns 🔒CRITICAL - Password Exposure:
Recommendations:
Code Quality ✅Good practices observed:
Minor improvements:
Best Practices 📋Documentation:
Error handling:
Testing 🧪Missing:
Suggested tests:
Performance ⚡No significant performance concerns. The changes are minimal and don't introduce any performance regressions. SummaryThe implementation is functionally sound and handles the password authentication use case well. The main concern is security - password-based authentication should be clearly documented as a last resort for specific deployment scenarios. Before merging:
Overall: Good work on the implementation and documentation. Address the security documentation concerns and this will be ready to merge! 👍 |
Code ReviewThis PR introduces several important improvements to the readonly VM access system, TUI user experience, and deployment scripts. Here's my detailed feedback: ✅ Strengths1. Excellent Security Enhancement
2. Outstanding UX Improvements
3. Improved Remote Deployment
4. Blog Post Visual EnhancementsThe styled execution flow diagrams make the security model much more accessible.
|
…, integration tests for preparation flow
Code Review: PR #62 - Username/Password Auth for Deploy ScriptsSummaryThis PR adds support for username/password authentication in deployment scripts and introduces comprehensive test coverage for the readonly preparation module. While the implementation is generally solid, there are critical security concerns that must be addressed before merging. 🔴 Critical Security Issues1. SSH Public Key Exposure in RepositoryFile: A real user's SSH public key (
Required Fix: git rm --cached scripts/ssh-users.conf
# Create ssh-users.conf.example with dummy keys instead
# Users should create their own ssh-users.conf locally (already in .gitignore)2. Hardcoded Default PasswordFiles: password: ubuntu
chpasswd: { expire: False }The default password "ubuntu" is hardcoded and never expires. Recommendation:
🟡 High Priority Issues3. Error Handling in Deployment ScriptsFile: Several critical operations use virsh net-start default || true # Line 98If the network fails to start, VMs won't get IPs, but the script continues silently. Fix: Remove 4. SSHD Restart May Disconnect SessionFile: restartCmd := `systemctl restart sshd ...`Restarting sshd during preparation will disconnect the current SSH session. Recommendation: Use 5. Inconsistent Timeout Values
Fix: Standardize timeout values or make them configurable. ✅ Excellent WorkTest CoverageThe new test files are exemplary:
This is exactly what the project's AGENTS.md requires. Great job! Architecture & Design
🔧 Minor Improvements6. Logger Creation in main.goFile: result, err := readonly.Prepare(ctx, sshRunFunc, string(caPubKey), nil, slog.Default())Suggestion: Pass the existing logger from context instead of creating a new default logger. 7. SSH Key Type AssumptionFile: Assumes all keys are ed25519. Should auto-detect key type or use generic naming. 8. Missing DocumentationNeed to document:
📊 Code Quality Metrics
🎯 Verdict: Request ChangesMust Fix Before Merge:
After Fixes: The PR will be ready to merge. The code quality, test coverage, and architecture are solid. The authentication implementation is appropriate for its use case once security concerns are addressed. 💬 Questions
Great work on the comprehensive test coverage! 🎉 Note: The base64-encoded sudo commands in |
PR Review: Make reset-ubuntu script work with username/passwordThis PR updates deployment scripts to support password-based SSH authentication and significantly improves the read-only VM preparation system with progress tracking, comprehensive testing, and better TUI integration. Strengths1. Excellent Test Coverage
2. Security Improvements
3. Code Quality
4. UX Improvements
Areas for Improvement1. Security ConcernsCritical: Password in Environment Variable SSH Host Key Verification 2. Error Handling IssuesSilent Failures in reset-ubuntu.sh Usermod Failure Logged as Warning 3. Code Style IssuesInconsistent Variable Naming Magic Numbers Potential Issues1. Race Condition in Mock SSH Runnerprepare_test.go lines 35-44: The m.responses map is accessed without a lock after releasing the mutex. 2. SSH_USERS_FILE Parsingreset-ubuntu.sh lines 217-229 assume format: username publickey. 3. MAC Address Collisionreset-ubuntu.sh lines 287-290: VM_INDEX 1-255 will have identical MAC prefixes. RecommendationsHigh Priority
Medium Priority
Low Priority
SummaryOverall Assessment: Approve with suggestions This is a solid PR that significantly improves the codebase with excellent test coverage and good documentation. The security concerns around password handling and SSH host key verification should be addressed, but they do not block the merge since this is for deployment scripts (not production runtime). The test suite is particularly impressive with comprehensive coverage of edge cases and security scenarios for the restricted shell. The progress tracking and TUI integration are clean additions. Impact: Medium (improves deployment workflow and VM preparation) Great work! |
Pull Request ReviewSummaryThis PR updates deployment scripts to support username/password authentication alongside SSH key-based auth, and adds comprehensive test coverage for the readonly VM preparation flow. The changes also include UI improvements for TUI progress reporting and mouse wheel scrolling. ✅ Strengths1. Excellent Test Coverage
2. Security Improvements
3. Better Progress Reporting
4. Script Flexibility
5. All CI Checks Passing ✓
|
Description
Type of Change
Checklist
Release Notes
Labels