Skip to content

feat(netproxy): refresh rotating proxy credentials (407 -> refresh -> retry once) - #51

Merged
TeoSlayer merged 2 commits into
mainfrom
feat/netproxy-credential-refresh
Sep 23, 2026
Merged

TeoSlayer merged 2 commits into
mainfrom
feat/netproxy-credential-refresh

Conversation

@TeoSlayer

Copy link
Copy Markdown
Contributor

Why

Meta Muse's egress proxy rotates the credentials embedded in HTTPS_PROXY every few minutes. A fresh shell sees the current value (bash -c 'printf %s "$https_proxy"'); a long-running process keeps its launch-time copy and gets 407 on every new CONNECT while its existing tunnels stay up — "node online, all apps broken" (see https://pilotprotocol.network/blog/rotating-egress-proxy-credentials). netproxy.FromEnvironment() snapshotted the env once, so a daemon (whose registry client redials constantly) broke within minutes.

This PR adds credential refresh inside netproxy only, so registry/client (via Dialer) and HTTP callers (via ProxyForRequest / RefreshingTransport) get it without changes.

What

Resolver: refreshable settings

  • Settings now live in an atomically swapped snapshot. A ModeAuto Resolver re-reads the environment (os.Getenv) on refresh, so processes whose env is updated in place follow it. Without a command, behaviour is otherwise unchanged.
  • Refresh source: WithRefreshCommand(cmd) runs sh -c cmd (10 s timeout, stdin/stderr discarded, WaitDelay so a stray child cannot hang it, output capped at 64 KiB). Stdout, trimmed, must be one http(s):// proxy URL. The output is never logged or put in errors. WithRefreshFunc(fn) is the Go-function form.
  • EnvRefreshCommand = "PILOT_PROXY_CMD" documents the convention; netproxy never reads it on its own. web4 wires it:
    r, err := netproxy.NewResolver(spec,
        netproxy.WithRefreshCommand(os.Getenv(netproxy.EnvRefreshCommand)),
        netproxy.WithRefreshErrorHandler(func(err error) { slog.Warn("proxy credential refresh failed", "err", err) }))
    (Muse installer: PILOT_PROXY_CMD='bash -c '\''printf %s "$https_proxy"'\'''.)
  • A timed refresh runs when a lookup happens after DefaultRefreshInterval (60 s, WithRefreshInterval). Refresh(ctx) forces one.
  • Singleflight: concurrent callers share one refresh. A failed refresh keeps the last good settings. WithRefreshErrorHandler hears about it once per run of consecutive failures.
  • The command's URL replaces the explicit URL, or in auto mode the environment's proxy URLs. NO_PROXY and the loopback exemption still apply.
  • Additive API only: NewResolver(spec, opts...), Option, WithRefreshCommand, WithRefreshFunc, WithRefreshInterval, WithRefreshErrorHandler, Resolver.Refresh, RefreshingTransport, EnvRefreshCommand, DefaultRefreshInterval. Parse, Explicit, FromEnvironment and Off keep their signatures.

Dialer: 407 → refresh → retry once

  • On a 407, the Dialer force-refreshes and retries the CONNECT once on a fresh connection, and only if the refreshed URL differs. Open tunnels are never touched.
  • An attempt counter means a refresh that already ran after this dial picked its proxy is not repeated. Identical credentials therefore never loop, and 50 concurrent 407s cause exactly one command run.
  • A CONNECT response http.ReadResponse cannot parse is also treated as a possible rejection. Muse reported the 407 surfacing as malformed HTTP status code. The refresh-and-compare rule still bounds this to one refresh and at most one retry.

RefreshingTransport(base *http.Transport, r *Resolver) http.RoundTripper

  • Verified empirically (pinned in TestNetHTTPReportsRefusedCONNECTWithoutStatusCode): for a refused CONNECT, net/http returns a bare errors.errorString holding only the proxy's reason phrase:

    • Proxy Authentication Required;
    • unknown status code when there is no phrase;
    • any proxy-supplied text, which can reflect secrets;
    • malformed HTTP status code "407Proxy" for an unparseable line.

    There is no status code in any of these, so matching on the text is unreliable.

  • The transport therefore reads the status from a chained OnProxyConnectResponse (on a clone of base, with Proxy = r.ProxyForRequest). It turns every non-200 into a *ConnectError, whose message never includes proxy text.

  • On 407 it refreshes, then retries once for:

    • GET, HEAD, OPTIONS and TRACE (or an Idempotency-Key header);
    • any request with GetBody, since the proxy refused the tunnel and the server never saw the first attempt.

    A POST without GetBody is not retried, but the refresh still happens, so the next request works. The malformed case retries idempotent methods only.

  • Implements CloseIdleConnections.

Tests (GOWORK=off go test -race ./... -count=1 green; vet, gofmt, staticcheck and gitleaks clean; netproxy coverage 95.3%; refresh/transport tests stable over -count=15 -race)

The tests use an in-process CONNECT proxy whose accepted password rotates, plus a refresh command (sh -c) that logs its runs:

  • a long-running dialer stays working across 4 rotations: one 407, one command run and one retry per rotation, and every earlier tunnel still echoes;
  • 407 → refresh → success, including for auto mode re-reading the env, real os env via FromEnvironment, and a retry that goes direct when the refreshed NO_PROXY exempts the target;
  • unchanged credentials → no retry, and at most one command run per failed dial;
  • command failure → last good credentials kept; the handler is called once per failure streak; the dial error carries 407 … (proxy credential refresh failed: … exit status 3); the command recovers afterwards;
  • 50 goroutines hitting a rotation → exactly one refresh command run (50/50 were rejected and shared it);
  • command timeout; output, stderr and misparsed values are never leaked; option semantics; TTL; negative interval;
  • RefreshingTransport:
    • GET is retried, and a pooled tunnel survives the rotation;
    • POST without GetBody is not retried, while a POST with GetBody is retried and the server sees it once;
    • errors are typed and redacted;
    • a malformed rejection is handled;
    • the base hook is chained.

Follow-ups (not in this PR)

  • web4: wire PILOT_PROXY_CMD into NewResolver. Wrap daemon HTTP clients and DefaultTransport in RefreshingTransport where the loopback exemption allows. proxyconf.RequestProxy already benefits from the timed refresh.
  • pilot-sandbox / pilot-mcp: see ZIP_FINDINGS phase-3 notes.

🤖 Generated with Claude Code

… retry once)

