-
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
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,24 @@ | |
| 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 route to the | ||
| kernel's *generic* OAuth flows with Azure values as overrides (the | ||
| kernel needs no Azure-specific code): | ||
|
|
||
| - ``azure-oauth`` (U2M) → ``oauth-u2m`` with the Azure app client id | ||
| (``96eecda7-…``), redirect port ``8030``, and the AAD delegated scope | ||
| ``{app_id}/user_impersonation offline_access`` (via | ||
| ``AzureOAuthEndpointCollection``, honoring ``DATABRICKS_AZURE_TENANT_ID``). | ||
| The kernel discovers endpoints via the workspace ``/oidc`` redirector, | ||
| which an Azure workspace redirects to Entra (PECOBLR-4120). | ||
| - ``azure-sp-m2m`` (M2M) → ``oauth-m2m`` with the Azure service-principal | ||
| credentials, an Entra v2.0 ``token_url``, and the | ||
| ``{effective_app_id}/.default`` scope (PECOBLR-4141). ``azure_tenant_id`` | ||
| is required (the kernel path does not auto-discover it). The Azure | ||
| management-token header and ``azure_workspace_resource_id`` are **not** | ||
| applied on the kernel path — no SQL connector uses them; an SP that is | ||
| not a workspace member (RBAC-only) is unsupported here. | ||
|
|
||
| ``identity_federation_client_id`` is forwarded with whichever auth shape | ||
| wins resolution. It selects mandatory SP-wide workload-identity token | ||
|
|
@@ -59,9 +72,15 @@ | |
| PYSQL_OAUTH_SCOPES, | ||
| ) | ||
| from databricks.sql.auth.authenticators import AccessTokenAuthProvider, AuthProvider | ||
| from databricks.sql.auth.common import get_effective_azure_login_app_id | ||
| from databricks.sql.auth.token_federation import TokenFederationProvider | ||
| from databricks.sql.exc import NotSupportedError, ProgrammingError | ||
|
|
||
| # Entra (Azure AD) v2.0 token endpoint template. The kernel's generic M2M | ||
| # provider sends the credentials as ``scope`` (v2.0), so we point it at the | ||
| # v2.0 endpoint (the connector's own SP path uses the v1.0 ``resource`` form). | ||
| _AZURE_AAD_LOGIN_HOST = "https://login.microsoftonline.com" | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
|
|
@@ -134,6 +153,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 +174,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 routed to the kernel's generic OAuth 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 +189,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 +208,80 @@ 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 to | ||
| # oauth-m2m with the Azure app credentials, an Entra v2.0 token endpoint, | ||
| # and the {effective_app_id}/.default scope. The kernel sends the client | ||
| # secret via HTTP Basic (which Entra v2.0 accepts) and, because a | ||
| # token_url override is set, skips workspace OIDC discovery. PECOBLR-4141. | ||
| # | ||
| # NOT applied on the kernel path: the Azure management-token header | ||
| # (X-Databricks-Azure-SP-Management-Token) and azure_workspace_resource_id. | ||
| # No SQL connector (Go, Node) uses them; the Databricks-audience token | ||
| # authenticates SPs that are workspace principals (the SQL norm). An SP with | ||
| # only an Azure RBAC role (not a workspace member) is unsupported here. | ||
| if auth_type == "azure-sp-m2m": | ||
| azure_client_id = opts.get("azure_client_id") | ||
| azure_client_secret = opts.get("azure_client_secret") | ||
| azure_tenant_id = opts.get("azure_tenant_id") | ||
| if not (azure_client_id and azure_client_secret): | ||
| raise ProgrammingError( | ||
| "auth_type='azure-sp-m2m' requires azure_client_id and " | ||
| "azure_client_secret." | ||
| ) | ||
| if not azure_tenant_id: | ||
| # The Thrift path auto-discovers the tenant from the workspace's | ||
| # /aad/auth redirect; the kernel path does not make that call, so | ||
| # require it explicitly rather than silently guessing. | ||
| raise NotSupportedError( | ||
| "use_kernel=True auth_type='azure-sp-m2m' requires an explicit " | ||
| "azure_tenant_id (the kernel path does not auto-discover the " | ||
| "Azure tenant from the workspace as the Thrift backend does)." | ||
| ) | ||
| if opts.get("azure_workspace_resource_id"): | ||
| logger.warning( | ||
| "azure_workspace_resource_id is ignored on use_kernel=True: the " | ||
| "Azure management-token flow (X-Databricks-Azure-SP-Management-" | ||
| "Token) is not applied on the kernel path. The Databricks-" | ||
| "audience token authenticates service principals that are " | ||
| "workspace principals; an RBAC-only SP is unsupported here." | ||
| ) | ||
| app_id = get_effective_azure_login_app_id(hostname or "") | ||
| kwargs = { | ||
|
Comment on lines
+274
to
+275
|
||
| "auth_type": "oauth-m2m", | ||
| "client_id": azure_client_id, | ||
| "client_secret": azure_client_secret, | ||
| "token_url": f"{_AZURE_AAD_LOGIN_HOST}/{azure_tenant_id}/oauth2/v2.0/token", | ||
| "oauth_scopes": [f"{app_id}/.default"], | ||
| } | ||
| 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 module docstring here describes
azure-oauthas routing→ ``oauth-u2m`` with the Azure app client id (``96eecda7-…``), redirect port ``8030``, and the AAD delegated scope ``{app_id}/user_impersonation offline_access`` (via ``AzureOAuthEndpointCollection``…). That does not match what the code actually does. Theazure-oauthbranch (line 226) forwardsauth_type="azure-oauth"(notoauth-u2m) and constructs none of those values — the inline comment at lines 218–225 explicitly states "this binding does NOT construct endpoints/scopes — it just passesauth_type='azure-oauth'plus any optional client_id / redirect_port passthrough" and that the kernel pins the client id / port 8030 / scope.So the top-of-file docstring directly contradicts both the executable code and the inline comment beside it: it claims the bridge maps to
oauth-u2mand synthesises the Azure bundle, when in fact it delegates all of that to the kernel via theazure-oauthselector. A maintainer reading the module docstring will expectAzureOAuthEndpointCollection.get_scopes_mappingto be called here (it never is on this path). Please align the docstring with the actual selector-forwarding behavior.