lhotari commented on PR #25546: URL: https://github.com/apache/pulsar/pull/25546#issuecomment-4266253618
> I have a question. > > This vulnerability allowed async-http-client to send the Authorization header to the redirected URL without removing it when `followRedirect(true)`. The fixed async-http-client will now automatically remove the Authorization header if the scheme, domain, or port of the redirected URL differs from the original. Would this behavior cause problems in Pulsar? @massakam good question. I'll address this. Claude made this analysis based on your question: > # Impact of async-http-client 2.14.5 Authorization-stripping fix on Pulsar > > ## Short answer > > **Yes, this would break `pulsar-client`'s `HttpLookupService`, but `pulsar-client-admin` is safe.** The two HTTP paths handle redirects very differently. > > ## The two cases > > ### 1. `pulsar-client-admin` → Safe > > `AsyncHttpConnector` explicitly calls `setFollowRedirect(false)` on the AHC config and implements its own redirect loop. On each hop it rebuilds the request through `prepareRequest(...)`, which invokes the `Authentication` plugin's `authenticate(...)` / `newRequestHeader(...)` again and re-injects the `Authorization` header. Because AHC never sees `followRedirect=true`, `Redirect30xInterceptor` never runs, and its new stripping logic never fires. Cross-broker 307s keep working. > > The proxy path (`AdminProxyHandler` with Jetty `HttpClient`) has its own `copyRequest` override that explicitly carries `Authorization` across 307s — also unaffected. > > ### 2. `pulsar-client` `HttpLookupService` → Breaks > > `pulsar-client/.../impl/HttpClient.java` configures AHC with `setFollowRedirect(true)` and `setMaxRedirects(...)`, i.e. AHC does the redirect itself. The `Authorization` header is added once on the initial request from `authentication.newRequestHeader(...)`; there's no rebuild-per-hop, so it relies entirely on AHC carrying the header across the redirect. > > This is exactly the code path the fix changes. `Redirect30xInterceptor.propagatedHeaders(...)` now strips `Authorization` (and `Proxy-Authorization`) when: > > - `!request.getUri().isSameBase(newUri)` — scheme, host, **or** port differs, or > - HTTPS → HTTP downgrade, or > - `stripAuthorizationOnRedirect=true` (default is `false`, so same-origin redirects are not affected). > > Pulsar's HTTP lookup redirects (`TEMPORARY_REDIRECT` in `TopicLookupBase`, cross-cluster lookups, bundle-ownership redirects) routinely point to a different broker's `httpUrl` / `httpUrlTls` — different host, sometimes different port, sometimes different scheme. Same-path redirects don't exist here by design; the point is to send the client to the broker that owns the bundle. > > Consequence: after upgrade, the redirected lookup request arrives at broker2 with no `Authorization` header → 401 → lookup fails. This hits every auth plugin that uses `Authorization`: `AuthenticationToken`, `AuthenticationBasic`, `AuthenticationOAuth2`, `AuthenticationAthenz`, `AuthenticationSasl` (SASL state header), and anything custom that sets it via `newRequestHeader(...)`. > > Note: `stripAuthorizationOnRedirect` does **not** help here — it's a tightening switch, not a disabling one. Setting it `false` (the default) still strips on cross-origin redirects; there is no opt-out for that. > > ## Fix options for Pulsar > > The cleanest fix is to make `HttpLookupService`'s client match the admin-client pattern: turn off AHC's follow-redirect and do the redirect loop in Pulsar code, re-invoking `authentication.newRequestHeader(...)` per hop. That also gives correct behavior for auth plugins that need per-request tokens (OAuth2 refresh, SASL challenge-response) — which the current "set once, let AHC replay" approach already handles sub-optimally. > > A narrower workaround would be a small interceptor/filter that re-applies the auth header after AHC's redirect, but given AHC strips it in `propagatedHeaders` before issuing the redirected request, you'd need to hook in at the right point — doing it in Pulsar's own redirect loop is simpler. > > Worth filing as a blocker on any PR that bumps AHC ≥ 2.14.5 in `pulsar-client`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