Some egress proxies (Meta Muse) rotate the credentials embedded in
HTTPS_PROXY every few minutes. A long-running process keeps its
launch-time copy and gets 407 on every new CONNECT while its open tunnels
stay up ("node online, all apps broken"). netproxy snapshotted the
environment once, so a daemon broke within minutes.

Resolver
- Settings live in an atomically swapped state; ModeAuto re-reads the
  environment, and a refresh source (WithRefreshCommand: "sh -c", 10 s
  timeout, output and stderr never logged; or WithRefreshFunc) supplies
  the current proxy URL. EnvRefreshCommand documents PILOT_PROXY_CMD as
  the convention callers wire.
- Timed refresh on lookup (DefaultRefreshInterval 60 s,
  WithRefreshInterval), Refresh(ctx) to force one, singleflight so
  concurrent callers share one run, last good settings kept on failure,
  WithRefreshErrorHandler told once per run of failures.
- NewResolver(spec, opts...) is Parse with options; existing
  constructors keep their signatures and behaviour (auto additionally
  follows in-place environment changes).

Dialer
- On 407 (or an unparseable CONNECT response, how Muse's rejections
  surfaced) refresh and retry once on a new connection, only if the
  refreshed URL differs. A refresh that already ran after the pick is not
  repeated, so identical credentials never loop.

RefreshingTransport(base, r)
- Clone of base with Proxy = r.ProxyForRequest and a chained
  OnProxyConnectResponse that turns non-200 CONNECT answers into
  *ConnectError. Verified empirically: net/http reports a refused CONNECT
  only as the proxy's reason phrase ("Proxy Authentication Required",
  "unknown status code", or arbitrary proxy text) with no status code.
- Retries once for GET/HEAD/OPTIONS/TRACE (or Idempotency-Key) and, after
  a 407, any request with GetBody; a POST without GetBody is not retried
  but the refresh still happens for the next request.

Tests use an in-process CONNECT proxy whose accepted password rotates and
a refresh command that counts its runs: long-running dialer across 4
rotations with earlier tunnels still echoing; 407 -> refresh -> success;
unchanged credentials -> no retry; failing command -> last good kept and
error reported once; 50 concurrent dials hitting a rotation -> exactly one
command run; timeout; output never leaked; GET retried, POST without
GetBody not retried.

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

codecov Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.77389% with 31 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
netproxy/refresh.go 91.91% 12 Missing and 4 partials ⚠️
netproxy/transport.go 92.70% 5 Missing and 2 partials ⚠️
netproxy/dialer.go 89.47% 2 Missing and 2 partials ⚠️
netproxy/refresh_unix.go 60.00% 2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

