Harden cross-machine routing, fix compile, add auth/error tests & docs

README.md

@@ -232,6 +232,13 @@ rcc myserver ~/.ssh/config ~/.ssh/config
 rcc myserver /app/dist /app/dist --stream
 ```
 
+> **Size limit:** a file (or directory archive) is sent as a **single** message,
+> so transfers are capped at **100 MB** by default — larger payloads are silently
+> dropped by the relay, so the CLI now refuses them with a clear error up front.
+> To move something bigger, split it (`split -b 16M`, copy each part, `cat` on the
+> target) or raise `REMOTECMD_MAX_TRANSFER_MB` if your self-hosted relay permits
+> larger frames.
+
 ---
 
 ## Pairing Flow
@@ -322,7 +329,7 @@ ALIASES:
   remotecmd-cli alias uninstall     Remove installed aliases
 
 RELAY:
-  remotecmd-cli relay daemon start [--port 3032] [-daemon]
+  remotecmd-cli relay daemon start [--port 3032] [-daemon] [--tls-cert <f>] [--tls-key <f>]
   remotecmd-cli relay daemon stop
   remotecmd-cli relay daemon status
 
@@ -400,6 +407,51 @@ DAEMON:
 
 ---
 
+## Security & authentication
+
+Every target is identified by a **name + token** pair. The token is auto-generated
+when the daemon first starts (or during pairing) and stored in
+`~/.remotecmd/config.json` on both sides — there are no SSH keys to distribute.
+
+How the relay enforces auth on each request:
+
+- **Registration** — a daemon must present a non-empty `name` *and* `token` to
+  register. The relay indexes the live connection by name.
+- **Per-command check** — before forwarding an `execute`, `file_transfer`, or a
+  multi-target sub-command, the relay compares the client-supplied token against
+  the registered target's token using a **constant-time comparison**
+  (`crypto/subtle`), so a wrong token never reveals its length or match position
+  through response timing. A mismatch returns `invalid token for target: <name>`
+  and the command is never forwarded.
+- **Independent multi-target auth** — in a fan-out (`--targets` / `--group`),
+  each target is authenticated on its own. A bad token for one target fails only
+  that target (`invalid token`); the rest still run.
+- **Blind router** — the relay never executes anything itself. It only forwards
+  an authenticated command to the matching target daemon and relays the result.
+- **No inbound ports** — daemons dial *out* to the relay, so targets need no open
+  ports and no firewall changes.
+
+**Encryption in transit.** Plain `ws://` is unencrypted. For untrusted networks,
+either terminate TLS at a reverse proxy and point clients at `wss://…`, or run the
+relay with a certificate directly:
+
+```bash
+remotecmd-cli relay daemon start --port 3032 --tls-cert cert.pem --tls-key key.pem -daemon
+```
+
+> **Token hygiene:** if a target restarts with a new token, re-add it on the
+> client with `add-target --name <n> --token <new>`. Treat tokens like passwords —
+> anyone holding a target's token can run commands on it through the relay.
+
+### Environment variables
+
+| Variable | Purpose | Default |
+|----------|---------|---------|
+| `REMOTECMD_FEEDBACK_RELAY` | Override the endpoint used by `remotecmd-cli feedback`. Set to `off` to disable feedback entirely. | Central feedback relay |
+| `REMOTECMD_MAX_TRANSFER_MB` | Max size (in MB) for a single `cp` payload. Set to `0` to disable the check (e.g. a self-hosted relay with a raised frame limit). | `100` |
+
+---
+
 ## Use Cases
 
 | Scenario | Command |
@@ -447,10 +499,13 @@ DAEMON:
 
 See [docs/vision.md](docs/vision.md) for the full vision and roadmap.
 
