Make Access login/token HTTP timeouts configurable - #1721
Open
alexmerm wants to merge 3 commits into
Open
Conversation
fetchMetadataJWT (7s) and isTokenValid (5s) hardcode independent http.Client timeouts on the access login/token path, with no flag or env var to raise them. Both were introduced without documented rationale (1d5cc45 / AUTH-2055, fcc393e / AUTH-3221) and have gone untouched since, while cloudflare#1637 and cloudflare#740 report exactly this failure mode on slow connections. Add a single --access-timeout flag (TUNNEL_ACCESS_TIMEOUT env var, default 7s) that governs both call sites. The two existing values look like independent arbitrary choices rather than distinct budgets for distinct request types, so one operator-facing knob replaces both instead of adding a second flag. Default matches today's larger value, so behavior is unchanged unless an operator opts in.
There was a problem hiding this comment.
Pull request overview
This PR makes the HTTP timeouts used by cloudflared access login / cloudflared access token configurable via a new --access-timeout flag (and TUNNEL_ACCESS_TIMEOUT env var), replacing previously hardcoded 5s and 7s timeouts in the Access token verification and app-metadata lookup flows.
Changes:
- Introduces
--access-timeout/TUNNEL_ACCESS_TIMEOUT(defaulting totoken.DefaultAccessTimeout= 7s) on theaccesscommand and threads it through relevant call paths. - Updates
token.GetAppInfo/fetchMetadataJWTto accept a timeout parameter rather than hardcoding the HTTP client timeout. - Extends
carrier.StartOptionswith aTimeoutfield and uses it in both token verification and websocket-based app info discovery; adds tests for flag/env precedence and timeout honoring.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
token/token.go |
Adds DefaultAccessTimeout and threads a configurable HTTP timeout into metadata JWT fetch + app info discovery. |
token/token_test.go |
Updates existing tests for new timeout parameter and adds coverage that the configured timeout is honored. |
cmd/cloudflared/access/cmd.go |
Registers --access-timeout flag/env var and passes it into token.GetAppInfo and token verification options. |
cmd/cloudflared/access/cmd_test.go |
Adds tests for default/env/flag precedence on the new timeout flag. |
cmd/cloudflared/access/carrier.go |
Passes the configured timeout into carrier.StartOptions for access ssh path. |
carrier/websocket.go |
Uses StartOptions.Timeout (with fallback) when calling token.GetAppInfo for Access-protected origins. |
carrier/carrier.go |
Adds Timeout time.Duration to carrier.StartOptions with documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+20
to
+28
| cmds := Commands() | ||
| require.Len(t, cmds, 1) | ||
| for _, f := range cmds[0].Flags { | ||
| if df, ok := f.(*cli.DurationFlag); ok && df.Name == accessTimeoutFlag { | ||
| return df | ||
| } | ||
| } | ||
| t.Fatal("access-timeout flag not registered on the access command") | ||
| return nil |
Comment on lines
+371
to
+373
| func GetAppInfo(reqURL *url.URL, timeout time.Duration) (*AppInfo, error) { | ||
| // Fetch the metadata JWT from the edge (no redirects followed). | ||
| rawJWT, err := fetchMetadataJWT(reqURL.String()) | ||
| rawJWT, err := fetchMetadataJWT(reqURL.String(), timeout) |
Author
There was a problem hiding this comment.
Fixed in 75213fc: fetchMetadataJWT now clamps timeout <= 0 to token.DefaultAccessTimeout at the top of the function, before constructing the http.Client. Since GetAppInfo passes timeout straight through to fetchMetadataJWT, this fixes all 4 call sites in cmd.go in one place. Added a regression test (TestFetchMetadataJWT_ZeroTimeoutFallsBackToDefault) that verifies a zero timeout falls back to the 7s default instead of disabling the timeout.
Comment on lines
+37
to
+40
| // Timeout is the HTTP timeout used for Access login/token requests, such as | ||
| // fetching app metadata from the edge and verifying a cached token against | ||
| // the origin. If zero, callers fall back to token.DefaultAccessTimeout. | ||
| Timeout time.Duration |
fetchMetadataJWT passed the configured timeout straight to http.Client.Timeout, where 0 disables timeouts entirely in net/http. A user setting --access-timeout=0 (or an invalid negative value) would hang indefinitely on login/curl/token paths, unlike isTokenValid and createWebsocketStream which already fall back to token.DefaultAccessTimeout for non-positive values. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…logic exchangeOrgToken still hardcoded a 7s HTTP timeout, ignoring the --access-timeout flag for the org-token-exchange step of Access login. Thread the resolved timeout through FetchToken/FetchTokenWithRedirect so it applies here too. Also extract the repeated `timeout <= 0 -> DefaultAccessTimeout` clamp (previously duplicated in token.go, cmd.go, and websocket.go) into a single token.ResolveAccessTimeout helper, since the duplication is how exchangeOrgToken was missed in the first place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two independent HTTP timeouts on the
access login/access tokenpath are hardcoded with no flag or env var to raise them, and fail hard on slow or high-latency connections:isTokenValid(cmd/cloudflared/access/cmd.go) uses a 5s timeout when checking a cached token against the origin.fetchMetadataJWT(token/token.go) uses a 7s timeout when fetching the Access application's metadata JWT from the Cloudflare edge.This is a real, reported problem, not a hypothetical one. #1637 hits
fetchMetadataJWT's timeout with the exact error:#740 hits
isTokenValid's timeout, reporting thataccess loginsucceeds but still prints a timeout error (Could not verify token). Neither issue has a maintainer response or a linked fix.I checked the git history for both hardcoded values before touching them:
1d5cc45a("AUTH-2055: Verifies token at edge on access login", Sep 2019).fcc393e2("AUTH-3221: Saves org token to disk and uses it to refresh the app token", Nov 2020).Both commit messages are one-line internal Jira references with no rationale, there's no code comment near either timeout explaining the duration, and neither has been touched since introduction. This isn't loosening a deliberately-tuned constraint — there doesn't appear to have been a documented one.
What changed
Added one new flag/env var pair, on the
accesscommand:--access-timeout(duration)TUNNEL_ACCESS_TIMEOUT(env var)This single knob governs both call sites. The 5s vs. 7s split doesn't look like two deliberately different budgets for two different kinds of request — it looks like two independently-chosen numbers that happened to land close together (see git history above). Adding a second flag would just be reintroducing that same unexplained asymmetry with more surface area. One operator-facing timeout for "Access login/token HTTP requests" is the right level of granularity here.
Threading:
token.fetchMetadataJWTandtoken.GetAppInfonow take atimeout time.Durationparameter instead of hardcoding7 * time.Second. All 4 callers incmd/cloudflared/access/cmd.go(login,curl,token,ssh-gen) now passc.Duration(accessTimeoutFlag).carrier.StartOptionsgained aTimeout time.Durationfield.verifyTokenAtEdge/isTokenValid(used byloginandcurl) now read it instead of hardcoding5 * time.Second, falling back totoken.DefaultAccessTimeoutif unset. Theaccess tcp/sshcommand sets it onStartOptionstoo, socarrier/websocket.go'stoken.GetAppInfocall (reached via the websocket/forwarder path) also gets a configured timeout, again falling back totoken.DefaultAccessTimeout(7s) when nothing is configured — this keeps the config-file-driven forwarder path (carrier.StartForwarder, which has no CLI flags at all) behaviorally unchanged.token.DefaultAccessTimeout = 7 * time.Secondas the single source of truth for the default/fallback value.No behavior change for anyone who doesn't pass the new flag/env var — the default preserves today's ceiling (7s) exactly.
Test plan
go build ./...go vet ./...gofmt -lclean on all touched filesgo test ./token/... ./cmd/cloudflared/access/...go test ./carrier/...(touchedcarrier.StartOptions)New tests added:
token.TestFetchMetadataJWT_HonorsConfiguredTimeout— server sleeps 500ms, request configured with a 100ms timeout, asserts the call fails in well under the old hardcoded 7s (i.e. the configured timeout actually fires).token.TestFetchMetadataJWT_DefaultTimeoutPreservesCurrentBehavior— assertsDefaultAccessTimeout == 7sand that a fast response still succeeds under the default.cmd/cloudflared/access.TestAccessTimeoutFlag_DefaultPreservesCurrentBehavior/TestAccessTimeoutFlag_EnvVarOverridesDefault/TestAccessTimeoutFlag_FlagOverridesEnvVar— precedence tests against the actual flag registered on theaccesscommand (flag > env var > default).Also updated existing
tokenpackage tests (TestGetAppInfo_*,TestFetchMetadataJWT_ReturnsAppInfoErrorOnError) to passDefaultAccessTimeoutfor the new required parameter; no behavioral change to those tests.Scope note: this PR is config-threading only — no unrelated refactors in either file. CLI help text (
--access-timeoutusage string) is the only docs change; cloudflared's docs site lives in a separatecloudflare-docsrepo and is out of scope here.Generated by Claude Code
Note on default-timeout behavior change
Correction to the "No behavior change" claim above: for
isTokenValidspecifically, the default ceiling actually rises from 5s → 7s for anyone who doesn't pass--access-timeout/TUNNEL_ACCESS_TIMEOUT, since both call sites now sharetoken.DefaultAccessTimeout(7s) as their fallback rather than each hardcoding its own value. This is intentional — see the "What changed" rationale above (the 5s/7s split looks like two independently-chosen numbers, not a deliberately tuned budget) — but it is a real behavior change worth flagging explicitly for review, not just an implementation detail.Context
Small footnote: this PR happened because I was stuck on a plane on the tarmac for about 5 hours with terrible in-flight wifi, and slow/flaky connections timing out on
access loginwas very much a lived problem at the time.