Skip to content

Commit 28d80c9

Browse files
authored
Merge pull request #453 from charliek/feature/remote-hardening
Remote backend: firewall check in server config, log redaction, model-review fixes
2 parents 99ac2a8 + 6e7b3b3 commit 28d80c9

89 files changed

Lines changed: 9479 additions & 315 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎CHANGELOG.md‎

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,12 +137,70 @@ All notable changes to this project will be documented in this file.
137137
FEATURE_UNSUPPORTED`. The config file names no project, zone or key: every value comes from
138138
an environment variable. Images are baked with `make gce-image`; the one-time cloud setup is
139139
printed by `scripts/remote/gce-setup.sh --print`. Not built yet: copying an agent into a VM
140-
(`agent_install` behaves as `never`) and any firewall mode but `static`. See the [Remote
140+
(`agent_install` behaves as `never`). See the [Remote
141141
Backend reference](https://charliek.github.io/shed/reference/remote-backend/).
142+
- **Remote backend: the server checks its cloud firewall.** It reads the environment's rule
143+
(`shed-<env>-ssh`) and the network's other ingress rules at start-up, once a minute, and
144+
before a create or start. While the rule is missing, altered, admits the wrong sources, or
145+
another rule reaches the sheds, `shed create` and `shed start` answer the new
146+
`503 FIREWALL_CHECK_FAILED` with what was expected and what was found; stop, delete, list and
147+
exec are unaffected and nothing is stopped or deleted. `remote.firewall.mode: static` now
148+
needs its expected sources: `source_ranges`, or `source_ranges_env` naming a variable
149+
(default `SHED_REMOTE_SOURCE_RANGES`) so the config file holds no address. **A remote server
150+
upgraded to this version does not start until one of them is set.** `mode: auto` is built:
151+
the server learns its public address from two agreeing `echo_urls` services and patches the
152+
rule's sources to that `/32`, under two extra roles that
153+
`gce-setup.sh --print --firewall-auto` prints. Each `echo_urls` entry must be on a different
154+
host. A static source range in private space needs `acknowledge_any: true`, a network or
155+
subnet in another project (a Shared VPC) is refused, and a create or start that reaches the
156+
cloud while the check fails is not sent: the shed is left in `error` or `stopped` with the new
157+
reason `firewall`.
158+
- **Remote backend: stricter config, and an abandoned create is destroyed.** An unknown key
159+
anywhere under `remote:` now stops the load (a misspelt limit or firewall setting used to
160+
fall back to its default). A static source range in `198.18.0.0/15` or `192.0.0.0/24` needs
161+
`acknowledge_any: true`, auto mode never believes an address in those ranges or in
162+
`240.0.0.0/4`, and an `echo_urls` host name outside ASCII must be written in its punycode
163+
form. A VM still labelled `creating` 15 minutes after the cloud made it, with no record on
164+
the server (a lost `state_dir`, or a server that died mid-create), is destroyed by the
165+
start-up and periodic passes; before, it ran to its run limit and its disk stayed. Every
166+
shed VM is made with `enable-oslogin=FALSE`. A streamed create refused by the firewall check
167+
inside its job ends with `503 FIREWALL_CHECK_FAILED`, not a 500, and a service client's
168+
`FIREWALL_CHECK_FAILED` message no longer names the rule, its source ranges or the server's
169+
address.
170+
- **Logs: URL credentials with shell punctuation are redacted.** A password holding a
171+
backslash, quote, backtick, brace, pipe, caret or angle bracket (`pa\$word` in a raw SSH
172+
command) ended the scan early and reached the server, agent and audit logs whole; so did a
173+
URL whose scheme the shell glues on (`"https"://`). Both are redacted now. In text with no
174+
white space, an `@` after a path-less URL's closing quote is hidden with it. A create whose
175+
`repo` URL does not parse gets a `400 INVALID_REPO_URL` that repeats none of the URL (the
176+
parser's error used to quote it, password included).
177+
- **Remote backend: a failed read of the cloud never destroys or stops a VM.** A create or start
178+
resumed after a server restart reads its VM first. When that read failed, the create was
179+
failed and its VM queued for destruction (a finished shed included, if the restart came
180+
between the create's last two writes), and a start was failed and its running VM stopped. Such
181+
a shed now stays `starting` with the new reason `cloud_unreadable`, nothing is changed, and
182+
the periodic pass looks again; a delete ends it at any time. A resumed create also leaves a VM
183+
whose labels are another shed version's untouched, where it used to connect, clone and
184+
provision before it noticed, and a start that finds such labels is kept the same way, where it
185+
used to record the shed `stopped` while its VM might be running. A delete whose marker was
186+
written but could not be flushed to disk is acknowledged and carried out; it used to answer
187+
500 and destroy the VM two minutes later all the same. A streamed delete whose destroy failed
188+
is no longer reported as done when the server cannot read its delete markers; it ends in an
189+
error. A create or start that needs a new firewall check waits 10 s at most, the wait for a
190+
check already in progress included, and gives up at once when its client has gone.
191+
- **Logs: three more ways a URL's credentials could reach a log are closed.** The second of
192+
two URLs with no white space between them (`["https://u:p@one","https://v:q@two"]`, as a
193+
JSON-encoded argv is written) kept its user and password in the exec log line and the audit
194+
record. Clone or hook output that stopped inside a URL, before its `@`, was logged as it
195+
stood; the server log now shows `scheme://REDACTED` for a URL still open where a stream
196+
ends, host included. And one 16 MiB write with no space or line break was copied whole
197+
before it was dropped; the redacting writer now holds 128 KiB at most.
142198
- **Development: a remote-backend dev server and suite parameter.**
143199
`make dev-server-up-remote` (and `-down-`, `-restart-`, `-status-`, `-logs-remote`) runs the
144200
dev build with `default_backend: remote` beside an installed server, reading the cloud
145-
settings from a private env file. The integration suite gains an opt-in `remote` parameter
201+
settings from a private env file. The env file also picks the firewall mode
202+
(`SHED_REMOTE_FIREWALL_MODE=auto` with `SHED_REMOTE_ECHO_URLS`; static otherwise). The
203+
integration suite gains an opt-in `remote` parameter
146204
(`SHED_REMOTE_SERVER`; `make test-integration-remote`, manual only, since every shed is a
147205
billed VM). See [Remote Backend
148206
Development](https://charliek.github.io/shed/development/remote-backend/).

‎Makefile‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -688,7 +688,11 @@ dev-server-restart-fc: dev-server-down-fc dev-server-up-fc
688688
# Cloud settings are private and never in this repo. The targets source them
689689
# from $(SHED_REMOTE_ENV_FILE) (VAR=value lines): SHED_REMOTE_ENV (the
690690
# environment name), SHED_GCE_PROJECT, SHED_GCE_ZONE, SHED_GCE_NETWORK,
691-
# SHED_GCE_SUBNET, SHED_GCE_CREDENTIALS_FILE and SHED_REMOTE_SSH_KEY_FILE.
691+
# SHED_GCE_SUBNET, SHED_GCE_CREDENTIALS_FILE, SHED_REMOTE_SSH_KEY_FILE and
692+
# SHED_REMOTE_SOURCE_RANGES (the firewall rule's source ranges). Two more
693+
# choose the firewall mode: SHED_REMOTE_FIREWALL_MODE (static, the default,
694+
# or auto) and, for auto, SHED_REMOTE_ECHO_URLS (two or more https echo
695+
# services on different hosts, comma-separated).
692696
# The config is rendered from configs/server.dev-remote.yaml.tmpl on every
693697
# start; edit the template, not the rendered file.
694698
SHED_REMOTE_ENV_FILE ?= $(HOME)/.config/shed/remote-dev/env
@@ -734,10 +738,27 @@ dev-server-up-remote: build-server build-cli
734738
echo "ERROR: set SHED_REMOTE_GITHUB_USER (the GitHub account whose SSH keys may enrol a CLI)."; \
735739
exit 1; \
736740
fi; \
741+
FW_MODE="$${SHED_REMOTE_FIREWALL_MODE:-static}"; \
742+
case "$$FW_MODE" in \
743+
static) FW_SETTING="source_ranges_env: SHED_REMOTE_SOURCE_RANGES" ;; \
744+
auto) \
745+
case "$$SHED_REMOTE_ECHO_URLS" in \
746+
""|*[!A-Za-z0-9:/._,-]*) \
747+
echo "ERROR: SHED_REMOTE_FIREWALL_MODE=auto needs SHED_REMOTE_ECHO_URLS in $(SHED_REMOTE_ENV_FILE):"; \
748+
echo " two or more https echo services on different hosts, comma-separated, no spaces."; \
749+
exit 1 ;; \
750+
esac; \
751+
FW_SETTING="echo_urls: [$$(printf '%s' "$$SHED_REMOTE_ECHO_URLS" | sed 's/,/, /g')]" ;; \
752+
*) \
753+
echo "ERROR: SHED_REMOTE_FIREWALL_MODE is '$$FW_MODE'; want static or auto."; \
754+
exit 1 ;; \
755+
esac; \
737756
umask 077; \
738757
sed -e "s|@DEV_REMOTE_DIR@|$(REMOTE_DEV_DIR)|g" \
739758
-e "s|@SHED_REMOTE_ENV@|$$SHED_REMOTE_ENV|g" \
740759
-e "s|@GITHUB_USER@|$$GITHUB_USER|g" \
760+
-e "s|@FIREWALL_MODE@|$$FW_MODE|g" \
761+
-e "s|@FIREWALL_SETTING@|$$FW_SETTING|g" \
741762
"$(REMOTE_DEV_TEMPLATE)" > "$(REMOTE_DEV_CONFIG)" || exit 1; \
742763
echo "=== dev-server-up-remote $$(date -u +%Y-%m-%dT%H:%M:%SZ) ===" >> "$(REMOTE_DEV_LOG_PATH)"; \
743764
START_LINE=$$(wc -l < "$(REMOTE_DEV_LOG_PATH)"); \