+Shipped: script-friendly exit codes, systemd unit generation, persistent client
+connections, and optional relay TLS (`--tls-cert` / `--tls-key`, see
+[Security & authentication](#security--authentication)).
+
 Upcoming priorities:
-- **v1.3**: Script-friendly exit codes, systemd unit generation
-- **v1.4**: Persistent client connections for faster sequential commands
-- **v1.5**: Optional TLS for relay encryption
+- Port forwarding (replace `ssh -L` for ad-hoc tunnels)
+- Team management and audit logging on the hosted relay
 
 ---
 

coverage_edge_test.go

@@ -104,6 +104,9 @@ func TestListTargetsWithGroups(t *testing.T) {
 }
 
 func TestSaveTokenError(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("running as root: read-only dir permissions are ignored")
+	}
 	_, cleanup := setupTestConfig(t)
 	defer cleanup()
 

exittest_test.go

@@ -274,6 +274,9 @@ func TestHandleCPMissingArgs(t *testing.T) {
 }
 
 func TestHandleRelayInstallSystemdNotRoot(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("running as root: the not-root guard does not trigger")
+	}
 	// Without root, this should tell the user to use sudo
 	assertExitCode(t, ExitConfigError, func() {
 		handleRelayInstallSystemd()

filetransfer.go

@@ -9,11 +9,49 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strconv"
 	"time"
 
 	"github.com/gorilla/websocket"
 )
 
+// defaultMaxTransferBytes bounds a single `cp` payload. The whole file is
+// base64-encoded into ONE WebSocket message (see handleFileTransfer), so a large
+// file is held in memory ~3x over and, more importantly, is silently dropped by
+// the hosted relay / its reverse proxy once it exceeds their frame limit — the
+// client would otherwise hang with no error (issue #1). 100 MB sits safely below
+// the observed ~200 MB break point. Override with REMOTECMD_MAX_TRANSFER_MB
+// (set to 0 to disable the check, e.g. for a self-hosted relay with a raised
+// proxy limit). Transparent chunking is tracked separately as the real fix.
+const defaultMaxTransferBytes = 100 << 20 // 100 MB
+
+// maxTransferBytes returns the current per-transfer payload limit in bytes. A
+// value of 0 disables the limit. Invalid overrides fall back to the default.
+func maxTransferBytes() int64 {
+	if v := os.Getenv("REMOTECMD_MAX_TRANSFER_MB"); v != "" {
+		if mb, err := strconv.ParseInt(v, 10, 64); err == nil && mb >= 0 {
+			return mb << 20
+		}
+	}
+	return defaultMaxTransferBytes
+}
+
+// checkTransferSize returns a clear, actionable error when a payload of size
+// bytes would exceed the configured transfer limit, instead of letting the
+// relay silently stall on an oversized frame.
+func checkTransferSize(size int64, what string) error {
+	limit := maxTransferBytes()
+	if limit <= 0 || size <= limit {
+		return nil
+	}
+	return fmt.Errorf(
+		"%s is %d MB, which exceeds the %d MB single-transfer limit — the relay "+
+			"silently drops payloads this large. Split it first "+
+			"(e.g. `split -b 16M`, cp each part, then `cat` on the target), or raise "+
+			"the limit with REMOTECMD_MAX_TRANSFER_MB if your relay allows larger frames",
+		what, size>>20, limit>>20)
+}
+
 func handleCP(args []string) {
 	fs := flag.NewFlagSet("cp", flag.ExitOnError)
 	target := fs.String("target", "", "target machine name")
@@ -57,6 +95,16 @@ func handleFileTransfer(target, src, dst string, stream bool) error {
 		return fmt.Errorf("stat source: %v", err)
 	}
 
+	// Reject oversized single files up front with a clear error rather than
+	// building the payload and stalling on the relay (issue #1). Directories are
+	// checked after their tar archive is built, since their on-wire size is the
+	// archive size, not the sum of file sizes on disk.
+	if !info.IsDir() {
+		if err := checkTransferSize(info.Size(), "file"); err != nil {
+			return err
+		}
+	}
+
 	var mode string
 	var content string
 
@@ -77,6 +125,9 @@ func handleFileTransfer(target, src, dst string, stream bool) error {
 		if err != nil {
 			return fmt.Errorf("creating tar archive: %v", err)
 		}
+		if err := checkTransferSize(int64(len(tarData)), "directory archive"); err != nil {
+			return err
+		}
 		content = base64.StdEncoding.EncodeToString(tarData)
 		if stream {
 			emitProgress("archived", map[string]interface{}{

filetransfer_size_test.go

@@ -0,0 +1,78 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestMaxTransferBytesDefault verifies the default per-transfer limit when no
+// override is set.
+func TestMaxTransferBytesDefault(t *testing.T) {
+	t.Setenv("REMOTECMD_MAX_TRANSFER_MB", "")
+	if got := maxTransferBytes(); got != defaultMaxTransferBytes {
+		t.Errorf("maxTransferBytes() = %d, want default %d", got, defaultMaxTransferBytes)
+	}
+}
+
+// TestMaxTransferBytesOverride verifies a valid override is honored and that a
+// zero disables the limit.
+func TestMaxTransferBytesOverride(t *testing.T) {
+	t.Setenv("REMOTECMD_MAX_TRANSFER_MB", "10")
+	if got, want := maxTransferBytes(), int64(10<<20); got != want {
+		t.Errorf("override 10 => %d, want %d", got, want)
+	}
+
+	t.Setenv("REMOTECMD_MAX_TRANSFER_MB", "0")
+	if got := maxTransferBytes(); got != 0 {
+		t.Errorf("override 0 (disabled) => %d, want 0", got)
+	}
+}
+
+// TestMaxTransferBytesInvalidOverrideFallsBack verifies that a malformed or
+// negative override is ignored in favor of the default rather than producing a
+// nonsensical limit.
+func TestMaxTransferBytesInvalidOverrideFallsBack(t *testing.T) {
+	for _, bad := range []string{"abc", "-5", "12.5"} {
+		t.Setenv("REMOTECMD_MAX_TRANSFER_MB", bad)
+		if got := maxTransferBytes(); got != defaultMaxTransferBytes {
+			t.Errorf("override %q => %d, want default %d", bad, got, defaultMaxTransferBytes)
+		}
+	}
+}
+
+// TestCheckTransferSizeUnderLimit verifies payloads at or below the limit pass.
+func TestCheckTransferSizeUnderLimit(t *testing.T) {
+	t.Setenv("REMOTECMD_MAX_TRANSFER_MB", "100")
+	if err := checkTransferSize(50<<20, "file"); err != nil {
+		t.Errorf("50MB under 100MB limit should pass, got %v", err)
+	}
+	if err := checkTransferSize(100<<20, "file"); err != nil {
+		t.Errorf("exactly 100MB should pass, got %v", err)
+	}
+}
+
+// TestCheckTransferSizeOverLimit verifies oversized payloads return a clear,
+// actionable error naming the size, the limit, and a workaround — instead of the
+// silent relay stall in issue #1.
+func TestCheckTransferSizeOverLimit(t *testing.T) {
+	t.Setenv("REMOTECMD_MAX_TRANSFER_MB", "100")
+	err := checkTransferSize(250<<20, "file")
+	if err == nil {
+		t.Fatal("250MB over 100MB limit should error")
+	}
+	msg := err.Error()
+	for _, want := range []string{"250 MB", "100 MB", "split", "REMOTECMD_MAX_TRANSFER_MB"} {
+		if !strings.Contains(msg, want) {
+			t.Errorf("error message missing %q: %s", want, msg)
+		}
+	}
+}
+
+// TestCheckTransferSizeDisabled verifies that a zero limit disables the guard so
+// self-hosted relays with larger frame limits are not blocked.
+func TestCheckTransferSizeDisabled(t *testing.T) {
+	t.Setenv("REMOTECMD_MAX_TRANSFER_MB", "0")
+	if err := checkTransferSize(5<<30, "file"); err != nil {
+		t.Errorf("limit disabled (0) should allow any size, got %v", err)
+	}
+}

integration_test.go

@@ -350,17 +350,33 @@ func TestIntegrationRegisterReplacesOld(t *testing.T) {
 	rs, port := startTestRelay(t)
 	defer func() { _ = rs }()
 
-	// First daemon connects
+	// First daemon connects with the target's real token and answers commands.
 	d1 := testDaemon(t, "http://127.0.0.1:"+fmt.Sprintf("%d", port), "testbox", "tok1")
 	defer d1.Close()
+	testDaemonResponder(t, d1, 0, "still-alive")
 
-	// Second daemon connects with same name but different token
-	d2 := testDaemon(t, "http://127.0.0.1:"+fmt.Sprintf("%d", port), "testbox", "tok2")
+	// A second connection re-registers the SAME name with a DIFFERENT token.
+	// This must be rejected as a hijack attempt — the original registration
+	// (tok1) stays authoritative and reachable.
+	u2 := wsURL("http://127.0.0.1:" + fmt.Sprintf("%d", port))
+	d2, _, err := websocket.DefaultDialer.Dial(u2, nil)
+	if err != nil {
+		t.Fatalf("impostor dial: %v", err)
+	}
 	defer d2.Close()
+	d2.WriteJSON(&Message{Type: "register", Name: "testbox", Token: "tok2"})
+	var regResp Message
+	if err := d2.ReadJSON(&regResp); err != nil {
+		t.Fatalf("impostor read register response: %v", err)
+	}
+	if regResp.Type != "error" {
+		t.Errorf("impostor re-register should be rejected with an error, got type %q", regResp.Type)
+	}
 
 	time.Sleep(50 * time.Millisecond)
 
-	// Client tries to execute with old token — should fail
+	// Client executing with the original token still succeeds: the hijack
+	// attempt did not evict the real target.
 	u := wsURL("http://127.0.0.1:" + fmt.Sprintf("%d", port))
 	client, _, err := websocket.DefaultDialer.Dial(u, nil)
 	if err != nil {
@@ -378,8 +394,26 @@ func TestIntegrationRegisterReplacesOld(t *testing.T) {
 
 	var result Message
 	client.ReadJSON(&result)
-	if result.OK != nil && *result.OK {
-		t.Error("expected failure with old token")
+	if result.OK == nil || !*result.OK {
+		errMsg := ""
+		if result.Error != "" {
+			errMsg = ": " + result.Error
+		}
+		t.Errorf("original target should stay reachable after a rejected re-register with a different token%s", errMsg)
+	}
+
+	// The impostor token must NOT be honored for routing.
+	client.WriteJSON(&Message{
+		Type:   "execute",
+		ID:     newID(),
+		Target: "testbox",
+		Token:  "tok2",
+		Cmd:    "echo hi",
+	})
+	var bad Message
+	client.ReadJSON(&bad)
+	if bad.OK != nil && *bad.OK {
+		t.Error("impostor token tok2 should be rejected for target testbox")
 	}
 }
 

main.go

@@ -27,7 +27,7 @@ func main() {
 	case "remove-target":
 		handleRemoveTarget(os.Args[2:])
 	case "list-targets":
-		handleListTargets(os.Args[2:])
+		handleListTargets()
 	case "set-relay":
 		handleSetRelay(os.Args[2:])
 	case "relay":
@@ -46,8 +46,6 @@ func main() {
 		handleGroupSubcommand(os.Args[2:])
 	case "client":
 		handleClientSubcommand(os.Args[2:])
-	case "tunnel":
-		handleTunnel(os.Args[2:])
 	case "feedback":
 		handleFeedback(os.Args[2:])
 	case "version":