5e0625410a
Background commit review flagged command/option injection: operator-provided host/user/ repo_path flow into restic's ssh -s sftp command. Reject leading '-' (ssh option injection, e.g. -oProxyCommand) + metacharacters/traversal; OffboxConfigured fails closed on an invalid target. Companion test covers the injection cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxLA1mZurFq9kt8hneFeCs
341 lines
12 KiB
Go
341 lines
12 KiB
Go
package backup
|
|
|
|
import (
|
|
"context"
|
|
"log"
|
|
"os"
|
|
"path/filepath"
|
|
"strings"
|
|
"sync"
|
|
"testing"
|
|
"time"
|
|
|
|
"gitea.dooplex.hu/admin/felhom-controller/internal/config"
|
|
"gitea.dooplex.hu/admin/felhom-controller/internal/settings"
|
|
)
|
|
|
|
// newOffboxManager builds a Manager with a temp data dir + a configured + enabled off-box target and the
|
|
// 0600 secret files written, so OffboxConfigured() is true.
|
|
func newOffboxManager(t *testing.T) (*Manager, *settings.Settings) {
|
|
t.Helper()
|
|
logger := log.New(os.Stderr, "", 0)
|
|
dataDir := t.TempDir()
|
|
sett, err := settings.Load(filepath.Join(dataDir, "settings.json"), logger)
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
cfg := &config.Config{}
|
|
cfg.Paths.DataDir = dataDir
|
|
cfg.Paths.SystemDataPath = filepath.Join(dataDir, "sys")
|
|
m := NewManager(cfg, sett, logger)
|
|
if err := sett.SetOffboxTarget(&settings.OffboxTarget{
|
|
Enabled: true, Host: "nas.local", Port: 22, User: "felhom", RepoPath: "/srv/repo", Schedule: "daily",
|
|
}); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := m.WriteOffboxSecrets("PRIVATE-KEY-MATERIAL", "nas.local ssh-ed25519 AAAAhostkey"); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
return m, sett
|
|
}
|
|
|
|
// argsContainTimeout reports whether the restic arg vector carries the load-bearing ConnectTimeout.
|
|
func argsContainTimeout(args []string) bool {
|
|
return strings.Contains(strings.Join(args, " "), "-oConnectTimeout=")
|
|
}
|
|
|
|
// TestOffbox_BaseArgsCarryConnectTimeout asserts the mandatory fail-fast + hardening args are present.
|
|
func TestOffbox_BaseArgsCarryConnectTimeout(t *testing.T) {
|
|
m, sett := newOffboxManager(t)
|
|
base, env := m.offboxBaseArgs(sett.GetOffboxTarget())
|
|
joined := strings.Join(base, " ")
|
|
for _, want := range []string{
|
|
"-oConnectTimeout=10", // THE spike Q8 fail-fast knob
|
|
"-oStrictHostKeyChecking=yes", // no blind TOFU
|
|
"-oUserKnownHostsFile=", // pinned host key
|
|
"-oBatchMode=yes", // no interactive hang
|
|
"sftp:felhom@nas.local:/srv/repo",
|
|
} {
|
|
if !strings.Contains(joined, want) {
|
|
t.Errorf("base args missing %q: %s", want, joined)
|
|
}
|
|
}
|
|
if len(env) != 1 || !strings.HasPrefix(env[0], "RESTIC_PASSWORD_FILE=") {
|
|
t.Errorf("env must set RESTIC_PASSWORD_FILE only, got %v", env)
|
|
}
|
|
}
|
|
|
|
// failFastFake models the SSH transport's ConnectTimeout honoring: if the restic args carry
|
|
// -oConnectTimeout it returns a connect error promptly (fail-fast); WITHOUT it, it blocks until the ctx
|
|
// deadline (the dead-NAS multi-minute hang). This is the seam the ConnectTimeout companion exercises.
|
|
func failFastFake(_ *testing.T) offboxRunner {
|
|
return func(ctx context.Context, _ []string, args ...string) ([]byte, error) {
|
|
if argsContainTimeout(args) {
|
|
return []byte("dial tcp: connect: connection refused"), context.DeadlineExceeded // fast, bounded
|
|
}
|
|
<-ctx.Done() // no timeout arg → hang until the caller's deadline (the bug)
|
|
return nil, ctx.Err()
|
|
}
|
|
}
|
|
|
|
// TestOffbox_ConnectTimeoutIsLoadBearing is the §10 companion red-proof: WITH the arg the (fake) connect
|
|
// fails fast (well under the bound); WITHOUT it the connect blocks past the bound. A build that dropped
|
|
// the ConnectTimeout arg would take the slow path → this proves the arg is load-bearing.
|
|
func TestOffbox_ConnectTimeoutIsLoadBearing(t *testing.T) {
|
|
m, sett := newOffboxManager(t)
|
|
realArgs, env := m.offboxBaseArgs(sett.GetOffboxTarget())
|
|
fake := failFastFake(t)
|
|
|
|
// WITH the arg: returns promptly (we model fast as an immediate error, not a ctx hang).
|
|
ctx1, c1 := context.WithTimeout(context.Background(), 2*time.Second)
|
|
defer c1()
|
|
start := time.Now()
|
|
_, err := fake(ctx1, env, append(append([]string{}, realArgs...), "cat", "config")...)
|
|
if elapsed := time.Since(start); elapsed > time.Second {
|
|
t.Fatalf("with ConnectTimeout the connect must fail fast, took %s", elapsed)
|
|
}
|
|
_ = err
|
|
|
|
// WITHOUT the arg (the bug): blocks until the ctx deadline.
|
|
stripped := stripConnectTimeout(realArgs)
|
|
if argsContainTimeout(stripped) {
|
|
t.Fatal("test setup: stripped args still contain the timeout")
|
|
}
|
|
ctx2, c2 := context.WithTimeout(context.Background(), 300*time.Millisecond)
|
|
defer c2()
|
|
start = time.Now()
|
|
_, err = fake(ctx2, env, append(append([]string{}, stripped...), "cat", "config")...)
|
|
if err != context.DeadlineExceeded {
|
|
t.Fatalf("without ConnectTimeout the connect should block to the deadline, got %v", err)
|
|
}
|
|
if elapsed := time.Since(start); elapsed < 250*time.Millisecond {
|
|
t.Fatalf("without ConnectTimeout it should have hung to the bound, only took %s", elapsed)
|
|
}
|
|
}
|
|
|
|
func stripConnectTimeout(args []string) []string {
|
|
out := make([]string, len(args))
|
|
for i, a := range args {
|
|
out[i] = strings.ReplaceAll(a, "-oConnectTimeout=10 ", "")
|
|
}
|
|
return out
|
|
}
|
|
|
|
// TestOffbox_RunFailsFastAndAlerts: a dead-NAS run returns an error promptly, records status=error, and
|
|
// fires the operator alert.
|
|
func TestOffbox_RunFailsFastAndAlerts(t *testing.T) {
|
|
m, sett := newOffboxManager(t)
|
|
m.SetOffboxRunner(func(ctx context.Context, _ []string, args ...string) ([]byte, error) {
|
|
// dead NAS: every op (incl. the repo probe) errors fast.
|
|
return []byte("unable to open repository: connection refused"), context.DeadlineExceeded
|
|
})
|
|
var mu sync.Mutex
|
|
var gotErr error
|
|
var notified bool
|
|
m.SetOffboxNotify(func(_ time.Duration, _ int, err error) { mu.Lock(); defer mu.Unlock(); notified = true; gotErr = err })
|
|
_ = sett.SetAppOffbox("rallly", true)
|
|
|
|
start := time.Now()
|
|
err := m.RunOffboxBackup(context.Background())
|
|
if err == nil {
|
|
t.Fatal("a dead NAS must produce a failed run")
|
|
}
|
|
if time.Since(start) > 5*time.Second {
|
|
t.Fatalf("run should fail fast, took %s", time.Since(start))
|
|
}
|
|
if !notified || gotErr == nil {
|
|
t.Fatal("a failed off-box run must alert the operator")
|
|
}
|
|
if st := sett.GetOffboxTarget(); st.LastStatus != "error" || st.LastError == "" {
|
|
t.Fatalf("status must record the failure, got %+v", st)
|
|
}
|
|
}
|
|
|
|
// TestOffbox_RepoIdempotent: when the repo exists (cat config succeeds), ensure must NOT init.
|
|
func TestOffbox_RepoIdempotent(t *testing.T) {
|
|
m, sett := newOffboxManager(t)
|
|
var inits int
|
|
m.SetOffboxRunner(func(_ context.Context, _ []string, args ...string) ([]byte, error) {
|
|
switch {
|
|
case contains(args, "cat") && contains(args, "config"):
|
|
return []byte(`{"version":2}`), nil // repo exists
|
|
case contains(args, "init"):
|
|
inits++
|
|
return nil, nil
|
|
}
|
|
return nil, nil
|
|
})
|
|
base, env := m.offboxBaseArgs(sett.GetOffboxTarget())
|
|
if err := m.ensureOffboxRepo(context.Background(), base, env); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if inits != 0 {
|
|
t.Fatalf("an existing repo must NOT be re-initialized, init called %d times", inits)
|
|
}
|
|
}
|
|
|
|
// TestOffbox_RestoreRoundTrip: backup a temp tree (fake records src per tag), restore (fake copies the
|
|
// recorded tree to target) → byte-identical. Exercises the orchestration without real restic.
|
|
func TestOffbox_RestoreRoundTrip(t *testing.T) {
|
|
m, sett := newOffboxManager(t)
|
|
// Lay down an app's recovery-unit tree on disk (what RunOffboxBackup will back up).
|
|
nsRoot := m.AppNamespaceRoot("rallly")
|
|
src := RecoveryUnitPath(nsRoot, "rallly")
|
|
if err := os.MkdirAll(filepath.Join(src, "db-dumps"), 0o755); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
want := []byte("CREATE TABLE x; -- dump bytes")
|
|
if err := os.WriteFile(filepath.Join(src, "db-dumps", "rallly.sql"), want, 0o644); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
_ = sett.SetAppOffbox("rallly", true)
|
|
|
|
captured := map[string]string{} // tag → src path
|
|
m.SetOffboxRunner(func(_ context.Context, _ []string, args ...string) ([]byte, error) {
|
|
switch {
|
|
case contains(args, "cat") && contains(args, "config"):
|
|
return []byte(`{}`), nil
|
|
case contains(args, "backup"):
|
|
captured[tagOf(args)] = args[len(args)-1] // last arg = src path
|
|
return nil, nil
|
|
case contains(args, "forget"):
|
|
return nil, nil
|
|
case contains(args, "restore"):
|
|
target := valAfter(args, "--target")
|
|
if err := copyTree(captured[tagOf(args)], target); err != nil {
|
|
return nil, err
|
|
}
|
|
return nil, nil
|
|
case contains(args, "snapshots"):
|
|
return []byte(`[{"id":"abc"}]`), nil
|
|
case contains(args, "stats"):
|
|
return []byte(`{"total_size":123}`), nil
|
|
}
|
|
return nil, nil
|
|
})
|
|
|
|
if err := m.RunOffboxBackup(context.Background()); err != nil {
|
|
t.Fatalf("backup: %v", err)
|
|
}
|
|
dest := t.TempDir()
|
|
if err := m.RestoreOffbox(context.Background(), "rallly", dest); err != nil {
|
|
t.Fatalf("restore: %v", err)
|
|
}
|
|
got, err := os.ReadFile(filepath.Join(dest, "db-dumps", "rallly.sql"))
|
|
if err != nil {
|
|
t.Fatalf("restored file missing: %v", err)
|
|
}
|
|
if string(got) != string(want) {
|
|
t.Fatalf("restore not byte-identical: got %q want %q", got, want)
|
|
}
|
|
}
|
|
|
|
// TestOffbox_SingleFlight: an off-box run while another backup holds m.running skips (no runner call).
|
|
func TestOffbox_SingleFlight(t *testing.T) {
|
|
m, _ := newOffboxManager(t)
|
|
called := false
|
|
m.SetOffboxRunner(func(context.Context, []string, ...string) ([]byte, error) { called = true; return nil, nil })
|
|
_ = m.acquireRunning() // simulate a concurrent backup holding the flag
|
|
defer m.releaseRunning()
|
|
if err := m.RunOffboxBackup(context.Background()); err != nil {
|
|
t.Fatalf("single-flight skip should not error, got %v", err)
|
|
}
|
|
if called {
|
|
t.Fatal("off-box must not run (race) while another backup holds the flag")
|
|
}
|
|
}
|
|
|
|
// TestOffbox_SecretsAre0600: the SSH key + repo password files are 0600; the password is non-empty.
|
|
func TestOffbox_SecretsAre0600(t *testing.T) {
|
|
m, _ := newOffboxManager(t)
|
|
for _, p := range []string{m.offboxKeyPath(), m.offboxPwPath()} {
|
|
info, err := os.Stat(p)
|
|
if err != nil {
|
|
t.Fatalf("secret file missing: %v", err)
|
|
}
|
|
if runtimeIsUnix() && info.Mode().Perm()&0o077 != 0 {
|
|
t.Errorf("%s is group/other-readable (mode %v) — must be 0600", p, info.Mode().Perm())
|
|
}
|
|
}
|
|
pw, _ := os.ReadFile(m.offboxPwPath())
|
|
if len(strings.TrimSpace(string(pw))) < 32 {
|
|
t.Errorf("repo password too short / empty")
|
|
}
|
|
}
|
|
|
|
// --- tiny test helpers ---
|
|
|
|
// TestOffbox_ValidateRejectsInjection is the security companion: host/user/repo values that could inject
|
|
// an ssh option (leading '-' → e.g. -oProxyCommand) or a shell metacharacter must be REFUSED; a clean
|
|
// target is accepted. A build without this guard would let a hostile target reach the ssh exec → FAIL.
|
|
func TestOffbox_ValidateRejectsInjection(t *testing.T) {
|
|
ok := &settings.OffboxTarget{Host: "nas.local", Port: 22, User: "felhom", RepoPath: "/srv/repo"}
|
|
if err := ValidateOffboxTarget(ok); err != nil {
|
|
t.Fatalf("clean target rejected: %v", err)
|
|
}
|
|
bad := []settings.OffboxTarget{
|
|
{Host: "-oProxyCommand=touch /tmp/pwn", User: "felhom", RepoPath: "/srv/repo"}, // ssh option injection
|
|
{Host: "nas;rm -rf /", User: "felhom", RepoPath: "/srv/repo"}, // metacharacters
|
|
{Host: "nas.local", User: "-oProxyCommand=x", RepoPath: "/srv/repo"}, // user option injection
|
|
{Host: "nas.local", User: "felhom", RepoPath: "/srv/repo; evil"}, // path metacharacters
|
|
{Host: "nas.local", User: "felhom", RepoPath: "/srv/../etc"}, // traversal
|
|
{Host: "nas local", User: "felhom", RepoPath: "/srv/repo"}, // space
|
|
{Host: "nas.local", User: "felhom", RepoPath: "relative/path"}, // non-absolute
|
|
}
|
|
for i, b := range bad {
|
|
bb := b
|
|
if err := ValidateOffboxTarget(&bb); err == nil {
|
|
t.Errorf("case %d (%+v) must be rejected", i, bb)
|
|
}
|
|
}
|
|
}
|
|
|
|
func runtimeIsUnix() bool { return os.PathSeparator == '/' }
|
|
|
|
func contains(ss []string, want string) bool {
|
|
for _, s := range ss {
|
|
if s == want {
|
|
return true
|
|
}
|
|
}
|
|
return false
|
|
}
|
|
|
|
// tagOf returns the LAST --tag value (the per-stack tag; backup adds "felhom-offbox" then the stack).
|
|
func tagOf(args []string) string {
|
|
tag := ""
|
|
for i, a := range args {
|
|
if a == "--tag" && i+1 < len(args) {
|
|
tag = args[i+1]
|
|
}
|
|
}
|
|
return tag
|
|
}
|
|
|
|
func valAfter(args []string, flag string) string {
|
|
for i, a := range args {
|
|
if a == flag && i+1 < len(args) {
|
|
return args[i+1]
|
|
}
|
|
}
|
|
return ""
|
|
}
|
|
|
|
func copyTree(src, dst string) error {
|
|
return filepath.Walk(src, func(p string, info os.FileInfo, err error) error {
|
|
if err != nil {
|
|
return err
|
|
}
|
|
rel, _ := filepath.Rel(src, p)
|
|
target := filepath.Join(dst, rel)
|
|
if info.IsDir() {
|
|
return os.MkdirAll(target, 0o755)
|
|
}
|
|
b, rerr := os.ReadFile(p)
|
|
if rerr != nil {
|
|
return rerr
|
|
}
|
|
return os.WriteFile(target, b, 0o644)
|
|
})
|
|
}
|