Skip to content

Commit f5cdf39

Browse files
committed
fix skiller launch review issues
1 parent 3d4e7d5 commit f5cdf39

2 files changed

Lines changed: 113 additions & 23 deletions

File tree

packages/api/src/services/skiller.ts

Lines changed: 105 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { spawn, spawnSync, type ChildProcess } from "node:child_process"
1+
import { spawn, type ChildProcess, type SpawnOptions } from "node:child_process"
22
import { chmodSync, chownSync, closeSync, existsSync, mkdirSync, openSync, readFileSync, statSync } from "node:fs"
33
import { createServer } from "node:net"
44
import { homedir } from "node:os"
@@ -188,6 +188,24 @@ const sleep = (durationMs: number): Promise<void> =>
188188
setTimeout(resolve, durationMs)
189189
})
190190

191+
const runProcess = (
192+
command: string,
193+
args: ReadonlyArray<string>,
194+
options: SpawnOptions = {}
195+
): Promise<void> =>
196+
new Promise((resolve, reject) => {
197+
const child = spawn(command, [...args], options)
198+
child.once("error", reject)
199+
child.once("exit", (exitCode, signal) => {
200+
if (exitCode === 0) {
201+
resolve()
202+
return
203+
}
204+
const reason = exitCode === null ? `signal ${signal}` : `exit code ${exitCode}`
205+
reject(new Error(`${command} ${args.join(" ")} failed with ${reason}.`))
206+
})
207+
})
208+
191209
const containerHomePath = (sshUser: string): string => `/home/${sshUser}`
192210

193211
const inspectContainerMounts = (
@@ -310,7 +328,10 @@ const prepareLaunchScript = [
310328
" mkdir -p out",
311329
" touch \"$DOCKER_GIT_SKILLER_PATCH_MARKER\"",
312330
"fi",
313-
"if [ ! -e out/preload/index.js ]; then ln -sf index.mjs out/preload/index.js; fi"
331+
"if [ ! -e out/preload/index.js ]; then",
332+
" mkdir -p out/preload",
333+
" ln -sf index.mjs out/preload/index.js",
334+
"fi"
314335
].join("\n")
315336

316337
const electronLaunchFlags = [
@@ -476,19 +497,84 @@ const nameForId = (
476497
return null
477498
}
478499

500+
const localUserNameForUid = (uid: number): string | null =>
501+
nameForId(readFileSync("/etc/passwd", "utf8"), uid, 2)
502+
503+
const localGroupNameForGid = (gid: number): string | null =>
504+
nameForId(readFileSync("/etc/group", "utf8"), gid, 2)
505+
506+
const skillerUserNameForUid = (uid: number): string => `dg-skiller-u${uid}`
507+
508+
const skillerGroupNameForGid = (gid: number): string => `dg-skiller-g${gid}`
509+
479510
const resolveSkillerProcessAccount = (user: SkillerProcessUser): SkillerProcessAccount => {
480511
if (user.uid === 0 || user.gid === 0) {
481512
throw new Error("Refusing to launch scoped Skiller as root; selected container home is root-owned.")
482513
}
483-
const userName = nameForId(readFileSync("/etc/passwd", "utf8"), user.uid, 2)
484-
if (userName === null) {
485-
throw new Error(`Cannot launch scoped Skiller: no local passwd entry for UID ${user.uid}.`)
514+
const userName = localUserNameForUid(user.uid) ?? skillerUserNameForUid(user.uid)
515+
const groupName = localGroupNameForGid(user.gid) ?? skillerGroupNameForGid(user.gid)
516+
return { ...user, groupName, userName }
517+
}
518+
519+
const ensureLocalGroup = async (gid: number, groupName: string): Promise<void> => {
520+
if (localGroupNameForGid(gid) !== null) {
521+
return
486522
}
487-
const groupName = nameForId(readFileSync("/etc/group", "utf8"), user.gid, 2)
488-
if (groupName === null) {
489-
throw new Error(`Cannot launch scoped Skiller: no local group entry for GID ${user.gid}.`)
523+
try {
524+
await runProcess("groupadd", ["--gid", String(gid), groupName])
525+
} catch (error) {
526+
if (localGroupNameForGid(gid) !== null) {
527+
return
528+
}
529+
throw error
530+
}
531+
if (localGroupNameForGid(gid) === null) {
532+
throw new Error(`Cannot launch scoped Skiller: failed to create local group entry for GID ${gid}.`)
490533
}
491-
return { ...user, groupName, userName }
534+
}
535+
536+
const ensureLocalUser = async (
537+
user: SkillerProcessUser,
538+
userName: string,
539+
groupName: string
540+
): Promise<void> => {
541+
if (localUserNameForUid(user.uid) !== null) {
542+
return
543+
}
544+
try {
545+
await runProcess("useradd", [
546+
"--uid",
547+
String(user.uid),
548+
"--gid",
549+
groupName,
550+
"--no-create-home",
551+
"--home-dir",
552+
"/nonexistent",
553+
"--shell",
554+
"/bin/false",
555+
userName
556+
])
557+
} catch (error) {
558+
if (localUserNameForUid(user.uid) !== null) {
559+
return
560+
}
561+
throw error
562+
}
563+
if (localUserNameForUid(user.uid) === null) {
564+
throw new Error(`Cannot launch scoped Skiller: failed to create local passwd entry for UID ${user.uid}.`)
565+
}
566+
}
567+
568+
const ensureSkillerProcessAccount = async (
569+
user: SkillerProcessUser | null
570+
): Promise<SkillerProcessAccount | null> => {
571+
if (user === null) {
572+
return null
573+
}
574+
const account = resolveSkillerProcessAccount(user)
575+
await ensureLocalGroup(account.gid, account.groupName)
576+
await ensureLocalUser(account, account.userName, account.groupName)
577+
return resolveSkillerProcessAccount(user)
492578
}
493579

494580
export const skillerLaunchCommand = (
@@ -519,19 +605,12 @@ export const skillerLaunchCommand = (
519605
}
520606
}
521607

522-
const prepareSkillerRuntime = (skillerDir: string, logFd: number): void => {
523-
const result = spawnSync("bash", ["-c", prepareLaunchScript], {
608+
const prepareSkillerRuntime = (skillerDir: string, logFd: number): Promise<void> =>
609+
runProcess("bash", ["-c", prepareLaunchScript], {
524610
cwd: skillerDir,
525611
env: process.env,
526612
stdio: ["ignore", logFd, logFd]
527613
})
528-
if (result.error !== undefined) {
529-
throw result.error
530-
}
531-
if (result.status !== 0) {
532-
throw new Error(`Skiller runtime preparation failed with exit code ${result.status ?? `signal ${result.signal}`}.`)
533-
}
534-
}
535614

536615
const stopSkillerProcess = (process: SkillerProcess): void => {
537616
const pid = process.process.pid
@@ -569,17 +648,20 @@ const registerSkillerProject = (
569648
}
570649
})
571650

572-
const launchSkillerProcess = (
651+
const launchSkillerProcess = async (
573652
skillerDir: string,
574653
trpcPort: number,
575654
scope: SkillerContainerScope | null
576-
): SkillerLaunch => {
655+
): Promise<SkillerLaunch> => {
577656
mkdirSync(dirname(launchLogPath), { recursive: true })
578657
const processUser = prepareSkillerScopeHome(scope)
579658
const logFd = openSync(launchLogPath, "a")
580659
try {
581-
prepareSkillerRuntime(skillerDir, logFd)
582-
const launchCommand = skillerLaunchCommand(processUser)
660+
const processAccount = await ensureSkillerProcessAccount(processUser)
661+
await prepareSkillerRuntime(skillerDir, logFd)
662+
const launchCommand = processAccount === null
663+
? skillerLaunchCommand(null)
664+
: skillerLaunchCommand(processAccount, () => processAccount)
583665
const child = spawn(launchCommand.command, launchCommand.args, {
584666
cwd: skillerDir,
585667
detached: true,
@@ -673,7 +755,7 @@ export const openSkiller = (
673755
}),
674756
try: () => findAvailablePort(skillerPreferredTrpcPort)
675757
}))
676-
const launch = yield* _(Effect.try({
758+
const launch = yield* _(Effect.tryPromise({
677759
catch: (cause) => new ApiInternalError({
678760
message: "Failed to launch Skiller.",
679761
cause

packages/api/tests/skiller-routes.test.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,14 @@ describe("skiller routes", () => {
6666
expect(launch.userName).toBe("ubuntu")
6767
})
6868

69+
it("uses deterministic scoped account names for missing local UID and GID entries", () => {
70+
const launch = skillerLaunchCommand({ gid: 2_147_483_002, uid: 2_147_483_001 })
71+
72+
expect(launch.command).toBe("runuser")
73+
expect(launch.groupName).toBe("dg-skiller-g2147483002")
74+
expect(launch.userName).toBe("dg-skiller-u2147483001")
75+
})
76+
6977
it("keeps the terminal session id on session-scoped app routes", () => {
7078
expect(parseSkillerRoute("/api/ssh/session/terminal-proof/skiller/app/")).toEqual({
7179
_tag: "App",

0 commit comments

Comments
 (0)