fix(security): P2 agent batch (P2-5/7/9/10) (#51)

* fix(security): strip version + uptime from /health response

The unauthenticated /health endpoint returned:

  {"status":"ok","version":"1.2.3","uptime":"2h30m15s","timestamp":...}

`version` is populated from compile-time ldflags. A remote scanner
probing /health learns the exact agent build and can correlate against
public CVEs without needing to authenticate. `uptime` reveals whether
the agent has been recently restarted (i.e. whether a patch was
applied). `timestamp` is mostly harmless but adds another datum to a
fingerprint.

Authenticated callers that legitimately need the version still get it
from /api/v1/metadata.

The Handlers struct keeps the `version` and `startTime` fields so the
NewHandlers signature doesn't churn — they're just no longer exposed
via /health.

Test asserts the response body is exactly `{"status":"ok"}` and
contains none of the substrings "1.2.3-leakytest", "uptime", or
"timestamp".

P2-5 from the 2026-05 security audit.

* fix(security): force TLS 1.3 minimum on the agent HTTPS listener

http.Server with no explicit TLSConfig falls through to Go's default,
which is TLS 1.2 floor. Acceptable per RFC, but it leaves the agent
willing to negotiate cipher suites that have been deprecated for years
(CBC modes, RC4 if a downgrade happens, etc.). Both ends of the
agent <-> dashboard channel are software we control; there's no
legitimate reason to support pre-1.3.

Explicit MinVersion: tls.VersionTLS13.

A client that genuinely cannot speak TLS 1.3 (Go ≥ 1.12, OpenSSL ≥
1.1.1, Node ≥ 10.13, basically anything from the last 8 years) will
fail the handshake — which is the correct behavior. The dashboard
binary that talks to this agent is built with the same Go toolchain;
it speaks 1.3 by default.

P2-7 from the 2026-05 security audit.

* fix(security): tighten apt package-name validation; add `--` separator

The /api/v1/packages/install and /packages/remove handlers validated
input as "non-empty AND ≤200 chars" before passing to apt-get. The
package-manager layer (isValidPackageName) already had the strict
Debian-style regex + leading-hyphen rejection, so the practical
exploit window was small — but errors surfaced as generic 500s and
the boundary check was misleadingly weak.

Two changes:

1. Handlers now call isValidPackageName directly. Invalid names return
   400 with a clear message instead of falling through to a 500 from
   the package-manager layer.

2. apt-get and apt-mark invocations get an explicit "--" between flags
   and the package operand:
     apt-get install -y -- <name>
     apt-get remove   -y -- <name>
     apt-get install -y --allow-downgrades -- <pkgs...>
     apt-mark hold   -- <name>
     apt-mark unhold -- <name>

   Defense-in-depth: a future loosening of isValidPackageName (or a
   missed validation site, or a new caller that forgets to call it)
   still can't smuggle a "package" name that apt-get would parse as a
   flag.

Tests cover the attack class:
  --allow-downgrades, --reinstall, -y, --help, .bashrc, name with
  spaces / semicolons / $() / newlines / slashes.

P2-9 from the 2026-05 security audit.

* fix(security): validate haproxy.acl.path + haproxy.acl.header at parse time

The audit's P2-10 framing was "drop the dead ACLPath/ACLHeader fields,"
but those labels are documented in
ubuntu-ha-proxy-install/docs/architecture.md and
ubuntu-ha-proxy-install/docs/example-docker-compose.yml as supported
forward-looking features ("Path-based routing", "Custom ACL conditions").
Deleting the BackendConfig fields would break that documented API.

Instead, validate them now so the latent injection class (same shape as
P0-1 BackendName and P0-2 ACLIP) is closed before any future PR wires
them into a generated directive like:

  acl req_path path_beg <path>
  acl req_hdr_cnt(<header>) gt 0

Two new validators in parser.go:

- validACLPath: leading slash + URL-path-safe charset
  (`^/[a-zA-Z0-9/_.\-~%]*$`). Rejects embedded newlines, semicolons,
  whitespace, query strings, fragments.

- validACLHeader: RFC 7230 HTTP header token characters
  (`^[a-zA-Z0-9!#$%&'*+\-.^_`+"`"+`|~]+$`). Rejects newlines, colons,
  slashes, spaces.

extractBackendConfig now rejects the whole backend if either field has
a non-empty malformed value. Empty values (the overwhelmingly common
case) skip validation as today.

Tests cover the legitimate-shape pass list, the injection class
(newline + HAProxy directive payload), and the common-injection-
character reject list.

P2-10 from the 2026-05 security audit.

* fix(security): address P2 agent batch Copilot review

- pm_apt.go: add "--" separator to BuildInstallCommand + InstallUpdates
  install paths. Was already on InstallPackage/RemovePackage but the two
  bulk-install paths were the actual gap — defense-in-depth against a
  caller that skips isValidPackageName.
- handlers.go: remove dead Handlers.version and Handlers.startTime
  fields. They were set but never read once the /health response was
  stripped to {status: "ok"}. Update HealthResponse doc to drop the
  inaccurate "available via /api/v1/metadata" claim — MetadataResponse
  exposes HAProxy metadata, not agent version/uptime.
- server.go: rewrite the TLS-1.3-floor comment. The previous text
  claimed Go's TLS 1.2 defaults included CBC and RC4; RC4 has been
  removed from Go's defaults for years and the default cipher suites
  are mostly AEAD. The concrete reason (shrink negotiation surface,
  guarantee AEAD + forward secrecy, prevent downgrade) is what
  matters.
- docs/: regenerate Swagger schema so the published OpenAPI doc
  matches the stripped HealthResponse (was still advertising version,
  uptime, timestamp fields).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: sarg3nt

gearbox-agent/docs/docs.go

@@ -18,42 +18,36 @@ type MetadataProvider interface {
 // Handlers holds HTTP handlers and their dependencies.
 type Handlers struct {
 	metadataProvider MetadataProvider
-	version          string
-	startTime        time.Time
 }
 
 // NewHandlers creates a new Handlers instance.
-func NewHandlers(metadataProvider MetadataProvider, version string) *Handlers {
+func NewHandlers(metadataProvider MetadataProvider) *Handlers {
 	return &Handlers{
 		metadataProvider: metadataProvider,
-		version:          version,
-		startTime:        time.Now(),
 	}
 }
 
 // HealthResponse represents the health check response.
+//
+// Only "status" is returned on the unauthenticated /health endpoint to avoid
+// leaking version / uptime to remote scanners probing for known-vulnerable
+// agent versions. Version and build info are still available on the build
+// itself (--version) and through service logs, but are not exposed over the
+// network on unauthenticated endpoints. See 2026-05 security audit P2-5.
 type HealthResponse struct {
-	Status    string    `json:"status" example:"ok"`
-	Version   string    `json:"version" example:"1.0.0"`
-	Uptime    string    `json:"uptime" example:"2h30m15s"`
-	Timestamp time.Time `json:"timestamp" example:"2024-01-17T10:30:00Z"`
+	Status string `json:"status" example:"ok"`
 }
 
 // Health handles GET /health (no auth required).
 //
 //	@Summary		Health check
-//	@Description	Returns the health status of the gearbox-agent service. No authentication required.
+//	@Description	Returns the health status of the gearbox-agent service. No authentication required. Only "status" is exposed; version and uptime are intentionally omitted from this unauthenticated endpoint to avoid fingerprinting by remote scanners.
 //	@Tags			Health
 //	@Produce		json
 //	@Success		200	{object}	HealthResponse	"Service is healthy"
 //	@Router			/health [get]
 func (h *Handlers) Health(w http.ResponseWriter, r *http.Request) {
-	resp := HealthResponse{
-		Status:    "ok",
-		Version:   h.version,
-		Uptime:    time.Since(h.startTime).Round(time.Second).String(),
-		Timestamp: time.Now(),
-	}
+	resp := HealthResponse{Status: "ok"}
 
 	w.Header().Set("Content-Type", "application/json")
 	json.NewEncoder(w).Encode(resp)

gearbox-agent/docs/swagger.json

@@ -0,0 +1,54 @@
+package api
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/sarg3nt/gearbox-agent/internal/framework/services/haproxy"
+)
+
+// stubMetadataProvider is the minimal MetadataProvider needed for handler
+// construction. Not actually exercised by the /health test.
+type stubMetadataProvider struct{}
+
+func (stubMetadataProvider) GetLastSyncTime() time.Time      { return time.Time{} }
+func (stubMetadataProvider) GetLastError() error             { return nil }
+func (stubMetadataProvider) GetMetadata() *haproxy.Metadata  { return nil }
+
+// 2026-05 audit P2-5: the unauthenticated /health endpoint must not leak
+// version or uptime. A remote scanner probing for known-vulnerable agent
+// versions should get "ok" and nothing else.
+func TestHealth_NoVersionLeak(t *testing.T) {
+	h := NewHandlers(stubMetadataProvider{})
+
+	req := httptest.NewRequest("GET", "/health", nil)
+	rr := httptest.NewRecorder()
+	h.Health(rr, req)
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status = %d, want 200", rr.Code)
+	}
+
+	body := rr.Body.String()
+	for _, leak := range []string{"1.2.3-leakytest", "uptime", "timestamp"} {
+		if strings.Contains(strings.ToLower(body), strings.ToLower(leak)) {
+			t.Errorf("/health response contains %q (potential info leak): %s", leak, body)
+		}
+	}
+
+	// Sanity: the actual response should be a small {"status":"ok"} JSON.
+	var got map[string]any
+	if err := json.Unmarshal([]byte(body), &got); err != nil {
+		t.Fatalf("response is not valid JSON: %v (%q)", err, body)
+	}
+	if got["status"] != "ok" {
+		t.Errorf("status = %v, want \"ok\"", got["status"])
+	}
+	if len(got) != 1 {
+		t.Errorf("response has %d fields; want exactly 1 (status): %v", len(got), got)
+	}
+}

gearbox-agent/docs/swagger.yaml

@@ -5,6 +5,7 @@ package api
 
 import (
 	"context"
+	"crypto/tls"
 	"fmt"
 	"log/slog"
 	"net/http"
@@ -69,7 +70,7 @@ type ServerConfig struct {
 // NewServer creates a new API server.
 func NewServer(cfg ServerConfig) *Server {
 	// Create handlers
-	handlers := NewHandlers(cfg.MetadataProvider, cfg.Version)
+	handlers := NewHandlers(cfg.MetadataProvider)
 
 	// Create chi router
 	r := chi.NewRouter()
@@ -157,6 +158,16 @@ func NewServer(cfg ServerConfig) *Server {
 			ReadTimeout:  15 * time.Second,
 			WriteTimeout: 15 * time.Second,
 			IdleTimeout:  60 * time.Second,
+			// Explicit TLS 1.3 floor. Go's default minimum is TLS 1.2 (still
+			// negotiable by misconfigured or older clients); both ends of the
+			// agent <-> dashboard channel are controlled by us, so we have
+			// no reason to leave TLS 1.2 reachable. Pinning to 1.3 shrinks
+			// the negotiation attack surface (no downgrade, fewer cipher
+			// suites, AEAD-only, forward secrecy guaranteed). See 2026-05
+			// security audit P2-7.
+			TLSConfig: &tls.Config{
+				MinVersion: tls.VersionTLS13,
+			},
 		},
 		router:   r,
 		logger:   cfg.Logger,

gearbox-agent/internal/api/handlers.go

@@ -43,6 +43,18 @@ var (
 	// newline / directive injection from a Docker Compose label into the
 	// generated haproxy.cfg (see 2026-05 security audit, P0-1).
 	validBackendName = regexp.MustCompile(`^[a-zA-Z0-9_][a-zA-Z0-9_.\-]{0,62}$`)
+	// validACLPath matches a URL path component for an HAProxy path-based
+	// ACL: leading `/`, then a safe URL-path character set. Used for the
+	// `haproxy.acl.path` label, which is currently parsed into BackendConfig
+	// but not yet wired into the generator. Validating now closes the
+	// latent injection class — if a future PR wires this into a generated
+	// `acl req_path path_beg %s` directive, no flag/newline can slip
+	// through. See 2026-05 security audit, P2-10.
+	validACLPath = regexp.MustCompile(`^/[a-zA-Z0-9/_.\-~%]*$`)
+	// validACLHeader matches an HTTP header name (RFC 7230 token chars).
+	// Same forward-looking rationale as validACLPath: parsed but not yet
+	// generated, validated now.
+	validACLHeader = regexp.MustCompile(`^[a-zA-Z0-9!#$%&'*+\-.^_` + "`" + `|~]+$`)
 )
 
 // validateACLIPList validates a comma-separated list of IPs/CIDRs against
@@ -312,6 +324,31 @@ func (p *Parser) extractBackendConfig(labels map[string]string, appName, service
 		return nil
 	}
 
+	// 2026-05 audit P2-10: haproxy.acl.path and haproxy.acl.header are
+	// documented labels (see ubuntu-ha-proxy-install/docs/) that gearbox-
+	// agent parses today but does NOT yet emit into the generated config.
+	// Validate them here so the latent injection class is closed BEFORE
+	// any future PR wires them into a directive like
+	// `acl req_path path_beg <path>` or `acl req_hdr_cnt(<header>) gt 0`.
+	// Empty is accepted (the common case); non-empty must match the
+	// allow-list pattern, otherwise the backend is rejected.
+	aclPath := labels[LabelPrefix+"acl.path"]
+	if aclPath != "" && !validACLPath.MatchString(aclPath) {
+		p.logger.Warn("Invalid haproxy.acl.path value; rejecting backend",
+			"app", appName,
+			"service", serviceName,
+		)
+		return nil
+	}
+	aclHeader := labels[LabelPrefix+"acl.header"]
+	if aclHeader != "" && !validACLHeader.MatchString(aclHeader) {
+		p.logger.Warn("Invalid haproxy.acl.header value; rejecting backend",
+			"app", appName,
+			"service", serviceName,
+		)
+		return nil
+	}
+
 	// Get and validate configurable values with safe defaults
 	mode := getValidatedOrDefault(labels, LabelPrefix+"backend.mode", "http", validMode)
 	balance := getValidatedOrDefault(labels, LabelPrefix+"backend.balance", "roundrobin", validBalance)
@@ -334,8 +371,8 @@ func (p *Parser) extractBackendConfig(labels map[string]string, appName, service
 		CheckFall:        checkFall,
 		CheckRise:        checkRise,
 		SSLRedirect:      labels[LabelPrefix+"ssl.redirect"] != "false",
-		ACLPath:          labels[LabelPrefix+"acl.path"],
-		ACLHeader:        labels[LabelPrefix+"acl.header"],
+		ACLPath:          aclPath,
+		ACLHeader:        aclHeader,
 		Public:           labels[LabelPrefix+"public"] == "true",
 		RateLimit:        rateLimit,
 		ACLIP:            aclIP,

gearbox-agent/internal/api/handlers_test.go

@@ -757,3 +757,104 @@ func TestParseFile_InjectionViaACLIP(t *testing.T) {
 		t.Errorf("ParseFile() returned %d backend(s) for an injection payload; want 0 (rejected)", len(backends))
 	}
 }
+
+// 2026-05 audit P2-10: haproxy.acl.path and haproxy.acl.header are
+// validated at parse time even though no current generator emits them.
+// Closes the latent injection class before a future PR wires them in.
+func TestValidACLPath(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  bool
+	}{
+		{"empty", "", true}, // empty handled by call-site short-circuit
+		{"root", "/", true},
+		{"single-segment", "/api", true},
+		{"multi-segment", "/api/v1/users", true},
+		{"trailing-slash", "/api/", true},
+		{"with-dash-underscore", "/api/_v2-beta", true},
+		{"url-encoded-allowed", "/api/%20space", true},
+
+		{"no-leading-slash", "api", false},
+		{"newline-injection", "/api\n  http-request deny", false},
+		{"semicolon", "/api; default-src *", false},
+		{"space", "/api /v1", false},
+		{"backslash", "/api\\v1", false},
+		{"query-string", "/api?x=1", false},
+		{"fragment", "/api#section", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.input == "" {
+				return // call-site skips validation for empty
+			}
+			if got := validACLPath.MatchString(tt.input); got != tt.want {
+				t.Errorf("validACLPath.MatchString(%q) = %v, want %v", tt.input, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestValidACLHeader(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  bool
+	}{
+		{"simple", "X-Custom-Header", true},
+		{"all-lower", "authorization", true},
+		{"underscore", "X_Custom", true},
+		{"dot-allowed-by-rfc", "X.Forwarded.For", true},
+
+		{"empty", "", false},
+		{"space", "X Custom", false},
+		{"newline-injection", "X-Custom\nhttp-request deny", false},
+		{"semicolon", "X-Custom;evil", false},
+		{"colon", "X-Custom:value", false},
+		{"slash", "X-Custom/extra", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := validACLHeader.MatchString(tt.input); got != tt.want {
+				t.Errorf("validACLHeader.MatchString(%q) = %v, want %v", tt.input, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestParseFile_InjectionViaACLPath(t *testing.T) {
+	// 2026-05 audit P2-10: even though the generator doesn't emit
+	// haproxy.acl.path today, a malicious value must not be stored on
+	// BackendConfig — otherwise a future generator wire-up would re-open
+	// the P0-1/P0-2 class of injection.
+	tmpDir := t.TempDir()
+	appDir := filepath.Join(tmpDir, "aclpath")
+	if err := os.MkdirAll(appDir, 0755); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
+
+	composeContent := "services:\n" +
+		"  web:\n" +
+		"    image: nginx\n" +
+		"    labels:\n" +
+		"      haproxy.enable: \"true\"\n" +
+		"      haproxy.hostname: \"evil.example.com\"\n" +
+		"      haproxy.backend.server: \"10.0.0.1:80\"\n" +
+		"      haproxy.acl.path: |\n" +
+		"        /api\n" +
+		"          http-request deny\n"
+
+	composePath := filepath.Join(appDir, "docker-compose.yml")
+	if err := os.WriteFile(composePath, []byte(composeContent), 0644); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+
+	parser := NewParser(tmpDir, testLogger())
+	backends, err := parser.ParseFile(composePath)
+	if err != nil {
+		t.Fatalf("ParseFile: %v", err)
+	}
+	if len(backends) != 0 {
+		t.Errorf("ParseFile returned %d backend(s); want 0 (rejected on aclpath injection)", len(backends))
+	}
+}

gearbox-agent/internal/api/server.go

@@ -219,8 +219,13 @@ func (r *AptRunner) runAptInstall(ctx context.Context, op *AptOperation, securit
 	// Build apt command
 	var args []string
 	if len(packages) > 0 {
-		// Install specific packages
-		args = append([]string{"install", "-y"}, packages...)
+		// Install specific packages. "--" between the flags and the package
+		// list prevents a flag-shaped element from being interpreted as an
+		// apt-get option (2026-05 audit P2-9). The packages slice flows
+		// from API input; even though every consumer validates with
+		// isValidPackageName, the explicit separator is a cheap
+		// defense-in-depth gate.
+		args = append([]string{"install", "-y", "--"}, packages...)
 		r.publishLine(op.ID, fmt.Sprintf("Installing %d specific package(s)...", len(packages)))
 	} else if securityOnly {
 		// Security updates only - use unattended-upgrade
@@ -502,7 +507,9 @@ func (r *AptRunner) runRestoreSnapshot(ctx context.Context, op *AptOperation, sn
 		r.publishLine(op.ID, fmt.Sprintf("Downgrading %d package(s) to snapshot versions...", len(downgrades)))
 		r.publishLine(op.ID, "")
 
-		args := append([]string{"install", "-y", "--allow-downgrades"}, downgrades...)
+		// "--" before downgrades for the same reason as the install path
+		// above. See 2026-05 audit P2-9.
+		args := append([]string{"install", "-y", "--allow-downgrades", "--"}, downgrades...)
 		downgradeCmd := exec.CommandContext(ctx, "apt-get", args...)
 		downgradeCmd.Env = append(os.Environ(),
 			"DEBIAN_FRONTEND=noninteractive",

gearbox-agent/internal/framework/services/compose/parser.go

@@ -0,0 +1,52 @@
+package updates
+
+import (
+	"strings"
+	"testing"
+)
+
+// 2026-05 audit P2-9: isValidPackageName must reject any name a shell user
+// would never plausibly type AND any name that apt-get would interpret as
+// a flag. The handler-level boundary validation now relies on this.
+func TestIsValidPackageName(t *testing.T) {
+	tests := []struct {
+		name string
+		want bool
+	}{
+		// Accepted: real Debian-style package names.
+		{"nginx", true},
+		{"libssl3", true},
+		{"python3.11", true},
+		{"linux-headers-6.1.0-12-amd64", true},
+		{"libstdc++6", true},
+		{"foo+bar", true},
+		{"a", true},
+
+		// Rejected: empty / length / character set.
+		{"", false},
+		{strings.Repeat("a", 257), false},
+		{"name with space", false},
+		{"name;rm /etc/passwd", false},
+		{"name$(whoami)", false},
+		{"name\nwith-newline", false},
+		{"name/with/slash", false},
+
+		// Rejected: would land as a flag if passed to apt-get install.
+		// This is the actual P2-9 attack class.
+		{"--allow-downgrades", false},
+		{"--reinstall", false},
+		{"-y", false},
+		{"--help", false},
+
+		// Rejected: leading dot would resolve relative paths in some
+		// apt internals.
+		{".bashrc", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isValidPackageName(tt.name); got != tt.want {
+				t.Errorf("isValidPackageName(%q) = %v, want %v", tt.name, got, tt.want)
+			}
+		})
+	}
+}