main #1

200mill · 2026-07-03T18:11:31Z

200mill commented

2026-07-03 18:11:31 +00:00

No description provided.

200mill added 2 commits 2026-07-03 18:11:32 +00:00

feat: rshc: add sshserve 45554836d8

fix: security de09d628ed

mas requested changes 2026-07-04 07:04:50 +00:00

mas left a comment

First-time contributor

PR #1 Review: `feat: rshc: add sshserve` (+ `fix: security`)

Neat idea — SSH-to-rsh bridging is a natural UX improvement. But there are some serious issues that need addressing before merge.

🔴 Critical

1. Hardcoded SSH private key in source code

The fix: security commit added a hardcoded private key:

let key_str = "-----BEGIN OPENSSH PRIVATE KEY-----\nb3BlbnNzaC1rZXktdjEAAAAA...";

This is a showstopper:

Anyone with access to the source (including this PR page) now has the private key and can MITM any serve-ssh instance
The key comment says etx@etx-desktop — this is someone's personal key, not a generated ephemeral one
Every serve-ssh instance worldwide will share the same host key — host key verification is completely useless
The key is pushed alongside the operator-derived host key (config.keys.push(key) + config.keys.push(host_key)), so clients will see two host keys and one is publicly known

Fix: Generate an ephemeral host key at startup (russh::keys::PrivateKey::random(&mut OsRng, russh::keys::Algorithm::Ed25519)) and optionally accept a --host-key flag for a persistent one. Never commit private keys.

2. First commit was a completely open SSH server

The initial feat commit had all three auth handlers returning Ok(Auth::Accept):

// Original (feat commit):
async fn auth_publickey(...) -> Result<Auth, Self::Error> { Ok(Auth::Accept) }  // anyone
async fn auth_password(...) -> Result<Auth, Self::Error> { Ok(Auth::Accept) }  // any password
async fn auth_none(...)    -> Result<Auth, Self::Error> { Ok(Auth::Accept) }  // no auth at all

While the fix: security commit patched this, the PR history shows an unauthenticated SSH reverse-shell was pushed to a public repo. Security-sensitive code should be correct from the first commit, not "fixed later."

🟠 Important

3. Operator key used as host key — dual-purpose key, identity leak

The operator's SSH private key serves as both the rsh-backend auth key and the SSH server's host key. Anyone connecting to serve-ssh learns the operator's key fingerprint. Compromising one key compromises both authentication surfaces. If serve-ssh is bound to a public interface, the operator's key identity is exposed to scanners.

Consider generating a separate host key at startup or accepting a --host-key path.

4. New AuthedClient per SSH channel — resource exhaustion risk

spawn_shell() calls AuthedClient::connect(&self.cfg) for every channel. Each SSH shell/exec request opens a brand-new WebSocket to the rsh-backend. A single SSH session with multiple channels (e.g., concurrent exec commands) will open many backend connections. This could exhaust backend resources or hit connection limits. Consider sharing one AuthedClient per SSH session.

5. channel_close sends Kill — always force-kills the remote shell

channel_eof already sends Eof (graceful close), but channel_close immediately sends Kill. If the SSH client closes the channel gracefully (EOF → close), the remote shell gets both Eof and then Kill. Consider only sending Kill if EOF hasn't been sent, or just rely on Eof + drop_stream.

6. No graceful shutdown

The accept loop runs forever with no way to shut down cleanly. A tokio::select! on listener.accept() + a cancellation token (or ctrlc signal) would allow clean teardown.

🟡 Minor / Style

7. Accept errors silently swallowed — Err(_) => continue on listener accept. Some errors (EMFILE, ENFILE) indicate resource exhaustion and should at least be logged.

8. auth_rejection_time_initial: Some(Duration::from_secs(0)) — This skips the initial delay on auth failure, removing a timing-sidechannel mitigation. Since only pubkey auth is advertised, the risk is low, but worth a comment explaining why.

9. String::from_utf8_lossy in exec_request — Silently replaces invalid bytes with \u{fffd}. For commands this is usually fine, but worth a brief comment.

10. No SSH signal handler — SSH signal requests (SIGINT, SIGTERM, etc.) aren't forwarded to the remote shell. Not a blocker but a functional gap.

11. Double blank line in main.rs — Extra blank line between dispatch arms after the ServeSsh match arm.

12. CLAUDE.md bundled with feature PR — The 140-line CLAUDE.md is tangential to the serve-ssh feature. Consider splitting into a separate commit/PR.

Summary

Category	Count
🔴 Critical (must fix)	2
🟠 Important (should fix)	4
🟡 Minor	6

The hardcoded private key is an automatic blocker, and the initial open-auth commit pattern is a serious concern for a reverse-shell project. Requesting changes — at minimum, the hardcoded key must be removed and proper host key generation implemented.

