Skip to content

Commit dea671f

Browse files
committed
fix(cloud-agent-next): deliver profile secrets to control-plane sessions
Profile encryptedSecrets never reached workspace_* sessions: only plain variables did, and only the legacy plane decrypted secrets. Carry the worker-encrypted snapshot inside the DO-private credential source and decrypt it only in the Sandbox DO, when it builds the session.prepare frame. The decrypted values and the new frame-only secretEnvKeys exist only in that frame, never in a route row or stored payload. A grant-issued credential of the same name is restored over a profile secret. A secret-bearing prepare is sent only to a wrapper that advertised redactsNamedSecrets in hello; otherwise the attempt fails with workspace_setup_failed rather than risk printing a secret in clear. A prepare without secrets is unchanged for older wrappers. Skip repository snapshots for a route with setup commands: capture runs after setup, and the snapshot key names only the user and repository, so setup output and anything setup wrote would be published under it.
1 parent 03baf4d commit dea671f

14 files changed

Lines changed: 896 additions & 53 deletions

‎services/cloud-agent-next/docs/control-plane.md‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,9 @@ path `/workspace/app`. A snapshot holds one repository at one path, so any other
381381
stopped it), the next launch discards the entry and starts from the image. A broken snapshot
382382
costs one attempt.
383383
- **Capture.** `session.prepare` carries `capture: true` when the route has a key and the provider
384-
can capture. The wrapper decides whether to capture (a fresh clone, or an adopted snapshot that is
384+
can capture. A route with setup commands has no key: capture runs after setup, so the snapshot
385+
would publish the whole container under a key that names only the user and the repository. The
386+
wrapper decides whether to capture (a fresh clone, or an adopted snapshot that is
385387
due for a refresh) and then sends `workspace.capture` after setup, with origin bare. The Sandbox DO
386388
calls the provider off its serial queue, bounded at 5 min 5 s, and answers `workspace.captured`
387389
(`ok: false` at once when nothing can be saved). `SandboxContainers` snapshots the running
@@ -413,11 +415,14 @@ path `/workspace/app`. A snapshot holds one repository at one path, so any other
413415
the wrapper sends the original hello on the same socket. A legacy welcome enables periodic
414416
heartbeats without an acknowledgement deadline; old wrappers receive neither the optional field
415417
nor acknowledgement frames. Duplicate hello on a bound socket is ignored.
416-
- A wrapper that redacts named secrets advertises optional `redactsNamedSecrets: true` in `hello`. A
417-
`session.prepare` frame may carry frame-only `secretEnvKeys`, the names of `env` entries whose
418-
values the wrapper redacts in setup output and auto-commit diagnostics even when the name does not
419-
look secret. The field is never stored in a route row, and a frame without it stays valid for a
420-
version-3 wrapper.
418+
- A wrapper that redacts named secrets advertises optional `redactsNamedSecrets: true` in `hello`; the
419+
Sandbox DO stores it on the socket attachment like `heartbeatAck`. Encrypted secrets ride only the
420+
DO-private credential source (`encryptedSecrets`) and are decrypted into the frame. A secret-bearing
421+
`session.prepare` carries the names as frame-only `secretEnvKeys`, never written to the route row; a
422+
frame with no secrets omits the field and stays valid for a version-3 wrapper. A secret-bearing
423+
prepare is never sent to a wrapper that did not advertise the capability: the attempt fails
424+
`workspace_setup_failed`. Secrets do not add runtime isolation; `runtimeIsolation` still follows
425+
modern runtime authorization only.
421426

422427
- Connect, send `hello` (`wrapperId`, `allocationId`, protocol version), wait for `welcome` or
423428
`shutdown`. On `shutdown`, permanently close the connection and exit with code 0, even when the
@@ -501,8 +506,8 @@ setup the wrapper writes the stamp (before the snapshot, so a restored container
501506
allocation's stamp), sets `origin` to the bare URL, clears the reflogs and `FETCH_HEAD`, sends
502507
`workspace.capture` and waits for `workspace.captured`. It then restores the authenticated URL,
503508
whatever the outcome, and continues to the Kilo runtime. A snapshot therefore holds no agent edits,
504-
no Kilo home and no git credential. Process env never reaches the disk. Files setup wrote from env stay in
505-
the snapshot; setup re-runs on every start and rewrites them.
509+
no Kilo home and no git credential. Process env never reaches the disk. A route with setup commands
510+
has no key, so it restores and captures nothing: setup output is never published to a snapshot.
506511

507512
### Prompts and turn outcome
508513

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
import { generateKeyPairSync } from 'node:crypto';
2+
import { beforeAll, describe, expect, it } from 'vitest';
3+
import { encryptWithPublicKey } from '../../utils/encryption.js';
4+
import { buildFrameEnv } from './frame-env.js';
5+
6+
let publicKey: string;
7+
let privateKey: string;
8+
9+
beforeAll(() => {
10+
({ publicKey, privateKey } = generateKeyPairSync('rsa', {
11+
modulusLength: 2048,
12+
publicKeyEncoding: { type: 'spki', format: 'pem' },
13+
privateKeyEncoding: { type: 'pkcs8', format: 'pem' },
14+
}));
15+
});
16+
17+
function secret(value: string) {
18+
return encryptWithPublicKey(value, publicKey);
19+
}
20+
21+
describe('buildFrameEnv', () => {
22+
it('returns the same env and no secret keys when there are no secrets', () => {
23+
const specEnv = { PLAIN: 'value', KILOCODE_TOKEN: 'grant-token' };
24+
const result = buildFrameEnv({ specEnv, encryptedSecrets: undefined, privateKey });
25+
expect(result.env).toEqual(specEnv);
26+
expect(result.secretEnvKeys).toEqual([]);
27+
});
28+
29+
it('overlays a secret over plaintext of the same key', () => {
30+
const result = buildFrameEnv({
31+
specEnv: { DATABASE_URL: 'plaintext', KEEP: 'kept' },
32+
encryptedSecrets: { DATABASE_URL: secret('encrypted-value') },
33+
privateKey,
34+
});
35+
expect(result.env).toEqual({ DATABASE_URL: 'encrypted-value', KEEP: 'kept' });
36+
expect(result.secretEnvKeys).toEqual(['DATABASE_URL']);
37+
});
38+
39+
it.each([
40+
'KILOCODE_TOKEN',
41+
'GH_TOKEN',
42+
'GITHUB_TOKEN',
43+
'GITLAB_TOKEN',
44+
'GLAB_IS_OAUTH2',
45+
'GITLAB_HOST',
46+
'GITLAB_SUBFOLDER',
47+
'BITBUCKET_TOKEN',
48+
'KILO_BITBUCKET_WORKSPACE_SLUG',
49+
'KILO_BITBUCKET_REPOSITORY_SLUG',
50+
'KILO_BITBUCKET_WORKSPACE_UUID',
51+
'KILO_BITBUCKET_REPOSITORY_UUID',
52+
])('restores the issued %s the spec already carried over a same-named secret', key => {
53+
const result = buildFrameEnv({
54+
specEnv: { [key]: 'grant-value' },
55+
encryptedSecrets: { [key]: secret('secret-value') },
56+
privateKey,
57+
});
58+
expect(result.env?.[key]).toBe('grant-value');
59+
// The grant value survived, not the decrypted secret, so nothing to redact.
60+
expect(result.secretEnvKeys).toEqual([]);
61+
});
62+
63+
it('keeps a plaintext GH_TOKEN the grant left in place when a different secret overlays', () => {
64+
const result = buildFrameEnv({
65+
specEnv: { GH_TOKEN: 'grant-gh', GITHUB_TOKEN: 'grant-gh' },
66+
encryptedSecrets: { DATABASE_URL: secret('db') },
67+
privateKey,
68+
});
69+
expect(result.env?.GH_TOKEN).toBe('grant-gh');
70+
expect(result.env?.GITHUB_TOKEN).toBe('grant-gh');
71+
expect(result.secretEnvKeys).toEqual(['DATABASE_URL']);
72+
});
73+
74+
it('deletes an org id a secret added when the uncontained spec did not carry it', () => {
75+
const result = buildFrameEnv({
76+
specEnv: {},
77+
encryptedSecrets: { KILOCODE_ORGANIZATION_ID: secret('org-from-secret') },
78+
privateKey,
79+
});
80+
expect(Object.hasOwn(result.env ?? {}, 'KILOCODE_ORGANIZATION_ID')).toBe(false);
81+
expect(result.secretEnvKeys).toEqual([]);
82+
});
83+
84+
it('keeps a contained spec org id, including a value the grant did not touch', () => {
85+
const result = buildFrameEnv({
86+
specEnv: { KILOCODE_ORGANIZATION_ID: 'org-from-grant' },
87+
encryptedSecrets: { KILOCODE_ORGANIZATION_ID: secret('org-from-secret') },
88+
privateKey,
89+
});
90+
expect(result.env?.KILOCODE_ORGANIZATION_ID).toBe('org-from-grant');
91+
});
92+
93+
it('deletes a control-runtime name a secret added when the spec did not carry it', () => {
94+
const result = buildFrameEnv({
95+
specEnv: {},
96+
encryptedSecrets: { SANDBOX_CONTROL_CREDENTIAL: secret('forged') },
97+
privateKey,
98+
});
99+
expect(Object.hasOwn(result.env ?? {}, 'SANDBOX_CONTROL_CREDENTIAL')).toBe(false);
100+
expect(result.secretEnvKeys).toEqual([]);
101+
});
102+
103+
it('keeps a secret name the grant did not issue and the spec did not carry', () => {
104+
const result = buildFrameEnv({
105+
specEnv: {},
106+
encryptedSecrets: { GH_TOKEN: secret('user-gh') },
107+
privateKey,
108+
});
109+
expect(result.env?.GH_TOKEN).toBe('user-gh');
110+
});
111+
112+
it('throws when secrets are present and no private key is configured', () => {
113+
expect(() =>
114+
buildFrameEnv({
115+
specEnv: {},
116+
encryptedSecrets: { DATABASE_URL: secret('value') },
117+
privateKey: undefined,
118+
})
119+
).toThrow(/Private key is required/);
120+
});
121+
});
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
import { CONTROL_RUNTIME_RESERVED_ENV_VARS } from '../../shared/runtime-environment.js';
2+
import { ISSUED_CREDENTIAL_ENV_KEYS } from '../../sandbox-control/session-credentials.js';
3+
import { mergeEnvVarsWithSecrets, type EncryptedSecrets } from '../../utils/encryption.js';
4+
5+
/**
6+
* Names a secret must never reintroduce when the pre-overlay spec did not carry
7+
* them: the grant clears the org id (`!contained`) and the control-runtime names
8+
* belong to the platform. SCM names are deliberately excluded — a plaintext var
9+
* of the same name survives today, so a secret may set one the grant did not
10+
* issue.
11+
*/
12+
const DELETE_WHEN_UNOWNED_ENV_KEYS: readonly string[] = [
13+
'KILOCODE_ORGANIZATION_ID',
14+
...CONTROL_RUNTIME_RESERVED_ENV_VARS,
15+
];
16+
17+
export type FrameEnvInput = {
18+
/** The grant-projected route spec env, before the secret overlay. */
19+
specEnv: Record<string, string> | undefined;
20+
/** The DO-private encrypted-secrets snapshot, opaque to the shared schema. */
21+
encryptedSecrets: Record<string, unknown> | undefined;
22+
privateKey: string | undefined;
23+
};
24+
25+
export type FrameEnvResult = {
26+
/** The merged env; the same object when there are no secrets. */
27+
env: Record<string, string> | undefined;
28+
/** Names the wrapper should redact: only keys whose value is the decrypted secret. */
29+
secretEnvKeys: string[];
30+
};
31+
32+
/**
33+
* Copies `specEnv`, overlays the decrypted profile secrets, then restores the
34+
* credential keys the grant already projected. Decryption throws when the
35+
* snapshot is non-empty and no private key is configured. Presence is
36+
* `Object.hasOwn`, not truthiness, so a key projected with an empty string is
37+
* still restored. The merged env is returned, never written back to the route.
38+
*/
39+
export function buildFrameEnv(input: FrameEnvInput): FrameEnvResult {
40+
const encrypted = input.encryptedSecrets;
41+
if (encrypted === undefined || Object.keys(encrypted).length === 0) {
42+
return { env: input.specEnv, secretEnvKeys: [] };
43+
}
44+
const preOverlay = input.specEnv ?? {};
45+
const merged = mergeEnvVarsWithSecrets(
46+
preOverlay,
47+
encrypted as EncryptedSecrets,
48+
input.privateKey
49+
);
50+
const restored = new Set<string>();
51+
for (const key of ISSUED_CREDENTIAL_ENV_KEYS) {
52+
if (Object.hasOwn(preOverlay, key)) {
53+
merged[key] = preOverlay[key];
54+
restored.add(key);
55+
}
56+
}
57+
for (const key of DELETE_WHEN_UNOWNED_ENV_KEYS) {
58+
if (!Object.hasOwn(preOverlay, key) && Object.hasOwn(encrypted, key)) delete merged[key];
59+
}
60+
const secretEnvKeys = Object.keys(encrypted).filter(
61+
key => Object.hasOwn(merged, key) && !restored.has(key)
62+
);
63+
return { env: merged, secretEnvKeys };
64+
}

‎services/cloud-agent-next/src/control-plane/sandbox/repo-key.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,15 @@ describe('repoSnapshotEligible', () => {
4949
})
5050
).toBe(false);
5151
});
52+
53+
it('excludes a route with setup commands and treats empty or absent the same', () => {
54+
const enrolled = gate({ enrolledIds: '*' });
55+
expect(repoSnapshotEligible(enrolled, { ...route, setupCommands: ['pnpm install'] })).toBe(
56+
false
57+
);
58+
expect(repoSnapshotEligible(enrolled, { ...route, setupCommands: [] })).toBe(true);
59+
expect(repoSnapshotEligible(enrolled, route)).toBe(true);
60+
});
5261
});
5362

