Skip to content

Remove the 10s header timeout from the upstream IdP client - #6755

Open
alex-feel wants to merge 2 commits into
stacklok:mainfrom
alex-feel:fix/upstream-idp-first-byte-timeout
Open

alex-feel wants to merge 2 commits into
stacklok:mainfrom
alex-feel:fix/upstream-idp-first-byte-timeout

Conversation

@alex-feel

Copy link
Copy Markdown
Contributor

Summary

  • The embedded auth server's client for the upstream identity provider inherited HttpClientBuilder's 10s response-header timeout. A token endpoint that takes longer than 10s to send its first byte failed the code exchange and the refresh, although the client timeout and the refresher's budget are both 30s. With a provider that revokes the presented refresh token when it processes the refresh, a refresh cut off this way leaves a revoked token stored, and the user has to sign in again. The shared grant client in pkg/oauthproto leaves this timeout unset for the same reason (grants.go, pinned by grants_test.go).
  • Add HttpClientBuilder.WithResponseHeaderTimeout. Zero removes the limit; the 10s default stays for every other caller.
  • newHTTPClientForHost sets it to zero, so the 30s client timeout bounds each call of the upstream provider client.

Fixes #6754

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

The new and existing tests of pkg/networking and pkg/authserver/upstream pass; the new assertion in TestNewHTTPClientForHost fails on main (Should be zero, but was 10s). The full task test run is left to CI. The reproduction from #6754, run against this branch, prints refresh ok after 12.0s: refresh_token=rt-2; on main it fails after 10.0s. With a token endpoint that answers after 35s, code exchange and refresh still stop at the 30s client timeout (Client.Timeout exceeded while awaiting headers).

Does this introduce a user-facing change?

Yes. An upstream identity provider that takes more than 10s to answer a token, discovery, JWKS or userinfo request now gets the full 30s client timeout instead of failing with timeout awaiting response headers.

Special notes for reviewers

The change removes the limit for the whole upstream provider client, so discovery, JWKS and userinfo get the same 30s budget as the token calls. They go to the same IdP, and a slow first byte there blocks sign-in the same way; the JWKS fetch in #6194 was cut off by this limit. pkg/oauthproto keeps short header timeouts on its DCR, discovery and CIMD clients. If you would rather limit the change to token requests, I can give the token endpoint its own client.

The embedded auth server's client for the upstream identity provider inherited the builder's 10s response-header timeout.
A token endpoint that takes longer than 10s to send its first byte therefore fails code exchange and refresh, although the client timeout and the refresher's budget are both 30s.
When a refresh is cut off after a provider that revokes the presented refresh token has processed it, the stored refresh token is no longer valid and the user has to sign in again.
The shared grant client in pkg/oauthproto leaves this timeout unset for the same reason, so that slow IdP chains can answer within the client timeout.
Add HttpClientBuilder.WithResponseHeaderTimeout, keeping the 10s default for every other caller, and have newHTTPClientForHost remove the limit.
Discovery, JWKS and userinfo use the same client and the same IdP, so they get the same 30s budget.

Signed-off-by: Aleksandr Filippov <71711753+alex-feel@users.noreply.github.com>
@codecov

codecov Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.36%. Comparing base (4c866b5) to head (d7b5846).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6755      +/-   ##
==========================================
+ Coverage   79.32%   79.36%   +0.03%     
==========================================
  Files         802      802              
  Lines       81430    81433       +3     
==========================================
+ Hits        64596    64629      +33     
+ Misses      16829    16799      -30     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Sanskarzz
Sanskarzz self-requested a review October 7, 2026 13:22

@Sanskarzz Sanskarzz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The change addresses issue 6754 and preserves the existing timeout and network protections. The broader upstream-client scope is reasonable. No blocking code findings; local race-enabled package tests passed. The remaining Actions check concerns unchanged workflows.

This branch has not been deployed

No deployments
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.

authserver: upstream IdP token requests time out after 10s, not 30s

2 participants