## PR #1 Review: `feat: rshc: add sshserve` (+ `fix: security`) Neat idea — SSH-to-rsh bridging is a natural UX improvement. But there are some serious issues that need addressing before merge. --- ### 🔴 Critical **1. Hardcoded SSH private key in source code** The `fix: security` commit _added_ a hardcoded private key: ```rust let key_str = "-----BEGIN OPENSSH PRIVATE KEY-----\nb3BlbnNzaC1rZXktdjEAAAAA..."; ``` This is a showstopper: - **Anyone with access to the source** (including this PR page) now has the private key and can MITM any `serve-ssh` instance - The key comment says `etx@etx-desktop` — this is someone's personal key, not a generated ephemeral one - Every `serve-ssh` instance worldwide will share the same host key — host key verification is completely useless - The key is pushed alongside the operator-derived host key (`config.keys.push(key)` + `config.keys.push(host_key)`), so clients will see two host keys and one is publicly known **Fix:** Generate an ephemeral host key at startup (`russh::keys::PrivateKey::random(&mut OsRng, russh::keys::Algorithm::Ed25519)`) and optionally accept a `--host-key` flag for a persistent one. Never commit private keys. **2. First commit was a completely open SSH server** The initial `feat` commit had all three auth handlers returning `Ok(Auth::Accept)`: ```rust // Original (feat commit): async fn auth_publickey(...) -> Result<Auth, Self::Error> { Ok(Auth::Accept) } // anyone async fn auth_password(...) -> Result<Auth, Self::Error> { Ok(Auth::Accept) } // any password async fn auth_none(...) -> Result<Auth, Self::Error> { Ok(Auth::Accept) } // no auth at all ``` While the `fix: security` commit patched this, the PR history shows an unauthenticated SSH reverse-shell was pushed to a public repo. Security-sensitive code should be correct from the first commit, not "fixed later." --- ### 🟠 Important **3. Operator key used as host key — dual-purpose key, identity leak** The operator's SSH private key serves as both the rsh-backend auth key _and_ the SSH server's host key. Anyone connecting to `serve-ssh` learns the operator's key fingerprint. Compromising one key compromises both authentication surfaces. If `serve-ssh` is bound to a public interface, the operator's key identity is exposed to scanners. Consider generating a separate host key at startup or accepting a `--host-key` path. **4. New `AuthedClient` per SSH channel — resource exhaustion risk** `spawn_shell()` calls `AuthedClient::connect(&self.cfg)` for every channel. Each SSH shell/exec request opens a brand-new WebSocket to the rsh-backend. A single SSH session with multiple channels (e.g., concurrent `exec` commands) will open many backend connections. This could exhaust backend resources or hit connection limits. Consider sharing one `AuthedClient` per SSH session. **5. `channel_close` sends `Kill` — always force-kills the remote shell** `channel_eof` already sends `Eof` (graceful close), but `channel_close` immediately sends `Kill`. If the SSH client closes the channel gracefully (EOF → close), the remote shell gets both `Eof` and then `Kill`. Consider only sending `Kill` if EOF hasn't been sent, or just rely on `Eof` + `drop_stream`. **6. No graceful shutdown** The accept loop runs forever with no way to shut down cleanly. A `tokio::select!` on `listener.accept()` + a cancellation token (or ctrlc signal) would allow clean teardown. --- ### 🟡 Minor / Style **7. Accept errors silently swallowed** — `Err(_) => continue` on listener accept. Some errors (EMFILE, ENFILE) indicate resource exhaustion and should at least be logged. **8. `auth_rejection_time_initial: Some(Duration::from_secs(0))`** — This skips the initial delay on auth failure, removing a timing-sidechannel mitigation. Since only pubkey auth is advertised, the risk is low, but worth a comment explaining why. **9. `String::from_utf8_lossy` in `exec_request`** — Silently replaces invalid bytes with `\u{fffd}`. For commands this is usually fine, but worth a brief comment. **10. No SSH `signal` handler** — SSH signal requests (SIGINT, SIGTERM, etc.) aren't forwarded to the remote shell. Not a blocker but a functional gap. **11. Double blank line in main.rs** — Extra blank line between dispatch arms after the `ServeSsh` match arm. **12. CLAUDE.md bundled with feature PR** — The 140-line CLAUDE.md is tangential to the serve-ssh feature. Consider splitting into a separate commit/PR. --- ### Summary | Category | Count | |----------|-------| | 🔴 Critical (must fix) | 2 | | 🟠 Important (should fix) | 4 | | 🟡 Minor | 6 | The hardcoded private key is an automatic blocker, and the initial open-auth commit pattern is a serious concern for a reverse-shell project. Requesting changes — at minimum, the hardcoded key must be removed and proper host key generation implemented.

200mill added 1 commit 2026-07-04 07:50:14 +00:00

fix: asdf 3697600c96

mincomk merged commit f5091e826e into main

2026-07-19 09:07:24 +00:00

mincomk referenced this issue from a commit

2026-07-19 09:07:25 +00:00

Merge pull request 'main' (#1) from 200mill/rsh:main into main

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mincomk/rsh#1

Allow edits from maintainers