feat(kernel): thread Azure Entra auth (U2M + SP M2M) through the kernel path - #505
feat(kernel): thread Azure Entra auth (U2M + SP M2M) through the kernel path#505eric-wang-1990 wants to merge 2 commits into
Conversation
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 2 Low
Looks good — the Entra-direct routing (useDatabricksOAuthInAzure splitting in-house vs Entra-direct, SP M2M → AzureSpM2m, Entra-direct U2M rejected) is correct and matches the described table and the updated tests. Two low-severity consistency notes: persistence is silently accepted on the new AzureSpM2m branch (rejected everywhere else on the M2M/U2M paths, and contradicts the contract docblock), and the oauthClientId-required rejection for Azure SP M2M lacks a unit test. The bundled bot-infra changes (.bot/config.yaml, learning-log, workflow) are unrelated to the auth feature but self-contained.
| 'app-registration client id) alongside `oauthClientSecret`.', | ||
| ); | ||
| } | ||
| const azure = { |
There was a problem hiding this comment.
🔵 Low — The new AzureSpM2m (Entra-direct SP M2M) branch does not reject a persistence hook, whereas both the regular M2M arm (persistence → HiveDriverError "not supported on OAuth M2M") and the U2M arm reject it. AzureSpM2m is fundamentally an M2M client-credentials flow (no refresh token), so the same rationale applies — but here a caller who passes persistence alongside Azure SP creds has it silently dropped instead of getting the clear, documented error. This also contradicts the function's own contract docblock, which lists "persistence on M2M → rejected with a clear error" as out-of-scope for the OAuth paths. Consider rejecting persistence in the entraDirect M2M path for parity.
| // `azureTenantId` is optional — the kernel auto-discovers it from the | ||
| // workspace `/aad/auth` redirect when omitted. | ||
| const azureClientId = oauth.oauthClientId; | ||
| if (azureClientId === undefined) { |
There was a problem hiding this comment.
🔵 Low — The Azure SP M2M branch introduces a divergence from the regular M2M path: regular M2M defaults a missing oauthClientId to DEFAULT_OAUTH_CLIENT_ID, but AzureSpM2m throws a HiveDriverError requiring an explicit oauthClientId. This is intentional (an Entra app registration has no sensible default), but the rejection branch (Azure host + secret + no oauthClientId) has no unit test. The new M2M tests cover Azure+secret+tenant, Azure+secret+no-tenant, and the in-house useDatabricksOAuthInAzure:true case, but not this rejection. Adding a test would lock in the intended divergence and prevent a regression that silently defaults the client id on the Azure path.
…el path On useKernel=true + authType='databricks-oauth', route Azure Entra auth to the kernel instead of rejecting it. Mirrors the Thrift OAuthManager.getManager: useDatabricksOAuthInAzure selects the flavour on an Azure host. - useDatabricksOAuthInAzure: true -> in-house workspace-federated flow, which the kernel runs natively: no secret -> OAuthU2m (browser), secret -> OAuthM2m (workspace-OIDC client-credentials). Works against Azure workspaces. - absent/false on an Azure host -> Entra-direct: with a secret -> the kernel's Azure SP M2M (AzureSpM2m; Entra SP creds ride oauthClientId/oauthClientSecret, azureTenantId optional/auto-discovered); without a secret -> Entra-direct browser U2M, which the kernel does not implement -> rejected with a pointer to useDatabricksOAuthInAzure: true or the Thrift backend. - Non-Azure host: the flags are inert (in-house is the only flow). Adds an AzureSpM2m variant to the native-options union + an isAzureHost helper. Replaces the previous blanket Azure-OAuth rejection. Verified end-to-end against a live Azure workspace (U2M browser flow + SP M2M data token) with a native module built from databricks-sql-kernel#280. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
| // browser U2M; Entra-direct SP M2M maps to the kernel's dedicated | ||
| // azure-sp-m2m. On a non-Azure host these flags are inert (the in-house flow | ||
| // is the only one), matching Thrift. | ||
| const entraDirect = isAzureHost(options.host) && oauth.useDatabricksOAuthInAzure !== true; |
There was a problem hiding this comment.
Why need check isAzureHost? Did Thrift auth path needs to check this?
Point the kernel-e2e KERNEL_REV at the kernel commit that adds the napi AzureSpM2m surface (databricks-sql-kernel#280, stacked on #263), so the connector's kernel-e2e builds a kernel that can accept azure-sp-m2m. This is a temporary pin to the unmerged #280 branch tip; re-point to a kernel main SHA once #263 + #280 merge and a kernel release is cut. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
64307cd to
acbccbf
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the NodeJS kernel backend (useKernel: true) to support Azure Entra (Azure AD) authentication routing for authType: 'databricks-oauth', including Entra-direct service-principal M2M (AzureSpM2m) and in-house/workspace-federated U2M/M2M flows, and updates tests accordingly. It also adds an “engineer-bot learning” daily workflow and supporting configuration/log scaffolding.
Changes:
- Add
AzureSpM2mto the kernel native auth options union and implement Azure-host routing logic inbuildKernelConnectionOptions. - Update kernel unit tests to reflect the new Azure routing and the Entra-direct U2M rejection behavior.
- Add engineer-bot retrospective learning workflow + config/log plumbing, and bump the shared bot engine pin.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
lib/kernel/KernelAuth.ts |
Adds AzureSpM2m native options and implements Azure host routing for in-house vs Entra-direct flows. |
tests/unit/kernel/auth-u2m.test.ts |
Updates U2M kernel tests for Azure routing and Entra-direct U2M rejection. |
tests/unit/kernel/auth-m2m.test.ts |
Updates M2M kernel tests to cover Azure Entra-direct SP M2M (AzureSpM2m) and in-house routing. |
CHANGELOG.md |
Documents the new Azure Entra auth routing behavior on the kernel backend. |
.github/workflows/engineer-bot-learning.yml |
Adds a daily cron workflow to extract retrospective learnings and open/update a rolling PR. |
.github/actions/bot-prelude/action.yml |
Updates the pinned bot-engine commit SHA used by bot workflows. |
.claude/knowledge/learning-log.md |
Seeds the shared learning log file consumed by the author flow and appended by retrospective. |
.bot/config.yaml |
Wires the author phase to read the learning log and enables the retrospective configuration block. |
Suppressed comments (1)
lib/kernel/KernelAuth.ts:688
- Azure routing diverges from the Thrift
OAuthManager.getManagerbehavior for.databricks.azure.us: Thrift only treats that suffix as Azure whenuseDatabricksOAuthInAzureis false (Entra-direct), but the kernel path will currently allowuseDatabricksOAuthInAzure: trueand route it to in-houseOAuthU2m/OAuthM2m. If the goal is to mirror Thrift routing (as described in the PR), add an explicit guard for.databricks.azure.uswhenuseDatabricksOAuthInAzure:trueto avoid a silent behavior mismatch.
staticToken?: string;
federationClientId?: string;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function isAzureHost(host: string): boolean { | ||
| const normalized = host | ||
| .toLowerCase() | ||
| .replace(/^https?:\/\//, '') | ||
| .split('/')[0]; | ||
| return AZURE_HOST_SUFFIXES.some((suffix) => normalized.endsWith(suffix)); | ||
| } |
There was a problem hiding this comment.
Verdict: 1 Medium
Looks solid overall — the Azure routing mirrors Thrift's getManager split cleanly and is well-tested. One medium concern: the new AzureSpM2m branch bypasses the M2M persistence rejection, silently dropping an option that every other OAuth path rejects loudly.
| 'app-registration client id) alongside `oauthClientSecret`.', | ||
| ); | ||
| } | ||
| const azure = { |
There was a problem hiding this comment.
🟡 Medium — The Entra-direct AzureSpM2m branch returns before the generic M2M persistence guard is reached, so a persistence hook is silently dropped on Azure SP M2M — while the exact same option throws a clear HiveDriverError on the non-Azure M2M path (line ~775) and on U2M.
Azure SP M2M is conceptually an M2M flow (no refresh token, re-issued on expiry), so the same rationale for rejecting persistence applies. A caller who sets persistence and connects to an Azure host with an Entra SP secret gets their hook quietly ignored rather than the actionable error every other OAuth path produces. This is an inconsistent contract and a silent-failure footgun.
Consider adding the same if (oauth.persistence !== undefined) throw ... guard inside the entraDirect M2M branch before returning the AzureSpM2m options.
What
Threads Azure Entra (Azure AD) auth — both U2M and service-principal M2M — through the
useKernel: truepath, replacing the previous blanket Azure-OAuth rejection. This is the NodeJS analog of the Python connector's kernel Azure bridge, and it consumes the kernel Azure surface added in databricks-sql-kernel#280 (napiAuthMode::AzureSpM2m).How
lib/kernel/KernelAuth.ts— onauthType: 'databricks-oauth',useDatabricksOAuthInAzureselects the flavour on an Azure host, mirroring the ThriftOAuthManager.getManager:useDatabricksOAuthInAzuretrue(in-house)OAuthU2m(workspace-federated browser flow)true(in-house)OAuthM2m(workspace-OIDC client-credentials)false(Entra-direct)AzureSpM2m(Entra SP M2M)false(Entra-direct)useDatabricksOAuthInAzure: trueor the Thrift backendOAuthU2m/OAuthM2mThe kernel runs the in-house / workspace-federated flow natively (workspace-OIDC discovery works against Azure workspaces). The only Entra-direct flow the kernel implements is SP M2M (
azure-sp-m2m, direct Entra client-credentials + tenant auto-discovery); Entra-direct browser U2M is not implemented, so it's rejected rather than silently switched. ForAzureSpM2mthe Entra SP creds ride the genericoauthClientId/oauthClientSecret(Thrift convention);azureTenantIdis forwarded when set and auto-discovered by the kernel otherwise.New: an
AzureSpM2mvariant on the native-options union and anisAzureHosthelper.azureWorkspaceResourceId(the management-token / RBAC-only-SP path) is not exposed here yet — a follow-up; the data token authenticates the common workspace-member SP.Tests
Unit (
tests/unit/kernel/auth-m2m.test.ts,auth-u2m.test.ts) — updated the former Azure-rejection tests to the new routing and added coverage: Entra-direct M2M →AzureSpM2m(with/without tenant); in-house M2M/U2M underuseDatabricksOAuthInAzure: true; Entra-direct U2M rejection. All 310 kernel unit tests pass;prettier --check+eslintclean.E2E (live Azure workspace, native module built from kernel #280):
useDatabricksOAuthInAzure: true, no secret) — full browser authorization-code flow against{host}/oidc/v1/authorize, returned{ one: 1, who: 'e.wang@databricks.com' }. ✅{ one: 1, who: '<sp-app-id>' }. ✅Depends on
AzureSpM2msurface (stacked on kernel Upgrade version ofapache-arrowfor security #263). This PR bumpsKERNEL_REVto the Create DBSQLOperation from IDBSQLSession #280 build (1495174d) so the connector's kernel-e2e builds a kernel that can acceptazure-sp-m2m. That is a temporary pin to the unmerged Create DBSQLOperation from IDBSQLSession #280 branch tip — re-point it to a kernelmainSHA once Upgrade version ofapache-arrowfor security #263 + Create DBSQLOperation from IDBSQLSession #280 merge and a kernel release is cut. The npm@databricks/databricks-sql-kernel-* @ 0.2.0pins inpackage.jsonalso need bumping to a published kernel that ships Create DBSQLOperation from IDBSQLSession #280 before end users get the Azure SP M2M path (0.2.0 lacks it). Azure U2M rides the existingOAuthU2m, which predates Create DBSQLOperation from IDBSQLSession #280, so it needs neither. Same release-sequencing dependency as the Python connector's azure-sp-m2m PR.This pull request and its description were written by Isaac.