Skip to content

Commit 2a635dd

Browse files
Merge pull request #88 from CodesWhat/dev/v1.3
🔒 v1.3 dev: code-review batch — security hardening, perf, dedupe, coverage
2 parents 4b03ee1 + 1d94203 commit 2a635dd

33 files changed

Lines changed: 2108 additions & 269 deletions

CHANGELOG.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Security
11+
12+
- **Admin endpoint paths are now normalized before matching.** The validate and policy-version interceptors compared `r.URL.Path` to the configured path with an exact string match, so variants like `/admin/validate/` (trailing slash) or `/admin//validate` fell through to the Docker-API rule evaluator instead of being handled by the admin layer. Default-deny meant they were still rejected, but any future `/admin/**` allow rule would have exposed them upstream. Paths are now `path.Clean`-normalized on both sides, with regression coverage for trailing-slash, doubled-separator, and dot-segment variants.
13+
- **Non-upgrade hijack responses strip hop-by-hop headers.** When the daemon rejects an attach/exec upgrade (non-101), the fallback response copied every upstream header verbatim — including `Connection`, `Upgrade`, `Keep-Alive`, and any header named in `Connection`, letting a hostile or misconfigured upstream inject connection-scoped headers into the client response. The fallback now applies the same hop-by-hop stripping the upgrade path already used.
14+
- **Container-label ACLs now log an exclusivity warning at startup.** Label grants (`com.sockguard.allow.*`) are only trustworthy when sockguard is the *only* consumer of the Docker socket — a workload with raw socket access can create a container with arbitrary permission labels and self-grant access. Sockguard cannot detect other socket consumers, so enabling `clients.container_labels` now emits an explicit warning stating the invariant.
15+
16+
### Fixed
17+
18+
- **`Defaults()` and the load-time Viper defaults disagreed on `image_trust.require_rekor_inclusion`.** The Viper default was a hardcoded `true` while `config.Defaults()` returned `false` for the same field. Loading was unaffected (the Viper value won), but any consumer of `Defaults()` saw the wrong posture. `Defaults()` now carries `true` for both `container_create` and `service` image trust, and load defaults read from it — one source of truth.
19+
20+
### Changed
21+
22+
- **The Docker builder stage now cross-compiles (`--platform=$BUILDPLATFORM` + `GOOS`/`GOARCH` from BuildKit target args)** instead of running the Go toolchain under emulation for non-native platforms. Multi-arch builds get native-speed compiles; this also fixes Go runtime faults observed when emulating the amd64 toolchain under qemu/Rosetta.
23+
- **`filters` query decoding is now a single shared decoder (`internal/dockerfilters`).** Ownership and visibility had drifted copies; the canonical version sorts legacy map-format (`{"label":{"k=v":true}}`) keys, so owner-label filter rewrites now produce deterministic ordering for legacy-format requests too.
24+
25+
### Performance
26+
27+
- **Name/image pattern visibility checks no longer cost one daemon round-trip per list item.** Resource-meta inspects (`name_patterns` / `image_patterns` axes) now go through the same TTL-LRU, request-coalescing cache as label lookups, so a monitoring tool polling `GET /containers/json` against 50 containers triggers at most one upstream inspect per resource per 10-second TTL window instead of 50 per poll.
28+
- **File logging uses a 64 KiB buffer (up from 4 KiB),** cutting flush frequency and writer-mutex hold time on busy hosts; the 1-second periodic flush still bounds record staleness on quiet ones.
29+
1030
## [1.2.0] - 2026-06-02
1131

1232
### Added

Dockerfile

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,19 @@
1-
FROM golang:1.26.4-alpine3.23@sha256:f23e8b227fb4493eabe03bede4d5a32d04092da71962f1fb79b5f7d1e6c2a17f AS builder
1+
# --platform=$BUILDPLATFORM: the builder always runs natively and CROSS-compiles
2+
# for $TARGETARCH. Running the amd64 toolchain under qemu/Rosetta emulation is
3+
# both slow and unreliable (Go runtime faults during go mod download).
4+
FROM --platform=$BUILDPLATFORM golang:1.26.4-alpine3.23@sha256:f23e8b227fb4493eabe03bede4d5a32d04092da71962f1fb79b5f7d1e6c2a17f AS builder
25

36
ARG VERSION=dev
47
ARG COMMIT=unknown
58
ARG BUILD_DATE=unknown
9+
ARG TARGETOS TARGETARCH
610
WORKDIR /build
711

812
COPY app/go.mod app/go.sum ./
913
RUN go mod download
1014

1115
COPY app/ .
12-
RUN CGO_ENABLED=0 GOOS=linux go build \
16+
RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build \
1317
-ldflags="-s -w \
1418
-X github.com/codeswhat/sockguard/internal/version.Version=${VERSION} \
1519
-X github.com/codeswhat/sockguard/internal/version.Commit=${COMMIT} \

README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -461,8 +461,9 @@ LinuxServer's socket-proxy env surface is already Tecnativa-compatible for the b
461461

462462
| Tier | Theme |
463463
|---|---|
464-
| Security hardening (v1.x) | Continued mutation-test hardening of the rule-evaluation core and config validators |
464+
| Security hardening (v1.x) | Continued mutation-test hardening of the rule-evaluation core and config validators; swarm `ContainerSpec.User` / `Privileges` enforcement parity with container create; wide-open admin listener promoted from startup warning to error behind an explicit opt-in flag |
465465
| Policy refinement (v1.x) | Multiple frontend listeners on the main proxy, named rule path aliases |
466+
| Internals (v1.x) | Code-review backlog: collapse the config → filter-options → policy translation layers behind a single source of truth (generated Viper defaults); allocation-free rate-limit bucket state (packed `atomic.Uint64`); profiling-gated JSON redaction fast path |
466467
| Compliance (v1.x) | CIS Docker Benchmark control mapping, audit-ready policy templates |
467468
| Multi-host (v1.3) | Remote Docker TCP upstreams, multi-upstream fan-out, remote daemon health checking, connection pooling, automatic failover |
468469
| Extensibility (v1.x+) | Optional plugin extension points (WASM or Go plugins), OPA/Rego policy integration |

app/internal/admin/admin.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,20 @@ import (
2626
"io"
2727
"log/slog"
2828
"net/http"
29+
"path"
2930

3031
"github.com/codeswhat/sockguard/internal/httpjson"
3132
"github.com/codeswhat/sockguard/internal/logging"
3233
)
3334

35+
// matchesAdminPath reports whether a request path addresses the admin
36+
// endpoint at configured. Cleaning the request path first means variants
37+
// like a trailing slash or doubled separators are still intercepted here
38+
// instead of leaking through to the Docker-API rule evaluator.
39+
func matchesAdminPath(requestPath, configured string) bool {
40+
return path.Clean(requestPath) == path.Clean(configured)
41+
}
42+
3443
// ValidateResponse is the JSON body returned by the validate endpoint.
3544
//
3645
// On success: OK=true, Rules is the number of top-level compiled rules,
@@ -89,7 +98,7 @@ func NewValidateInterceptor(opts Options) func(http.Handler) http.Handler {
8998
}
9099
return func(next http.Handler) http.Handler {
91100
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
92-
if r.URL.Path != opts.Path {
101+
if !matchesAdminPath(r.URL.Path, opts.Path) {
93102
next.ServeHTTP(w, r)
94103
return
95104
}
@@ -141,10 +150,10 @@ func handlePOST(w http.ResponseWriter, r *http.Request, opts Options) {
141150
// requests whose path matches path and passes all other requests through to
142151
// next. Scoping the 503 to a specific path prevents a misconfigured admin
143152
// endpoint from blocking unrelated Docker API traffic on the same listener.
144-
func serviceUnavailableMiddleware(path, reason string, logger *slog.Logger) func(http.Handler) http.Handler {
153+
func serviceUnavailableMiddleware(endpoint, reason string, logger *slog.Logger) func(http.Handler) http.Handler {
145154
return func(next http.Handler) http.Handler {
146155
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
147-
if r.URL.Path != path {
156+
if !matchesAdminPath(r.URL.Path, endpoint) {
148157
next.ServeHTTP(w, r)
149158
return
150159
}

app/internal/admin/admin_test.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,33 @@ func TestInterceptorPassesThroughNonMatchingPath(t *testing.T) {
4545
}
4646
}
4747

48+
func TestInterceptorMatchesPathVariants(t *testing.T) {
49+
t.Parallel()
50+
for _, variant := range []string{
51+
testPath + "/",
52+
"/admin//validate",
53+
"/admin/./validate",
54+
"/admin/x/../validate",
55+
} {
56+
t.Run(variant, func(t *testing.T) {
57+
t.Parallel()
58+
next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
59+
t.Fatalf("path variant %q leaked past the admin interceptor", variant)
60+
})
61+
handler := NewValidateInterceptor(Options{Path: testPath, Validate: newOKValidator()})(next)
62+
63+
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("rules: []"))
64+
req.URL.Path = variant
65+
rec := newRecorder()
66+
handler.ServeHTTP(rec, req)
67+
68+
if rec.Code != http.StatusOK {
69+
t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK)
70+
}
71+
})
72+
}
73+
}
74+
4875
func TestInterceptorRejectsNonPOST(t *testing.T) {
4976
t.Parallel()
5077
handler := NewValidateInterceptor(Options{Path: testPath, Validate: newOKValidator()})(noopHandler())

app/internal/admin/policy_version.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ func NewPolicyVersionInterceptor(opts PolicyVersionOptions) func(http.Handler) h
120120
}
121121
return func(next http.Handler) http.Handler {
122122
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
123-
if r.URL.Path != opts.Path {
123+
if !matchesAdminPath(r.URL.Path, opts.Path) {
124124
next.ServeHTTP(w, r)
125125
return
126126
}

app/internal/cmd/serve.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -747,9 +747,25 @@ func withMetrics(registry *metrics.Registry) func(http.Handler) http.Handler {
747747
}
748748

749749
func withClientACL(cfg *config.Config, logger *slog.Logger) func(http.Handler) http.Handler {
750+
warnIfLabelACLEnabled(cfg, logger)
750751
return clientacl.Middleware(cfg.Upstream.Socket, logger, serveClientACLOptions(cfg))
751752
}
752753

754+
// warnIfLabelACLEnabled reminds operators that container-label ACLs are only
755+
// trustworthy when sockguard is the exclusive path to the Docker socket: a
756+
// workload that can reach the raw socket can create a container carrying
757+
// arbitrary <label_prefix>* permission labels and self-grant access the
758+
// policy never approved. Sockguard cannot detect other socket consumers, so
759+
// the invariant is stated at chain-build time rather than enforced.
760+
func warnIfLabelACLEnabled(cfg *config.Config, logger *slog.Logger) {
761+
if !cfg.Clients.ContainerLabels.Enabled {
762+
return
763+
}
764+
logger.Warn("container-label ACLs are enabled: label grants are only trustworthy if sockguard is the ONLY consumer of the Docker socket — any workload with raw socket access can self-grant permissions via labels",
765+
"label_prefix", cfg.Clients.ContainerLabels.LabelPrefix,
766+
)
767+
}
768+
753769
// withAdminClientACL applies ONLY the client CIDR allowlist to the dedicated
754770
// admin listener. The dedicated admin chain intentionally omits the full
755771
// clientacl middleware — container-label ACLs and per-profile selection are

app/internal/config/config.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -700,7 +700,13 @@ func Defaults() Config {
700700
RedactSensitiveData: true,
701701
},
702702
RequestBody: RequestBodyConfig{
703-
ContainerCreate: ContainerCreateRequestBodyConfig{},
703+
// Image trust defaults to requiring a Rekor inclusion proof for
704+
// keyless signatures (matching policy_bundle), so old/revoked
705+
// signatures cannot be replayed without a transparency-log entry.
706+
// Operators must opt out explicitly.
707+
ContainerCreate: ContainerCreateRequestBodyConfig{
708+
ImageTrust: ImageTrustConfig{RequireRekorInclusion: true},
709+
},
704710
ImagePull: ImagePullRequestBodyConfig{
705711
AllowOfficial: true,
706712
},
@@ -709,6 +715,7 @@ func Defaults() Config {
709715
},
710716
Service: ServiceRequestBodyConfig{
711717
AllowOfficial: true,
718+
ImageTrust: ImageTrustConfig{RequireRekorInclusion: true},
712719
},
713720
Plugin: PluginRequestBodyConfig{
714721
AllowOfficial: true,

app/internal/config/load.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,11 +158,8 @@ func setLoadDefaults(v *viper.Viper, defaults Config) {
158158
v.SetDefault("request_body.service.allow_all_capabilities", defaults.RequestBody.Service.AllowAllCapabilities)
159159
v.SetDefault("request_body.service.allowed_capabilities", defaults.RequestBody.Service.AllowedCapabilities)
160160
v.SetDefault("request_body.service.allow_sysctls", defaults.RequestBody.Service.AllowSysctls)
161-
// Image trust defaults to requiring a Rekor inclusion proof for keyless
162-
// signatures (matching policy_bundle), so old/revoked signatures cannot be
163-
// replayed without a transparency-log entry. Operators must opt out explicitly.
164-
v.SetDefault("request_body.container_create.image_trust.require_rekor_inclusion", true)
165-
v.SetDefault("request_body.service.image_trust.require_rekor_inclusion", true)
161+
v.SetDefault("request_body.container_create.image_trust.require_rekor_inclusion", defaults.RequestBody.ContainerCreate.ImageTrust.RequireRekorInclusion)
162+
v.SetDefault("request_body.service.image_trust.require_rekor_inclusion", defaults.RequestBody.Service.ImageTrust.RequireRekorInclusion)
166163
v.SetDefault("request_body.swarm.allow_force_new_cluster", defaults.RequestBody.Swarm.AllowForceNewCluster)
167164
v.SetDefault("request_body.swarm.allow_external_ca", defaults.RequestBody.Swarm.AllowExternalCA)
168165
v.SetDefault("request_body.swarm.allowed_join_remote_addrs", defaults.RequestBody.Swarm.AllowedJoinRemoteAddrs)
Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
package config
2+
3+
import (
4+
"strings"
5+
"testing"
6+
)
7+
8+
// TestValidateUpstreamRequestTimeout exercises the validateUpstream branches
9+
// for upstream.request_timeout that are not covered by existing tests.
10+
func TestValidateUpstreamRequestTimeout(t *testing.T) {
11+
cases := []struct {
12+
name string
13+
timeout string
14+
wantError bool
15+
wantMsg string
16+
}{
17+
{
18+
name: "invalid_with_extra_text",
19+
timeout: "5s extra",
20+
wantError: true,
21+
wantMsg: "upstream.request_timeout must be a positive duration",
22+
},
23+
{
24+
name: "negative_duration",
25+
timeout: "-1s",
26+
wantError: true,
27+
wantMsg: "upstream.request_timeout must be a positive duration",
28+
},
29+
{
30+
name: "zero_duration",
31+
timeout: "0s",
32+
wantError: true,
33+
wantMsg: "upstream.request_timeout must be a positive duration",
34+
},
35+
{
36+
name: "empty_is_skipped",
37+
timeout: "",
38+
wantError: false,
39+
},
40+
{
41+
name: "valid_positive_duration",
42+
timeout: "30s",
43+
wantError: false,
44+
},
45+
}
46+
47+
for _, tc := range cases {
48+
t.Run(tc.name, func(t *testing.T) {
49+
cfg := Defaults()
50+
cfg.Upstream.RequestTimeout = tc.timeout
51+
err := Validate(&cfg)
52+
if tc.wantError {
53+
if err == nil {
54+
t.Fatalf("Validate() = nil, want error containing %q", tc.wantMsg)
55+
}
56+
if !strings.Contains(err.Error(), tc.wantMsg) {
57+
t.Fatalf("Validate() = %v, want error containing %q", err, tc.wantMsg)
58+
}
59+
} else {
60+
// Only fail if the upstream.request_timeout field itself errors.
61+
if err != nil && strings.Contains(err.Error(), "upstream.request_timeout") {
62+
t.Fatalf("Validate() produced unexpected upstream.request_timeout error: %v", err)
63+
}
64+
}
65+
})
66+
}
67+
}
68+
69+
// TestValidateContainerCreateAllowedDeviceRequests exercises the per-entry
70+
// validation branches inside validateContainerCreateConfig for
71+
// allowed_device_requests, which were previously unreachable from existing tests.
72+
func TestValidateContainerCreateAllowedDeviceRequests(t *testing.T) {
73+
intPtr := func(v int) *int { return &v }
74+
75+
t.Run("empty_driver_rejected", func(t *testing.T) {
76+
cfg := Defaults()
77+
cfg.RequestBody.ContainerCreate.AllowedDeviceRequests = []AllowedDeviceRequest{
78+
{Driver: "", AllowedCapabilities: [][]string{{"gpu"}}, MaxCount: nil},
79+
}
80+
err := Validate(&cfg)
81+
if err == nil {
82+
t.Fatal("Validate() = nil, want error for empty driver")
83+
}
84+
if !strings.Contains(err.Error(), "request_body.container_create.allowed_device_requests[0].driver is required") {
85+
t.Fatalf("Validate() = %v, want driver-required error", err)
86+
}
87+
})
88+
89+
t.Run("whitespace_only_driver_rejected", func(t *testing.T) {
90+
cfg := Defaults()
91+
cfg.RequestBody.ContainerCreate.AllowedDeviceRequests = []AllowedDeviceRequest{
92+
{Driver: " ", AllowedCapabilities: [][]string{{"gpu"}}, MaxCount: nil},
93+
}
94+
err := Validate(&cfg)
95+
if err == nil {
96+
t.Fatal("Validate() = nil, want error for whitespace-only driver")
97+
}
98+
if !strings.Contains(err.Error(), "request_body.container_create.allowed_device_requests[0].driver is required") {
99+
t.Fatalf("Validate() = %v, want driver-required error", err)
100+
}
101+
})
102+
103+
t.Run("empty_capability_set_rejected", func(t *testing.T) {
104+
cfg := Defaults()
105+
cfg.RequestBody.ContainerCreate.AllowedDeviceRequests = []AllowedDeviceRequest{
106+
{
107+
Driver: "nvidia.com/gpu",
108+
// The outer slice has one entry; that entry is an empty capability set.
109+
AllowedCapabilities: [][]string{{}},
110+
MaxCount: nil,
111+
},
112+
}
113+
err := Validate(&cfg)
114+
if err == nil {
115+
t.Fatal("Validate() = nil, want error for empty capability set")
116+
}
117+
if !strings.Contains(err.Error(), "request_body.container_create.allowed_device_requests[0].allowed_capabilities[0] must be a non-empty capability set") {
118+
t.Fatalf("Validate() = %v, want non-empty capability set error", err)
119+
}
120+
})
121+
122+
t.Run("max_count_below_minus_one_rejected", func(t *testing.T) {
123+
cfg := Defaults()
124+
cfg.RequestBody.ContainerCreate.AllowedDeviceRequests = []AllowedDeviceRequest{
125+
{Driver: "nvidia.com/gpu", AllowedCapabilities: [][]string{{"compute"}}, MaxCount: intPtr(-2)},
126+
}
127+
err := Validate(&cfg)
128+
if err == nil {
129+
t.Fatal("Validate() = nil, want error for max_count < -1")
130+
}
131+
if !strings.Contains(err.Error(), "request_body.container_create.allowed_device_requests[0].max_count must be -1 or a non-negative integer") {
132+
t.Fatalf("Validate() = %v, want max_count error", err)
133+
}
134+
})
135+
136+
t.Run("valid_entry_max_count_minus_one", func(t *testing.T) {
137+
cfg := Defaults()
138+
cfg.RequestBody.ContainerCreate.AllowedDeviceRequests = []AllowedDeviceRequest{
139+
{Driver: "nvidia.com/gpu", AllowedCapabilities: [][]string{{"compute", "utility"}}, MaxCount: intPtr(-1)},
140+
}
141+
err := Validate(&cfg)
142+
if err != nil && strings.Contains(err.Error(), "allowed_device_requests") {
143+
t.Fatalf("Validate() produced unexpected allowed_device_requests error: %v", err)
144+
}
145+
})
146+
147+
t.Run("valid_entry_max_count_zero", func(t *testing.T) {
148+
cfg := Defaults()
149+
cfg.RequestBody.ContainerCreate.AllowedDeviceRequests = []AllowedDeviceRequest{
150+
{Driver: "nvidia.com/gpu", AllowedCapabilities: [][]string{{"compute"}}, MaxCount: intPtr(0)},
151+
}
152+
err := Validate(&cfg)
153+
if err != nil && strings.Contains(err.Error(), "allowed_device_requests") {
154+
t.Fatalf("Validate() produced unexpected allowed_device_requests error: %v", err)
155+
}
156+
})
157+
158+
t.Run("valid_entry_max_count_positive", func(t *testing.T) {
159+
cfg := Defaults()
160+
cfg.RequestBody.ContainerCreate.AllowedDeviceRequests = []AllowedDeviceRequest{
161+
{Driver: "nvidia.com/gpu", AllowedCapabilities: [][]string{{"compute"}}, MaxCount: intPtr(4)},
162+
}
163+
err := Validate(&cfg)
164+
if err != nil && strings.Contains(err.Error(), "allowed_device_requests") {
165+
t.Fatalf("Validate() produced unexpected allowed_device_requests error: %v", err)
166+
}
167+
})
168+
169+
t.Run("valid_entry_nil_max_count", func(t *testing.T) {
170+
cfg := Defaults()
171+
cfg.RequestBody.ContainerCreate.AllowedDeviceRequests = []AllowedDeviceRequest{
172+
{Driver: "nvidia.com/gpu", AllowedCapabilities: [][]string{{"compute", "utility"}, {"video"}}, MaxCount: nil},
173+
}
174+
err := Validate(&cfg)
175+
if err != nil && strings.Contains(err.Error(), "allowed_device_requests") {
176+
t.Fatalf("Validate() produced unexpected allowed_device_requests error: %v", err)
177+
}
178+
})
179+
}

0 commit comments

Comments
 (0)