-
Notifications
You must be signed in to change notification settings - Fork 145
feat(kernel): thread Azure Entra OAuth (U2M + SP M2M) through the auth bridge #919
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
base: main
Are you sure you want to change the base?
Changes from all commits
8f2a73a
c2dec51
01acb39
311e3f0
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 |
|---|---|---|
|
|
@@ -20,11 +20,26 @@ | |
| app bundle (``client_id`` + ``redirect_ports`` list, 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). | ||
| runs the browser flow itself. | ||
| - **Azure Entra (Azure AD)** — both Azure auth types forward the selector | ||
| and Azure credentials to the KERNEL, which is the Azure-aware auth core | ||
| (it owns the endpoints, scopes, app ids, and tenant discovery). The | ||
| binding stays thin — it does not construct endpoints or scopes: | ||
|
|
||
| - ``azure-oauth`` (U2M) → forward ``auth_type='azure-oauth'`` (plus any | ||
| optional ``oauth_client_id`` / ``oauth_redirect_port`` passthrough). The | ||
| kernel pins the workspace v2.0 authorize/token endpoints, the Azure app | ||
| client id (``96eecda7-…``), port ``8030``, and the | ||
| ``{app_id}/user_impersonation offline_access`` scope (PECOBLR-4120). | ||
| - ``azure-sp-m2m`` (M2M) → forward ``auth_type='azure-sp-m2m'`` with the | ||
| Azure service-principal ``azure_client_id`` / ``azure_client_secret``. | ||
| The kernel builds the Entra v2.0 token endpoint and the | ||
| ``{effective_app_id}/.default`` scope, and auto-discovers the tenant from | ||
| the workspace's ``/aad/auth`` redirect when ``azure_tenant_id`` is omitted | ||
| (Thrift parity). ``azure_workspace_resource_id`` is an optional add-on: | ||
| forward it and the kernel additionally sends the Azure management-token | ||
| header pair, so an RBAC-only SP (not a workspace member) can authenticate | ||
| (PECOBLR-4141). | ||
|
|
||
| ``identity_federation_client_id`` is forwarded with whichever auth shape | ||
| wins resolution. It selects mandatory SP-wide workload-identity token | ||
|
|
@@ -134,6 +149,7 @@ def _extract_bearer_token(auth_provider: Optional[AuthProvider]) -> Optional[str | |
| def kernel_auth_kwargs( | ||
| auth_provider: Optional[AuthProvider], | ||
| auth_options: Optional[Dict[str, Any]] = None, | ||
| hostname: Optional[str] = None, | ||
| ) -> Dict[str, Any]: | ||
| """Build the kwargs passed to ``databricks_sql_kernel.Session(...)``. | ||
|
|
||
|
|
@@ -154,8 +170,9 @@ def kernel_auth_kwargs( | |
| - a U2M ``auth_type`` (``databricks-oauth``) *and* | ||
| ``oauth_client_secret`` together. | ||
|
|
||
| (``azure-oauth`` is rejected as unsupported before these guards — | ||
| PECOBLR-4120.) | ||
| (The Azure Entra auth types — ``azure-oauth`` and ``azure-sp-m2m`` — | ||
| are forwarded to the kernel's Azure-aware flows up front, before these | ||
| guards; see the module docstring.) | ||
| 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 | ||
|
|
@@ -168,7 +185,6 @@ def kernel_auth_kwargs( | |
| forwarding the connector's own OAuth app rather than the kernel's | ||
| ``databricks-sql-connector`` default (PECOBLR-4039/4040). Unlike the | ||
| Thrift path, a caller-supplied ``oauth_scopes`` is honored here. | ||
| ``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``. | ||
|
|
@@ -188,24 +204,71 @@ 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. | ||
| # Azure Entra (Azure AD) auth types route to the kernel's GENERIC OAuth | ||
| # flows with Azure values supplied as overrides — the kernel needs no | ||
| # Azure-specific code. Handled up front, keyed on the explicit auth_type, | ||
| # before the generic M2M/PAT/U2M routing below (azure-sp-m2m carries its | ||
| # creds in azure_* kwargs, not oauth_client_id/secret, so it would | ||
| # otherwise fall through to the final "unsupported" error). | ||
|
|
||
|
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. 🟡 Medium — The Concretely, Consider applying the same ambiguity checks (secret-with-U2M, credentials_provider-with-secret) to the Azure U2M branch before returning, so an ambiguous Azure request fails at session-open rather than silently choosing the browser flow. |
||
| # azure-oauth (Azure AD U2M): forward the selector; the KERNEL owns Azure | ||
| # resolution (it is the auth core). The kernel pins the workspace v2.0 | ||
| # authorize/token endpoints (`{host}/oidc/oauth2/v2.0/{authorize,token}` — | ||
| # NOT the discovered `/oidc/v1/authorize`, which the workspace redirects to | ||
| # a malformed Entra URL), the Azure app client id, port 8030, and the | ||
| # `{app_id}/user_impersonation offline_access` scope. So this binding does | ||
| # NOT construct endpoints/scopes — it just passes `auth_type='azure-oauth'` | ||
| # plus any optional client_id / redirect_port passthrough. PECOBLR-4120. | ||
|
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. 🔵 Low — Asymmetric handling of (Anchored to the nearest changed line — see the description for the exact location.) |
||
| 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." | ||
| ) | ||
| kwargs = {"auth_type": "azure-oauth"} | ||
| if client_id: | ||
| kwargs["client_id"] = client_id | ||
| redirect_port = opts.get("oauth_redirect_port") | ||
| if redirect_port is not None: | ||
| kwargs["redirect_ports"] = [_coerce_redirect_port(redirect_port)] | ||
| if federation_client_id: | ||
| kwargs["identity_federation_client_id"] = federation_client_id | ||
| return kwargs | ||
|
|
||
| # azure-sp-m2m (Azure service principal, client-credentials): forward the | ||
| # selector + Azure SP credentials; the KERNEL owns Azure resolution (it is | ||
| # the auth core). The kernel builds the Entra v2.0 token endpoint | ||
| # (`{login}/{tenant}/oauth2/v2.0/token`) and the `{effective_app_id}/.default` | ||
| # scope, and — when azure_tenant_id is omitted — auto-discovers the tenant | ||
| # from the workspace's /aad/auth redirect, matching the Thrift backend | ||
| # (so connect() is byte-identical between Thrift and use_kernel=True). | ||
| # PECOBLR-4141. | ||
| # | ||
| # azure_workspace_resource_id is an optional add-on: forward it and the | ||
| # kernel additionally fetches an Azure-management token and sends the | ||
| # X-Databricks-Azure-SP-Management-Token + X-Databricks-Azure-Workspace- | ||
| # Resource-Id pair, so an SP that holds only an Azure RBAC role (not a | ||
| # workspace member) can authenticate. Omit it (the common case) and the SP | ||
| # authenticates with the Databricks-audience data token alone. | ||
| if auth_type == "azure-sp-m2m": | ||
| azure_client_id = opts.get("azure_client_id") | ||
| azure_client_secret = opts.get("azure_client_secret") | ||
| if not (azure_client_id and azure_client_secret): | ||
| raise ProgrammingError( | ||
| "auth_type='azure-sp-m2m' requires azure_client_id and " | ||
| "azure_client_secret." | ||
| ) | ||
| kwargs = { | ||
| "auth_type": "azure-sp-m2m", | ||
| "azure_client_id": azure_client_id, | ||
| "azure_client_secret": azure_client_secret, | ||
| } | ||
| # Optional passthroughs: the kernel auto-discovers the tenant when | ||
| # absent, and sends the data token alone when no resource id is set. | ||
| azure_tenant_id = opts.get("azure_tenant_id") | ||
| if azure_tenant_id: | ||
| kwargs["azure_tenant_id"] = azure_tenant_id | ||
| azure_workspace_resource_id = opts.get("azure_workspace_resource_id") | ||
| if azure_workspace_resource_id: | ||
| kwargs["azure_workspace_resource_id"] = azure_workspace_resource_id | ||
| if federation_client_id: | ||
| kwargs["identity_federation_client_id"] = federation_client_id | ||
| return kwargs | ||
|
|
||
| # 0. Ambiguity guards — fail before any flow is chosen. | ||
| if client_secret and opts.get("credentials_provider") is not None: | ||
|
|
||
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.
🟡 Medium — The new
hostnameparameter is never used.kernel_auth_kwargsnow acceptshostname(line 152) andclient.pywas changed to passself._server_hostname, but the function body never referenceshostnameon any code path — the azure-oauth and azure-sp-m2m branches forward the selector/creds verbatim and let the kernel own resolution.The PR description says the param exists "for the effective Azure app id per cloud," which matches the intent behind the test-file import of
get_effective_azure_login_app_id— but that computation was ultimately delegated to the kernel, leaving this parameter (and theclient.pythreading) as dead plumbing. Either drop the parameter and theclient.pycall-site change, or use it. Threading a value in that the function ignores invites future readers to assume it affects resolution when it does not.