Repository navigation
docs: durable SSH tunnel for day-to-day access (closes #16) - #26
Conversation
The ad-hoc master socket lapses after an hour idle. Document a launchd-supervised port-forward that reconnects on its own, and correct the Tailscale recommendation it replaces. Tailscale on the device was evaluated and rejected on memory. tailscaled settles around 40 MB resident on this hardware (sipeed/NanoKVM#366) against the 43 MB available measured in device recon; upstream needs GOMEMLIMIT=100, GOGC=50 and a two-day cron reboot to keep it up on a board with more free RAM than ours, and still saw an OOM kill at 78 MB (sipeed/NanoKVM#660). GOMEMLIMIT does not recover it — ~86% of that heap is wireguard-go per-interface buffer pools, an allocation floor rather than collectable garbage (tailscale/tailscale#16258). The README's "costs nothing" claim was wrong and is corrected here and in the design spec; the exposure reasoning behind it still holds. The tunnel leaves the security model unchanged: the bind stays on loopback, and reaching it costs an SSH credential plus the bearer token. Off-LAN reach, when it is wanted, belongs on a subnet router on another host rather than on the device. Documents the SSH key prerequisite unattended reconnect requires, which replaces PR #18's dropbear agent-flood workaround rather than adding to it, and the three plist details that otherwise fail silently: no -f, ControlMaster=no, and ExitOnForwardFailure with server-alive limits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe documentation changes revise MCP access from device-side Tailscale exposure to loopback binding with SSH forwarding, update client configuration examples and token guidance, and add launchd instructions for maintaining a persistent tunnel across reboots and network interruptions. ChangesMCP access guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
257-259: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not recommend storing the bearer token in a shell profile.
This contradicts the preceding claim that the secret stays out of configs and persists the full token in plaintext. Keep the export session-scoped or document a secure secret-store mechanism.
Suggested adjustment
-export NANOKVM_MCP_TOKEN=$(cat ~/.nanokvm-token) # add to your shell profile +export NANOKVM_MCP_TOKEN="$(cat ~/.nanokvm-token)" # current shell only; do not add to shell profiles🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 257 - 259, Update the README instructions around the NANOKVM_MCP_TOKEN export to remove the recommendation to add the bearer token to a shell profile. Keep the token session-scoped, or replace it with documented secure secret-store guidance without persisting the full token in plaintext configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-07-22-nanokvm-mcp-sidecar-design.md`:
- Around line 257-266: Remove or revise the preceding paragraph that recommends
Tailscale and claims it “costs nothing,” so the specification consistently
recommends SSH port-forwarding to the loopback listener and off-LAN access
through a subnet router on another host. Preserve the corrected 2026-07-30
guidance and eliminate the contradictory exposure recommendation.
In `@README.md`:
- Around line 351-360: Update the plist example’s SSH command to use non-XML
placeholder text instead of angle-bracket placeholders such as <you> and
<device>, and state that users must replace those placeholders with their actual
values before loading or validating the plist with plutil. Keep the example
valid, copyable XML.
- Around line 303-310: Update the SSH key setup documentation around the
ssh-keygen command to explicitly state that -N '' creates an unencrypted private
key and installing its public key for root grants full device SSH access.
Explain that this is an intentional tradeoff for unattended launchd reconnects,
and mention keychain/agent-backed storage or supported Dropbear key restrictions
as safer alternatives.
- Around line 201-205: The documentation references conflicting bearer-token
locations. Align the token source described near NANOKVM_MCP_TOKEN in README.md
and the corresponding sidecar design specification, choosing one canonical path
and preserving the generated-token fallback behavior consistently in both
documents.
- Around line 356-358: Add the SSH option ConnectTimeout=10 alongside the
existing options in the documented launchd configuration, preserving the current
ServerAliveInterval and ServerAliveCountMax settings.
- Around line 355-356: Update both README examples containing the SSH options to
add ControlPath=none alongside ControlMaster=no, and revise the accompanying
explanations to state that disabling multiplexing also requires preventing any
configured control path from being used.
---
Outside diff comments:
In `@README.md`:
- Around line 257-259: Update the README instructions around the
NANOKVM_MCP_TOKEN export to remove the recommendation to add the bearer token to
a shell profile. Keep the token session-scoped, or replace it with documented
secure secret-store guidance without persisting the full token in plaintext
configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 20a65abb-67d4-4399-88b7-865a3e357ba1
📒 Files selected for processing (2)
README.mddocs/superpowers/specs/2026-07-22-nanokvm-mcp-sidecar-design.md
ControlMaster=no does not prevent multiplexing. Per ssh_config(5) it is the default and is exactly the value that lets a session join an existing master; it governs whether this ssh becomes a master, not whether it reuses one. Only ControlPath=none disables sharing. The plist and the explanation for it were both wrong — a reader with ControlPath in their ssh config would have hit the respawn loop that bullet claimed to prevent. Add ConnectTimeout=10 so a connect to a sleeping device fails in 10s rather than sitting in the system TCP timeout before launchd sees an exit. The plist used <you>/<device> placeholders, which XML parses as elements, so the block as printed was not valid and was not what plutil validated earlier. Switched to YOUR-USER/DEVICE-ADDRESS and noted why the angle-bracket convention used elsewhere cannot apply inside XML. The block now lints verbatim. Document what the SSH key actually grants: -N '' leaves it unencrypted at rest, and an unrestricted key in root's authorized_keys is a root shell, not just a forward. Install it behind restrict,no-pty,command="/bin/false" so -N forwarding still works while interactive use does not, with the dropbear-version caveat and a verification that tests the forward rather than a shell. Note that neither failure mode locks you out, since password auth remains. Spec: strike through the superseded Tailscale sentence rather than only appending a correction under it, and correct the bearer-token source — it is the NANOKVM_MCP_TOKEN environment variable per internal/config/config.go, not /etc/kvm/.nanokvm_mcp_token, which nothing reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review addressed — 6 of 7 applied in 7e9c42bFive were straightforwardly right; one I applied differently than suggested; one I am declining on the facts.
The Declining: "do not store the bearer token in a shell profile"The premise does not hold. The line is: export NANOKVM_MCP_TOKEN=$(cat ~/.nanokvm-token) # add to your shell profileWhat goes into the profile is that line — a command substitution that reads the mode-0600 file at shell startup. The token itself is never written to the profile, so the finding's stated harm ("persists the full token in plaintext") does not occur. This is the by-reference pattern working as designed, not a contradiction of it. The suggested diff would also make the export session-only, which breaks the documented workflow: Claude Code expands There is a residual — the token lands in the environment of every process in that shell — but that is inherent to the One thing I could not verify: whether this device's dropbear build parses 🤖 Generated with Claude Code |
Keeps the stacked branch current with the corrections made on PR #26 (ControlPath=none, restricted SSH key, plist XML validity). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
Documents a launchd-supervised SSH port-forward as the durable day-to-day path, replacing the ad-hoc master socket that lapses after an hour idle. Direction confirmed on #16 — LAN-scoped durability; off-LAN reach is explicitly out of scope for now.
Why not Tailscale on the device
Evaluated and rejected on memory, with upstream measurements on this same hardware:
tailscaledsettles around 40 MB resident on a NanoKVM (sipeed/NanoKVM#366), against the 43 MB available this project measured in device recon.GOMEMLIMIT=100,GOGC=50, and a cron reboot every two days to stay up — and still saw an OOM kill at 78 MB — on a board with the larger 158 MB user-space split, i.e. ~11 MB more free RAM than ours (sipeed/NanoKVM#660).An OOM here isn't graceful — the kernel takes the largest resident process, which may be the firmware's video pipeline, so the KVM function dies along with the way back in to fix it.
This makes the README's existing "costs nothing" claim wrong, so it's corrected here and in the design spec. The reasoning behind it still holds — the LAN is the wrong place for this listener — it's the cost that was misstated.
Security model: unchanged
The tunnel needs no revision to the Security model section. The bind stays
127.0.0.1:8080, and reaching it costs an SSH credential plus the bearer token — two independent layers preserved. Binding to a tailnet address would have deleted the loopback-default bullet;tailscale servewould have kept it but still needed the daemon resident.Changes
### Durable access: a launchd-supervised tunnelunder "Connecting an MCP client"; rewritten Tailscale paragraph in "Security model"; generic client config now points at127.0.0.1with the reasoning; the Claude Code section's "Durable remote access path for day-to-day use #16 is tracking this" aside now links to the new section.Two things the docs call out because they fail silently otherwise:
/etc/dropbear/authorized_keysvariant, a verify step, and the caveat that it survives app updates but not a rootfs reflash. It replaces PR docs: day-to-day Claude Code setup (closes #9) #18'sPubkeyAuthentication=noagent-flood workaround rather than adding to it.-f(launchd supervises a foreground process),ControlMaster=no(aControlMaster autoin~/.ssh/configwould make the job hand off its forward and exit into a respawn loop), andExitOnForwardFailure=yeswith the server-alive limits (launchd can only restart something that exits).Verification
The plist was extracted from the rendered README, placeholders substituted, and checked with
plutil -lint(OK) andplutil -convert jsonto confirm it parses to exactly the intended argv — the column alignment insideProgramArgumentsis ignored as expected.Not run end-to-end against hardware: I have no tunnel to the device from this workspace, and I did not install Tailscale on it to measure directly — that's an invasive change I shouldn't make unasked, and #366/#660 measured it on the same board.
Docs only; no code, no
CHANGELOG.md(release PR in flight).Closes #16.
🤖 Generated with Claude Code