Skip to content

nhi: sync SP OAuth secrets + workspace PATs as STATIC_SECRET resources (Phase-1 class-B)#48

Open
c1-squire-dev[bot] wants to merge 5 commits into
mainfrom
pqprime/IGA-NHI/sync-sp-oauth-secrets-and-workspace-pats
Open

nhi: sync SP OAuth secrets + workspace PATs as STATIC_SECRET resources (Phase-1 class-B)#48
c1-squire-dev[bot] wants to merge 5 commits into
mainfrom
pqprime/IGA-NHI/sync-sp-oauth-secrets-and-workspace-pats

Conversation

@c1-squire-dev

@c1-squire-dev c1-squire-dev Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

🔗 Preview: C1

Summary

Adds two new TRAIT_SECRET resource types to baton-databricks. This is Phase-1 of the pqprime NHI program (bd pqprime-16u), closing the class-B credential gap: the connector previously synced zero secret-shaped resources.

A1 — service_principal_secret (SP OAuth client secrets)

  • API: GET /api/2.0/accounts/{account_id}/servicePrincipals/{service_principal_id}/credentials/secrets
    Ref: Databricks Service Principal Secrets API
  • Account-level, no new auth scope (connector's existing account SP credential).
  • Per-SP fan-out; cursor-based page_token/next_page_token pagination.
  • STATIC_SECRET, detail databricks.sp_oauth_secret, created_at from create_time.
  • identity_id back-ref → the parent SP resource (K3 APP_REGISTRATION already emitted).
  • Secret value is never returned by the API (only secret_hash).
  • expires_at / last_used absent from SecretInfo → omitted per spec.

A2 — workspace_pat (workspace personal access tokens)

  • API: GET /api/2.0/token-management/tokens (workspace-admin endpoint)
    Ref: Databricks Token Management API
  • Per-workspace fan-out; workspace-admin required.
  • STATIC_SECRET, detail databricks.pat, created_at from creation_time, expires_at from expiry_time (skipped when ≤0 / "never expires").
  • identity_id back-ref → owning user resource via created_by_id.
  • Graceful degradation: 401/403 → log + skip workspace, sync continues.

Implementation notes

  • PageTokenVars added to pkg/databricks/vars.go for cursor pagination.
  • No V1Identifier set anywhere (per precedent baton-temporalcloud#82 r3380796810).
  • 13 unit tests: basic listing, pagination, 401/403 graceful degrade, wrong/nil parent, timestamp parsing, entitlements/grants invariants.

Test plan

  • go test ./pkg/connector/... — 13 new tests, all pass
  • go build ./... — clean
  • go vet ./pkg/... — clean

Built with pqprime · bd pqprime-16u · spec


Exposed endpoints

🏰 Squire environment: sunny-dragon-90701
Task: 1870ba14-58ea-4835-8fb5-affd78ad616e

…s (Phase-1 class-B)

Adds two new TRAIT_SECRET resource types to baton-databricks (pqprime NHI
program, bd pqprime-16u):

A1 — service_principal_secret (SP OAuth client secrets)
- New client method ListServicePrincipalSecrets: account-level API
  GET /api/2.0/accounts/{acct}/servicePrincipals/{sp_id}/credentials/secrets
  with cursor-based page_token/next_page_token pagination.
- Builder fans out per synced SP (parent = service_principal resource).
- STATIC_SECRET, detail "databricks.sp_oauth_secret", created_at from
  create_time, identity_id back-ref to the parent SP resource (K3
  APP_REGISTRATION already emitted by the SP builder).
- Secret value is never returned by the API; metadata only.

A2 — workspace_pat (workspace personal access tokens)
- New client method ListTokenManagementTokens: workspace-level admin endpoint
  GET /api/2.0/token-management/tokens; fans out per synced workspace.
- Builder lists all workspace PATs (parent = workspace resource).
- STATIC_SECRET, detail "databricks.pat", created_at from creation_time,
  expires_at from expiry_time (skipped when ≤0), identity_id from
  created_by_id → user resource.
- Graceful degradation: 401/403 responses are logged and the workspace is
  skipped; the rest of the sync continues.

Also adds:
- PageTokenVars to pkg/databricks/vars.go for cursor-based pagination.
- 13 unit tests covering basic listing, pagination, 401/403 graceful degrade,
  wrong/nil parent types, timestamp parsing, and no-entitlements invariant.
- Both resource types registered in ResourceSyncers.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@c1-squire-dev c1-squire-dev Bot requested a review from a team June 10, 2026 17:53
Comment on lines +31 to +32
newServicePrincipalSecretBuilder(d.client),
newTokenBuilder(d.client),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: These builders expect fan-out over parent resources (servicePrincipalResourceType and workspaceResourceType respectively), but neither parent resource declares the new types as children via ChildResourceType annotations. Without those annotations, the SDK will never call these builders' List methods with a parent resource ID — so no SP secrets or workspace PATs will be synced.

Fix: Add &v2.ChildResourceType{ResourceTypeId: servicePrincipalSecretResourceType.Id} to the SP resource in service-principals.go:servicePrincipalResource, and &v2.ChildResourceType{ResourceTypeId: workspacePATResourceType.Id} to the workspace resource in workspaces.go:workspaceResource.

Comment thread pkg/connector/sp_secrets.go Outdated
Comment on lines +63 to +82
secrets, nextToken, _, err := b.client.ListServicePrincipalSecrets(ctx, spID, pageToken)
if err != nil {
return nil, nil, fmt.Errorf("databricks-connector: failed to list SP secrets for %s: %w", spID, err)
}

l.Debug("listed SP OAuth secrets", zap.String("sp_id", spID), zap.Int("count", len(secrets)))

for i := range secrets {
r, err := b.spSecretResource(&secrets[i], parentResourceID)
if err != nil {
return nil, nil, err
}
rv = append(rv, r)
}

if nextToken == "" {
break
}
pageToken = nextToken
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: This loop fetches all pages in-connector before returning, which buffers all secrets in memory. Per F2/R8, the SDK should drive pagination one page at a time for checkpointing and cancellation. In practice SP secret counts are tiny so this is unlikely to cause real problems, but consider using SDK pagination bags (like servicePrincipalBuilder.List does) to stay consistent with the codebase pattern and let the SDK checkpoint between pages.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Connector PR Review: nhi: sync SP OAuth secrets + workspace PATs as STATIC_SECRET resources (Phase-1 class-B)

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Review mode: incremental since c2b5add
View review run

Review Summary

The new commits add documentation updates: README.md now lists the two new resource types, and docs/connector.mdx adds capability table rows for "Service principal secrets" and "Workspace personal access tokens" (sync-only). This directly addresses the previous review finding about the stale capabilities table (D1). Full PR diff re-scanned for security and correctness — no new issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see review comments.

servicePrincipalSecretBuilder.List was silently unrolling all pages for
a given SP inside a single sync-engine call. Use opts.PageToken.Token as
the API cursor and return SyncOpResults.NextPageToken so the sync engine
drives pagination one page per call — bounded memory, checkpointable.

Tests updated to exercise the two-call round-trip instead of the
internal-loop behaviour.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see review comments.

…otations

- Wrap json.NewEncoder(w).Encode calls in require.NoError(t, ...) in
  sp_secrets_test.go and tokens_test.go to satisfy errcheck linter.
- Add //nolint:gosec to workspaceTokensEndpoint URL constant (G101 false
  positive — path string, not a credential).
- Add ChildResourceType annotations: SP resource declares
  servicePrincipalSecretResourceType as a child so the SDK fans out
  List calls per SP; workspace resource adds workspacePATResourceType
  alongside the existing roleResourceType child.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

gosec v2.11.4 (used by CI) flags the hardcoded "cursor-page-2" string
as a potential credential when it appears as the value of a key whose
name contains "token". Rename it to paginatorCursor so the variable
name carries no sensitive semantics.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

…s table

The PR introduced two new sync-only resource types (service_principal_secret
and workspace_pat) but the docs/connector.mdx capabilities table and README
data model list were not updated. Add rows for both types with sync enabled
and provision disabled.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

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.

1 participant