Skip to content

auth fixes - #928

Merged
plorenz merged 7 commits into
mainfrom
auth-fixes
May 28, 2026
Merged

auth fixes#928
plorenz merged 7 commits into
mainfrom
auth-fixes

Conversation

@plorenz

@plorenz plorenz commented Apr 29, 2026

Copy link
Copy Markdown
Member

@plorenz
plorenz requested a review from a team as a code owner April 29, 2026 16:03
Comment thread ziti/ziti.go Outdated

if !context.lastAuthAttempt.IsZero() && context.currentWait > 0 {
if remaining := context.currentWait - time.Since(context.lastAuthAttempt); remaining > 0 {
logrus.Debugf("auth backoff: %v remaining, returning cached error", remaining)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no pfxlog?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@andrewpmartinez

Copy link
Copy Markdown
Member

Sent some private comments.

Comment thread ziti/ziti.go
// re-auth is required.
const refreshRaceWindow = 15 * time.Second

func recentlyAuthenticated(context *ContextImpl) bool {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should have some godoc here considering how complex this logic is.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the doc is above, on refreshRaceWindow

@plorenz

plorenz commented May 5, 2026

Copy link
Copy Markdown
Member Author

Review comments addressed

@plorenz
plorenz requested a review from andrewpmartinez May 5, 2026 19:19
@plorenz
plorenz force-pushed the auth-fixes branch 2 times, most recently from 459307a to 3e05ce6 Compare May 6, 2026 22:44
plorenz added 6 commits May 28, 2026 09:55
- passes the current API session as the authorization argument so the
  request is authenticated, matching the rest of the CtrlClient methods.
  GetService was previously calling ListServices(params, nil), which
  produced an unauthenticated request and could surface as an unexpected
  401 on a path that should already be authenticated.
…#924

- adds Options.HttpTimeout, plumbed through ApiClientConfig and
  ComponentsConfig to NewHttpClient.
- raises the default HTTP client timeout from 10s to 30s, since OIDC
  token-exchange round trips on slow controllers were exceeding 10s and
  surfacing as spurious failures that drove the SDK into re-auth retries.
- exposes DefaultHttpTimeout for callers that want the new default.
…ug. Fixes #925

- expands errorIndicatesControllerSwap to recognize HTTP timeouts,
  context cancellation, EOF / ErrUnexpectedEOF, 5xx responses (via
  runtime.ClientResponseStatus), and url.Error timeouts so the SDK
  rotates to a different controller endpoint on transient transport
  failures instead of hammering an unhealthy one.
- moves the errors.As targets (opError, urlError) from package-level
  vars into local declarations. errors.As mutates the target, so the
  shared instances were racing across concurrent callers.
- replaces the fixed expiresAt - 10s schedule with tokenRefreshTime, which
  picks a random point between 1/2 and 5/6 of the token's remaining
  lifetime. The earlier window leaves headroom for slow refreshes and the
  jitter avoids thundering-herd token exchanges across SDK clients.
- skips the periodic service-list refresh when a token refresh is due in
  under 15 seconds, so the service refresh doesn't race the expiring
  token into an unnecessary re-auth on a 401.
- tokenRefreshTime returns time.Now() when the remaining lifetime is too
  small to jitter against (avoids rand.Int63n(0)).
- after a failed Authenticate(), subsequent calls within the backoff
  window return the cached error without retrying. Prevents callers from
  hammering an unreachable controller.
- backoff doubles per failure with jitter, capped at AuthBackoffMax.
  Defaults: AuthBackoffInitial=5s, AuthBackoffMax=5m.
- separates the un-jittered base (authBackoff) from the per-failure
  wait (currentWait) so jitter doesn't compound into the doubling
  sequence.
- exposes Options.AuthBackoffInitial / AuthBackoffMax for embedders.
- a 401 on a service-related call within 15s of a successful api-session
  refresh is almost always a propagation race (server saw the request
  before processing the new token). Retry with the current session
  instead of going through Authenticate(), which would needlessly drive
  the refresh machinery and risk churning sessions and ER connections.
- converts lastSuccessfulApiSessionRefresh to concurrenz.AtomicValue so
  recentlyAuthenticated can read it without taking authAttemptLock; also
  records the refresh time after successful periodic refreshes in
  runRefreshes, not just on Authenticate-time refreshes.
@plorenz
plorenz force-pushed the auth-fixes branch 2 times, most recently from 753da99 to e2feb07 Compare May 28, 2026 14:03
@plorenz
plorenz merged commit 9cc5622 into main May 28, 2026
6 checks passed
@plorenz
plorenz deleted the auth-fixes branch May 28, 2026 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants