Skip to content

Support bearer-token authentication in BuilderRWBufferFromHTTP - #110313

Open
serxa wants to merge 14 commits into
masterfrom
http-builder-bearer-token
Open

Support bearer-token authentication in BuilderRWBufferFromHTTP#110313
serxa wants to merge 14 commits into
masterfrom
http-builder-bearer-token

Conversation

@serxa

@serxa serxa commented Jul 13, 2026

Copy link
Copy Markdown
Member

BuilderRWBufferFromHTTP had no way to authenticate with a bearer token: callers spliced an Authorization: Bearer entry into the header list themselves and had to remember not to pass HTTP Basic credentials at the same time, because both occupy the same header. create now takes the credential directly, in three overloads: Basic credentials, a bearer token, or both — with a non-empty token taking precedence, so the either/or rule lives in one place instead of in every caller. create also appends the bearer header to a local copy of the header list rather than mutating the builder, so a builder can be reused without carrying over a stale token.

Every bearer-token call site of the class is migrated to the new overloads:

  • HTTPBasedCatalogUtils and UnityCatalog: built the header by hand and passed never-used HTTP Basic credentials alongside; now call create with the token.
  • RestCatalog, OneLakeCatalog, BigLakeCatalog: getAuthHeaders now returns an AuthHeaders (a bearer token plus extra headers). The bearer goes to create; only genuine extra headers (a user-supplied auth_header, x-goog-user-project, User-Agent) stay in the header list. OneLakeCatalog's pre-obtained token also moved from a spliced header to the bearer.
  • PaimonRestCatalog: the bearer token provider (the dlf request-signing branch keeps its own signed headers).

Out of scope — different transport (plain Poco requests or S3-client headers), not BuilderRWBufferFromHTTP: OpenAIProvider, CloudJWTProvider, ArrowFlight AuthMiddleware, GCS diskSettings.

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

...

serxa and others added 2 commits July 13, 2026 19:00
Callers that authenticate with a bearer token currently splice an
`Authorization: Bearer` entry into the header list themselves and must
also remember not to pass HTTP Basic credentials, because both occupy the
same header. The builder now owns that rule: `withBearerToken` attaches
the header, and a non-empty token takes precedence over the Basic
credentials passed to `create`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
UnityCatalog spliced the bearer into the header list itself and passed
never-used HTTP Basic credentials alongside. The catalog helpers now take
the token and hand it to the builder, which owns the header formatting and
the bearer-over-Basic precedence.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [8543461]

Summary:


AI Review

Summary

This PR moves bearer-token handling for the Unity and Paimon catalog HTTP paths into BuilderRWBufferFromHTTP::createWithBearerToken, keeps bearer-over-Basic precedence inside the builder, and fixes the OneLake header-validation regression with a focused malformed-token test. The current code paths look correct, but the builder-reuse invariant that already regressed once in this PR still has no focused regression coverage, so I would not approve it yet.

Findings

