From 23d1a85c396e1e9b15a78c81201caab290d9cb71 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Thu, 20 Aug 2026 04:50:03 +0000 Subject: [PATCH 1/4] fix: synchronize remotestorage SSRF test-seam swap with atomic pointers Test helpers substitute permissive SSRF checks for the duration of a test, then restore the originals in t.Cleanup. Production dial logic reads the active check from inside net.Dialer.Control, which net/http.Transport can invoke on a background goroutine that outlives the synchronous test call that spawned it, so the previous unsynchronized swap/restore raced against that goroutine's read under go test -race. Back the swap with atomic.Pointer instead of plain vars, and consolidate the two previously-duplicated swap/restore implementations onto one shared helper. No production behavior, defaults, or exported signatures change. --- backend/internal/services/remotestorage/s3.go | 5 +- .../remotestorage/sftp_discovery_test.go | 12 ++-- .../internal/services/remotestorage/ssrf.go | 67 +++++++++++++++---- .../services/remotestorage/ssrf_test.go | 59 ++++++++++++++++ .../internal/services/remotestorage/webdav.go | 4 +- 5 files changed, 123 insertions(+), 24 deletions(-) diff --git a/backend/internal/services/remotestorage/s3.go b/backend/internal/services/remotestorage/s3.go index 1224c819d..ff01d0780 100644 --- a/backend/internal/services/remotestorage/s3.go +++ b/backend/internal/services/remotestorage/s3.go @@ -60,8 +60,9 @@ func newS3Uploader(cfg S3Config, secrets S3Secrets) (Uploader, error) { if h, _, err := net.SplitHostPort(host); err == nil { host = h } - // ssrfValidateHost defaults to ValidateHostSSRF; see ssrf.go for why it - // is indirected through a var. + // ssrfValidateHost defaults to ValidateHostSSRF; see ssrf.go — it is a + // func backed by an atomic function pointer so tests can substitute it + // without racing. if err := ssrfValidateHost(host); err != nil { return nil, fmt.Errorf("s3 endpoint failed SSRF validation: %w", err) } diff --git a/backend/internal/services/remotestorage/sftp_discovery_test.go b/backend/internal/services/remotestorage/sftp_discovery_test.go index 5a40df57e..8faa55301 100644 --- a/backend/internal/services/remotestorage/sftp_discovery_test.go +++ b/backend/internal/services/remotestorage/sftp_discovery_test.go @@ -84,13 +84,11 @@ func startFakeSSHServer(t *testing.T) (addr string, authAttempted func() bool) { // production defaults on cleanup. Production code never touches these vars. func withPermissiveSSRFForLocalTest(t *testing.T) { t.Helper() - origHost, origDial := ssrfValidateHost, ssrfValidateDialAddress - ssrfValidateHost = func(string) error { return nil } - ssrfValidateDialAddress = func(net.IP) error { return nil } - t.Cleanup(func() { - ssrfValidateHost = origHost - ssrfValidateDialAddress = origDial - }) + restore := swapSSRFValidators( + func(string) error { return nil }, + func(net.IP) error { return nil }, + ) + t.Cleanup(restore) } // TestDiscoverSFTPHostKey_NeverAuthenticates is required test #8 from the diff --git a/backend/internal/services/remotestorage/ssrf.go b/backend/internal/services/remotestorage/ssrf.go index 70cae56a5..ffa96c09f 100644 --- a/backend/internal/services/remotestorage/ssrf.go +++ b/backend/internal/services/remotestorage/ssrf.go @@ -4,24 +4,50 @@ import ( "context" "fmt" "net" + "sync/atomic" "syscall" "time" "github.com/Wikid82/charon/backend/internal/network" ) -// ssrfValidateHost / ssrfValidateDialAddress are indirected through -// package-level vars (rather than called directly) purely so white-box -// tests in this package can substitute a permissive check when exercising -// dial logic against a local test fixture (e.g. the SFTP host-key discovery +// ssrfValidateHost / ssrfValidateDialAddress are backed by atomic function +// pointers (rather than plain package-level vars) purely so white-box tests +// in this package can substitute a permissive check when exercising dial +// logic against a local test fixture (e.g. the SFTP host-key discovery // test, which must dial 127.0.0.1) without weakening the default policy -// every production code path in this file uses. Tests restore the original -// via t.Cleanup; production code never reassigns these. +// every production code path in this file uses, AND without racing +// concurrent readers: safeDialer's net.Dialer.Control hook (below) can run +// on a background net/http.Transport dial goroutine that outlives the +// synchronous test call that spawned it, so a plain unsynchronized var +// swap/restore in t.Cleanup is a genuine data race (caught by +// `go test -race`) against that goroutine's read. Tests restore the +// original via t.Cleanup; production code never reassigns these. var ( - ssrfValidateHost = ValidateHostSSRF - ssrfValidateDialAddress = validateIPSSRF + ssrfValidateHostFn atomic.Pointer[func(string) error] + ssrfValidateDialAddressFn atomic.Pointer[func(net.IP) error] ) +func init() { + defaultHost := ValidateHostSSRF + ssrfValidateHostFn.Store(&defaultHost) + defaultDial := validateIPSSRF + ssrfValidateDialAddressFn.Store(&defaultDial) +} + +// ssrfValidateHost calls the currently-active host-validation func (the +// production default, or a test override installed via +// swapSSRFValidators/withPermissiveSSRFForLocalTest/WithPermissiveSSRFForTesting). +func ssrfValidateHost(host string) error { + return (*ssrfValidateHostFn.Load())(host) +} + +// ssrfValidateDialAddress calls the currently-active dial-address-validation +// func. See ssrfValidateHost. +func ssrfValidateDialAddress(ip net.IP) error { + return (*ssrfValidateDialAddressFn.Load())(ip) +} + // ValidateHostSSRF resolves host and rejects it unless every resolved IP is // permitted by spec §3.7's rules: RFC1918 private ranges are allowed (the // primary use case is a self-hosted NAS on the operator's own LAN), but @@ -97,11 +123,26 @@ func dialContext(ctx context.Context, dialNetwork, address string, timeout time. // keep using that helper instead; this exported wrapper is for cross-package // test use only. Production code never calls this. func WithPermissiveSSRFForTesting() (restore func()) { - origHost, origDial := ssrfValidateHost, ssrfValidateDialAddress - ssrfValidateHost = func(string) error { return nil } - ssrfValidateDialAddress = func(net.IP) error { return nil } + return swapSSRFValidators( + func(string) error { return nil }, + func(net.IP) error { return nil }, + ) +} + +// swapSSRFValidators atomically replaces both the host- and +// dial-address-validation funcs and returns a restore func that atomically +// puts back whatever was active before the swap (not necessarily the +// production default — swaps may nest across helpers). Both +// withPermissiveSSRFForLocalTest (in-package tests) and +// WithPermissiveSSRFForTesting (cross-package tests) are thin wrappers +// around this so the swap-and-restore logic exists in exactly one place. +func swapSSRFValidators(host func(string) error, dial func(net.IP) error) (restore func()) { + origHost := ssrfValidateHostFn.Load() + origDial := ssrfValidateDialAddressFn.Load() + ssrfValidateHostFn.Store(&host) + ssrfValidateDialAddressFn.Store(&dial) return func() { - ssrfValidateHost = origHost - ssrfValidateDialAddress = origDial + ssrfValidateHostFn.Store(origHost) + ssrfValidateDialAddressFn.Store(origDial) } } diff --git a/backend/internal/services/remotestorage/ssrf_test.go b/backend/internal/services/remotestorage/ssrf_test.go index cc0941b62..5dbfac1ac 100644 --- a/backend/internal/services/remotestorage/ssrf_test.go +++ b/backend/internal/services/remotestorage/ssrf_test.go @@ -3,6 +3,7 @@ package remotestorage import ( "context" "net" + "sync" "testing" "time" @@ -95,3 +96,61 @@ func TestSafeDialer_RejectsLoopbackAtDialTime(t *testing.T) { require.Error(t, dialErr, "dial-time SSRF check must reject a loopback destination") assert.Nil(t, conn) } + +// TestSwapSSRFValidators_ConcurrentAccess_NoRace is a deterministic +// regression guard for the data race fixed by backing ssrfValidateHost / +// ssrfValidateDialAddress with atomic.Pointer rather than plain package-level +// vars: one goroutine repeatedly reads via ssrfValidateHost/ +// ssrfValidateDialAddress while another concurrently swaps and restores via +// swapSSRFValidators, mirroring the swap/restore pattern +// withPermissiveSSRFForLocalTest and WithPermissiveSSRFForTesting use in +// production tests. The only pass/fail signal is whether `go test -race` +// reports a DATA RACE; no other assertion is meaningful here since either +// the production default or a swapped-in permissive result is a valid +// outcome on any given iteration. +func TestSwapSSRFValidators_ConcurrentAccess_NoRace(t *testing.T) { + const iterations = 1000 + + permissiveHost := func(string) error { return nil } + permissiveDial := func(net.IP) error { return nil } + + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + for i := 0; i < iterations; i++ { + _ = ssrfValidateHost("127.0.0.1") + _ = ssrfValidateDialAddress(net.ParseIP("127.0.0.1")) + } + }() + + go func() { + defer wg.Done() + for i := 0; i < iterations; i++ { + restore := swapSSRFValidators(permissiveHost, permissiveDial) + restore() + } + }() + + wg.Wait() +} + +// TestWithPermissiveSSRFForTesting_SwapsAndRestores proves the exported +// cross-package test seam (used by backup_remote_service_regression_test.go +// in the sibling `services` package) actually swaps both validators to +// permissive no-ops and restores the production defaults afterward. The +// cross-package caller already exercises this behaviorally, but this +// in-package test gives it direct, same-package coverage as well. +func TestWithPermissiveSSRFForTesting_SwapsAndRestores(t *testing.T) { + require.Error(t, ssrfValidateHost("127.0.0.1"), "production default must reject loopback before swap") + require.Error(t, ssrfValidateDialAddress(net.ParseIP("127.0.0.1")), "production default must reject loopback before swap") + + restore := WithPermissiveSSRFForTesting() + assert.NoError(t, ssrfValidateHost("127.0.0.1"), "swapped-in host validator must be permissive") + assert.NoError(t, ssrfValidateDialAddress(net.ParseIP("127.0.0.1")), "swapped-in dial validator must be permissive") + + restore() + assert.Error(t, ssrfValidateHost("127.0.0.1"), "restore must reinstate the production default") + assert.Error(t, ssrfValidateDialAddress(net.ParseIP("127.0.0.1")), "restore must reinstate the production default") +} diff --git a/backend/internal/services/remotestorage/webdav.go b/backend/internal/services/remotestorage/webdav.go index c0d8b8156..54717592a 100644 --- a/backend/internal/services/remotestorage/webdav.go +++ b/backend/internal/services/remotestorage/webdav.go @@ -47,8 +47,8 @@ type webdavUploader struct { } // newWebDAVUploader constructs the Uploader for a webdav-type target. The -// host is SSRF-validated at construction time via the same indirected -// ssrfValidateHost var s3.go/sftp.go already use (spec §3.5 — "reuse it, +// host is SSRF-validated at construction time via the same atomic-backed +// ssrfValidateHost func s3.go/sftp.go already use (spec §3.5 — "reuse it, // don't reinvent"), and every subsequent request dials through the // SSRF-safe dialContext (defeating DNS-rebinding TOCTOU, spec §3.8) // exactly like s3.go's custom http.Transport. From 7f62ad1de431522c1952c8873eab2bdacb21a86e Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Thu, 20 Aug 2026 10:16:16 +0000 Subject: [PATCH 2/4] chore(ci): bump golangci-lint action version to v2.13.0 --- .github/workflows/quality-checks.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/quality-checks.yml b/.github/workflows/quality-checks.yml index f808676dc..ff48a4e1b 100644 --- a/.github/workflows/quality-checks.yml +++ b/.github/workflows/quality-checks.yml @@ -223,7 +223,7 @@ jobs: uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0 with: # renovate: datasource=github-releases depName=golangci/golangci-lint - version: v2.12.2 + version: v2.13.0 working-directory: backend args: --timeout=5m continue-on-error: true @@ -309,7 +309,7 @@ jobs: uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0 with: # renovate: datasource=github-releases depName=golangci/golangci-lint - version: v2.12.2 + version: v2.13.0 working-directory: agent args: --config ../.golangci-fast.yml --timeout=5m From 49ab6123f05ec3eed4c309f7ee6c72d2754cc547 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Thu, 20 Aug 2026 10:24:43 +0000 Subject: [PATCH 3/4] ci: bump docker/setup-buildx-action to v4.3.0 --- .github/workflows/docker-build.yml | 6 +++--- .github/workflows/orthrus-build.yml | 2 +- .github/workflows/security-weekly-rebuild.yml | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/docker-build.yml b/.github/workflows/docker-build.yml index 269447c99..ac365311c 100644 --- a/.github/workflows/docker-build.yml +++ b/.github/workflows/docker-build.yml @@ -417,7 +417,7 @@ jobs: echo "IMAGE_NAME=${IMAGE_NAME}" >> "$GITHUB_ENV" - name: Set up Docker Buildx - uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4.2.0 + uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0 - name: Log in to GitHub Container Registry uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 @@ -503,7 +503,7 @@ jobs: - name: Set up QEMU uses: docker/setup-qemu-action@96fe6ef7f33517b61c61be40b68a1882f3264fb8 # v4.2.0 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4.2.0 + uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0 - name: Log in to GitHub Container Registry uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 @@ -603,7 +603,7 @@ jobs: echo "IMAGE_NAME=${IMAGE_NAME}" >> "$GITHUB_ENV" - name: Set up Docker Buildx - uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4.2.0 + uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0 - name: Log in to GitHub Container Registry uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 diff --git a/.github/workflows/orthrus-build.yml b/.github/workflows/orthrus-build.yml index a2de7f187..82536817b 100644 --- a/.github/workflows/orthrus-build.yml +++ b/.github/workflows/orthrus-build.yml @@ -122,7 +122,7 @@ jobs: uses: docker/setup-qemu-action@96fe6ef7f33517b61c61be40b68a1882f3264fb8 # v4.2.0 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4.2.0 + uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0 - name: Log in to Docker Hub if: env.HAS_DOCKERHUB_TOKEN == 'true' diff --git a/.github/workflows/security-weekly-rebuild.yml b/.github/workflows/security-weekly-rebuild.yml index 84aa89b1a..ec8001595 100644 --- a/.github/workflows/security-weekly-rebuild.yml +++ b/.github/workflows/security-weekly-rebuild.yml @@ -56,7 +56,7 @@ jobs: uses: docker/setup-qemu-action@96fe6ef7f33517b61c61be40b68a1882f3264fb8 # v4.2.0 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4.2.0 + uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0 - name: Resolve Debian base image digest id: base-image From 3e0a9e8a82dccc19cf74af2a0a2f4653e8d71c1f Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Thu, 20 Aug 2026 10:28:30 +0000 Subject: [PATCH 4/4] ci: pin docker/setup-buildx-action to correct SHA digest --- .github/workflows/e2e-tests-split.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/e2e-tests-split.yml b/.github/workflows/e2e-tests-split.yml index a7739544a..e7953f281 100644 --- a/.github/workflows/e2e-tests-split.yml +++ b/.github/workflows/e2e-tests-split.yml @@ -207,7 +207,7 @@ jobs: - name: Set up Docker Buildx if: steps.resolve-image.outputs.image_source == 'build' - uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4 + uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4 - name: Build Docker image id: build-image