- F1: a timed refresh no longer blocks the lookup that notices it is due.
  Resolver.current starts the refresh in the background and answers from
  the settings in hand (the last good credentials), so a slow or hung
  refresh command cannot fail dials whose budget (e.g. the registry
  client's 5 s) is shorter than the 10 s refresh timeout. Only a rejected
  credential (reauth after a 407) waits for a refresh.
- F2: on Unix the refresh command runs in its own process group; a
  timeout SIGKILLs the whole group (exec.Cmd.Cancel), and so does a
  command that exits leaving a child holding its stdout, so hung
  children no longer pile up across refreshes.
- F3: RefreshingTransport treats an unparseable response as a proxy
  rejection only while the tunnel is being set up (no GotConn since the
  last GetConn, via httptrace). A server sending a malformed response
  through the tunnel gets net/http's error untouched and never triggers
  the refresh command. The proxy URL a refused CONNECT used is captured
  from the Proxy call itself.
- F4: net/http keys its pool by proxy URL including credentials, so once
  a refresh changes the proxy settings, the first request afterwards
  closes the stranded idle connections. A refresh that reads the same
  settings again leaves the pool alone. Doc corrected.
- F5: an unparseable CONNECT response is reported by what was wrong with
  it ("malformed HTTP status code (response text withheld)"), never by
  its text, from both Dialer and RefreshingTransport; the Dialer keeps
  wrapping only genuine read errors (EOF, net.Error).

Each fix has a regression test that fails on the previous commit.

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

Copy link
Copy Markdown
Contributor Author

Review fixes (70607b9)

All five findings were confirmed and are fixed. Each one has a regression test, and each test fails on d6417bc.

# Fix Test
F1 A timed refresh now runs in the background. Resolver.current starts it and answers from the settings it already has, which are the last good credentials. Only reauth (after a 407) waits for a refresh. With a hung command, dials under a 5 s budget no longer fail while the credentials are still valid. TestTimedRefreshNeverBlocksALookup
F2 On Unix the refresh command runs in its own process group (Setpgid). On timeout, exec.Cmd.Cancel SIGKILLs the whole group. The group is also killed when sh exits but leaves a child holding stdout (ErrWaitDelay). Refreshes that hang no longer pile up processes. TestRefreshCommandKillsWhatItStarted
F3 An unparseable response counts as a proxy rejection only while the tunnel is being set up. httptrace shows this as no GotConn since the last GetConn. A malformed response from the server, arriving through the tunnel, is returned exactly as net/http reports it and never runs the refresh command. The proxy URL a refused CONNECT used is now taken from the Proxy call itself. TestRefreshingTransportIgnoresMalformedServerResponses
F4 net/http keys pooled connections by proxy URL, credentials included. So when a refresh changes the proxy settings, the next request closes the idle connections left behind. A refresh that returns the same settings keeps the pool. The doc is corrected. TestRefreshingTransportDropsTunnelsStrandedByARotation
F5 When a CONNECT response can't be parsed, the error names only what was wrong, e.g. read CONNECT response: malformed HTTP status code (response text withheld). This holds for both Dialer and RefreshingTransport, and covers malformed status lines, MIME headers, Content-Length and Transfer-Encoding. The Dialer still wraps genuine read errors (EOF, net.Error) with %w. TestUnparseableRejectionNeverQuotesTheProxy

Behaviour changes to note:

  • A lookup never waits for a timed refresh. The first lookup after the interval uses the previous settings.
  • The Dialer now treats every unparseable CONNECT response as a possible credential rejection, not only the malformed … ones. It still refreshes at most once and retries only if the credentials changed.

Checks run locally:

  • GOWORK=off go test -race ./... -count=1: all passing.
  • netproxy: -race -count=8: passing.
  • go vet ./...: clean.
  • gofmt: clean on every file this PR touches.
  • GOOS=windows go build ./netproxy: builds.
  • The reviewer's repro file (zz_review_test.go, including the stress test under -race) now shows each finding fixed:
    • dial 2 succeeds in 1 ms;
    • 0 leftover processes;
    • 0 refresh runs from server responses;
    • 1 server connection left open instead of 6;
    • the credential text is withheld.

🤖 Generated with Claude Code

@TeoSlayer
TeoSlayer merged commit 0f00b3d into main Sep 23, 2026
11 checks passed
@TeoSlayer
TeoSlayer deleted the feat/netproxy-credential-refresh branch September 23, 2026 23:56
TeoSlayer added a commit to pilot-protocol/pilotprotocol that referenced this pull request Sep 24, 2026
common v0.5.15 (pilot-protocol/common#51, #52): idempotent registry
Client.Close (fixes the 'close of closed channel' panic in every nightly
since 07-26), pool reconnects keep their cause, log at Debug with a
periodic summary, back off with jitter and retry on a healthy conn
(113,804 'reconnected' INFO lines were 57% of one daemon.log), and the
proxy credential refresh used by the upcoming Muse support.

forceReconnectRegistry now declines once Stop has begun and closes the
conn it installed if Stop won the race, so a heartbeat or rx-watchdog
reconnect during shutdown no longer leaks a registry pool.

Co-authored-by: Teodor Calin <teodor@vulturelabs.io>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants