forked from ClickHouse/ClickHouse
-
Notifications
You must be signed in to change notification settings - Fork 17
Add OAuth2 login to clickhouse-client (--login / --login=device) #1606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
BorisTyshkevich
wants to merge
15
commits into
antalya-26.2
Choose a base branch
from
feature/client-IdP
base: antalya-26.2
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
bd0cfb8
Add OAuth2 login to clickhouse-client via credentials JSON file
bvt123 f8f11a2
Fix Google JWT exp claim type in TokenProcessorsOpaque
bvt123 2c7c933
Fix review blockers in OAuth2 login implementation
bvt123 3d03550
Robustness fixes, issuer discovery, state CSRF, and docs
bvt123 2584ed9
Merge --login-device into --login=device
bvt123 1ce5b15
Fix --login=device crash and --login=browser routing
bvt123 537268d
Fix device flow crash on Google verification_url field name
bvt123 145f276
Document --login/--oauth-credentials and add OAuth option validation …
bvt123 6b9a4a1
DCO Remediation Commit
bvt123 cd027be
Fix security and reliability issues in OAuth2 login
bvt123 8c0bc56
Fix Google OAuth scope: use access_type=offline instead of offline_ac…
bvt123 1c71246
Split OAuthJWTProvider into its own file; fix silent cache catch
bvt123 f577fc3
Add OAuth2 integration tests and fix remaining review items
bvt123 6cf7c55
Fix blockers in Keycloak integration tests
bvt123 c78caad
Fix OAuth HTTPS sessions to respect configured SSL context
bvt123 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| #include <config.h> | ||
|
|
||
| #if USE_JWT_CPP && USE_SSL | ||
|
|
||
| #include <Client/JWTProvider.h> | ||
| #include <Client/OAuthLogin.h> | ||
|
|
||
| #include <Poco/Timespan.h> | ||
| #include <Poco/Timestamp.h> | ||
|
|
||
| #include <iostream> | ||
| #include <memory> | ||
|
|
||
| namespace DB | ||
| { | ||
|
|
||
| /// JWTProvider subclass for the credentials-file OIDC path (--login=browser / | ||
| /// --login=device). Extends JWTProvider so that Connection::sendQuery can call | ||
| /// getJWT() transparently to refresh the id_token before it expires, eliminating | ||
| /// the 1-hour session limit that arises when the token is obtained only once at | ||
| /// startup. | ||
| /// | ||
| /// getJWT() delegates to obtainIDToken() which already handles the full lifecycle: | ||
| /// 1. try cached refresh token from disk | ||
| /// 2. run interactive flow (browser or device) if the refresh token is absent | ||
| /// or rejected | ||
| class OAuthJWTProvider : public JWTProvider | ||
| { | ||
| public: | ||
| OAuthJWTProvider(OAuthCredentials creds, OAuthFlowMode mode) | ||
| : JWTProvider("", creds.client_id, "", std::cerr, std::cerr) | ||
| , creds_(std::move(creds)) | ||
| , mode_(mode) | ||
| {} | ||
|
|
||
| std::string getJWT() override | ||
| { | ||
| constexpr int EXPIRY_BUFFER_SECONDS = 30; | ||
|
|
||
| if (!idp_access_token.empty() | ||
| && Poco::Timestamp() < idp_access_token_expires_at - Poco::Timespan(EXPIRY_BUFFER_SECONDS, 0)) | ||
| return idp_access_token; | ||
|
|
||
| // obtainIDToken tries the disk-cached refresh token first and falls back | ||
| // to an interactive flow only when necessary. | ||
| idp_access_token = obtainIDToken(creds_, mode_); | ||
| idp_access_token_expires_at = getJwtExpiry(idp_access_token); | ||
| return idp_access_token; | ||
| } | ||
|
|
||
| private: | ||
| OAuthCredentials creds_; | ||
| OAuthFlowMode mode_; | ||
| }; | ||
|
|
||
| std::shared_ptr<JWTProvider> createOAuthJWTProvider( | ||
| const OAuthCredentials & creds, OAuthFlowMode mode) | ||
| { | ||
| return std::make_shared<OAuthJWTProvider>(creds, mode); | ||
| } | ||
|
|
||
| } // namespace DB | ||
|
|
||
| #endif // USE_JWT_CPP && USE_SSL |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expmeans the number of seconds since Unix epoch. Usually it is integer, and it is expected to be integer, isn\t it?At least, Azure and Keycloak stick to integer values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// picojson only has one numeric type: double
typedef double number_type;
So value.is() returns false, value.is<time_t>() returns false, but value.is() returns true — for any JSON number, including 1719500000.
The original code getValueByKey<time_t>(token_info, "exp") would always fail with "Value for key 'exp' has incorrect type" because picojson doesn't have time_t as a native type. The double fix is actually the
correct fix for picojson's type system.
So I retract my earlier comment. Using double is not "accidentally working" — it's the only way to extract a number from picojson. The static_cast<time_t> then truncates to integer, which is fine since the
value was integer to begin with (JSON 1719500000 → picojson double(1719500000.0) → time_t(1719500000)).
The commit message and the fix are correct. If anything, a brief comment explaining picojson's type model would help future readers:
// picojson stores all JSON numbers as double; cast to time_t for epoch seconds.
But that's a style nit, not a bug.