5463
describe('computeRepoKey', () => {

‎services/cloud-agent-next/src/control-plane/sandbox/repo-key.ts‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,19 @@ export type RepoSnapshotGate = {
2424
* (personal user, or the org when the session is org-owned) is enrolled, and the
2525
* route has a repository at the constant path the snapshots are taken at. A
2626
* snapshot holds one repository at one path, so a per-session path or a shared
27-
* sandbox could only capture something no later session can reuse.
27+
* sandbox could only capture something no later session can reuse. A route with
28+
* any setup command has no key: capture runs after setup, so the snapshot would
29+
* carry setup output under a key that names only the user and the repository.
2830
*
2931
* Enrollment follows the `CONTROL_PLANE_IDS`/`SANDBOX_SELECTION_IDS` convention:
3032
* one list matched against the user ID and then the org ID.
3133
*/
3234
export function repoSnapshotEligible(
3335
gate: RepoSnapshotGate,
34-
route: Pick<RepoKeyInput, 'repoUrl' | 'directory'>
36+
route: Pick<RepoKeyInput, 'repoUrl' | 'directory'> & { setupCommands?: string[] }
3537
): boolean {
3638
return (
39+
(route.setupCommands?.length ?? 0) === 0 &&
3740
(isOrgInList(gate.enrolledIds, gate.userId) || isOrgInList(gate.enrolledIds, gate.orgId)) &&
3841
route.repoUrl !== undefined &&
3942
route.directory === ISOLATED_CONTAINER_WORKSPACE_PATH
@@ -42,9 +45,11 @@ export function repoSnapshotEligible(
4245

4346
/**
4447
* The keyed hash that names a repository snapshot: scope and repository. Env is
45-
* not part of it: setup re-runs on every start and rewrites what it derives from
46-
* the env. It is hashed under a Worker secret so the digest cannot be guessed to
47-
* address another user's snapshot.
48+
* not part of it on purpose: setup re-runs on every start and rewrites what it
49+
* derives from the env. Setup-command output is kept out by not computing a key
50+
* for a route that has setup commands, rather than by narrowing the hash. It is
51+
* hashed under a Worker secret so the digest cannot be guessed to address
52+
* another user's snapshot.
4853
*/
4954
export async function computeRepoKey(input: RepoKeyInput): Promise<string | null> {
5055
if (input.secret === null || input.repoUrl === undefined) return null;

0 commit comments

Comments
 (0)