Skip to content

Commit 1a8ef09

Browse files
committed
fix(credentials): refuse a keyless subject another live credential already holds
A keyless subject proof stays valid for 24 hours and is not bound to one credential, so a member removed from a keyless AWS or Databricks credential could save a second credential with the same subject and proof and keep assuming the customer's role. Creating or reconnecting a credential onto a subject it did not already hold now refuses (409 conflict) when another non-revoked credential of the same owner holds it. The subject lives only in the encrypted secret, so the check decrypts the owner's keyless credentials inside the write transaction, under a per-subject advisory lock so concurrent saves serialize. A reconnect keeping its stored subject binds nothing new and skips the check.
1 parent 07d5c25 commit 1a8ef09

7 files changed

Lines changed: 331 additions & 9 deletions

File tree

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
/**
2+
* A keyless trust setup (subject plus its day-long proof) binds to one live
3+
* credential: saving it onto a second credential is refused, even concurrently,
4+
* while the credential that holds it can still be reconnected.
5+
*/
6+
7+
import { db } from '@sim/db'
8+
import { credential, credentialMember, permissions, user, workspace } from '@sim/db/schema'
9+
import { deleteWorkspaceFixture, insertWorkspaceFixture } from '@sim/db/testing/workspace-fixtures'
10+
import { jsonResponse } from '@sim/testing/helpers/http'
11+
import { posthogServerMock } from '@sim/testing/mocks/posthog-server.mock'
12+
import { generateId } from '@sim/utils/id'
13+
import { eq } from 'drizzle-orm'
14+
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'
15+
16+
vi.mock('@/lib/posthog/server', () => posthogServerMock)
17+
vi.mock('@/lib/credentials/workload-identity/issuer', async (importOriginal) => ({
18+
...(await importOriginal<typeof import('@/lib/credentials/workload-identity/issuer')>()),
19+
mintWorkloadIdentityToken: async ({ subject }: { subject: string }) => `token-for:${subject}`,
20+
}))
21+
22+
import { updateCredentialRecord } from '@/lib/credentials/orchestration'
23+
import { createCredentialRecord } from '@/lib/credentials/orchestration/credential-create'
24+
import { issueWorkloadIdentitySubject } from '@/lib/credentials/workload-identity/subject'
25+
import { DATABRICKS_SERVICE_ACCOUNT_PROVIDER_ID } from '@/lib/oauth/types'
26+
27+
const HOST = 'dbc-1234abcd-5678.cloud.databricks.com'
28+
const CLIENT_ID = '00000000-0000-0000-0000-000000000000'
29+
const userId = generateId()
30+
const workspaceId = generateId()
31+
const owner = { workspaceId }
32+
const originalFetch = globalThis.fetch
33+
34+
/** Subjects the fixture service principal's federation policy pins. */
35+
const pinnedSubjects = new Set<string>()
36+
37+
function grant() {
38+
const issued = issueWorkloadIdentitySubject('databricks', owner, userId)
39+
pinnedSubjects.add(issued.subject)
40+
return issued
41+
}
42+
43+
function keyless(subject?: { subject: string; subjectProof: string }) {
44+
return JSON.stringify({
45+
authType: 'oidc_federation',
46+
host: HOST,
47+
clientId: CLIENT_ID,
48+
...(subject ? { subject: subject.subject, subjectProof: subject.subjectProof } : {}),
49+
})
50+
}
51+
52+
function create(displayName: string, serviceAccountJson: string) {
53+
return createCredentialRecord(
54+
{
55+
workspaceId,
56+
type: 'service_account',
57+
userId,
58+
providerId: DATABRICKS_SERVICE_ACCOUNT_PROVIDER_ID,
59+
displayName,
60+
serviceAccountJson,
61+
},
62+
{ authorizeWorkspace: true }
63+
)
64+
}
65+
66+
async function reconnect(credentialId: string, serviceAccountJson: string) {
67+
const [row] = await db.select().from(credential).where(eq(credential.id, credentialId))
68+
return updateCredentialRecord({
69+
credentialId,
70+
credential: row,
71+
actorUserId: userId,
72+
serviceAccountJson,
73+
})
74+
}
75+
76+
describe('reusing a keyless trust setup', () => {
77+
beforeAll(async () => {
78+
const now = new Date()
79+
await db.insert(user).values({
80+
id: userId,
81+
name: 'Subject reuse fixture',
82+
email: `${userId}@subject-reuse.test`,
83+
emailVerified: true,
84+
createdAt: now,
85+
updatedAt: now,
86+
})
87+
await insertWorkspaceFixture(db, {
88+
id: workspaceId,
89+
name: 'Subject reuse fixture',
90+
ownerId: userId,
91+
billedAccountUserId: userId,
92+
})
93+
await db.insert(permissions).values({
94+
id: generateId(),
95+
userId,
96+
entityType: 'workspace',
97+
entityId: workspaceId,
98+
permissionType: 'admin',
99+
})
100+
101+
vi.stubGlobal('fetch', async (input: string | URL | Request, init?: RequestInit) => {
102+
const target = typeof input === 'string' || input instanceof URL ? String(input) : input.url
103+
if (target === `https://${HOST}/oidc/v1/token`) {
104+
const subjectToken = new URLSearchParams(String(init?.body)).get('subject_token') ?? ''
105+
const subject = subjectToken.replace(/^token-for:/, '')
106+
return pinnedSubjects.has(subject)
107+
? jsonResponse({ access_token: `access-for:${subject}` })
108+
: jsonResponse({ error: 'invalid_client' }, 401)
109+
}
110+
if (target === `https://${HOST}/api/2.0/preview/scim/v2/Me`) {
111+
return jsonResponse({ userName: 'service-principal@example.com' })
112+
}
113+
return originalFetch(input, init)
114+
})
115+
}, 60_000)
116+
117+
afterAll(async () => {
118+
vi.unstubAllGlobals()
119+
await db.delete(credential).where(eq(credential.workspaceId, workspaceId))
120+
await deleteWorkspaceFixture(db, eq(workspace.id, workspaceId))
121+
await db.delete(user).where(eq(user.id, userId))
122+
})
123+
124+
it('refuses a second credential with a subject another credential holds', async () => {
125+
const setup = grant()
126+
const first = await create('Lakehouse', keyless(setup))
127+
expect(first.success).toBe(true)
128+
129+
const second = await create('Lakehouse again', keyless(setup))
130+
131+
expect(second).toMatchObject({ success: false, errorCode: 'conflict' })
132+
const rows = await db
133+
.select({ id: credential.id })
134+
.from(credential)
135+
.where(eq(credential.workspaceId, workspaceId))
136+
expect(rows.map((row) => row.id)).toEqual([first.credential?.id])
137+
})
138+
139+
it('refuses reconnecting another credential onto a subject that is held', async () => {
140+
const held = grant()
141+
expect((await create('Held', keyless(held))).success).toBe(true)
142+
const other = await create('Other', keyless(grant()))
143+
const otherId = other.credential?.id ?? ''
144+
145+
const result = await reconnect(otherId, keyless(held))
146+
147+
expect(result).toMatchObject({ success: false, errorCode: 'conflict' })
148+
})
149+
150+
it('lets the holding credential reconnect with the subject it already has', async () => {
151+
const setup = grant()
152+
const created = await create('Reconnects', keyless(setup))
153+
const credentialId = created.credential?.id ?? ''
154+
155+
expect((await reconnect(credentialId, keyless())).success).toBe(true)
156+
expect((await reconnect(credentialId, keyless(setup))).success).toBe(true)
157+
})
158+
159+
it('lets only one of two concurrent creates of one subject through', async () => {
160+
const setup = grant()
161+
162+
const results = await Promise.all([
163+
create('Racer one', keyless(setup)),
164+
create('Racer two', keyless(setup)),
165+
])
166+
167+
expect(results.filter((result) => result.success)).toHaveLength(1)
168+
expect(results.filter((result) => result.errorCode === 'conflict')).toHaveLength(1)
169+
})
170+
171+
it('lets only one of two concurrent reconnects onto one subject through', async () => {
172+
const first = await create('Reconnect racer one', keyless(grant()))
173+
const second = await create('Reconnect racer two', keyless(grant()))
174+
const setup = grant()
175+
176+
const results = await Promise.all([
177+
reconnect(first.credential?.id ?? '', keyless(setup)),
178+
reconnect(second.credential?.id ?? '', keyless(setup)),
179+
])
180+
181+
expect(results.filter((result) => result.success)).toHaveLength(1)
182+
expect(results.filter((result) => result.errorCode === 'conflict')).toHaveLength(1)
183+
})
184+
185+
it('frees the subject once the credential holding it is deleted', async () => {
186+
const setup = grant()
187+
const created = await create('Deleted holder', keyless(setup))
188+
const credentialId = created.credential?.id ?? ''
189+
await db.delete(credentialMember).where(eq(credentialMember.credentialId, credentialId))
190+
await db.delete(credential).where(eq(credential.id, credentialId))
191+
192+
expect((await create('Successor', keyless(setup))).success).toBe(true)
193+
})
194+
})

