Skip to content

fix(databricks-jdbc-driver): support OIDC/workload identity for export buckets#11143

Merged
bsod90 merged 1 commit into
masterfrom
fix/databricks-oidc-export-bucket
Jun 23, 2026
Merged

fix(databricks-jdbc-driver): support OIDC/workload identity for export buckets#11143
bsod90 merged 1 commit into
masterfrom
fix/databricks-oidc-export-bucket

Conversation

@bsod90

@bsod90 bsod90 commented Jun 23, 2026

Copy link
Copy Markdown
Member

Problem

A Databricks deployment configured to authenticate to its S3 export bucket via OIDC / workload identity (no static accessKeyId/secretAccessKey) fails when building pre-aggregations:

AuthorizationHeaderMalformed: The authorization header is malformed;
a non-empty Access Key (AKID) must be provided in the credential.
    at S3RestXmlProtocol.handleError (@aws-sdk/core/.../protocols/index.js)
    ...

The Databricks driver's export-bucket S3 path passed credentials unconditionally:

credentials: {
  accessKeyId: this.config.awsKey || '',
  secretAccessKey: this.config.awsSecret || '',
},
region: this.config.awsRegion || '',

Under OIDC there are no static keys, so the SDK receives accessKeyId: '' / secretAccessKey: '' and treats them as explicit (malformed) credentials instead of falling back to its default provider chain (AWS_WEB_IDENTITY_TOKEN_FILE, IRSA, env vars, ...).

Athena and BigQuery already work in this setup — Athena omits credentials when none are configured, and BigQuery's export bucket is GCS (default-chain-friendly). This PR brings Databricks in line and hardens the shared storage helpers.

Changes

  • cubejs-databricks-jdbc-drivergetCsvFiles omits credentials when none are configured, across all three bucket types:
    • S3: pass credentials only when both key and secret are set; omit a blank region too → AWS SDK default chain resolves the web-identity token file.
    • Azure: pass undefined (not '') → falls through to DefaultAzureCredential, honoring AZURE_FEDERATED_TOKEN_FILE + AZURE_CLIENT_ID/AZURE_TENANT_ID.
    • GCS: gcsCredentials || undefined → Google SDK falls back to Application Default Credentials (GOOGLE_APPLICATION_CREDENTIALS, including WIF external_account configs).
  • cubejs-base-driver (defense-in-depth at the library boundary):
    • aws.fs: new normalizeS3ClientConfig strips blank static credentials and a blank region before new S3(...). Protects every S3-using driver. Credential provider functions (e.g. fromTemporaryCredentials) and fully-populated static credentials are left untouched.
    • gcs.fs: new hasGCSCredentials treats empty string/object as absent so the Google SDK uses ADC.
    • azure.fs: unchanged — already falls through to DefaultAzureCredential correctly.

Testing

  • cubejs-base-driver/test/unit/storage-fs.test.ts (new): verifies the normalizers, and that the S3 / GCS / Azure SDK clients construct without throwing when no credentials are passed (so the default chain can engage).
  • cubejs-databricks-jdbc-driver/test/DatabricksDriver.test.ts (extended): drives unload() with the S3 SDK mocked — asserts credentials are omitted under OIDC and passed through when static keys are configured.

🤖 Generated with Claude Code

…t buckets

When a Databricks export bucket is configured for OIDC / workload identity
there are no static credentials, but the driver passed
`{ accessKeyId: '', secretAccessKey: '' }` to the S3 client unconditionally.
The AWS SDK treats those empty strings as explicit credentials and the request
fails with `AuthorizationHeaderMalformed: ... a non-empty Access Key (AKID)
must be provided`, instead of falling back to the default provider chain
(`AWS_WEB_IDENTITY_TOKEN_FILE`, IRSA, ...).

Athena and BigQuery already work in this setup (Athena omits credentials when
none are set; BigQuery uses GCS). This brings Databricks in line:

- DatabricksDriver omits credentials across all three bucket types (S3, GCS,
  Azure) when none are configured, so the cloud SDK default chain /
  `DefaultAzureCredential` (`AZURE_FEDERATED_TOKEN_FILE`) resolves them.
- base-driver `aws.fs` adds `normalizeS3ClientConfig`, which strips blank static
  credentials and a blank region before constructing the S3 client — protecting
  every S3-using driver, while leaving credential provider functions and
  fully-populated static credentials untouched.
- base-driver `gcs.fs` adds `hasGCSCredentials`, treating empty string/object as
  absent so the Google SDK falls back to Application Default Credentials.

Tests: base-driver unit tests cover the normalizers and verify the S3/GCS/Azure
SDK clients construct without throwing when no credentials are passed; the
Databricks driver test covers OIDC (no keys) and static-key S3 cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@bsod90 bsod90 requested a review from a team as a code owner June 23, 2026 15:52
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @bsod90's task in 1m 51s —— View job


Claude Code Review

Verdict: LGTM — clean fix, well-targeted, defense-in-depth at the right boundary. 0 high, 0 medium, 2 low.

Full review (expand)
  • Read PR description and changed files list
  • Review aws.fs.ts changes
  • Review gcs.fs.ts changes
  • Review DatabricksDriver.ts changes
  • Review test files
  • Check for stale review threads
  • Post final review

Summary

The fix correctly identifies the root cause: passing empty-string credentials to the AWS SDK is interpreted as explicit credentials, blocking the default provider chain (web identity token file for OIDC/IRSA). The fix is applied in two layers:

  1. Driver-level (DatabricksDriver.getCsvFiles) — omits credentials per bucket type.
  2. Library-level (base-driver/storage-fs/{aws,gcs}.fs) — normalizes inputs so every S3/GCS-using driver is protected.

Since extractUnloadedFilesFromS3 is reused by Snowflake, Redshift, Presto, ClickHouse, BigQuery, Athena, and Databricks, the normalizeS3ClientConfig hardening fixes the same class of bug for all of them — good call.

Code quality

  • normalizeS3ClientConfig correctly distinguishes static-credential objects ('accessKeyId' in credentials) from credential providers (functions used by fromTemporaryCredentials/fromWebToken), so it doesn't break temporary-credential flows.
  • Non-mutating copy via { ...clientOptions } — input config is preserved (covered by test).
  • The driver-side conditional spreads (...(credentials ? { credentials } : {})) are redundant with normalizeS3ClientConfig but match the PR's stated defense-in-depth philosophy. Fine.
  • Doc comments are clear and reference the actual failure mode and env vars.

Tests

  • Unit tests cover the key normalization cases plus the construction smoke tests for all three SDKs (S3/GCS/Azure).
  • Driver-level tests exercise both OIDC (no keys) and static-keys paths and assert the actual config the S3 SDK receives.
  • Good coverage of the originally-failing scenario.

Minor / low-priority observations

1. normalizeS3ClientConfig does not drop an empty-object credentials value.
At packages/cubejs-base-driver/src/storage-fs/aws.fs.ts:26-33, the guard 'accessKeyId' in credentials returns false for credentials: {}, so an empty object passes through to the SDK. In practice no in-tree caller does this (they pass either a fully-populated object or undefined), but for symmetry with hasGCSCredentials you could also drop credentials when the object has no own accessKeyId key at all. Not blocking.

2. gcs.fs.ts still does gcsConfig.credentials.project_id.
At packages/cubejs-base-driver/src/storage-fs/gcs.fs.ts:34, when credentials are present we read gcsConfig.credentials.project_id. The DatabricksDriver types gcsCredentials as a string and passes it through as the credentials value, so if someone configures a (JSON-string) value, this .project_id would be undefined (today and before this PR). This isn't introduced by this PR, but the call site might warrant a follow-up to either parse the string upstream or document that callers must pass a parsed object. Not in scope here.

Security / perf

No security concerns — the change moves credential resolution to the SDK's default chain, which is the documented safe path for OIDC/workload identity. No performance impact.

Nice fix.
· branch: fix/databricks-oidc-export-bucket

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.80%. Comparing base (4d4b8cd) to head (c8086f6).

Files with missing lines Patch % Lines
...ckages/cubejs-base-driver/src/storage-fs/aws.fs.ts 90.00% 1 Missing ⚠️
...ckages/cubejs-base-driver/src/storage-fs/gcs.fs.ts 85.71% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (4d4b8cd) and HEAD (c8086f6). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (4d4b8cd) HEAD (c8086f6)
cubesql 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #11143       +/-   ##
===========================================
- Coverage   79.18%   58.80%   -20.38%     
===========================================
  Files         474      217      -257     
  Lines       96880    17511    -79369     
  Branches     3558     3563        +5     
===========================================
- Hits        76712    10298    -66414     
+ Misses      19650     6695    -12955     
  Partials      518      518               
Flag Coverage Δ
cube-backend 58.80% <88.23%> (+0.06%) ⬆️
cubesql ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bsod90 bsod90 merged commit f4d2dde into master Jun 23, 2026
87 of 88 checks passed
@bsod90 bsod90 deleted the fix/databricks-oidc-export-bucket branch June 23, 2026 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants