Fix issue with GCP auth and unity catalog#98456
Merged
Conversation
|
Melvyn Peignon seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
|
Workflow [PR], commit [05ce6e6] Summary: ✅ AI ReviewSummaryThis PR adds Missing context
ClickHouse Rules
Final Verdict
|
clickhouse-gh
Bot
added
the
pr-bugfix
scanhex12
approved these changes
Mar 17, 2026
| secret_access_key = s3_creds->getSecretAccessKey(); | ||
| session_token = s3_creds->getSessionToken(); | ||
| LOG_DEBUG(getLogger("getClient"), "Got new S3 access tokens"); | ||
| } |
Member
There was a problem hiding this comment.
let's throw an exception in other cases
Contributor
|
I tested it with GCP/Polaris combo. There are a lot of exceptions and retries. It does work after retries. |
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| { | ||
| access_key_id = s3_creds->getAccessKeyId(); | ||
| secret_access_key = s3_creds->getSecretAccessKey(); | ||
| session_token = s3_creds->getSessionToken(); |
Contributor
There was a problem hiding this comment.
❌ gcs_creds->getToken() comes from external catalog data and is copied into an HTTP header without header-value validation on the refresh path.
StorageS3Configuration::check validates headers parsed from AST, but this branch bypasses that path and pushes Authorization directly. If the token contains \n, it can inject extra headers.
Please validate refreshed headers before client creation, e.g. call context->getGlobalContext()->getHTTPHeaderFilter().checkAndNormalizeHeaders(headers) after replacing Authorization.
added 3 commits
March 22, 2026 21:28
- Use erase-remove idiom instead of in-place loop for Authorization header, fixing stale connection issues (suggested by @bacek) - Throw BAD_ARGUMENTS exception for unrecognized credential types (requested by @scanhex12)
- Call checkAndNormalizeHeaders on refreshed headers before creating the S3 client, preventing header injection from untrusted catalog tokens - Add unit test for GCSCredentials and header validation
melvynator
force-pushed
the
fix-gcs-auth-iceberg-rest-catalog
branch
from
fc34c26 to
05ce6e6
Compare
Contributor
LLVM Coverage Report
PR changed lines: PR changed-lines coverage: 60.55% (66/109, 0 noise lines excluded) |
robot-ch-test-poll3
added
the
pr-synced-to-cloud
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
5 participants
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.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix a bug when using Unity catalog on top of GCS
Documentation entry for user-facing changes