Skip to content

Commit 574f9fd

Browse files
HXYerrorclaude
andcommitted
fix(auth): address crew review findings for #28
Security and correctness fixes for the keys/bootstrap implementation: - bootstrap: invert check order (admin count first, file warning second) so server can restart after first-run without manual file deletion - bootstrap/admin-recover: use O_EXCL flag (wx) on key file writes to prevent symlink TOCTOU attacks and parallel-start race - bootstrap: ensure APP_DIR exists before createKey() so FS failure doesn't orphan a DB row silently; surface error with recovery instructions - admin-recover: add --force guard against accidental duplicate admin keys, refuse to overwrite existing key file, apply TTY gate on plaintext output - admin-recover: use path.join instead of template literal for portability - keys: extract validateAllowedModels/resolveRateLimit helpers to reduce complexity below ESLint limit - keys: validate allowedModels entries against model-name regex (SSRF prevention) - keys: reject non-integer and negative rateLimitOverride with explicit error - keys: revokeKey returns boolean; idempotent (AND revoked_at IS NULL guard) - keys: setDebugEnabled returns boolean - keys: listKeys uses stable ORDER BY created_at, id - keys: generateKey uses 33 bytes (264 bits) to eliminate zero-padding on last char - keys: document unsalted SHA-256 rationale in hashKey comment - migration: remove redundant CREATE INDEX (UNIQUE already creates one); add json_valid CHECK on allowed_models column - tests: fix migration path to use import.meta.dir (not process.cwd()) - tests: add 15 new tests covering edge cases from review Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 303eb40 commit 574f9fd

5 files changed

Lines changed: 305 additions & 73 deletions

File tree

src/cli/admin-recover.ts

Lines changed: 52 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,28 @@
11
import { defineCommand } from "citty"
22
import consola from "consola"
33
import fs from "node:fs"
4+
import path from "node:path"
45

56
import { initDb } from "~/lib/db"
67
import { PATHS } from "~/lib/paths"
7-
import { createKey } from "~/services/keys"
8+
import { countActiveAdminKeys, createKey } from "~/services/keys"
89

9-
const ADMIN_KEY_FILE_RECOVER = `${PATHS.APP_DIR}/admin.key.txt`
10+
const ADMIN_KEY_FILE_RECOVER = path.join(PATHS.APP_DIR, "admin.key.txt")
1011

