-
Notifications
You must be signed in to change notification settings - Fork 50
feat(kernel): thread Azure Entra auth (U2M + SP M2M) through the kernel path #505
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
38b4fce
acbccbf
a748e80
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 |
|---|---|---|
| @@ -1 +1 @@ | ||
| eff8950428f4e6cc9975c663ec919f334962f7d0 | ||
| 1495174dac3aae39f23c34040fe639c54a3c3f35 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -252,6 +252,14 @@ export type KernelNativeConnectionOptions = KernelSessionDefaults & | |
| oauthScopes?: Array<string>; | ||
| oauthClientId?: string; | ||
| } | ||
| | { | ||
| hostName: string; | ||
| httpPath: string; | ||
| authMode: 'AzureSpM2m'; | ||
| azureClientId: string; | ||
| azureClientSecret: string; | ||
| azureTenantId?: string; | ||
| } | ||
| ); | ||
|
|
||
| function prependSlash(str: string): string { | ||
|
|
@@ -261,6 +269,28 @@ function prependSlash(str: string): string { | |
| return str; | ||
| } | ||
|
|
||
| /** | ||
| * Azure Databricks host suffixes — the superset the Thrift driver's | ||
| * `OAuthManager.getManager` recognises (`.azuredatabricks.net`, | ||
| * `.databricks.azure.us`, `.databricks.azure.cn`). Used to decide whether an | ||
| * OAuth connection is on Azure and therefore subject to the in-house-vs- | ||
| * Entra-direct split. | ||
| */ | ||
| const AZURE_HOST_SUFFIXES = ['.azuredatabricks.net', '.databricks.azure.us', '.databricks.azure.cn']; | ||
|
|
||
| /** | ||
| * True when `host` is an Azure Databricks workspace host. Normalises the input | ||
| * the same way `getManager` does (lowercase, strip scheme + any path) so a | ||
| * caller passing a bare host or a full URL is treated identically. | ||
| */ | ||
| function isAzureHost(host: string): boolean { | ||
| const normalized = host | ||
| .toLowerCase() | ||
| .replace(/^https?:\/\//, '') | ||
| .split('/')[0]; | ||
| return AZURE_HOST_SUFFIXES.some((suffix) => normalized.endsWith(suffix)); | ||
| } | ||
|
Comment on lines
+286
to
+292
|
||
|
|
||
| /** | ||
| * Reject inputs that pass `typeof === 'string' && length > 0` but are | ||
| * structurally useless as credentials: whitespace-only strings, and the | ||
|
|
@@ -481,11 +511,25 @@ export function buildKernelHttpOptions(options: ConnectionOptions): KernelHttpOp | |
| * binding makes them, happen below the TypeScript layer and are not | ||
| * observable from this repo. | ||
| * | ||
| * Azure (Entra) on the OAuth path. The kernel runs a single, cloud-blind | ||
| * in-house U2M flow and workspace-OIDC M2M; only Entra-direct **M2M** gets a | ||
| * dedicated kernel mode: | ||
| * - **U2M (no secret), any cloud, any `useDatabricksOAuthInAzure`** → | ||
| * `OAuthU2m`. The kernel uses the workspace's OIDC-discovered authorize | ||
| * endpoint (`{host}/oidc/v1/authorize`) verbatim; that in-house | ||
| * workspace-federated flow works against Azure workspaces too (they federate | ||
| * the browser login to Entra server-side — verified E2E). So Azure U2M is | ||
| * NOT special-cased and NOT rejected — it forwards the in-house app | ||
| * (`databricks-sql-connector`) + `sql offline_access`, exactly like AWS/GCP. | ||
| * - **M2M (secret) with `useDatabricksOAuthInAzure: true`** (or non-Azure) → | ||
| * `OAuthM2m` (workspace-OIDC client-credentials). | ||
| * - **M2M (secret) on an Azure host with `useDatabricksOAuthInAzure` absent/ | ||
| * `false`** (Entra-direct) → Azure service-principal M2M (`AzureSpM2m`); the | ||
| * Entra SP creds ride `oauthClientId`/`oauthClientSecret`, `azureTenantId` | ||
| * optional (kernel auto-discovers). | ||
| * - On a non-Azure host `useDatabricksOAuthInAzure` is inert. | ||
| * | ||
| * Out of scope on the OAuth paths (rejected with a clear error): | ||
| * - `azureTenantId` / `useDatabricksOAuthInAzure` → Microsoft Entra | ||
| * direct flow. The kernel uses workspace-OIDC discovery (which works | ||
| * against Azure workspaces too — they serve `/oidc/.well-known/...`) | ||
| * and does not implement the Entra-direct scope-rewrite path. | ||
| * - `persistence` on M2M → M2M tokens are not cached (re-issuing is | ||
| * cheap; no refresh token). | ||
| * - `persistence` on U2M → custom token store is a parity gap; | ||
|
|
@@ -499,7 +543,7 @@ export function buildKernelHttpOptions(options: ConnectionOptions): KernelHttpOp | |
| * | ||
| * Throws: | ||
| * - `AuthenticationError` for missing/blank required credentials. | ||
| * - `HiveDriverError` for unsupported auth modes / Azure-direct / | ||
| * - `HiveDriverError` for unsupported auth modes / Entra-direct U2M / | ||
|
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 — Stale docstring: the |
||
| * custom persistence / ambiguous combinations. | ||
| */ | ||
| /** | ||
|
|
@@ -667,12 +711,46 @@ export function buildKernelConnectionOptions(options: ConnectionOptions): Kernel | |
| ); | ||
| } | ||
|
|
||
| if (oauth.azureTenantId !== undefined || oauth.useDatabricksOAuthInAzure === true) { | ||
| throw new HiveDriverError( | ||
| 'kernel backend: Azure-direct OAuth (azureTenantId / useDatabricksOAuthInAzure) ' + | ||
| 'is not supported. The workspace-OIDC discovery path handles Azure workspaces ' + | ||
| 'today without these options.', | ||
| ); | ||
| // Azure Entra-direct **M2M** → the kernel's dedicated azure-sp-m2m. Mirroring | ||
| // the Thrift driver's `OAuthManager.getManager`, an Azure host with | ||
| // `useDatabricksOAuthInAzure` NOT set to true (the Entra-direct default) plus a | ||
| // secret is an Entra service-principal client-credentials flow: the Entra SP | ||
| // credentials ride the generic `oauthClientId` / `oauthClientSecret` (Thrift | ||
| // convention); forward them as `azureClientId` / `azureClientSecret`. | ||
| // `azureTenantId` is optional — the kernel auto-discovers it from the workspace | ||
| // `/aad/auth` redirect when omitted. | ||
| // | ||
| // Azure **U2M** is deliberately NOT special-cased and NOT rejected. The kernel | ||
| // runs a single, cloud-blind in-house U2M flow: it uses the workspace's | ||
| // OIDC-discovered authorize endpoint (`{host}/oidc/v1/authorize`) verbatim, and | ||
| // that in-house workspace-federated flow works against Azure workspaces (the | ||
| // workspace federates the browser login to Entra server-side; verified E2E). So | ||
| // ALL U2M — including Azure, with or without `useDatabricksOAuthInAzure` — falls | ||
| // through to the standard `OAuthU2m` path below, which forwards the in-house app | ||
| // (`databricks-sql-connector`) + `sql offline_access` scopes, exactly like | ||
| // AWS/GCP. Handing the kernel the Thrift Azure Entra-direct app / scope instead | ||
| // would derail its in-house flow to a broken AAD authorize URL. | ||
| // The `oauthClientSecret !== undefined` check is inline (not extracted to a | ||
| // const) so TypeScript narrows it to `string` for the AzureSpM2m literal below. | ||
| if ( | ||
| isAzureHost(options.host) && | ||
|
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 new Azure SP M2M branch |
||
| oauth.useDatabricksOAuthInAzure !== true && | ||
| oauth.oauthClientSecret !== undefined | ||
| ) { | ||
| const azureClientId = oauth.oauthClientId; | ||
| if (azureClientId === undefined) { | ||
|
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 — The Azure SP M2M branch introduces a divergence from the regular M2M path: regular M2M defaults a missing |
||
| throw new HiveDriverError( | ||
| 'kernel backend: Azure service-principal M2M requires `oauthClientId` (the Entra ' + | ||
| 'app-registration client id) alongside `oauthClientSecret`.', | ||
| ); | ||
|
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 — New error path lacks test coverage. The Azure SP M2M branch throws |
||
| } | ||
| const azure = { | ||
|
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 — The new 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 Entra-direct Azure SP M2M is conceptually an M2M flow (no refresh token, re-issued on expiry), so the same rationale for rejecting Consider adding the same |
||
| ...base, | ||
| authMode: 'AzureSpM2m' as const, | ||
| azureClientId, | ||
| azureClientSecret: oauth.oauthClientSecret, | ||
| }; | ||
| return oauth.azureTenantId !== undefined ? { ...azure, azureTenantId: oauth.azureTenantId } : azure; | ||
| } | ||
|
|
||
| // Flow selector + client-id resolution mirror the Thrift driver EXACTLY | ||
|
|
||
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 —
KERNEL_REVis bumped to1495174d, which the PR description states is a temporary pin to the tip of the unmerged kernel branch #280 (stacked on #263). Pinning kernel-e2e to an unmerged branch tip is fragile — if that branch is rebased, force-pushed, or deleted, the connector's kernel build breaks with no code change here. Additionally, the@databricks/databricks-sql-kernel-*@0.2.0npm pins inpackage.jsonstill lack theAzureSpM2msurface, so end users on the published kernel who hit the new Azure SP M2M path will get a native-side rejection at connect time. Please confirm this PR is gated behind the kernel #263/#280 merge + release (re-pointKERNEL_REVto amainSHA and bump the npm pins) before it lands, rather than merging with the temporary branch-tip pin.