Remote backend: firewall check in server config, log redaction, model-review fixes

CHANGELOG.md

@@ -137,12 +137,49 @@ All notable changes to this project will be documented in this file.
   FEATURE_UNSUPPORTED`. The config file names no project, zone or key: every value comes from
   an environment variable. Images are baked with `make gce-image`; the one-time cloud setup is
   printed by `scripts/remote/gce-setup.sh --print`. Not built yet: copying an agent into a VM
-  (`agent_install` behaves as `never`) and any firewall mode but `static`. See the [Remote
+  (`agent_install` behaves as `never`). See the [Remote
   Backend reference](https://charliek.github.io/shed/reference/remote-backend/).
+- **Remote backend: the server checks its cloud firewall.** It reads the environment's rule
+  (`shed-<env>-ssh`) and the network's other ingress rules at start-up, once a minute, and
+  before a create or start. While the rule is missing, altered, admits the wrong sources, or
+  another rule reaches the sheds, `shed create` and `shed start` answer the new
+  `503 FIREWALL_CHECK_FAILED` with what was expected and what was found; stop, delete, list and
+  exec are unaffected and nothing is stopped or deleted. `remote.firewall.mode: static` now
+  needs its expected sources: `source_ranges`, or `source_ranges_env` naming a variable
+  (default `SHED_REMOTE_SOURCE_RANGES`) so the config file holds no address. **A remote server
+  upgraded to this version does not start until one of them is set.** `mode: auto` is built:
+  the server learns its public address from two agreeing `echo_urls` services and patches the
+  rule's sources to that `/32`, under two extra roles that
+  `gce-setup.sh --print --firewall-auto` prints. Each `echo_urls` entry must be on a different
+  host. A static source range in private space needs `acknowledge_any: true`, a network or
+  subnet in another project (a Shared VPC) is refused, and a create or start that reaches the
+  cloud while the check fails is not sent: the shed is left in `error` or `stopped` with the new
+  reason `firewall`.
+- **Remote backend: stricter config, and an abandoned create is destroyed.** An unknown key
+  anywhere under `remote:` now stops the load (a misspelt limit or firewall setting used to
+  fall back to its default). A static source range in `198.18.0.0/15` or `192.0.0.0/24` needs
+  `acknowledge_any: true`, auto mode never believes an address in those ranges or in
+  `240.0.0.0/4`, and an `echo_urls` host name outside ASCII must be written in its punycode
+  form. A VM still labelled `creating` 15 minutes after the cloud made it, with no record on
+  the server (a lost `state_dir`, or a server that died mid-create), is destroyed by the
+  start-up and periodic passes; before, it ran to its run limit and its disk stayed. Every
+  shed VM is made with `enable-oslogin=FALSE`. A streamed create refused by the firewall check
+  inside its job ends with `503 FIREWALL_CHECK_FAILED`, not a 500, and a service client's
+  `FIREWALL_CHECK_FAILED` message no longer names the rule, its source ranges or the server's
+  address.
+- **Logs: URL credentials with shell punctuation are redacted.** A password holding a
+  backslash, quote, backtick, brace, pipe, caret or angle bracket (`pa\$word` in a raw SSH
+  command) ended the scan early and reached the server, agent and audit logs whole; so did a
+  URL whose scheme the shell glues on (`"https"://`). Both are redacted now. In text with no
+  white space, an `@` after a path-less URL's closing quote is hidden with it. A create whose
+  `repo` URL does not parse gets a `400 INVALID_REPO_URL` that repeats none of the URL (the
+  parser's error used to quote it, password included).
 - **Development: a remote-backend dev server and suite parameter.**
   `make dev-server-up-remote` (and `-down-`, `-restart-`, `-status-`, `-logs-remote`) runs the
   dev build with `default_backend: remote` beside an installed server, reading the cloud
-  settings from a private env file. The integration suite gains an opt-in `remote` parameter
+  settings from a private env file. The env file also picks the firewall mode
+  (`SHED_REMOTE_FIREWALL_MODE=auto` with `SHED_REMOTE_ECHO_URLS`; static otherwise). The
+  integration suite gains an opt-in `remote` parameter
   (`SHED_REMOTE_SERVER`; `make test-integration-remote`, manual only, since every shed is a
   billed VM). See [Remote Backend
   Development](https://charliek.github.io/shed/development/remote-backend/).

Makefile

@@ -688,7 +688,11 @@ dev-server-restart-fc: dev-server-down-fc dev-server-up-fc
 # Cloud settings are private and never in this repo. The targets source them
 # from $(SHED_REMOTE_ENV_FILE) (VAR=value lines): SHED_REMOTE_ENV (the
 # environment name), SHED_GCE_PROJECT, SHED_GCE_ZONE, SHED_GCE_NETWORK,
-# SHED_GCE_SUBNET, SHED_GCE_CREDENTIALS_FILE and SHED_REMOTE_SSH_KEY_FILE.
+# SHED_GCE_SUBNET, SHED_GCE_CREDENTIALS_FILE, SHED_REMOTE_SSH_KEY_FILE and
+# SHED_REMOTE_SOURCE_RANGES (the firewall rule's source ranges). Two more
+# choose the firewall mode: SHED_REMOTE_FIREWALL_MODE (static, the default,
+# or auto) and, for auto, SHED_REMOTE_ECHO_URLS (two or more https echo
+# services on different hosts, comma-separated).
 # The config is rendered from configs/server.dev-remote.yaml.tmpl on every
 # start; edit the template, not the rendered file.
 SHED_REMOTE_ENV_FILE    ?= $(HOME)/.config/shed/remote-dev/env
@@ -734,10 +738,27 @@ dev-server-up-remote: build-server build-cli
 	  echo "ERROR: set SHED_REMOTE_GITHUB_USER (the GitHub account whose SSH keys may enrol a CLI)."; \
 	  exit 1; \
 	fi; \
+	FW_MODE="$${SHED_REMOTE_FIREWALL_MODE:-static}"; \
+	case "$$FW_MODE" in \
+	  static) FW_SETTING="source_ranges_env: SHED_REMOTE_SOURCE_RANGES" ;; \
+	  auto) \
+	    case "$$SHED_REMOTE_ECHO_URLS" in \
+	      ""|*[!A-Za-z0-9:/._,-]*) \
+	        echo "ERROR: SHED_REMOTE_FIREWALL_MODE=auto needs SHED_REMOTE_ECHO_URLS in $(SHED_REMOTE_ENV_FILE):"; \
+	        echo "       two or more https echo services on different hosts, comma-separated, no spaces."; \
+	        exit 1 ;; \
+	    esac; \
+	    FW_SETTING="echo_urls: [$$(printf '%s' "$$SHED_REMOTE_ECHO_URLS" | sed 's/,/, /g')]" ;; \
+	  *) \
+	    echo "ERROR: SHED_REMOTE_FIREWALL_MODE is '$$FW_MODE'; want static or auto."; \
+	    exit 1 ;; \
+	esac; \
 	umask 077; \
 	sed -e "s|@DEV_REMOTE_DIR@|$(REMOTE_DEV_DIR)|g" \
 	    -e "s|@SHED_REMOTE_ENV@|$$SHED_REMOTE_ENV|g" \
 	    -e "s|@GITHUB_USER@|$$GITHUB_USER|g" \
+	    -e "s|@FIREWALL_MODE@|$$FW_MODE|g" \
+	    -e "s|@FIREWALL_SETTING@|$$FW_SETTING|g" \
 	    "$(REMOTE_DEV_TEMPLATE)" > "$(REMOTE_DEV_CONFIG)" || exit 1; \
 	echo "=== dev-server-up-remote $$(date -u +%Y-%m-%dT%H:%M:%SZ) ===" >> "$(REMOTE_DEV_LOG_PATH)"; \
 	START_LINE=$$(wc -l < "$(REMOTE_DEV_LOG_PATH)"); \

cmd/shed-agent/exec.go

@@ -17,6 +17,8 @@ import (
 	"time"
 
 	"github.com/creack/pty"
+
+	"github.com/charliek/shed/internal/redact"
 )
 
 // connKeepaliveInterval is how often the exec paths probe the host connection
@@ -58,7 +60,9 @@ func handleExecConnection(conn net.Conn, user *userInfo) {
 		req.Cmd = []string{"/bin/bash", "--login"}
 	}
 
-	log.Printf("Executing: %v (TTY: %v)", req.Cmd, req.TTY)
+	// The log gets argv with URL credentials redacted; the command runs as
+	// given. Env is never logged.
+	log.Printf("Executing: %v (TTY: %v)", redact.Args(req.Cmd), req.TTY)
 
 	// Create the command
 	cmd := exec.Command(req.Cmd[0], req.Cmd[1:]...)
@@ -154,7 +158,7 @@ func runWithPTY(conn net.Conn, cmd *exec.Cmd, rows, cols uint16) {
 	// Start command with PTY
 	ptmx, err := pty.Start(cmd)
 	if err != nil {
-		log.Printf("Failed to start command with PTY: %v", err)
+		log.Printf("Failed to start command with PTY: %s", redact.String(err.Error()))
 		if err := writeExitCode(conn, 1); err != nil {
 			log.Printf("Warning: failed to write exit code: %v", err)
 		}
@@ -292,7 +296,7 @@ func runWithoutPTY(conn net.Conn, cmd *exec.Cmd) {
 
 	// Start command
 	if err := cmd.Start(); err != nil {
-		log.Printf("Failed to start command: %v", err)
+		log.Printf("Failed to start command: %s", redact.String(err.Error()))
 		if err := writeExitCode(conn, 1); err != nil {
 			log.Printf("Warning: failed to write exit code: %v", err)
 		}

cmd/shed-agent/exec_redact_test.go

@@ -0,0 +1,50 @@
+//go:build linux
+
+package main
+
+import (
+	"bytes"
+	"log"
+	"net"
+	"os"
+	"strings"
+	"testing"
+)
+
+// TestHandleExecLogsRedactedArgvAndNoEnv: the command runs exactly as sent
+// (credentials and env reach it), while the agent's log line has URL
+// credentials redacted and never names env.
+func TestHandleExecLogsRedactedArgvAndNoEnv(t *testing.T) {
+	var logged bytes.Buffer
+	log.SetOutput(&logged)
+	t.Cleanup(func() { log.SetOutput(os.Stderr) })
+
+	c, s := net.Pipe()
+	defer c.Close()
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		handleExecConnection(s, nil)
+	}()
+
+	req := ExecRequest{
+		Cmd: []string{"sh", "-c", `printf '%s %s' "$1" "$SHED_TEST_TOKEN"`, "sh", "https://u:sentinel-pass@h/r.git"},
+		Env: []string{"SHED_TEST_TOKEN=sentinel-env"},
+	}
+	send(t, c, MsgTypeExecRequest, mustJSON(t, req))
+	send(t, c, MsgTypeStdinEOF, nil)
+	out, _ := readOutputFrames(t, c)
+	_ = c.Close()
+	<-done
+
+	if got, want := string(out), "https://u:sentinel-pass@h/r.git sentinel-env"; got != want {
+		t.Errorf("command saw %q, want %q", got, want)
+	}
+	line := logged.String()
+	if strings.Contains(line, "sentinel") || strings.Contains(line, "SHED_TEST_TOKEN=") {
+		t.Errorf("agent log carries a credential or env: %q", line)
+	}
+	if !strings.Contains(line, "https://REDACTED@h/r.git") {
+		t.Errorf("agent log = %q, want the argv with the URL redacted", line)
+	}
+}

cmd/shed-server/remote.go

@@ -47,6 +47,7 @@ func newRemoteBackend(cfg *config.ServerConfig, bridge *plugin.Bridge) (*remote.
 		return nil, fmt.Errorf("remote backend: %w", err)
 	}
 	stateRoot := serverStateDir(cfg)
+	firewallSources := rt.FirewallSources
 
 	var prov provider.Provider
 	switch rcfg.Provider {
@@ -69,7 +70,13 @@ func newRemoteBackend(cfg *config.ServerConfig, bridge *plugin.Bridge) (*remote.
 		}
 		prov = client
 	case config.RemoteProviderFake:
-		prov = fake.New(fake.Options{Env: rcfg.Environment})
+		// The fake's firewall rule carries whatever static mode expects, so
+		// the check passes; with no ranges given it is the fake's own
+		// documentation address.
+		if rcfg.Firewall.Mode == config.RemoteFirewallStatic && len(firewallSources) == 0 {
+			firewallSources = []string{fake.DefaultFirewallSource}
+		}
+		prov = fake.New(fake.Options{Env: rcfg.Environment, FirewallSources: firewallSources})
 	default:
 		return nil, fmt.Errorf("remote backend: unknown provider %q", rcfg.Provider)
 	}
@@ -106,6 +113,7 @@ func newRemoteBackend(cfg *config.ServerConfig, bridge *plugin.Bridge) (*remote.
 		Links:            links,
 		StateDir:         rcfg.StateDirPath(stateRoot),
 		AuthorizedKeys:   authorizedKeys,
+		FirewallSources:  firewallSources,
 		Bridge:           bridge,
 		KeepFailedCreate: os.Getenv(keepFailedCreateEnv) == "1",
 	})

cmd/shed/console_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/anmitsu/go-shlex"
 
 	"github.com/charliek/shed/internal/config"
+	"github.com/charliek/shed/internal/redact"
 )
 
 // TestShellQuoteRoundTrip verifies that every element of argv survives a
@@ -382,3 +383,35 @@ func TestSSHSessionArgs(t *testing.T) {
 		}
 	})
 }