1112
export const adminRecover = defineCommand({
1213
meta: {
1314
name: "recover",
1415
description:
1516
"Mint a new admin key (requires local data-dir access as proof of operator identity)",
1617
},
17-
run() {
18+
args: {
19+
force: {
20+
type: "boolean",
21+
description: "Create a new admin key even if active admin keys exist",
22+
default: false,
23+
},
24+
},
25+
run({ args }) {
1826
// Proof of operator identity: must be able to stat the data directory
1927
try {
2028
fs.statSync(PATHS.APP_DIR)
@@ -28,11 +36,49 @@ export const adminRecover = defineCommand({
2836
// Init DB (runs migrations) — needed if invoked independently
2937
initDb()
3038

39+
// Guard: warn if active admin keys already exist unless --force is set
40+
const existing = countActiveAdminKeys()
41+
if (existing > 0 && !args.force) {
42+
consola.warn(
43+
`${existing} active admin key(s) already exist. Pass --force to create a new one anyway.`,
44+
)
45+
process.exit(1)
46+
}
47+
48+
// Guard: refuse to overwrite an unread key file
49+
if (fs.existsSync(ADMIN_KEY_FILE_RECOVER)) {
50+
consola.error(
51+
`${ADMIN_KEY_FILE_RECOVER} already exists. Read and delete it first:\n`
52+
+ ` cat ${ADMIN_KEY_FILE_RECOVER} && rm ${ADMIN_KEY_FILE_RECOVER}`,
53+
)
54+
process.exit(1)
55+
}
56+
3157
const { plain } = createKey({ tier: "admin", label: "recovery-admin" })
3258

33-
// Write to file and print
34-
fs.writeFileSync(ADMIN_KEY_FILE_RECOVER, plain + "\n", { mode: 0o600 })
35-
consola.success(`Recovery admin key generated: ${plain}`)
59+
// O_EXCL prevents symlink attacks and clobber on race
60+
try {
61+
fs.writeFileSync(ADMIN_KEY_FILE_RECOVER, plain + "\n", {
62+
mode: 0o600,
63+
flag: "wx",
64+
})
65+
} catch (err) {
66+
consola.error(`Failed to write recovery key file: ${String(err)}`)
67+
process.exit(1)
68+
}
69+
70+
// Mirror bootstrap.ts: full key on TTY only — avoid log capture on non-TTY
71+
if (process.stdout.isTTY) {
72+
consola.success(`Recovery admin key generated: ${plain}`)
73+
} else {
74+
consola.info(
75+
`Recovery admin key written to ${ADMIN_KEY_FILE_RECOVER}. Read and delete the file.`,
76+
)
77+
}
3678
consola.info(`Also written to ${ADMIN_KEY_FILE_RECOVER}`)
3779
},
3880
})
81+
82+
// Re-export the canonical file path so callers don't duplicate the join
83+
84+
export { ADMIN_KEY_FILE } from "~/lib/bootstrap"

src/lib/bootstrap.ts

Lines changed: 45 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,12 @@ import consola from "consola"
22
import fs from "node:fs"
33
import path from "node:path"
44

5-
import { countActiveAdminKeys, createKey, hashKey } from "~/services/keys"
5+
import { countActiveAdminKeys, createKey } from "~/services/keys"
66

77
import { getConfig } from "./config-store"
88
import { PATHS } from "./paths"
99

10-
const ADMIN_KEY_FILE = path.join(PATHS.APP_DIR, "admin.key.txt")
10+
export const ADMIN_KEY_FILE = path.join(PATHS.APP_DIR, "admin.key.txt")
1111

1212
/**
1313
* Returns true if the admin bootstrap file exists (operator hasn't read it yet).
@@ -19,40 +19,64 @@ export function bootstrapFilePending(): boolean {
1919
/**
2020
* Run bootstrap: if auth is enabled and no admin keys exist, generate one.
2121
* Called during startup AFTER initDb() and BEFORE HTTP listener binds.
22+
*
23+
* Logic:
24+
* 1. If auth is disabled → no-op.
25+
* 2. If admin keys already exist in DB → warn if file still present, return.
26+
* 3. Otherwise → create first admin key and write to ADMIN_KEY_FILE.
2227
*/
2328
export function runBootstrap(): void {
2429
const { features } = getConfig()
2530
if (!features.auth) return // auth off — no bootstrap needed
2631

27-
if (bootstrapFilePending()) {
28-
// Bootstrap file still exists — operator must read and delete it first
29-
consola.error(
30-
`Startup blocked: admin bootstrap file exists at ${ADMIN_KEY_FILE}.\n`
31-
+ `Please read your admin key and delete this file before restarting:\n`
32-
+ ` cat ${ADMIN_KEY_FILE} && rm ${ADMIN_KEY_FILE}`,
33-
)
34-
process.exit(1)
32+
const adminCount = countActiveAdminKeys()
33+
34+
if (adminCount > 0) {
35+
// Already bootstrapped. Remind operator to delete the key file if it lingers.
36+
if (bootstrapFilePending()) {
37+
consola.warn(
38+
`Admin key file still present at ${ADMIN_KEY_FILE}. Delete it after reading:\n`
39+
+ ` rm ${ADMIN_KEY_FILE}`,
40+
)
41+
}
42+
return
3543
}
3644

37-
const adminCount = countActiveAdminKeys()
38-
if (adminCount > 0) return // admin keys exist — no bootstrap needed
45+
// No active admin keys — generate the first one.
46+
// Ensure APP_DIR exists before writing key file.
47+
fs.mkdirSync(PATHS.APP_DIR, { recursive: true, mode: 0o700 })
3948

40-
// Generate first admin key
4149
const { plain } = createKey({ tier: "admin", label: "bootstrap-admin" })
42-
const sha256prefix = hashKey(plain).slice(0, 8)
4350

44-
// Write plaintext to file with mode 0600
45-
fs.mkdirSync(PATHS.APP_DIR, { recursive: true, mode: 0o700 })
46-
fs.writeFileSync(ADMIN_KEY_FILE, plain + "\n", { mode: 0o600 })
51+
// Write plaintext key with O_EXCL to prevent symlink/TOCTOU attacks.
52+
// If the file somehow already exists (race between two server starts),
53+
// the write fails — the first writer wins and the second exits cleanly.
54+
try {
55+
fs.writeFileSync(ADMIN_KEY_FILE, plain + "\n", { mode: 0o600, flag: "wx" })
56+
} catch (err) {
57+
const code = (err as NodeJS.ErrnoException).code
58+
if (code === "EEXIST") {
59+
// Another instance won the race — its key is valid.
60+
consola.warn(
61+
`Bootstrap key file already exists at ${ADMIN_KEY_FILE} (parallel start?). Using existing file.`,
62+
)
63+
return
64+
}
65+
// Unexpected write failure: key is in DB but not on disk.
66+
// Surface the error so the operator can run `admin recover`.
67+
consola.error(
68+
`Admin key created in DB but file write failed. Run 'copilot-api admin recover' to retrieve it. Error: ${String(err)}`,
69+
)
70+
throw err
71+
}
4772

48-
// Output to stdout: full key only on TTY, path+hash on non-TTY (Docker/journald)
49-
const isTty = process.stdout.isTTY
50-
if (isTty) {
73+
// Output: full key on TTY; path-only on non-TTY (Docker/journald — avoid log capture)
74+
if (process.stdout.isTTY) {
5175
consola.success(`Admin key generated: ${plain}`)
5276
consola.info(`Also written to ${ADMIN_KEY_FILE} (delete after reading)`)
5377
} else {
5478
consola.info(
55-
`Admin key written to ${ADMIN_KEY_FILE} (sha256:${sha256prefix}…). Read & delete this file.`,
79+
`Admin key written to ${ADMIN_KEY_FILE}. Read it and delete the file before restarting.`,
5680
)
5781
}
5882
}

src/lib/migrations/002_keys.sql

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@ CREATE TABLE IF NOT EXISTS keys (
33
hash TEXT UNIQUE NOT NULL,
44
tier TEXT NOT NULL CHECK(tier IN ('admin','client')),
55
label TEXT,
6-
allowed_models TEXT NOT NULL DEFAULT '["*"]',
6+
allowed_models TEXT NOT NULL DEFAULT '["*"]' CHECK(json_valid(allowed_models)),
77
rate_limit_override INTEGER,
88
debug_enabled INTEGER NOT NULL DEFAULT 0,
99
created_at INTEGER NOT NULL,
1010
revoked_at INTEGER
1111
);
12-
CREATE INDEX IF NOT EXISTS idx_keys_hash ON keys(hash);
12+
-- Note: UNIQUE on hash automatically creates an index; no explicit CREATE INDEX needed.

src/services/keys.ts

Lines changed: 80 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import crypto from "node:crypto"
22

33
import { getDb } from "~/lib/db"
44

5-
// Row type from DB
5+
// Row type from DB (full — includes hash for internal auth use)
66
export interface KeyRow {
77
id: string
88
hash: string
@@ -18,13 +18,17 @@ export interface KeyRow {
1818
// Base32 alphabet (RFC 4648, no padding)
1919
const BASE32_CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"
2020

21+
// Regex that mirrors the upstream model-name validation in config-store (SSRF prevention)
22+
const MODEL_RE = /^\w[\w.:-]*$/
23+
2124
/**
2225
* Generate a new API key: "sk-cap-" + 52 base32 chars = 59 chars total.
23-
* Uses 32 random bytes = 256 bits entropy.
26+
* Uses 33 random bytes = 264 bits of entropy; 264 / 5 = 52 full 5-bit groups
27+
* (260 bits encoded) with 4 bits remaining — no zero-padding required.
2428
*/
2529
export function generateKey(): string {
26-
const bytes = crypto.randomBytes(32)
27-
// Encode to base32: each 5 bits → one char; 32 bytes = 256 bits = 51.2 → pad to 52 chars
30+
const bytes = crypto.randomBytes(33)
31+
// Encode to base32: each 5 bits → one char; 33 bytes = 264 bits → exactly 52 chars
2832
let result = ""
2933
let buffer = 0
3034
let bitsLeft = 0
@@ -36,51 +40,81 @@ export function generateKey(): string {
3640
result += BASE32_CHARS[(buffer >> bitsLeft) & 0x1f]
3741
}
3842
}
39-
// Pad to exactly 52 chars if needed
40-
while (result.length < 52) {
41-
result += BASE32_CHARS[0]
42-
}
43+
// 33 bytes yields exactly 52 chars; the slice+guard is belt-and-suspenders
4344
return `sk-cap-${result.slice(0, 52)}`
4445
}
4546

4647
/**
47-
* Hash a plain key to sha256 hex for storage.
48+
* Hash a plain key to SHA-256 hex for storage.
49+
*
50+
* Unsalted SHA-256 is intentional: API keys have ≥260 bits of random entropy
51+
* so dictionary attacks and rainbow tables are meaningless.
52+
* Do NOT use this function for user-chosen secrets (passwords, PINs, etc.).
53+
*
4854
* The plain key value must NEVER be written to the DB.
4955
*/
5056
export function hashKey(plain: string): string {
5157
return crypto.createHash("sha256").update(plain).digest("hex")
5258
}
5359

60+
/** Validate allowedModels: non-empty array of valid model identifiers or "*" */
61+
function validateAllowedModels(models: Array<string> | undefined): void {
62+
if (models === undefined) return
63+
if (models.length === 0) {
64+
throw new Error(
65+
'allowedModels must not be empty; use ["*"] for unrestricted access',
66+
)
67+
}
68+
for (const m of models) {
69+
if (m !== "*" && !MODEL_RE.test(m)) {
70+
throw new Error(
71+
`Invalid model name in allowedModels: "${m}". Must match /^\\w[\\w.:-]*$/ or be "*"`,
72+
)
73+
}
74+
}
75+
}
76+
77+
/**
78+
* Compute the rate-limit integer to store: null means "inherit global".
79+
* Positive values are capped at 10× globalDefault.
80+
*/
81+
function resolveRateLimit(
82+
override: number | undefined,
83+
globalDefault: number,
84+
): number | null {
85+
if (override === undefined || override === 0) return null
86+
if (!Number.isInteger(override) || override < 0) {
87+
throw new Error("rateLimitOverride must be a non-negative integer")
88+
}
89+
const cap = globalDefault * 10
90+
if (override > cap) {
91+
throw new Error(
92+
`rate_limit_override ${override} exceeds cap ${cap} (10× global default ${globalDefault})`,
93+
)
94+
}
95+
return override
96+
}
97+
5498
export function createKey(options: {
5599
tier: "admin" | "client"
56100
label?: string
57101
allowedModels?: Array<string>
58102
rateLimitOverride?: number
59103
debugEnabled?: boolean
60-
globalRateLimit?: number // for cap enforcement
104+
globalRateLimit?: number // for cap enforcement; defaults to 60 if not provided
61105
}): { plain: string; row: KeyRow } {
106+
validateAllowedModels(options.allowedModels)
107+
62108
const db = getDb()
63109
const plain = generateKey()
64110
const hash = hashKey(plain)
65111
const id = crypto.randomUUID()
66112
const now = Date.now()
67113

68-
// Rate limit override safety: 0/negative = use default (null).
69-
// Hard cap at 10× global default; reject above cap.
70-
let rateLimit: number | null = null
71-
if (
72-
options.rateLimitOverride !== undefined
73-
&& options.rateLimitOverride > 0
74-
) {
75-
const globalDefault = options.globalRateLimit ?? 60
76-
const cap = globalDefault * 10
77-
if (options.rateLimitOverride > cap) {
78-
throw new Error(
79-
`rate_limit_override ${options.rateLimitOverride} exceeds cap ${cap} (10× global default ${globalDefault})`,
80-
)
81-
}
82-
rateLimit = options.rateLimitOverride
83-
}
114+
const rateLimit = resolveRateLimit(
115+
options.rateLimitOverride,
116+
options.globalRateLimit ?? 60,
117+
)
84118

85119
const allowedModels = JSON.stringify(options.allowedModels ?? ["*"])
86120

@@ -114,13 +148,23 @@ export function createKey(options: {
114148
return { plain, row }
115149
}
116150

117-
export function revokeKey(id: string): void {
118-
getDb().run(`UPDATE keys SET revoked_at = ? WHERE id = ?`, [Date.now(), id])
151+
/**
152+
* Revoke a key by ID.
153+
* Idempotent: only sets revoked_at if the key is currently active.
154+
* Returns true if the key was revoked, false if not found or already revoked.
155+
* Callers are responsible for verifying the tier before calling (no tier guard here).
156+
*/
157+
export function revokeKey(id: string): boolean {
158+
const result = getDb().run(
159+
`UPDATE keys SET revoked_at = ? WHERE id = ? AND revoked_at IS NULL`,
160+
[Date.now(), id],
161+
)
162+
return result.changes === 1
119163
}
120164

121165
export function listKeys(): Array<KeyRow> {
122166
return getDb()
123-
.query<KeyRow, []>("SELECT * FROM keys ORDER BY created_at")
167+
.query<KeyRow, []>("SELECT * FROM keys ORDER BY created_at, id")
124168
.all()
125169
}
126170

@@ -142,9 +186,14 @@ export function countActiveAdminKeys(): number {
142186
return row?.n ?? 0
143187
}
144188

145-
export function setDebugEnabled(id: string, enabled: boolean): void {
146-
getDb().run(`UPDATE keys SET debug_enabled = ? WHERE id = ?`, [
189+
/**
190+
* Toggle the debug flag on a key.
191+
* Returns true if the key was found and updated, false otherwise.
192+
*/
193+
export function setDebugEnabled(id: string, enabled: boolean): boolean {
194+
const result = getDb().run(`UPDATE keys SET debug_enabled = ? WHERE id = ?`, [
147195
enabled ? 1 : 0,
148196
id,
149197
])
198+
return result.changes === 1
150199
}

0 commit comments

Comments
 (0)