‎cmd/shed-agent/exec.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ import (
1717
"time"
1818

1919
"github.com/creack/pty"
20+
21+
"github.com/charliek/shed/internal/redact"
2022
)
2123

2224
// connKeepaliveInterval is how often the exec paths probe the host connection
@@ -58,7 +60,9 @@ func handleExecConnection(conn net.Conn, user *userInfo) {
5860
req.Cmd = []string{"/bin/bash", "--login"}
5961
}
6062

61-
log.Printf("Executing: %v (TTY: %v)", req.Cmd, req.TTY)
63+
// The log gets argv with URL credentials redacted; the command runs as
64+
// given. Env is never logged.
65+
log.Printf("Executing: %v (TTY: %v)", redact.Args(req.Cmd), req.TTY)
6266

6367
// Create the command
6468
cmd := exec.Command(req.Cmd[0], req.Cmd[1:]...)
@@ -154,7 +158,7 @@ func runWithPTY(conn net.Conn, cmd *exec.Cmd, rows, cols uint16) {
154158
// Start command with PTY
155159
ptmx, err := pty.Start(cmd)
156160
if err != nil {
157-
log.Printf("Failed to start command with PTY: %v", err)
161+
log.Printf("Failed to start command with PTY: %s", redact.String(err.Error()))
158162
if err := writeExitCode(conn, 1); err != nil {
159163
log.Printf("Warning: failed to write exit code: %v", err)
160164
}
@@ -292,7 +296,7 @@ func runWithoutPTY(conn net.Conn, cmd *exec.Cmd) {
292296

293297
// Start command
294298
if err := cmd.Start(); err != nil {
295-
log.Printf("Failed to start command: %v", err)
299+
log.Printf("Failed to start command: %s", redact.String(err.Error()))
296300
if err := writeExitCode(conn, 1); err != nil {
297301
log.Printf("Warning: failed to write exit code: %v", err)
298302
}

‎cmd/shed-agent/exec_redact_test.go‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
//go:build linux
2+
3+
package main
4+
5+
import (
6+
"bytes"
7+
"log"
8+
"net"
9+
"os"
10+
"strings"
11+
"testing"
12+
)
13+
14+
// TestHandleExecLogsRedactedArgvAndNoEnv: the command runs exactly as sent
15+
// (credentials and env reach it), while the agent's log line has URL
16+
// credentials redacted and never names env.
17+
func TestHandleExecLogsRedactedArgvAndNoEnv(t *testing.T) {
18+
var logged bytes.Buffer
19+
log.SetOutput(&logged)
20+
t.Cleanup(func() { log.SetOutput(os.Stderr) })
21+
22+
c, s := net.Pipe()
23+
defer c.Close()
24+
done := make(chan struct{})
25+
go func() {
26+
defer close(done)
27+
handleExecConnection(s, nil)
28+
}()
29+
30+
req := ExecRequest{
31+
Cmd: []string{"sh", "-c", `printf '%s %s' "$1" "$SHED_TEST_TOKEN"`, "sh", "https://u:sentinel-pass@h/r.git"},
32+
Env: []string{"SHED_TEST_TOKEN=sentinel-env"},
33+
}
34+
send(t, c, MsgTypeExecRequest, mustJSON(t, req))
35+
send(t, c, MsgTypeStdinEOF, nil)
36+
out, _ := readOutputFrames(t, c)
37+
_ = c.Close()
38+
<-done
39+
40+
if got, want := string(out), "https://u:sentinel-pass@h/r.git sentinel-env"; got != want {
41+
t.Errorf("command saw %q, want %q", got, want)
42+
}
43+
line := logged.String()
44+
if strings.Contains(line, "sentinel") || strings.Contains(line, "SHED_TEST_TOKEN=") {
45+
t.Errorf("agent log carries a credential or env: %q", line)
46+
}
47+
if !strings.Contains(line, "https://REDACTED@h/r.git") {
48+
t.Errorf("agent log = %q, want the argv with the URL redacted", line)
49+
}
50+
}

‎cmd/shed-server/remote.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ func newRemoteBackend(cfg *config.ServerConfig, bridge *plugin.Bridge) (*remote.
4747
return nil, fmt.Errorf("remote backend: %w", err)
4848
}
4949
stateRoot := serverStateDir(cfg)
50+
firewallSources := rt.FirewallSources
5051

5152
var prov provider.Provider
5253
switch rcfg.Provider {
@@ -69,7 +70,13 @@ func newRemoteBackend(cfg *config.ServerConfig, bridge *plugin.Bridge) (*remote.
6970
}
7071
prov = client
7172
case config.RemoteProviderFake:
72-
prov = fake.New(fake.Options{Env: rcfg.Environment})
73+
// The fake's firewall rule carries whatever static mode expects, so
74+
// the check passes; with no ranges given it is the fake's own
75+
// documentation address.
76+
if rcfg.Firewall.Mode == config.RemoteFirewallStatic && len(firewallSources) == 0 {
77+
firewallSources = []string{fake.DefaultFirewallSource}
78+
}
79+
prov = fake.New(fake.Options{Env: rcfg.Environment, FirewallSources: firewallSources})
7380
default:
7481
return nil, fmt.Errorf("remote backend: unknown provider %q", rcfg.Provider)
7582
}
@@ -106,6 +113,7 @@ func newRemoteBackend(cfg *config.ServerConfig, bridge *plugin.Bridge) (*remote.
106113
Links: links,
107114
StateDir: rcfg.StateDirPath(stateRoot),
108115
AuthorizedKeys: authorizedKeys,
116+
FirewallSources: firewallSources,
109117
Bridge: bridge,
110118
KeepFailedCreate: os.Getenv(keepFailedCreateEnv) == "1",
111119
})

‎cmd/shed/console_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"github.com/anmitsu/go-shlex"
1010

1111
"github.com/charliek/shed/internal/config"
12+
"github.com/charliek/shed/internal/redact"
1213
)
1314

1415
// TestShellQuoteRoundTrip verifies that every element of argv survives a
@@ -382,3 +383,35 @@ func TestSSHSessionArgs(t *testing.T) {
382383
}
383384
})
384385
}
386+
387+
// TestQuotedArgvRedacts: the command string the CLI sends is what the server
388+
// and the agent log, after redact has been over it. A URL password must stay
389+
// hidden whatever the quoting did to it, an apostrophe included (which is
390+
// sent as '\” and so puts a backslash in the middle of the userinfo).
391+
func TestQuotedArgvRedacts(t *testing.T) {
392+
passwords := []struct {
393+
name string
394+
password string
395+
}{
396+
{"plain", "s3cr3t-token"},
397+
{"an apostrophe", "s3cr3t'token"},
398+
{"two apostrophes", "'s3cr3t'token"},
399+
{"an apostrophe at the end", "s3cr3t-token'"},
400+
}
401+
for _, tt := range passwords {
402+
t.Run(tt.name, func(t *testing.T) {
403+
quoted, err := validateAndQuoteArgs([]string{"git", "clone", "https://user:" + tt.password + "@git.example/o/r.git"})
404+
if err != nil {
405+
t.Fatal(err)
406+
}
407+
wire := strings.Join(quoted, " ")
408+
got := redact.String(wire)
409+
if strings.Contains(got, "s3cr3t") || strings.Contains(got, "token") {
410+
t.Errorf("the redacted command still carries the password: %s", got)
411+
}
412+
if want := "'git' 'clone' 'https://REDACTED@git.example/o/r.git'"; got != want {
413+
t.Errorf("redacted = %s, want %s", got, want)
414+
}
415+
})
416+
}
417+
}