‎apps/sim/lib/credentials/aws-service-account.server.ts‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -258,13 +258,14 @@ function verificationFailure(config: AwsCredentialConfig, error: unknown): AwsCr
258258

259259
/**
260260
* Validates the submitted configuration, proves it works by resolving it and
261-
* calling `sts:GetCallerIdentity`, and returns the encrypted blob plus the
262-
* caller ARN AWS reports.
261+
* calling `sts:GetCallerIdentity`, and returns the encrypted blob, the caller
262+
* ARN AWS reports, and the keyless subject it newly binds, if any (a
263+
* reconnect keeping its stored subject binds none).
263264
*/
264265
export async function verifyAndEncryptAwsCredential(
265266
serialized: string,
266267
owner: WorkloadIdentityCredentialOwner
267-
): Promise<{ encryptedServiceAccountKey: string; callerArn: string }> {
268+
): Promise<{ encryptedServiceAccountKey: string; callerArn: string; newSubject?: string }> {
268269
const parsed = parseServiceAccountConfig(serialized, submittedConfigSchema, 'AWS credential')
269270
if (!parsed.success) throw new AwsCredentialError(parsed.message)
270271
const submitted = parsed.data
@@ -292,7 +293,13 @@ export async function verifyAndEncryptAwsCredential(
292293
const { encrypted } = await encryptSecret(
293294
JSON.stringify({ type: AWS_CREDENTIAL_SECRET_TYPE, config })
294295
)
295-
return { encryptedServiceAccountKey: encrypted, callerArn }
296+
return {
297+
encryptedServiceAccountKey: encrypted,
298+
callerArn,
299+
...(config.authType === 'oidc_role' && config.subject !== readStoredSubject(owner.storedSecret)
300+
? { newSubject: config.subject }
301+
: {}),
302+
}
296303
}
297304

298305
/** Resolves the configuration and returns the caller ARN AWS reports for it. */

‎apps/sim/lib/credentials/databricks-service-account.server.ts‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -290,13 +290,14 @@ async function assertPolicyPinsSubject(
290290

291291
/**
292292
* Validates the submitted configuration, proves it by calling the workspace's
293-
* SCIM `Me` endpoint with the resolved token, and returns the encrypted blob
294-
* plus the identity Databricks reports.
293+
* SCIM `Me` endpoint with the resolved token, and returns the encrypted blob,
294+
* the identity Databricks reports, and the keyless subject it newly binds, if
295+
* any (a reconnect keeping its stored subject binds none).
295296
*/
296297
export async function verifyAndEncryptDatabricksCredential(
297298
serialized: string,
298299
owner: WorkloadIdentityCredentialOwner
299-
): Promise<{ encryptedServiceAccountKey: string; identity: string }> {
300+
): Promise<{ encryptedServiceAccountKey: string; identity: string; newSubject?: string }> {
300301
const parsed = parseServiceAccountConfig(
301302
serialized,
302303
submittedConfigSchema,
@@ -325,7 +326,14 @@ export async function verifyAndEncryptDatabricksCredential(
325326
const { encrypted } = await encryptSecret(
326327
JSON.stringify({ type: DATABRICKS_CREDENTIAL_SECRET_TYPE, config })
327328
)
328-
return { encryptedServiceAccountKey: encrypted, identity }
329+
return {
330+
encryptedServiceAccountKey: encrypted,
331+
identity,
332+
...(config.authType === 'oidc_federation' &&
333+
config.subject !== readStoredSubject(owner.storedSecret)
334+
? { newSubject: config.subject }
335+
: {}),
336+
}
329337
}
330338

331339
/**

‎apps/sim/lib/credentials/orchestration/credential-create.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@ import {
2929
} from '@/lib/credentials/service-account-secret'
3030
import { isTokenServiceAccountProviderId } from '@/lib/credentials/token-service-accounts/descriptors'
3131
import { TokenServiceAccountValidationError } from '@/lib/credentials/token-service-accounts/errors'
32+
import {
33+
assertWorkloadIdentitySubjectUnclaimed,
34+
WorkloadIdentitySubjectError,
35+
} from '@/lib/credentials/workload-identity/subject'
3236
import { getServiceConfigByProviderId } from '@/lib/oauth'
3337
import { SLACK_CUSTOM_BOT_PROVIDER_ID } from '@/lib/oauth/types'
3438
import { captureServerEvent } from '@/lib/posthog/server'
@@ -269,6 +273,7 @@ export async function createCredentialRecord(
269273
: null
270274
let resolvedEnvOwnerUserId: string | null = null
271275
let resolvedEncryptedServiceAccountKey: string | null = null
276+
let resolvedWorkloadIdentitySubject: string | undefined
272277
const extraAuditMetadata: Record<string, unknown> = {}
273278

274279
if (type === 'oauth') {
@@ -335,6 +340,7 @@ export async function createCredentialRecord(
335340
resolvedEnvOwnerUserId = null
336341
if (!resolvedDisplayName) resolvedDisplayName = secret.displayName
337342
resolvedEncryptedServiceAccountKey = secret.encryptedServiceAccountKey
343+
resolvedWorkloadIdentitySubject = secret.newWorkloadIdentitySubject
338344
Object.assign(extraAuditMetadata, secret.auditMetadata)
339345
} catch (error) {
340346
if (error instanceof ServiceAccountSecretError) {
@@ -517,6 +523,19 @@ export async function createCredentialRecord(
517523
if (innerExisting) throw new DuplicateCredentialError()
518524
}
519525

526+
if (resolvedWorkloadIdentitySubject) {
527+
try {
528+
await assertWorkloadIdentitySubjectUnclaimed(tx, {
529+
subject: resolvedWorkloadIdentitySubject,
530+
scope,
531+
})
532+
} catch (error) {
533+
if (error instanceof WorkloadIdentitySubjectError)
534+
return failure(error.message, 'conflict')
535+
throw error
536+
}
537+
}
538+
520539
await tx.insert(credential).values({
521540
id: credentialId,
522541
...resourceScopeColumns(scope),

‎apps/sim/lib/credentials/orchestration/index.ts‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ import {
4747
verifyAndBuildServiceAccountSecret,
4848
} from '@/lib/credentials/service-account-secret'
4949
import { TokenServiceAccountValidationError } from '@/lib/credentials/token-service-accounts/errors'
50+
import {
51+
assertWorkloadIdentitySubjectUnclaimed,
52+
WorkloadIdentitySubjectError,
53+
} from '@/lib/credentials/workload-identity/subject'
5054
import { invalidateEffectiveDecryptedEnvCache } from '@/lib/environment/utils'
5155
import { findSlackSearchInstallation } from '@/lib/knowledge/application/slack-search/repository'
5256
import {
@@ -312,6 +316,7 @@ export async function updateCredentialRecord(
312316

313317
let rotatedSlackBotUserId: string | undefined
314318
let rotatedAuditMetadata: Record<string, string> | undefined
319+
let rotatedWorkloadIdentitySubject: string | undefined
315320
if (hasRotationSecret) {
316321
const providerId = params.credential.providerId ?? ''
317322

@@ -453,6 +458,7 @@ export async function updateCredentialRecord(
453458
updates.encryptedServiceAccountKey = secret.encryptedServiceAccountKey
454459
rotatedSlackBotUserId = secret.botUserId
455460
rotatedAuditMetadata = secret.auditMetadata
461+
rotatedWorkloadIdentitySubject = secret.newWorkloadIdentitySubject
456462

457463
if (needsStoredIdentity) {
458464
const previousIdentity = deriveStoredDisplayName(storedBlob)
@@ -515,7 +521,26 @@ export async function updateCredentialRecord(
515521
}
516522

517523
updates.updatedAt = new Date()
518-
await db.update(credential).set(updates).where(eq(credential.id, params.credentialId))
524+
if (rotatedWorkloadIdentitySubject) {
525+
const subject = rotatedWorkloadIdentitySubject
526+
try {
527+
await db.transaction(async (tx) => {
528+
await assertWorkloadIdentitySubjectUnclaimed(tx, {
529+
subject,
530+
scope: resourceScopeFromOwner(params.credential),
531+
credentialId: params.credentialId,
532+
})
533+
await tx.update(credential).set(updates).where(eq(credential.id, params.credentialId))
534+
})
535+
} catch (error) {
536+
if (error instanceof WorkloadIdentitySubjectError) {
537+
return { success: false, error: error.message, errorCode: 'conflict' }
538+
}
539+
throw error
540+
}
541+
} else {
542+
await db.update(credential).set(updates).where(eq(credential.id, params.credentialId))
543+
}
519544

520545
// The flag rides the environment snapshot into every run's redaction catalog, so a flip
521546
// must not serve stale from the same-process snapshot cache. Cross-process readers are

‎apps/sim/lib/credentials/service-account-secret.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,11 @@ export interface ServiceAccountSecretResult {
104104
principal: ServiceAccountPrincipal | null
105105
/** Slack custom bot: the derived bot user id (for reaction self-drop). */
106106
botUserId?: string
107+
/**
108+
* Keyless AWS or Databricks: a subject this save binds that the credential did
109+
* not already hold, which must not belong to another credential of the owner.
110+
*/
111+
newWorkloadIdentitySubject?: string
107112
}
108113

109114
/** Thrown when a service-account secret is missing or fails provider verification. */
@@ -272,6 +277,7 @@ async function buildAwsServiceAccountSecret(
272277
displayName: result.callerArn,
273278
auditMetadata: serviceAccountPrincipalMetadata(principal),
274279
principal,
280+
newWorkloadIdentitySubject: result.newSubject,
275281
}
276282
}
277283

@@ -299,6 +305,7 @@ async function buildDatabricksServiceAccountSecret(
299305
displayName: result.identity,
300306
auditMetadata: serviceAccountPrincipalMetadata(principal),
301307
principal,
308+
newWorkloadIdentitySubject: result.newSubject,
302309
}
303310
}
304311

0 commit comments

Comments
 (0)