Repository navigation
fix(connect): proxy OIDC token requests and close refresh sessions - #2223
Open
dudanogueira wants to merge 2 commits into
Open
dudanogueira wants to merge 2 commits into
dudanogueira wants to merge 2 commits into
Conversation
…te#2220) With OIDC auth the authlib session was the HTTP client for every request. Authlib >= 1.8 builds its sessions on httpx2 whenever httpx2 is installed (e.g. pulled in by openai/langchain), so the httpx Timeout we pass on each request was read as a single number and every call failed: async with "unsupported operand type(s) for +: 'float' and 'Timeout'", sync with "'Timeout' object cannot be interpreted as an integer". The session now only holds and refreshes the token. Requests go through the same httpx client API-key and anonymous connections use; we already set the bearer header per request and refresh in the background, and authlib's inline refresh lives in request()/stream(), which we never called. close() closes the session too, and token-fetch failures from an httpx2-based session raise AuthenticationFailedError like httpx ones. As a side effect OIDC requests now honour the configured proxies, pool limits and retries, which the session never received.
Two problems in the OIDC token handling, both independent of the httpx2 fix they build on: - The authlib sessions were built without transports, so token requests (fetch and refresh) ignored the configured proxies, pool limits and retries. They now get the same transports as our own client, built with the library authlib uses (httpx2 for Authlib >= 1.8 when installed, else httpx), since a session must not be handed another library's transports. - With client credentials, every background refresh built a new session (which already fetches a token), fetched a second token with it and never closed it: one leaked connection pool and two token requests per refresh. The refresher now keeps the new session's token and closes it.
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
This branch has not been deployed
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.
Motivation
While fixing #2220 in #2222, I found two more bugs in the OIDC token handling. Neither depends on
httpx2. Both reproduce with authlib 1.6/1.7 as well.fetch_tokenand the refresh requests bypassAdditionalConfig(proxies=...), pool limits and retries. The OIDC discovery requests do use the proxy, because they go through our own client. In a network where the IdP is only reachable through the proxy, OIDC login fails.get_auth_session(), which builds a new session that already fetches a token. It then callsfetch_token()again and never closes the session. Each refresh costs one leaked connection pool and two token requests instead of one. Refreshes run everyexpires_in - 30seconds, so a long-running client keeps accumulating open pools.Approach
_make_mountsbecame_make_mounts_with(http, colour), which takes the HTTP library as a parameter. Our own client still useshttpx. The sessions get transports built with the library authlib builds them on:httpx2for Authlib ≥ 1.8 when installed, otherwisehttpx. That library is detected once, from the session class's bases (_session_http_module). Building them with plainhttpxwould bring back the error when client connect #2220 class of bug: anhttpx2client receivinghttpxobjects.finally.Key areas for review
authentication.py:_session_http_module: detects the library from the MRO. It falls back tohttpx, which matches authlib's own fallback.v4.py:_make_mounts_with: the same transport settings as before, built with a given library.Risks and mitigations
proxies, and it's what every other request already does. Users with no proxies configured get an emptymounts, so nothing changes for them.get_auth_session()has always fetched its token explicitly before returning (see the comment in_get_session_client_credential). Reusing that token instead of fetching a second one doesn't change which token is used.Testing
All four new tests fail on #2222 and pass here.
test_oidc_token_requests_use_proxies(+ async): Weaviate and the IdP sit behind*.invalidhosts that only a proxy (a secondHTTPServer) can reach. Connect and an authenticated call succeed only if the token request also goes through the proxy.test_client_credentials_refresh_closes_sessions(+ async): tokens expire right away, sessions are tracked over about 3 refreshes, and the test asserts every session is closed and exactly one token request is made per session.mock_tests/suite (81 tests) passes with authlib 1.6.12 (CI's pin), authlib 1.9.1 +httpx2, and authlib 1.9.1 withouthttpx2(test_auth.py).POSTgoes through the proxy with authlib 1.9.1 +httpx2and with 1.7.2. Over 3.5 s of refreshes, open sessions afterclose()went from 3 to 0 per connection, and token requests from 7 to 4.