‎configs/server.dev-remote.yaml.tmpl‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
# Shed Server — Parallel Dev Configuration (remote backend)
22
#
33
# A TEMPLATE, not a config: `make dev-server-up-remote` renders it to
4-
# ~/.shed/dev-remote/server.yaml, replacing the three @...@ placeholders, and
4+
# ~/.shed/dev-remote/server.yaml, replacing the @...@ placeholders, and
55
# runs the dev build with it. Nothing private lives here. The cloud project,
6-
# zone, network, subnet, credentials and SSH client key are read from the
7-
# process environment at start-up through the `*_env` names in the `remote:`
8-
# block; the Make target sources them from a private env file
6+
# zone, network, subnet, credentials, SSH client key and the firewall rule's
7+
# source ranges are read from the process environment at start-up through the
8+
# `*_env` names in the `remote:` block; the Make target sources them from a
9+
# private env file
910
# (default ~/.config/shed/remote-dev/env, override with SHED_REMOTE_ENV_FILE).
1011
#
1112
# The server runs ALONGSIDE an installed shed-server on the same machine:
@@ -78,8 +79,31 @@ remote:
7879
ssh:
7980
client_key_env: SHED_REMOTE_SSH_KEY
8081
client_key_file_env: SHED_REMOTE_SSH_KEY_FILE
82+
# The server reads the environment's cloud firewall rule (shed-<env>-ssh)
83+
# and refuses creates and starts (503 FIREWALL_CHECK_FAILED) while the rule
84+
# or its network is not as scripts/remote/gce-setup.sh made it.
8185
firewall:
82-
mode: static
86+
# `make dev-server-up-remote` renders the two lines after this comment
87+
# from the private env file, so neither an address nor a service you
88+
# trust is named here.
89+
#
90+
# static (the default): the rule's source ranges must equal the
91+
# comma-separated list in SHED_REMOTE_SOURCE_RANGES (your public address
92+
# as a /32). The server only reads the rule. Rendered as
93+
# mode: static
94+
# source_ranges_env: SHED_REMOTE_SOURCE_RANGES
95+
#
96+
# auto (SHED_REMOTE_FIREWALL_MODE=auto), for a machine whose public
97+
# address changes: the server asks the echo services for its address
98+
# and, when two agree, patches the rule's sources to that one /32. It
99+
# needs the extra roles that `gce-setup.sh --print --firewall-auto`
100+
# prints, and SHED_REMOTE_ECHO_URLS: two or more https services you
101+
# trust, on different hosts, comma-separated, that answer with the
102+
# caller's IPv4 address as plain text. Rendered as
103+
# mode: auto
104+
# echo_urls: [https://<first-echo-service>/, https://<second-echo-service>/]
105+
mode: @FIREWALL_MODE@
106+
@FIREWALL_SETTING@
83107
gce:
84108
project_env: SHED_GCE_PROJECT
85109
zone_env: SHED_GCE_ZONE

0 commit comments

Comments
 (0)