⚠️ Majors

  • [src/IO/ReadWriteBufferFromHTTP.cpp:825] [dismissed by author -- https://github.com/Support bearer-token authentication in BuilderRWBufferFromHTTP #110313#discussion_r3583287166] createWithBearerToken now owns the stale-header fix, but the test added in this PR only covers malformed OneLake tokens, not builder reuse. If a later edit starts mutating http_header_entries again, one reused BuilderRWBufferFromHTTP instance would silently leak the first request's Authorization header into the next one and nothing in the current suite would catch it. Suggested fix: add a small regression test that reuses one builder across two requests and asserts the second request does not carry the first bearer token.
Final Verdict

Status: ⚠️ Request changes

Minimum required action:

  • Add focused regression coverage for the builder-reuse case in createWithBearerToken, not just the malformed-token path.

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jul 13, 2026
Comment thread src/IO/ReadWriteBufferFromHTTP.cpp Outdated
The token has one entry point now — the terminal create call — so a
conflicting withBearerToken-plus-create state cannot be constructed.
Three overloads cover the auth modes: Basic credentials, a bearer token,
and both (the non-empty token wins; the two occupy the same
`Authorization` header, so a request carries one or the other).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@serxa serxa changed the title Add withBearerToken to BuilderRWBufferFromHTTP Support bearer-token authentication in BuilderRWBufferFromHTTP Jul 13, 2026
serxa and others added 7 commits July 14, 2026 16:40
`create` appended the bearer `Authorization` header to the builder's
`http_header_entries` member, so reusing a builder would carry over a
stale token. Append it to a local copy passed to `ReadWriteBufferFromHTTP`
instead, so `create` no longer mutates the builder.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`RestCatalog` (with `OneLakeCatalog`, `BigLakeCatalog`) and
`PaimonRestCatalog` built the `Authorization: Bearer` header by hand and
passed unused Basic credentials to `create`. `getAuthHeaders` now returns
an `AuthHeaders` of a bearer token plus extra headers: the bearer goes to
`create` and only genuine extra headers (a user-supplied `auth_header`,
`x-goog-user-project`, `User-Agent`) stay in the header list.
`OneLakeCatalog`'s pre-obtained token likewise moves from a spliced header
to the bearer, so every catalog bearer now flows through `create`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`.create({})` brace-initializes both one-argument overloads, making such
calls ambiguous. Two overloads remain: `create(credentials)` and
`create(bearer_token, credentials)` (the non-empty token wins). The catalog
helper passes empty credentials explicitly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`.create({})` brace-initializes both one-argument overloads, making such calls
ambiguous. Two overloads remain: `create(credentials)` and
`create(bearer_token, credentials)` (the non-empty token wins). The catalog
call sites pass empty credentials explicitly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A bearer-token overload of `create` would be ambiguous with
`create(credentials)` for `.create({})` call sites, and bearer-only callers
had to pass a throwaway empty credentials object because the buffer stores the
credentials by reference. A distinct `createWithBearerToken` removes the
ambiguity and needs no credentials argument; the single empty-credentials
object now lives once inside the builder instead of at every call site.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous commit renamed the declarations but left the out-of-line
definitions on the old `create` names; align them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/Databases/DataLake/RestCatalog.cpp Outdated
serxa and others added 2 commits July 14, 2026 18:07
`getAuthHeaders` returned an `AuthHeaders` of a bearer token plus extra
headers, so every `RestCatalog` call site had to unpack it. Match
`PaimonRestCatalog`'s existing shape instead: `getAuthHeaders` appends its
extra headers to a caller-owned list and returns the bearer token. The
struct is gone and the call sites just forward their header list and the
token to `createWithBearerToken`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…aders

`createWithBearerToken` appends the `Authorization: Bearer` header
internally without going through the `HTTPHeaderFilter` check, so a
user-supplied `onelake_bearer_token` that contains a forbidden or
malformed value could reach the request path unchecked.

Add the same validation that `RestCatalog` applies to a user-supplied
`auth_header` in its own constructor: call `validateAuthHeaders` with the
synthetic `Authorization: Bearer <token>` header before storing the
token. This restores the pre-refactor behavior where the value went
through the header list and was subject to `http_forbid_headers` / CR-LF
checks.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread src/IO/ReadWriteBufferFromHTTP.cpp
serxa and others added 2 commits July 16, 2026 15:29
Resolve the conflict in `RestCatalog.{h,cpp}`: master's `CatalogState`-based
auth refactor (atomic credential snapshots plus the ALTER framework
`prepareSettingsChanges`/`applySettingsChangesToState`, and `getAuthHeaders`
returning `HTTPHeaderEntries` from a state snapshot) superseded this branch's
`getAuthHeaders` simplification, so `RestCatalog.{h,cpp}` are taken from master.

The branch's remaining contribution is preserved: `createWithBearerToken` on
`BuilderRWBufferFromHTTP` and its adoption in `HTTPBasedCatalogUtils`,
`PaimonRestCatalog` and `UnityCatalog` (which master did not refactor).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Construct OneLakeCatalog with a bearer token containing a newline and
assert the constructor rejects it with BAD_ARGUMENTS via
validateAuthHeaders, before any request is issued. Locks the fix that
routes a pre-obtained bearer token through http_forbid_headers
validation instead of splicing it into the header list unchecked.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 86.20% 86.20% +0.00%
Functions 92.00% 92.10% +0.10%
Branches 78.30% 78.30% +0.00%

Changed lines: Changed C/C++ lines covered: 58/80 (72.50%) · Uncovered code

Full report · Diff report

@serxa
serxa marked this pull request as ready for review July 27, 2026 10:47
@clickhouse-gh

clickhouse-gh Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

📊 Cloud Performance Report

✅ AI verdict: no_change — no significant changes across 37 queries analysed

The only flagged query is clickbench Q15 (+14.0%, median 215 -> 245 ms), which the deterministic gates already suppressed as within master's current variance band [191, 275] and left as not_sure. CPU time is essentially unchanged (2611 vs 2617 ms), so the extra wall time is cache/IO variance rather than added work. This PR changes only DataLake REST-catalog authentication (bearer token vs HTTP Basic credentials) and the shared HTTP read buffer — code Q15 never runs — so there is no plausible path from the diff to this query. Verdict stands: no real regression here.

clickbench

⚠️ 1 inconclusive

Flagged queries (1 of 43)
Query Verdict Baseline median (ms) PR median (ms) Change q-value Hint
⚠️ 15 not_sure 215 245 +13.9% <0.0001 cache: CPU is flat (2611 vs 2617 ms) while wall time rose +14.0%, and this PR only touches DataLake catalog HTTP auth — nothing Q15 executes — so the delta reads as page-cache/IO variance, not a PR effect.

Change = percent below ×2; the ratio of medians (×N faster/slower) beyond, where percent understates the scale. q-value = BH-FDR adjusted p; smaller is stronger evidence. MIRAI flags a query when q < fdr_q (default 0.10) — the value the verdict is based on.

tpch_adapted_1_official

🟢 No significant changes

Debug info
  • StressHouse run: 1a895a66-35e6-4cba-8301-8f32da9f425a
  • MIRAI run: e788884e-51b7-4b8e-96c2-dd22e01ffffb
  • PR check IDs:
    • clickbench_528622_1785161446
    • clickbench_528628_1785161446
    • clickbench_528644_1785161446
    • tpch_adapted_1_official_528652_1785161446
    • tpch_adapted_1_official_528677_1785161446
    • tpch_adapted_1_official_528691_1785161446

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant