fix: security audit findings, bug fixes, and CI hardening#37
Merged
timhartmann7 merged 11 commits intoJun 6, 2026
Merged
Conversation
- Validate public key content after reading from disk: reject keys containing newlines, carriage returns, or null bytes, and verify the key type prefix matches ssh-ed25519/ssh-rsa/ecdsa-sha2-*. - Add input validation to force_sshd_directive() to prevent sed injection if ever called with user-supplied arguments. - Narrow Include directive commenting to only target cloud-init (/etc/ssh/sshd_config.d/) instead of all Include directives. - Replace ls-based rollback with find for safer filename handling. Addresses shell injection risks in remote command construction. Refs: timhartmann7#32 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…iles - Set restrictive file permissions (0600) on hosts.toml, snippets.toml, and config.toml after writing, since they may contain credentials. - Clean up .tmp files if the atomic rename step fails, preventing stale temp file accumulation in the config directory. Refs: timhartmann7#35 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Split Eof from Close/None in collect_output match arms so ExitStatus messages arriving after Eof are correctly captured. - Remove id_dsa from default key paths (DSA is cryptographically broken, removed from OpenSSH 9.8). - Add TOFU warning log when accepting an unknown host key. Refs: timhartmann7#34 Refs: timhartmann7#35 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…al restore - Set pending_ops before sending remote delete commands so the progress indicator counts down correctly instead of closing after the first file. - Collect all local delete errors instead of only reporting the last one. - Replace blocking std::fs::remove_dir with tokio::fs::remove_dir to avoid blocking the async worker thread. - Make terminal restore unconditional: run all restore steps even if disable_raw_mode fails, preventing a broken shell on error. - Remove dead pending_connect/connect_system_ssh code path (never set). Refs: timhartmann7#34 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ation - Thread host_name into sftp_task_loop so SftpDisconnected events carry the actual host name instead of an empty string. - Add null byte checks to do_download and do_upload. - Add path traversal (.. component) guard to do_upload, matching the existing protection in do_download. Refs: timhartmann7#34 Refs: timhartmann7#36 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ners Use xargs -n 50 to batch container IDs instead of expanding all IDs in a single command substitution, which fails silently when the argument list exceeds ARG_MAX (~2 MiB) with hundreds of containers. Refs: timhartmann7#36 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Pin all GitHub Actions to immutable commit SHAs instead of mutable tags (v4, v2, stable) to prevent supply chain attacks via tag force-push. - Add SHA256SUMS verification to install.sh after downloading the release archive, before extraction and installation. - Fetch man page from the tagged release commit instead of the mutable main branch. Refs: timhartmann7#33 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
|
Hey @PowerAppsDarren - really happy to see this, thank you for digging into the project! I pushed a few small follow-ups on top of your commits:
|
Contributor
Author
|
Thanks, man! You're a genius with this project! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Community security audit and code review covering shell injection, config file permissions, SFTP path validation, CI supply chain, and several functional bugs.
Security fixes:
force_sshd_directive()to prevent sed injectionIncludedirective commenting to cloud-init only (avoids disabling legitimate sshd security policies)Bug fixes:
collect_outputto continue reading afterEofsoExitStatusis not lostpending_opsfor multi-file remote delete (progress popup was closing after first file)std::fs::remove_dirwithtokio::fs::remove_dirin async contextdisable_raw_modefails)pending_connect/connect_system_sshcode pathdocker inspectwithxargs -n 50to avoidARG_MAXwith many containersCI/supply chain:
install.shmainbranchCrypto hardening:
id_dsafrom default key paths (DSA is broken, removed from OpenSSH 9.8)Not included in this PR (discussed in issues, need maintainer input):
unsafe impl Send→ Mutex (Hardening: Path traversal, resource leaks, performance #36) — performance implications need profilingTest plan
cargo test— all 26 tests passcargo clippy -- -D warnings— zero warningscargo fmt --check— cleanRelated issues
Refs #32, #33, #34, #35, #36