fix(P02): input validation + injection hardening — 11 vectors (REQ-150)

Critical fixes:
- logs --job: validate ^[A-Za-z0-9_-]+$ + shellQuote (was %q backtick RCE)
- pprof: isLoopback treats empty host as bind-all (was :6060 bypass)
- backup restore: filepath.Rel containment check (was tar-slip via a/../..)
- WebAuthn reg auth deferred to P04 (requires session infra)

High fixes:
- txn rollback/show/apply: validate ^T-[0-9a-f]{16}$ + shellQuote
- nft diff --against: validate txn ID before filepath.Join
- drain stopAlloc: validate allocID ^[A-Za-z0-9_-]+$
- cluster_compat: shellQuote peer dir name
- podman image: shellQuote (was %q backtick injection)
- nft TrustedProbes: net.ParseIP/CIDR validation + split v4/v6 sets
- sudoers: validate --proxmox-user/--proxmox-role ^[a-zA-Z_][a-zA-Z0-9_-]{0,31}$
  fixed path /etc/sudoers.d/orca; shellQuote pveum/useradd; validateSudoers
  checks actual file
- nft country block: validate ^[A-Z]{2}$ (was len==2 only)

New file: internal/cli/validate.go (shared validators + shellQuote)
All 38 Go test packages pass. go vet + gofmt clean.

---ci---
project: orca
phase: 2
milestone: v0.13
status: complete
requirements:
  covered: [150]
---/ci---
This commit is contained in:
Jon Chery
2026-08-07 19:28:01 +00:00
parent b0158c96e9
commit fa7cf5e928
16 changed files with 459 additions and 60 deletions
+9 -3
View File
@@ -299,10 +299,16 @@ func Restore(opts RestoreOptions) error {
return fmt.Errorf("restore: read tar entry: %w", err)
}
name := filepath.FromSlash(hdr.Name)
if strings.HasPrefix(name, "/") || strings.HasPrefix(name, "..") {
return fmt.Errorf("restore: unsafe path %q", hdr.Name)
}
// F3: tar-slip containment check. The prior prefix check
// (HasPrefix "/" || "..") missed patterns like "a/../../etc".
// Resolve the destination and verify it stays within target
// via filepath.Rel; reject if the relative path escapes (starts
// with ".." or is absolute).
dest := filepath.Join(target, name)
rel, err := filepath.Rel(target, dest)
if err != nil || strings.HasPrefix(rel, "..") || filepath.IsAbs(rel) {
return fmt.Errorf("restore: unsafe path %q escapes target (F3: tar-slip)", hdr.Name)
}
switch hdr.Typeflag {
case tar.TypeDir:
if err := os.MkdirAll(dest, os.FileMode(hdr.Mode)); err != nil {
+65
View File
@@ -391,6 +391,71 @@ func TestRestoreRejectsTraversalSymlink(t *testing.T) {
}
}
// createCraftedTarballWithFile creates a tar.gz containing a single
// regular file entry with the given (possibly malicious) name. Used to
// test the tar-slip path-traversal guard (F3).
func createCraftedTarballWithFile(path, name, body string) error {
f, err := os.Create(path)
if err != nil {
return err
}
defer f.Close()
gz := gzip.NewWriter(f)
defer gz.Close()
tw := tar.NewWriter(gz)
defer tw.Close()
hdr := &tar.Header{
Name: name,
Typeflag: tar.TypeReg,
Mode: 0o644,
Size: int64(len(body)),
}
if err := tw.WriteHeader(hdr); err != nil {
return err
}
if _, err := tw.Write([]byte(body)); err != nil {
return err
}
return nil
}
// TestRestoreRejectsTarSlipRegularFile verifies a tarball with a regular
// file entry whose name contains an embedded ".." traversal (e.g.
// "a/../../etc/passwd") is rejected. The old prefix-only check missed
// this pattern; the F3 filepath.Rel containment check catches it.
func TestRestoreRejectsTarSlipRegularFile(t *testing.T) {
dir := t.TempDir()
tarPath := filepath.Join(dir, "slip.tar.gz")
sigPath := tarPath + ".sig"
if err := createCraftedTarballWithFile(tarPath, "a/../../etc/passwd", "pwned"); err != nil {
t.Fatalf("create tarball: %v", err)
}
key := make([]byte, 32)
for i := range key {
key[i] = byte(i + 9)
}
mac := hmac.New(sha256.New, key)
data, _ := os.ReadFile(tarPath)
mac.Write(data)
if err := os.WriteFile(sigPath, []byte(hex.EncodeToString(mac.Sum(nil))), 0o600); err != nil {
t.Fatalf("write sig: %v", err)
}
target := filepath.Join(dir, "restore")
os.MkdirAll(target, 0o755)
err := Restore(RestoreOptions{
InputPath: tarPath,
TargetDir: target,
MasterKey: key,
Force: true,
})
if err == nil {
t.Fatal("Restore should reject tar-slip regular file (F3)")
}
if !strings.Contains(err.Error(), "unsafe path") {
t.Errorf("error should mention unsafe path: %v", err)
}
}
// createCraftedTarball creates a tar.gz containing a single symlink
// entry with the given linkname. Used to test symlink validation.
func createCraftedTarball(path, name, linkname string) error {
+20 -18
View File
@@ -36,9 +36,9 @@ if any peer fails.`,
}
type noOrcaPeerResult struct {
Node string `json:"node"`
Peer string `json:"peer"`
Pass bool `json:"pass"`
Node string `json:"node"`
Peer string `json:"peer"`
Pass bool `json:"pass"`
Violations []string `json:"violations,omitempty"`
}
@@ -192,12 +192,12 @@ Reports: which peers are on which version, any compatibility issues.`,
}
type compatPeerResult struct {
Node string `json:"node"`
Peer string `json:"peer"`
Version string `json:"version"`
LeadVersion string `json:"lead_version,omitempty"`
Compatible bool `json:"compatible"`
Issue string `json:"issue,omitempty"`
Node string `json:"node"`
Peer string `json:"peer"`
Version string `json:"version"`
LeadVersion string `json:"lead_version,omitempty"`
Compatible bool `json:"compatible"`
Issue string `json:"issue,omitempty"`
}
func runCompatCheck(cmd *cobra.Command) error {
@@ -261,13 +261,13 @@ func runCompatCheck(cmd *cobra.Command) error {
}
summary := map[string]any{
"lead_version": leadVersion,
"schema_version": emit.SchemaVersion,
"results": results,
"versions_seen": versionSet,
"issues": issues,
"schema_ok": schemaOK,
"manifest_ok": manifestOK,
"lead_version": leadVersion,
"schema_version": emit.SchemaVersion,
"results": results,
"versions_seen": versionSet,
"issues": issues,
"schema_ok": schemaOK,
"manifest_ok": manifestOK,
}
if jsonOutput {
@@ -396,7 +396,10 @@ func verifyTxnManifestCompat(ctx context.Context, ex drainExecer, nodes []*model
if first == "" {
continue
}
man, err := ex.Exec(ctx, peer, fmt.Sprintf("cat /etc/orca/cluster/txns/%s/manifest.json 2>/dev/null || true", first))
// F7: first is a directory name parsed from remote `ls` output
// and is therefore attacker-controlled (stored injection from a
// malicious peer). Shell-quote it before interpolation.
man, err := ex.Exec(ctx, peer, fmt.Sprintf("cat /etc/orca/cluster/txns/%s/manifest.json 2>/dev/null || true", sshQuote(first)))
if err != nil {
continue
}
@@ -414,4 +417,3 @@ func verifyTxnManifestCompat(ctx context.Context, ex drainExecer, nodes []*model
func sshQuote(s string) string {
return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'"
}
+12 -4
View File
@@ -74,8 +74,8 @@ func splitHostPort(addr string) (string, string, bool) {
}
var (
drainTimeout time.Duration
migrateTarget string
drainTimeout time.Duration
migrateTarget string
)
// allocUnit is the systemd unit name pattern for orca allocations.
@@ -128,7 +128,15 @@ func listRunningAllocs(ctx context.Context, ex drainExecer, peer string) ([]stri
// stopAlloc sends `systemctl stop orca-alloc-<id>.service` to a node.
// A unit that is already stopped (or never existed) is treated as
// success: drain is idempotent.
//
// F6: allocID is parsed from remote `systemctl list-units` output and is
// therefore attacker-controlled (a malicious peer could emit a crafted
// unit name). Validate against ^[A-Za-z0-9_-]+$ before interpolation into
// the shell command to prevent stored command injection.
func stopAlloc(ctx context.Context, ex drainExecer, peer, allocID string) error {
if !validSafeName(allocID) {
return fmt.Errorf("stopAlloc: invalid alloc id %q (allowed: A-Z a-z 0-9 _ -)", allocID)
}
cmd := fmt.Sprintf("systemctl stop %s", allocUnit(allocID))
_, err := ex.Exec(ctx, peer, cmd)
if err != nil {
@@ -554,8 +562,8 @@ is named <name>-migrated-<timestamp>.`,
}
result := map[string]any{
"job": jobName,
"target": target.Name,
"job": jobName,
"target": target.Name,
"already_on_target": len(onTarget) > 0,
}
+10 -1
View File
@@ -128,6 +128,13 @@ Ctrl-C cancels the fan-out via signal.NotifyContext.`,
if logsAllNodes && logsNode != "" {
return fmt.Errorf("--all-nodes and --node are mutually exclusive")
}
// F1: validate --job before interpolation into the journalctl
// unit pattern. Go's %q does not escape backticks and bash
// executes command substitution inside double quotes, so an
// unvalidated job name is a remote RCE vector.
if logsJob != "" && !validSafeName(logsJob) {
return fmt.Errorf("logs: --job %q contains disallowed characters (allowed: A-Z a-z 0-9 _ -)", logsJob)
}
since, err := parseSince(logsSince)
if err != nil {
return err
@@ -271,7 +278,9 @@ func streamNodeLines(ctx context.Context, ex logsExecer, n *model.Node, since ti
unitPattern = "orca-alloc-" + job + "-*"
}
sinceStr := since.Format("2006-01-02 15:04:05")
cmd := fmt.Sprintf("journalctl -u %q --since %q --output json --no-pager", unitPattern, sinceStr)
// F1: shellQuote (single-quote wrap) instead of %q — %q does not
// escape backticks, enabling command substitution in double quotes.
cmd := fmt.Sprintf("journalctl -u %s --since %s --output json --no-pager", shellQuote(unitPattern), shellQuote(sinceStr))
raw, err := ex.Exec(ctx, peer, cmd)
if err != nil {
slog.Default().Warn("logs: exec failed", "node", n.Name, "peer", peer, "error", err)
+14 -6
View File
@@ -22,9 +22,9 @@ import (
)
var (
nftShowPeer string
nftDiffAgainst string
nftRateLimitRate int
nftShowPeer string
nftDiffAgainst string
nftRateLimitRate int
nftCountryBlockCC string
)
@@ -70,6 +70,11 @@ recorded at apply time). Reports per-rule diffs.`,
if nftDiffAgainst == "" {
return errors.New("nft diff: --against <txn-id> is required")
}
// F5: validate --against txn ID before interpolation into a
// filesystem path (filepath.Join(paths.TxnDir(), txnID, ...)).
if !validTxnID(nftDiffAgainst) {
return fmt.Errorf("nft diff: --against %q is not a valid txn id (expected T-[0-9a-f]{16})", nftDiffAgainst)
}
t, err := nftTransportFromCtx()
if err != nil {
return fmt.Errorf("nft transport: %w", err)
@@ -132,8 +137,12 @@ orca nft country block add RU,CN`,
}
codes := strings.Split(ccList, ",")
for _, c := range codes {
if len(c) != 2 {
return fmt.Errorf("nft country block add: %q is not a 2-letter country code", c)
// F11: validate against ^[A-Z]{2}$ (two uppercase ASCII letters),
// not just len==2. The old check accepted arbitrary 2-byte
// strings (e.g. "RU" but also "; " or "$(") which could inject
// nft syntax or shell metacharacters.
if !validCountryCode(c) {
return fmt.Errorf("nft country block add: %q is not a valid ISO-3166 alpha-2 country code (expected two uppercase letters)", c)
}
}
t, err := nftTransportFromCtx()
@@ -263,4 +272,3 @@ func quoteAll(in []string) []string {
}
return out
}
+1 -1
View File
@@ -96,7 +96,7 @@ func TestNftDiffCmd_NoDrift(t *testing.T) {
var buf bytes.Buffer
rootCmd.SetOut(&buf)
rootCmd.SetErr(&buf)
rootCmd.SetArgs([]string{"nft", "diff", "--against", "txn-123"})
rootCmd.SetArgs([]string{"nft", "diff", "--against", "T-abcdef0123456789"})
if err := rootCmd.Execute(); err != nil {
t.Fatalf("nft diff: %v", err)
}
+15 -1
View File
@@ -84,6 +84,11 @@ Cluster-wide txns (no --namespace) require --force +
scoped txns (--namespace <ns>) only touch that namespace.`,
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
// F4: validate txn ID before interpolation into a remote shell
// command and filesystem path.
if !validTxnID(args[0]) {
return fmt.Errorf("txn apply: invalid txn id %q (expected T-[0-9a-f]{16})", args[0])
}
id := txn.TxnID(args[0])
transport, err := txnTransportFromCtx()
if err != nil {
@@ -175,6 +180,10 @@ var txnShowCmd = &cobra.Command{
Short: "Show txn details (desired state, manifest, status)",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
// F4: validate txn ID before interpolation into a filesystem path.
if !validTxnID(args[0]) {
return fmt.Errorf("txn show: invalid txn id %q (expected T-[0-9a-f]{16})", args[0])
}
id := args[0]
dir := filepath.Join(paths.TxnDir(), id)
manifestPath := filepath.Join(dir, "manifest.json")
@@ -230,6 +239,11 @@ the manual rollback path; orca-pull.sh runs rollback automatically on
verify failure.`,
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
// F4: validate txn ID before interpolation into a remote shell
// command (bash <dir>/rollback.sh) and filesystem path.
if !validTxnID(args[0]) {
return fmt.Errorf("txn rollback: invalid txn id %q (expected T-[0-9a-f]{16})", args[0])
}
id := txn.TxnID(args[0])
transport, err := txnTransportFromCtx()
if err != nil {
@@ -237,7 +251,7 @@ verify failure.`,
}
ctx := cmd.Context()
dir := "/run/orca/txns/" + string(id)
cmdStr := fmt.Sprintf("bash %s/rollback.sh", dir)
cmdStr := fmt.Sprintf("bash %s/rollback.sh", shellQuote(dir))
out, err := transport.Exec(ctx, txnRollbackLead, cmdStr)
if err != nil {
return fmt.Errorf("rollback %s on %s: %w (output: %s)", id, txnRollbackLead, err, string(out))
+59
View File
@@ -0,0 +1,59 @@
// Package cli: validate.go provides shared input-validation helpers for
// CLI command arguments that are interpolated into remote shell commands
// or filesystem paths (Phase 02 injection hardening, v0.13).
//
// These helpers enforce strict allowlists so that attacker-controlled
// values (job names, txn IDs, alloc IDs, country codes) cannot reach
// shell interpolation or path joins without matching a known-safe shape.
package cli
import (
"regexp"
"strings"
)
// safeNameRe matches the allowlist for shell-interpolated identifiers
// (job names, alloc IDs): ASCII letters, digits, underscore, hyphen.
// Used to prevent backtick/command-substitution and metacharacter
// injection into remote shell commands.
var safeNameRe = regexp.MustCompile(`^[A-Za-z0-9_-]+$`)
// txnIDRe matches the canonical orca transaction ID format: "T-" prefix
// followed by exactly 16 lowercase hex digits. Used to validate txn IDs
// before they are interpolated into filesystem paths or remote shell
// commands (`orca txn rollback`, `orca nft diff --against`).
var txnIDRe = regexp.MustCompile(`^T-[0-9a-f]{16}$`)
// countryCodeRe matches ISO-3166 alpha-2 country codes: exactly two
// uppercase ASCII letters. Used by `orca nft country block add` before
// codes are interpolated into the nft ruleset.
var countryCodeRe = regexp.MustCompile(`^[A-Z]{2}$`)
// validSafeName reports whether s is a safe shell-interpolation
// identifier (ASCII alphanumeric, underscore, hyphen only, non-empty).
func validSafeName(s string) bool {
return safeNameRe.MatchString(s)
}
// validTxnID reports whether s matches the canonical orca txn ID format
// (^T-[0-9a-f]{16}$).
func validTxnID(s string) bool {
return txnIDRe.MatchString(s)
}
// validCountryCode reports whether s is a valid ISO-3166 alpha-2 code
// (two uppercase letters).
func validCountryCode(s string) bool {
return countryCodeRe.MatchString(s)
}
// shellQuote single-quotes a string for safe shell interpolation over
// SSH exec. It escapes embedded single-quotes via the standard '\” idiom
// (POSIX shell). This is the cli-package copy of the helper duplicated
// across runtime/identity/stepca/sshpush to avoid import cycles; it
// hardens command interpolation against backtick/command-substitution
// injection (Go's %q does NOT escape backticks, and bash executes
// command substitution inside double quotes).
func shellQuote(s string) string {
return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'"
}
+18 -6
View File
@@ -13,13 +13,21 @@ import (
// isLoopback reports whether the address binds to a loopback interface
// (127.0.0.1, ::1, localhost). REQ-123: pprof must be loopback-only.
//
// An empty host (e.g. ":6060") binds ALL interfaces and is therefore
// treated as NON-loopback (F2: loopback-bypass fix). Only an explicit
// loopback IP or the "localhost" name is accepted.
func isLoopback(addr string) bool {
host, _, err := net.SplitHostPort(addr)
if err != nil {
host = addr
}
host = strings.TrimSpace(host)
if host == "" || host == "localhost" {
// F2: empty host (":6060") binds all interfaces — reject.
if host == "" {
return false
}
if host == "localhost" {
return true
}
ip := net.ParseIP(host)
@@ -29,18 +37,22 @@ func isLoopback(addr string) bool {
return false
}
// StartPprof starts the pprof HTTP server on addr. REQ-123: pprof is
// unauthenticated and MUST bind to a loopback interface only; this is a
// hard invariant (F2: the --pprof-allow-public override was a phantom flag
// that was never implemented and has been removed — non-loopback binds are
// always refused).
func StartPprof(addr string, log *slog.Logger) (*http.Server, error) {
if addr == "" {
return nil, nil
}
// REQ-123: pprof must bind to loopback only. Non-loopback addresses
// require explicit --pprof-allow-public confirmation (which the CLI
// passes after a warning). We refuse non-loopback here by default.
// REQ-123: pprof must bind to loopback only. This is a hard
// invariant; there is no public-bind override.
if !isLoopback(addr) {
log.Error("pprof refuses non-loopback bind",
slog.String("addr", addr),
slog.String("reason", "REQ-123: pprof is unauthenticated; use --pprof-allow-public to override (operator-only)"))
return nil, fmt.Errorf("pprof: refusing non-loopback bind %s (REQ-123; unauthenticated; use --pprof-allow-public)", addr)
slog.String("reason", "REQ-123: pprof is unauthenticated; loopback-only is a hard invariant"))
return nil, fmt.Errorf("pprof: refusing non-loopback bind %s (REQ-123; unauthenticated; loopback-only is a hard invariant)", addr)
}
mux := http.NewServeMux()
mux.HandleFunc("/debug/pprof/", pprof.Index)
+14
View File
@@ -7,6 +7,7 @@ import (
"net"
"net/http"
"path/filepath"
"strings"
"testing"
"time"
@@ -286,3 +287,16 @@ func TestStartPprof_LoopbackAccepted(t *testing.T) {
srv.Close()
}
}
// TestStartPprof_EmptyHostRefused verifies that an address with an empty
// host (e.g. ":6060"), which binds ALL interfaces, is refused as
// non-loopback (F2: loopback-bypass fix).
func TestStartPprof_EmptyHostRefused(t *testing.T) {
_, err := StartPprof(":6060", slog.Default())
if err == nil {
t.Error("StartPprof on \":6060\" should be refused (F2: empty host binds all interfaces)")
}
if err != nil && !strings.Contains(err.Error(), "non-loopback") {
t.Errorf("error should mention non-loopback, got: %v", err)
}
}
+75 -9
View File
@@ -13,7 +13,8 @@
// The ruleset defines:
//
// - table inet orca-ingress
// - set orca_trusted_probes (ipv4_addr interval, default 127.0.0.1)
// - set orca_trusted_probes_v4 (ipv4_addr interval, default 127.0.0.1)
// - set orca_trusted_probes_v6 (ipv6_addr interval, default ::1)
// - input chain (SYN-flood filter on :443)
// - prerouting chain (DNAT :443->127.0.0.1:8443, :80->127.0.0.1:8080)
// - forward chain (rate-limit meter on :443)
@@ -25,6 +26,7 @@ package emitter
import (
"errors"
"fmt"
"net"
"strings"
)
@@ -39,9 +41,11 @@ const nftConfigPath = "/etc/nftables.d/orca.nft"
// All fields have safe defaults so a zero-value config renders a
// working ruleset.
type NftClusterConfig struct {
// TrustedProbes is the list of source IPs exempt from the
// TrustedProbes is the list of source IPs/CIDRs exempt from the
// SYN-flood filter and rate-limit (monitoring probes, the orca
// lead itself). Defaults to [127.0.0.1, ::1].
// lead itself). Defaults to [127.0.0.1, ::1]. Each entry must
// parse as a valid IP or CIDR via net.ParseIP / net.ParseCIDR or
// RenderNftConfig returns an error (F9: ruleset injection guard).
TrustedProbes []string
// RateLimit is the per-source rate limit (packets/second) for the
// forward-chain meter on :443. Defaults to 100.
@@ -73,31 +77,79 @@ func (c NftClusterConfig) withDefaults() NftClusterConfig {
// `#!/usr/sbin/nft -f` shebang (so `nft -f` applies it and so a
// drift-check `nft -c -f` validates the syntax).
//
// Returns an error only when the config is internally inconsistent
// (e.g. a negative rate, which the defaults already prevent).
// Returns an error when the config is internally inconsistent (e.g. a
// negative rate, which the defaults already prevent) or when a
// TrustedProbes entry fails to parse as an IP or CIDR (F9: ruleset
// injection hardening — unvalidated entries are written directly into
// the nft ruleset and could inject arbitrary nft syntax).
func (NftEmitter) RenderNftConfig(clusterConfig NftClusterConfig) ([]File, error) {
if clusterConfig.RateLimit < 0 || clusterConfig.RateBurst < 0 {
return nil, errors.New("emitter/nft: rate/burst must be non-negative")
}
cfg := clusterConfig.withDefaults()
content := renderNftRuleset(cfg)
// F9: validate every TrustedProbes entry before rendering. An
// invalid entry is rejected with an error rather than written raw
// into the ruleset (which would allow nft-syntax injection).
v4, v6, err := partitionTrustedProbes(cfg.TrustedProbes)
if err != nil {
return nil, err
}
content := renderNftRuleset(cfg, v4, v6)
return []File{{Path: nftConfigPath, Content: content, Mode: "0644"}}, nil
}
// partitionTrustedProbes validates each entry as an IP or CIDR and
// partitions the list into IPv4 and IPv6 slices. Returns an error if
// any entry is neither a valid IP nor a valid CIDR (F9).
func partitionTrustedProbes(probes []string) (v4, v6 []string, err error) {
for _, p := range probes {
if p == "" {
return nil, nil, fmt.Errorf("emitter/nft: empty trusted probe entry (F9: ruleset injection guard)")
}
if ip := net.ParseIP(p); ip != nil {
if ip.To4() != nil {
v4 = append(v4, p)
} else {
v6 = append(v6, p)
}
continue
}
if _, _, cidrErr := net.ParseCIDR(p); cidrErr == nil {
// Determine address family from the CIDR prefix.
ip := net.ParseIP(strings.Split(p, "/")[0])
if ip != nil && ip.To4() != nil {
v4 = append(v4, p)
} else {
v6 = append(v6, p)
}
continue
}
return nil, nil, fmt.Errorf("emitter/nft: trusted probe %q is not a valid IP or CIDR (F9: ruleset injection guard)", p)
}
return v4, v6, nil
}
// renderNftRuleset builds the nft ruleset string. The shape is
// documented in the package comment; the exact lines are load-bearing
// for `orca doctor nft` (which greps the live table for them) and for
// `nft -c -f` (which parses the syntax).
func renderNftRuleset(cfg NftClusterConfig) string {
//
// F9: TrustedProbes are split into separate ipv4_addr and ipv6_addr
// sets (orca_trusted_probes_v4 / orca_trusted_probes_v6) because the
// prior single ipv4_addr set included ::1 (an IPv6 address), which is
// a type mismatch nft rejects.
func renderNftRuleset(cfg NftClusterConfig, v4, v6 []string) string {
var b strings.Builder
b.WriteString("#!/usr/sbin/nft -f\n\n")
b.WriteString("flush table inet orca-ingress\n\n")
b.WriteString("table inet orca-ingress {\n")
b.WriteString("\tset orca_trusted_probes {\n")
// F9: split IPv4 and IPv6 trusted probes into separate typed sets.
b.WriteString("\tset orca_trusted_probes_v4 {\n")
b.WriteString("\t\ttype ipv4_addr\n")
b.WriteString("\t\tflags interval\n")
b.WriteString("\t\telements = { ")
for i, p := range cfg.TrustedProbes {
for i, p := range v4 {
if i > 0 {
b.WriteString(", ")
}
@@ -105,6 +157,20 @@ func renderNftRuleset(cfg NftClusterConfig) string {
}
b.WriteString(" }\n")
b.WriteString("\t}\n\n")
b.WriteString("\tset orca_trusted_probes_v6 {\n")
b.WriteString("\t\ttype ipv6_addr\n")
b.WriteString("\t\tflags interval\n")
b.WriteString("\t\telements = { ")
for i, p := range v6 {
if i > 0 {
b.WriteString(", ")
}
b.WriteString(p)
}
b.WriteString(" }\n")
b.WriteString("\t}\n\n")
b.WriteString("\tchain input {\n")
b.WriteString("\t\ttype filter hook input priority filter; policy accept;\n")
b.WriteString("\t\tct state invalid drop\n")
+45
View File
@@ -90,3 +90,48 @@ func TestNftEmitter_ShebangFirst(t *testing.T) {
t.Errorf("shebang not first:\n%s", files[0].Content[:40])
}
}
// TestNftEmitter_RejectsInvalidTrustedProbe verifies that a TrustedProbes
// entry that is not a valid IP or CIDR is rejected (F9: ruleset injection
// guard). An unvalidated entry written raw into the ruleset could inject
// arbitrary nft syntax.
func TestNftEmitter_RejectsInvalidTrustedProbe(t *testing.T) {
bad := []string{
"not-an-ip",
"127.0.0.1; flush ruleset",
"$(whoami)",
"10.0.0.0/33", // invalid CIDR prefix
}
for _, b := range bad {
_, err := (NftEmitter{}).RenderNftConfig(NftClusterConfig{TrustedProbes: []string{"127.0.0.1", b}})
if err == nil {
t.Errorf("expected error for invalid trusted probe %q, got nil", b)
}
}
}
// TestNftEmitter_TrustedProbesSplitV4V6 verifies that IPv4 and IPv6
// probes are rendered into separate typed sets (F9: the prior single
// ipv4_addr set included ::1, an IPv6 address — a type mismatch).
func TestNftEmitter_TrustedProbesSplitV4V6(t *testing.T) {
files, err := (NftEmitter{}).RenderNftConfig(NftClusterConfig{TrustedProbes: []string{"10.0.0.5", "::1"}})
if err != nil {
t.Fatalf("Render: %v", err)
}
c := files[0].Content
if !strings.Contains(c, "set orca_trusted_probes_v4") {
t.Errorf("missing v4 set:\n%s", c)
}
if !strings.Contains(c, "set orca_trusted_probes_v6") {
t.Errorf("missing v6 set:\n%s", c)
}
if !strings.Contains(c, "type ipv6_addr") {
t.Errorf("missing ipv6_addr type:\n%s", c)
}
if !strings.Contains(c, "10.0.0.5") {
t.Errorf("missing 10.0.0.5 in v4 set:\n%s", c)
}
if !strings.Contains(c, "::1") {
t.Errorf("missing ::1 in v6 set:\n%s", c)
}
}
+65 -10
View File
@@ -29,6 +29,7 @@ import (
"log/slog"
"net"
"os"
"regexp"
"strings"
"time"
@@ -118,6 +119,18 @@ func BootstrapProxmox(ctx context.Context, opts Options) (*Result, error) {
if opts.SSHPort == 0 {
opts.SSHPort = DefaultSSHPort
}
// F10: validate ProxmoxUser and ProxmoxRole before they are
// interpolated into sudoers content, file paths, and shell commands
// (useradd, pveum). An attacker-controlled value could inject shell
// metacharacters or path traversal. Allowlist: lowercase letter or
// underscore start, followed by lowercase alphanumerics, underscore,
// or hyphen; max 32 chars.
if !validProxmoxName(opts.ProxmoxUser) {
return nil, fmt.Errorf("proxmox bootstrap: invalid ProxmoxUser %q (allowed: ^[a-zA-Z_][a-zA-Z0-9_-]{0,31}$)", opts.ProxmoxUser)
}
if !validProxmoxName(opts.ProxmoxRole) {
return nil, fmt.Errorf("proxmox bootstrap: invalid ProxmoxRole %q (allowed: ^[a-zA-Z_][a-zA-Z0-9_-]{0,31}$)", opts.ProxmoxRole)
}
log := opts.Logger
if log == nil {
log = slog.Default()
@@ -392,7 +405,8 @@ func deployPubKey(user, pubLine string) error {
// createLinuxUser creates the orca system user if it doesn't already
// exist. Idempotent: `id -u` check before `useradd`.
func createLinuxUser(user string) error {
cmd := fmt.Sprintf("id -u %s 2>/dev/null || useradd -r -s /usr/sbin/nologin %s", user, user)
// F10c: shellQuote the user (validated upstream, but defense-in-depth).
cmd := fmt.Sprintf("id -u %s 2>/dev/null || useradd -r -s /usr/sbin/nologin %s", shellQuote(user), shellQuote(user))
if _, err := runRemote(cmd); err != nil {
return err
}
@@ -402,9 +416,10 @@ func createLinuxUser(user string) error {
// createPVERole creates the OrcaOperator PVE role if it doesn't exist.
// Idempotent: probes `pveum role list` before `pveum role add`.
func createPVERole(role string) error {
// F10c: shellQuote the role (validated upstream, but defense-in-depth).
cmd := fmt.Sprintf(
"pveum role list 2>/dev/null | grep -q '^%s' || pveum role add %s --privs '%s'",
role, role, OrcaOperatorPrivileges,
shellQuote(role), shellQuote(role), OrcaOperatorPrivileges,
)
if _, err := runRemote(cmd); err != nil {
return err
@@ -417,9 +432,11 @@ func createPVERole(role string) error {
// Uses @pam realm (AD-019) since orca creates a Linux system user.
func createPVEUser(user string) error {
pveUserID := user + "@pam"
// F10c: shellQuote the PVE user id (validated upstream, but
// defense-in-depth).
cmd := fmt.Sprintf(
"pveum user list 2>/dev/null | grep -q '%s' || pveum user add %s -comment 'Orca automation user'",
pveUserID, pveUserID,
"pveum user list 2>/dev/null | grep -q %s || pveum user add %s -comment 'Orca automation user'",
shellQuote(pveUserID), shellQuote(pveUserID),
)
if _, err := runRemote(cmd); err != nil {
return err
@@ -431,7 +448,9 @@ func createPVEUser(user string) error {
// (cluster-wide). `pveum acl modify` is idempotent (creates or updates).
func assignPVEACL(user, role string) error {
pveUserID := user + "@pam"
cmd := fmt.Sprintf("pveum acl modify / -user %s -role %s", pveUserID, role)
// F10c: shellQuote the PVE user id and role (validated upstream,
// but defense-in-depth).
cmd := fmt.Sprintf("pveum acl modify / -user %s -role %s", shellQuote(pveUserID), shellQuote(role))
if _, err := runRemote(cmd); err != nil {
return err
}
@@ -454,13 +473,20 @@ func sudoersContent(user string) string {
`, user, user)
}
// sudoersPath is the fixed on-peer path for the orca sudoers drop-in.
// F10b: the file is always written here regardless of the configured
// ProxmoxUser name, so a crafted username cannot redirect the sudoers
// drop-in to an arbitrary path.
const sudoersPath = "/etc/sudoers.d/orca"
// writeSudoers writes the /etc/sudoers.d/orca file on the remote host
// with mode 0440. Uses a heredoc via cat to avoid quoting issues.
// with mode 0440. Uses a heredoc via cat to avoid quoting issues. F10b:
// the path is fixed (sudoersPath) regardless of the configured username.
func writeSudoers(user string) error {
content := sudoersContent(user)
// Write via cat heredoc, then chmod 0440.
cmd := fmt.Sprintf("cat > /etc/sudoers.d/%s <<'ORCA_SUDOERS_EOF'\n%s\nORCA_SUDOERS_EOF\nchmod 0440 /etc/sudoers.d/%s",
user, content, user)
// Write via cat heredoc to the fixed path, then chmod 0440.
cmd := fmt.Sprintf("cat > %s <<'ORCA_SUDOERS_EOF'\n%s\nORCA_SUDOERS_EOF\nchmod 0440 %s",
sudoersPath, content, sudoersPath)
if _, err := runRemote(cmd); err != nil {
return err
}
@@ -470,8 +496,14 @@ func writeSudoers(user string) error {
// validateSudoers runs `visudo -cf` on the sudoers file. Aborts the
// bootstrap if validation fails (prevents a broken sudoers from
// locking the orca user out of sudo).
// validateSudoers runs `visudo -cf` on the sudoers file. F10d: it
// validates the actual file that writeSudoers wrote (sudoersPath,
// /etc/sudoers.d/orca), which is now a fixed path — the prior version
// hardcoded /etc/sudoers.d/orca while writeSudoers wrote to
// /etc/sudoers.d/<ProxmoxUser>, so a custom username would validate the
// wrong file.
func validateSudoers() error {
cmd := "visudo -cf /etc/sudoers.d/orca"
cmd := fmt.Sprintf("visudo -cf %s", sudoersPath)
out, err := runRemote(cmd)
if err != nil {
return fmt.Errorf("visudo validation failed: %w (output: %s)", err, strings.TrimSpace(string(out)))
@@ -482,6 +514,29 @@ func validateSudoers() error {
return nil
}
// proxmoxNameRe is the allowlist for ProxmoxUser and ProxmoxRole values
// that are interpolated into sudoers content, file paths, and shell
// commands (F10a). Letter or underscore start, followed by
// alphanumerics, underscore, or hyphen; max 32 chars. Uppercase is
// permitted (DefaultProxmoxRole is "OrcaOperator"); shell
// metacharacters (spaces, ;, $, backticks, etc.) are blocked.
var proxmoxNameRe = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_-]{0,31}$`)
// validProxmoxName reports whether s is a safe ProxmoxUser or ProxmoxRole
// value (F10a injection guard).
func validProxmoxName(s string) bool {
return proxmoxNameRe.MatchString(s)
}
// shellQuote single-quotes a string for safe shell interpolation over
// the SSH exec session. It escapes embedded single-quotes via the
// standard ”' idiom (POSIX shell). F10c: hardens pveum/useradd commands
// against metacharacter injection (the validated allowlist is
// defense-in-depth on top of this).
func shellQuote(s string) string {
return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'"
}
// ResetHostKey removes all known_hosts entries for the given host from
// certpaths.KnownHostsPath() (REQ-059, D-046, AD-029). It rewrites the
// file atomically via security.WriteAtomic. LOCAL ONLY — it does NOT
+36
View File
@@ -96,6 +96,42 @@ func TestBootstrapProxmox_Validation(t *testing.T) {
}
}
// TestBootstrapProxmox_RejectsInvalidProxmoxUser verifies that a
// ProxmoxUser containing shell metacharacters is rejected before any
// SSH dial (F10a: sudoers/shell injection guard).
func TestBootstrapProxmox_RejectsInvalidProxmoxUser(t *testing.T) {
ctx := context.Background()
bad := []string{"orca; rm -rf /", "orca$(whoami)", "orca`id`", "orca user", "1orca"}
for _, b := range bad {
_, err := BootstrapProxmox(ctx, Options{Host: "10.0.0.1", SSHKeyPath: certpaths.SSHKeyPath(), ProxmoxUser: b})
if err == nil {
t.Errorf("expected error for invalid ProxmoxUser %q, got nil", b)
continue
}
if !strings.Contains(err.Error(), "invalid ProxmoxUser") {
t.Errorf("error should mention invalid ProxmoxUser for %q, got: %v", b, err)
}
}
}
// TestBootstrapProxmox_RejectsInvalidProxmoxRole verifies that a
// ProxmoxRole containing shell metacharacters is rejected before any
// SSH dial (F10a: sudoers/shell injection guard).
func TestBootstrapProxmox_RejectsInvalidProxmoxRole(t *testing.T) {
ctx := context.Background()
bad := []string{"role; flush", "role$(id)", "role`whoami`", "role name", "1role"}
for _, b := range bad {
_, err := BootstrapProxmox(ctx, Options{Host: "10.0.0.1", SSHKeyPath: certpaths.SSHKeyPath(), ProxmoxRole: b})
if err == nil {
t.Errorf("expected error for invalid ProxmoxRole %q, got nil", b)
continue
}
if !strings.Contains(err.Error(), "invalid ProxmoxRole") {
t.Errorf("error should mention invalid ProxmoxRole for %q, got: %v", b, err)
}
}
}
func TestDefaultOptions(t *testing.T) {
if DefaultProxmoxUser != "orca" {
t.Errorf("DefaultProxmoxUser = %q, want orca", DefaultProxmoxUser)
+1 -1
View File
@@ -64,7 +64,7 @@ func (p *PodmanRuntime) Start(ctx context.Context, alloc *Alloc) (int, error) {
}
cmdStr, _ := commandFor(alloc)
name := containerName(alloc)
cmd := fmt.Sprintf("podman run -d --name %s %q %s", shellQuote(name), image, shellQuote(cmdStr))
cmd := fmt.Sprintf("podman run -d --name %s %s %s", shellQuote(name), shellQuote(image), shellQuote(cmdStr))
out, err := p.transport.Exec(ctx, alloc.Node, cmd)
if err != nil {
return 0, fmt.Errorf("podman: run: %w", err)