+
+// TestQuotedArgvRedacts: the command string the CLI sends is what the server
+// and the agent log, after redact has been over it. A URL password must stay
+// hidden whatever the quoting did to it, an apostrophe included (which is
+// sent as '\” and so puts a backslash in the middle of the userinfo).
+func TestQuotedArgvRedacts(t *testing.T) {
+	passwords := []struct {
+		name     string
+		password string
+	}{
+		{"plain", "s3cr3t-token"},
+		{"an apostrophe", "s3cr3t'token"},
+		{"two apostrophes", "'s3cr3t'token"},
+		{"an apostrophe at the end", "s3cr3t-token'"},
+	}
+	for _, tt := range passwords {
+		t.Run(tt.name, func(t *testing.T) {
+			quoted, err := validateAndQuoteArgs([]string{"git", "clone", "https://user:" + tt.password + "@git.example/o/r.git"})
+			if err != nil {
+				t.Fatal(err)
+			}
+			wire := strings.Join(quoted, " ")
+			got := redact.String(wire)
+			if strings.Contains(got, "s3cr3t") || strings.Contains(got, "token") {
+				t.Errorf("the redacted command still carries the password: %s", got)
+			}
+			if want := "'git' 'clone' 'https://REDACTED@git.example/o/r.git'"; got != want {
+				t.Errorf("redacted = %s, want %s", got, want)
+			}
+		})
+	}
+}

configs/server.dev-remote.yaml.tmpl

@@ -1,11 +1,12 @@
 # Shed Server — Parallel Dev Configuration (remote backend)
 #
 # A TEMPLATE, not a config: `make dev-server-up-remote` renders it to
-# ~/.shed/dev-remote/server.yaml, replacing the three @...@ placeholders, and
+# ~/.shed/dev-remote/server.yaml, replacing the @...@ placeholders, and
 # runs the dev build with it. Nothing private lives here. The cloud project,
-# zone, network, subnet, credentials and SSH client key are read from the
-# process environment at start-up through the `*_env` names in the `remote:`
-# block; the Make target sources them from a private env file
+# zone, network, subnet, credentials, SSH client key and the firewall rule's
+# source ranges are read from the process environment at start-up through the
+# `*_env` names in the `remote:` block; the Make target sources them from a
+# private env file
 # (default ~/.config/shed/remote-dev/env, override with SHED_REMOTE_ENV_FILE).
 #
 # The server runs ALONGSIDE an installed shed-server on the same machine:
@@ -78,8 +79,31 @@ remote:
   ssh:
     client_key_env: SHED_REMOTE_SSH_KEY
     client_key_file_env: SHED_REMOTE_SSH_KEY_FILE
+  # The server reads the environment's cloud firewall rule (shed-<env>-ssh)
+  # and refuses creates and starts (503 FIREWALL_CHECK_FAILED) while the rule
+  # or its network is not as scripts/remote/gce-setup.sh made it.
   firewall:
-    mode: static
+    # `make dev-server-up-remote` renders the two lines after this comment
+    # from the private env file, so neither an address nor a service you
+    # trust is named here.
+    #
+    # static (the default): the rule's source ranges must equal the
+    # comma-separated list in SHED_REMOTE_SOURCE_RANGES (your public address
+    # as a /32). The server only reads the rule. Rendered as
+    #   mode: static
+    #   source_ranges_env: SHED_REMOTE_SOURCE_RANGES
+    #
+    # auto (SHED_REMOTE_FIREWALL_MODE=auto), for a machine whose public
+    # address changes: the server asks the echo services for its address
+    # and, when two agree, patches the rule's sources to that one /32. It
+    # needs the extra roles that `gce-setup.sh --print --firewall-auto`
+    # prints, and SHED_REMOTE_ECHO_URLS: two or more https services you
+    # trust, on different hosts, comma-separated, that answer with the
+    # caller's IPv4 address as plain text. Rendered as
+    #   mode: auto
+    #   echo_urls: [https://<first-echo-service>/, https://<second-echo-service>/]
+    mode: @FIREWALL_MODE@
+    @FIREWALL_SETTING@
   gce:
     project_env: SHED_GCE_PROJECT
     zone_env: SHED_GCE_ZONE