Skip to content

Reject secret scope permissions with no principal - #6215

Open
radakam wants to merge 3 commits into
mainfrom
fix-secret-scope-permission-principal
Open

Reject secret scope permissions with no principal#6215
radakam wants to merge 3 commits into
mainfrom
fix-secret-scope-permission-principal

Conversation

@radakam

@radakam radakam commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Changes

Add errorForInvalidSecretScopePermissions to validate.Required(). Validate and deploy fail early with:

Error: secret scope permission principal is required
  at resources.secret_scopes.<key>.permissions[0]

Set one of user_name, group_name or service_principal_name

Diagnostics point at the scope (ApplyBundlePermissions drops per-entry locations). sortDiagnostics ties on path when locations match.

Why

A secret scope permission with no principal passes bundle validate (only a type warning for wrong-typed values). Three shapes slip through: missing, empty-string, and wrong-typed principal (normalization warns and drops it).

Without this check, failure is late and engine-specific:

Direct — deploy fails in SecretScopeFixups before any //secrets calls:

Error: Failed to collapse permissions for secret scope
  at resources.secret_scopes.no_principal

missing principal in permissions for secret scope "test-scope-no-principal"

Terraform — deploy succeeds and creates scopes plus ACLs with "principal": "". Destroy then fails and cannot clean up:

Error: cannot read secret acl: principal cannot be empty

  with databricks_secret_acl.secret_acl_no_principal_0,
  on bundle.tf.json line 30, in resource.databricks_secret_acl.secret_acl_no_principal_0:
  30:       },

Found by fuzz testing.

Tests

New acceptance test bundle/validate/secret_scope_required_principal covering the three bad shapes plus a valid one. Asserts early rejection and zero //secrets requests on deploy.

A secret scope permission that names no principal is accepted by
`bundle validate` today, but the backend rejects a secret ACL with an
empty principal. The direct engine only fails later while collapsing
permissions, and Terraform creates the scope before the ACL call fails,
leaving a partial deploy that `destroy` cannot clean up.

Reject the input at validation time so both engines fail early with a
located, actionable error. A principal of the wrong type counts as
missing: normalization only warns and drops the value, so the permission
would otherwise still reach the backend without a principal.
@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 65171ea

Run: 31486274522

Env 🔄​flaky 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
💚​ aws linux 4 4 294 1129 6:33
💚​ aws windows 4 4 296 1127 6:48
🔄​ azure linux 4 4 4 289 1129 7:12
💚​ azure windows 4 4 295 1127 6:36
🔄​ gcp linux 1 1 5 293 1129 8:30
💚​ gcp windows 1 5 296 1127 6:44
13 interesting tests: 5 flaky, 4 RECOVERED, 4 SKIP
Test Name aws linux aws windows azure linux azure windows gcp linux gcp windows
💚​ TestAccept 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
🙈​ TestAccept/bundle/invariant/no_drift 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🔄​ TestFsCpFileToFileFileNotOverwritten ✅​p ✅​p 🔄​f ✅​p ✅​p ✅​p
🔄​ TestFsCpFileToFileFileNotOverwritten/uc-volumes_to_uc-volumes ✅​p ✅​p 🔄​f ✅​p ✅​p ✅​p
🔄​ TestSyncNestedFolderSync ✅​p ✅​p ✅​p ✅​p 🔄​f ✅​p
🔄​ TestFilerWorkspaceNotebook ✅​p ✅​p 🔄​f ✅​p ✅​p ✅​p
🔄​ TestFilerWorkspaceNotebook/scalaJupyterNb.ipynb ✅​p ✅​p 🔄​f ✅​p ✅​p ✅​p
💚​ TestFetchRepositoryInfoAPI_FromRepo 💚​R 💚​R 💚​R 💚​R 🙈​S 🙈​S
💚​ TestFetchRepositoryInfoAPI_FromRepo/root 💚​R 💚​R 💚​R 💚​R
💚​ TestFetchRepositoryInfoAPI_FromRepo/subdir 💚​R 💚​R 💚​R 💚​R
Top 6 slowest tests (at least 2 minutes):
duration env testname
6:40 aws windows TestAccept
6:36 gcp windows TestAccept
6:27 azure windows TestAccept
3:03 azure linux TestAccept
3:02 gcp linux TestAccept
3:01 aws linux TestAccept

@radakam
radakam marked this pull request as ready for review August 11, 2026 08:18
@github-actions

Copy link
Copy Markdown
Contributor

Approval status: pending

/acceptance/bundle/ - needs approval

5 files changed
Suggested: @janniklasrose
Also eligible: @denik, @pietern, @andrewnester, @shreyas-goenka, @lennartkats-db, @anton-107

/bundle/ - needs approval

Files: bundle/config/validate/required.go
Suggested: @janniklasrose
Also eligible: @denik, @pietern, @andrewnester, @shreyas-goenka, @lennartkats-db, @anton-107

General files (require maintainer)

Files: .nextchanges/bundles/secret-scope-permission-principal-required.md
Based on git history:

  • @janniklasrose -- recent work in .nextchanges/bundles/

Any maintainer (@andrewnester, @anton-107, @denik, @pietern, @shreyas-goenka, @simonfaltum, @renaudhartert-db, @janniklasrose, @lennartkats-db) can approve all areas.
See OWNERS for ownership rules.

Drop dyn.MapByPattern in favor of iterating SecretScopes, and keep
comments to the non-obvious bits (wrong-typed drop, without-fix behavior).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants