-
Notifications
You must be signed in to change notification settings - Fork 149
fix(kernel): forward full OAuth U2M app bundle into kernel (PECOBLR-4040) #914
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
Changes from 5 commits
e53fb37
5b9b623
26d6e74
fdc20e5
bc6702f
bfd3629
b3d03d1
5409fae
a03205a
9715f07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,11 +15,16 @@ | |
| connector's own OAuth provider because the kernel re-mints tokens | ||
| itself and the client secret is not recoverable from a built | ||
| provider. | ||
| - **OAuth U2M** — for ``auth_type`` ``databricks-oauth`` / | ||
| ``azure-oauth`` (the browser authorization-code flow), the optional | ||
| ``oauth_client_id`` / ``oauth_redirect_port`` are forwarded to the | ||
| kernel's ``auth_type='oauth-u2m'`` and the kernel runs the browser | ||
| flow itself. | ||
| - **OAuth U2M** — for ``auth_type`` ``databricks-oauth`` (the browser | ||
| authorization-code flow), the connector's ``databricks-sql-python`` | ||
| app bundle (``client_id`` + ``redirect_port``, with the optional | ||
| ``oauth_client_id`` / ``oauth_redirect_port`` overriding it) is | ||
| forwarded to the kernel's ``auth_type='oauth-u2m'`` and the kernel | ||
| runs the browser flow itself. ``azure-oauth`` (Azure AD) is **not yet | ||
| supported** on the kernel path and is rejected with | ||
| ``NotSupportedError`` — the kernel resolves OAuth endpoints only from | ||
| the workspace-native OIDC config and cannot drive the Azure AD flow | ||
| (PECOBLR-4120). | ||
|
|
||
| ``identity_federation_client_id`` is forwarded with whichever auth shape | ||
| wins resolution. It selects mandatory SP-wide workload-identity token | ||
|
|
@@ -48,6 +53,11 @@ | |
| import re | ||
| from typing import Any, Dict, Optional | ||
|
|
||
| from databricks.sql.auth.auth import ( | ||
| PYSQL_OAUTH_CLIENT_ID, | ||
| PYSQL_OAUTH_REDIRECT_PORT_RANGE, | ||
| PYSQL_OAUTH_SCOPES, | ||
| ) | ||
| from databricks.sql.auth.authenticators import AccessTokenAuthProvider, AuthProvider | ||
| from databricks.sql.auth.token_federation import TokenFederationProvider | ||
| from databricks.sql.exc import NotSupportedError, ProgrammingError | ||
|
|
@@ -141,15 +151,23 @@ def kernel_auth_kwargs( | |
| rather than silently picking one flow (and failing later as a | ||
| confusing 401 against the wrong principal): | ||
| - a custom ``credentials_provider`` *and* M2M kwargs together; | ||
| - a U2M ``auth_type`` (``databricks-oauth`` / ``azure-oauth``) | ||
| *and* ``oauth_client_secret`` together. | ||
| - a U2M ``auth_type`` (``databricks-oauth``) *and* | ||
| ``oauth_client_secret`` together. | ||
|
|
||
| (``azure-oauth`` is rejected as unsupported before these guards — | ||
| PECOBLR-4120.) | ||
| 1. **OAuth M2M** — ``oauth_client_id`` + ``oauth_client_secret`` | ||
| both present → forward raw creds to the kernel's ``oauth-m2m``. | ||
| 2. **PAT** — the built provider is (or wraps) an | ||
| ``AccessTokenAuthProvider`` → extract the bearer token. | ||
| 3. **OAuth U2M** — ``auth_type`` is ``databricks-oauth`` / | ||
| ``azure-oauth`` → forward optional ``oauth_client_id`` / | ||
| ``oauth_redirect_port`` to the kernel's ``oauth-u2m``. | ||
| 3. **OAuth U2M** — ``auth_type`` is ``databricks-oauth`` → forward the | ||
| connector's coupled ``databricks-sql-python`` bundle (``client_id`` | ||
| + ``redirect_port``, with fixed ``PYSQL_OAUTH_SCOPES``) to the | ||
| kernel's ``oauth-u2m``, so a bare U2M connection authenticates as | ||
| ``databricks-sql-python`` — parity with the Thrift path — rather | ||
| than the kernel's own ``databricks-sql-connector`` default | ||
| (PECOBLR-4039/4040). ``azure-oauth`` is rejected as unsupported | ||
| (PECOBLR-4120). | ||
| 4. **Custom credentials_provider** → ``NotSupportedError`` (opaque | ||
| token source; no raw creds for the kernel to own). | ||
| 5. Anything else → ``NotSupportedError``. | ||
|
|
@@ -169,6 +187,25 @@ def kernel_auth_kwargs( | |
| auth_type = opts.get("auth_type") | ||
| has_m2m = bool(client_id and client_secret) | ||
|
|
||
| # azure-oauth (Azure AD U2M) is not yet supported on the kernel path. | ||
| # Reject it up front — before any M2M/U2M routing — so ANY azure-oauth | ||
| # request gets a clear "not supported" error rather than being silently | ||
| # misrouted (e.g. azure-oauth + client_id + secret would otherwise look | ||
| # like M2M). The kernel resolves OAuth endpoints only from the | ||
| # workspace-native OIDC config and has no Azure AD path, so the Thrift | ||
| # azure-oauth flow (AAD token endpoint + /user_impersonation scope, see | ||
| # AzureOAuthEndpointCollection) cannot be reproduced here. Forwarding an | ||
| # azure bundle would authenticate against the wrong endpoints, so we fail | ||
| # loudly at session-open. Tracked by PECOBLR-4120. | ||
| if auth_type == "azure-oauth": | ||
| raise NotSupportedError( | ||
| "use_kernel=True does not support auth_type='azure-oauth' (Azure " | ||
| "AD U2M) yet: the kernel resolves OAuth endpoints only from the " | ||
| "workspace-native OIDC configuration and cannot drive the Azure AD " | ||
| "authorization/token flow. Use the Thrift backend (default) for " | ||
| "azure-oauth. Tracked by PECOBLR-4120." | ||
| ) | ||
|
|
||
| # 0. Ambiguity guards — fail before any flow is chosen. | ||
| if client_secret and opts.get("credentials_provider") is not None: | ||
| raise NotSupportedError( | ||
|
|
@@ -178,7 +215,7 @@ def kernel_auth_kwargs( | |
| "kernel-managed M2M, or use the Thrift backend (default) for " | ||
| "credentials_provider." | ||
| ) | ||
| if client_secret and auth_type in ("databricks-oauth", "azure-oauth"): | ||
| if client_secret and auth_type == "databricks-oauth": | ||
| raise NotSupportedError( | ||
| f"Ambiguous auth on use_kernel=True: auth_type={auth_type!r} selects " | ||
| "the U2M browser flow, but oauth_client_secret was also provided " | ||
|
|
@@ -214,16 +251,54 @@ def kernel_auth_kwargs( | |
| return kwargs | ||
|
|
||
| # 3. OAuth U2M — browser authorization-code flow; the kernel runs it. | ||
| if auth_type in ("databricks-oauth", "azure-oauth"): | ||
| kwargs = {"auth_type": "oauth-u2m"} | ||
| if client_id: | ||
| kwargs["client_id"] = client_id | ||
| # Only databricks-oauth reaches here (azure-oauth was rejected up | ||
| # front — see the guard near the top of this function). | ||
| # | ||
| # The kernel's core default U2M app is databricks-sql-connector / | ||
| # sql offline_access / port 8030 (PECOBLR-4039). The Python | ||
| # connector is an OVERRIDE of that default: on this path we forward | ||
| # its OWN full bundle rather than letting the kernel fall back to | ||
| # the connector default. Forwarding a bare oauth-u2m would | ||
| # authenticate as databricks-sql-connector, breaking parity with | ||
| # the Thrift path (which authenticates as databricks-sql-python). | ||
| # | ||
| # client_id + redirect_port are coupled per OAuth app — each app | ||
| # registers its own redirect URI — so both are resolved together: | ||
| # an explicit caller value wins; otherwise the connector's | ||
| # registered databricks-sql-python bundle is used, mirroring the | ||
| # defaults get_python_sql_connector_auth_provider applies on the | ||
| # Thrift path. scopes are NOT caller-overridable: the Thrift path | ||
| # hardcodes PYSQL_OAUTH_SCOPES for U2M (a caller's oauth_scopes | ||
| # kwarg is never read there), so we forward the same fixed scopes | ||
| # here to keep the two backends in parity. | ||
| # | ||
| # Only the redirect PORT is routable into the kernel: it derives | ||
| # http://localhost:{port}, with scheme/host/path fixed. The | ||
| # connector registers a port *range* for its app but the kernel | ||
| # accepts a single port, so we forward the first (canonical) | ||
| # registered port. A caller-supplied port only overrides that | ||
| # default when an explicit client_id is ALSO supplied — matching | ||
| # the Thrift path's coupling (a bare oauth_redirect_port paired | ||
| # with the default databricks-sql-python app would resolve to an | ||
| # unregistered redirect URI and fail the flow). | ||
| if auth_type == "databricks-oauth": | ||
| redirect_port = opts.get("oauth_redirect_port") | ||
| if redirect_port is not None: | ||
| kwargs["redirect_port"] = int(redirect_port) | ||
| scopes = _normalize_scopes(opts.get("oauth_scopes")) | ||
| if scopes is not None: | ||
| kwargs["oauth_scopes"] = scopes | ||
| # Validate any caller-supplied oauth_scopes (a bad type is still a | ||
| # caller error worth flagging) but do NOT forward it: the Thrift | ||
| # path hardcodes PYSQL_OAUTH_SCOPES for U2M, so we do the same for | ||
| # parity rather than letting the kernel path honor an override the | ||
| # other backend silently ignores. | ||
| _normalize_scopes(opts.get("oauth_scopes")) | ||
| kwargs = { | ||
| "auth_type": "oauth-u2m", | ||
| "client_id": client_id or PYSQL_OAUTH_CLIENT_ID, | ||
| "redirect_port": ( | ||
|
peco-review-bot[bot] marked this conversation as resolved.
Outdated
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. redirect_port should just get passed in, we either use the pass in one or default one, no need to judge based on client_id existense.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current coupling (forward caller redirect_port only when an explicit client_id is also supplied) deliberately mirrors the Thrift path in src/databricks/sql/auth/auth.py:127-130, which overrides oauth_redirect_port_range only when both oauth_client_id and oauth_redirect_port are set. The rationale is correctness: the default databricks-sql-python app only registers its default port range as redirect URIs, so forwarding an arbitrary caller port without a matching custom client_id yields an unregistered redirect URI and fails the U2M flow. Rather than silently regress this parity/correctness behavior on a one-line request, I replied asking whether the kernel is intended to decouple port from app registration (differing from Thrift). If confirmed, I'll drop the |
||
| int(redirect_port) | ||
| if client_id and redirect_port is not None | ||
| else PYSQL_OAUTH_REDIRECT_PORT_RANGE[0] | ||
| ), | ||
|
peco-review-bot[bot] marked this conversation as resolved.
|
||
| "oauth_scopes": list(PYSQL_OAUTH_SCOPES), | ||
|
peco-review-bot[bot] marked this conversation as resolved.
Outdated
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not hardcode, we should be able to take in customized scopes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made the change the reviewer asked for: the kernel U2M path now accepts customized Pushed bfd3629 (bundled with 1 other thread(s)). |
||
| } | ||
| if federation_client_id: | ||
| kwargs["identity_federation_client_id"] = federation_client_id | ||
| return kwargs | ||
|
|
@@ -251,7 +326,7 @@ def kernel_auth_kwargs( | |
| raise NotSupportedError( | ||
| f"use_kernel=True requires PAT (access_token), OAuth M2M " | ||
| f"(oauth_client_id + oauth_client_secret), or OAuth U2M " | ||
| f"(auth_type='databricks-oauth' / 'azure-oauth'), but got " | ||
| f"(auth_type='databricks-oauth'), but got " | ||
| f"{provider_desc} with auth_type={auth_type!r}. Use the Thrift " | ||
| "backend (default) for other auth flows." | ||
| ) | ||
|
|
||
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.
Comment too verbose, make it concise
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.
Shortened the comment as requested. The 32-line block on
auth_bridge.py:254is now an 8-line summary that keeps the essentials (databricks-oauth-only, forwarding the connector's own bundle for Thrift parity, client_id/redirect_port coupling, non-overridable scopes) and drops the redundant prose. No code behavior changed.Pushed bfd3629 (bundled with 1 other thread(s)).