fix(auth): compare HEALTH_TOKEN in constant time via tokenMatches #30
3 changed files with 79 additions and 17 deletions
|
|
@ -24,7 +24,7 @@ Ordre **non negociable**, fige par deux tests `404 (not 401)` dans `__tests__/au
|
||||||
Consequence voulue : un appelant non authentifie sur une route inconnue voit 404, pas 401. Ne pas « corriger » cet ordre en mettant l'auth d'abord.
|
Consequence voulue : un appelant non authentifie sur une route inconnue voit 404, pas 401. Ne pas « corriger » cet ordre en mettant l'auth d'abord.
|
||||||
|
|
||||||
- Fail-closed : `HEALTH_TOKEN` absent -> 401 sur les lectures ; `HOSTS_INGEST_TOKEN` absent -> 503 sur l'ingestion
|
- Fail-closed : `HEALTH_TOKEN` absent -> 401 sur les lectures ; `HOSTS_INGEST_TOKEN` absent -> 503 sur l'ingestion
|
||||||
- `tokenMatches()` (`timingSafeEqual` sur empreintes SHA-256) protege **uniquement** le chemin d'ingestion. Le realignement du `HEALTH_TOKEN` existant est suivi separement (issue #17) : il toucherait tous les chemins de lecture en production
|
- `tokenMatches()` (`timingSafeEqual` sur empreintes SHA-256, longueurs egalisees par le hachage) compare **les deux tokens** depuis l'issue #17 : `checkAuth()` n'a qu'une expression de comparaison, seul le token attendu varie. Ne jamais revenir a une egalite de chaine (`` header === `Bearer ${x}` ``) : elle s'arrete au premier octet different et fuit longueur et prefixe par le temps de reponse. Un test de timing serait instable ; la garde est une epingle sur le source dans `__tests__/auth.test.js`
|
||||||
- 401 passe avant 403 : un appelant sans token ne peut pas sonder l'allowlist
|
- 401 passe avant 403 : un appelant sans token ne peut pas sonder l'allowlist
|
||||||
- Chaque rejet 401/403 est journalise (`[auth] <code> <method> <url> ip=<X-Real-IP> id=<host> reason=<motif>`), valeurs filtrees en ASCII imprimable pour empecher la forge de lignes de log
|
- Chaque rejet 401/403 est journalise (`[auth] <code> <method> <url> ip=<X-Real-IP> id=<host> reason=<motif>`), valeurs filtrees en ASCII imprimable pour empecher la forge de lignes de log
|
||||||
- **Coolify** : `HEALTH_TOKEN` et `HOSTS_INGEST_TOKEN` doivent etre `is_runtime=true, is_buildtime=false`. Buildtime fait fuiter le secret en clair dans `application_deployment_queues.logs`. Voir `la-compagnie-maximus/docs/coolify-ops.md` section "Secrets en buildtime".
|
- **Coolify** : `HEALTH_TOKEN` et `HOSTS_INGEST_TOKEN` doivent etre `is_runtime=true, is_buildtime=false`. Buildtime fait fuiter le secret en clair dans `application_deployment_queues.logs`. Voir `la-compagnie-maximus/docs/coolify-ops.md` section "Secrets en buildtime".
|
||||||
|
|
@ -59,11 +59,11 @@ curl -H "Authorization: Bearer $(cat ~/.coolify-token)" \
|
||||||
|
|
||||||
## Tests
|
## Tests
|
||||||
|
|
||||||
- `npm test` (vitest) — 263 cas
|
- `npm test` (vitest) — 284 cas
|
||||||
- `__tests__/findings.test.js` — `/defenseurs/findings` (14 cas : auth, validation, filtres severity/category, asymetrie INFO, scan clean vs no_data, JSON corrompu)
|
- `__tests__/findings.test.js` — `/defenseurs/findings` (14 cas : auth, validation, filtres severity/category, asymetrie INFO, scan clean vs no_data, JSON corrompu)
|
||||||
- `__tests__/state.test.js` — `/defenseurs/state` + non-regression `/defenseurs` apres extraction de `serveJsonFile()` (9 cas : corps identique octet pour octet sur un JSON non canonique, no_data, isolation des deux chemins, 401 sans token / mauvais token / token d'ingestion, 404 sur POST)
|
- `__tests__/state.test.js` — `/defenseurs/state` + non-regression `/defenseurs` apres extraction de `serveJsonFile()` (9 cas : corps identique octet pour octet sur un JSON non canonique, no_data, isolation des deux chemins, 401 sans token / mauvais token / token d'ingestion, 404 sur POST)
|
||||||
- `__tests__/health.test.js` — module `metrics.js` + `/health` (6 cas : exports, payload champ pour champ, garde de latence)
|
- `__tests__/health.test.js` — module `metrics.js` + `/health` (6 cas : exports, payload champ pour champ, garde de latence)
|
||||||
- `__tests__/auth.test.js` — filet de securite auth sur les routes de lecture (29 cas : matrice route x mode d'echec, en-tetes malformes, fail-closed, et **2 cas `404 (not 401)` qui figent l'ordre routage -> auth**)
|
- `__tests__/auth.test.js` — filet de securite auth sur les routes de lecture (50 cas : matrice route x mode d'echec, en-tetes malformes, quasi-rates du token sur les 6 routes de lecture, fail-closed, **2 cas `404 (not 401)` qui figent l'ordre routage -> auth**, et **une epingle sur le source** qui interdit toute egalite directe contre une chaine `Bearer` et exige un seul appel a `tokenMatches()` dans `checkAuth()`)
|
||||||
- `__tests__/hosts.test.js` — matrice exhaustive des endpoints postes (155 cas : regex de route comme controle anti-traversee, 401 avant 403, cloisonnement des deux tokens, 503 fail-closed, reconstruction par liste blanche, 413, `neverSeen`, fraicheur serveur sous horloge figee, degradation de lecture)
|
- `__tests__/hosts.test.js` — matrice exhaustive des endpoints postes (155 cas : regex de route comme controle anti-traversee, 401 avant 403, cloisonnement des deux tokens, 503 fail-closed, reconstruction par liste blanche, 413, `neverSeen`, fraicheur serveur sous horloge figee, degradation de lecture)
|
||||||
- `__tests__/agent.test.js` — agent poste (50 cas : contrat de payload verifie contre `sanitizeSnapshot()`, une seule tentative sans rejeu, timeout, codes de sortie, wrapper shell, et **le token absent de toute sortie d'echec, fragments de 5 caracteres compris**)
|
- `__tests__/agent.test.js` — agent poste (50 cas : contrat de payload verifie contre `sanitizeSnapshot()`, une seule tentative sans rejeu, timeout, codes de sortie, wrapper shell, et **le token absent de toute sortie d'echec, fragments de 5 caracteres compris**)
|
||||||
- Runtime reste 0-dep ; vitest en devDep uniquement
|
- Runtime reste 0-dep ; vitest en devDep uniquement
|
||||||
|
|
|
||||||
|
|
@ -117,8 +117,8 @@ describe("auth gate — invalid token", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("auth gate — malformed Authorization header", () => {
|
describe("auth gate — malformed Authorization header", () => {
|
||||||
// The header is compared byte for byte against `Bearer <token>`, so these
|
// The scheme must be exactly `Bearer `, then the token is compared in
|
||||||
// near-misses are all rejected. /defenseurs stands in for the read routes:
|
// constant time via tokenMatches(), so these near-misses are all rejected. /defenseurs stands in for the read routes:
|
||||||
// it serves the parc-wide Defenseurs report and is the costliest to leak.
|
// it serves the parc-wide Defenseurs report and is the costliest to leak.
|
||||||
const MALFORMED = [
|
const MALFORMED = [
|
||||||
["raw token without the Bearer scheme", TOKEN],
|
["raw token without the Bearer scheme", TOKEN],
|
||||||
|
|
@ -133,6 +133,71 @@ describe("auth gate — malformed Authorization header", () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("auth gate — near-miss tokens on the read routes", () => {
|
||||||
|
// A comparison that stops at the first differing byte leaks the token's
|
||||||
|
// length and prefix through timing (issue #17). These cases pin the
|
||||||
|
// observable contract — exact match only — on every read route. They would
|
||||||
|
// also pass against a naive `===`: the timing property itself is guarded by
|
||||||
|
// the source pin below, not by these.
|
||||||
|
const NEAR_MISSES = [
|
||||||
|
["token prefix", `Bearer ${TOKEN.slice(0, -1)}`],
|
||||||
|
["token with a suffix", `Bearer ${TOKEN}x`],
|
||||||
|
["double space after the scheme", `Bearer ${TOKEN}`],
|
||||||
|
];
|
||||||
|
const CASES = ROUTES.flatMap((route) => NEAR_MISSES.map(([label, header]) => [route, label, header]));
|
||||||
|
|
||||||
|
test.each(CASES)("401 on GET %s — %s", async (route, _label, header) => {
|
||||||
|
const { status, body } = await request(route, { auth: header });
|
||||||
|
expect(status).toBe(401);
|
||||||
|
expect(body.error).toBe("Unauthorized");
|
||||||
|
});
|
||||||
|
|
||||||
|
// `Bearer ` with an empty token cannot be sent over the wire: the HTTP
|
||||||
|
// parser strips trailing whitespace and the server sees `Bearer`, already
|
||||||
|
// covered above. The empty-token branch is pinned on the helper directly.
|
||||||
|
test("tokenMatches rejects an empty token", () => {
|
||||||
|
const { tokenMatches } = require("../index.js");
|
||||||
|
expect(tokenMatches("", TOKEN)).toBe(false);
|
||||||
|
expect(tokenMatches(TOKEN, TOKEN)).toBe(true);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("auth gate — source pin: one constant-time comparison", () => {
|
||||||
|
// A timing test would be flaky, so the guard against a silent return to a
|
||||||
|
// short-circuiting comparison is structural: read index.js, comments
|
||||||
|
// stripped, and check how checkAuth() compares the header.
|
||||||
|
const source = fs
|
||||||
|
.readFileSync(path.join(__dirname, "..", "index.js"), "utf-8")
|
||||||
|
.replace(/\/\*[\s\S]*?\*\//g, "")
|
||||||
|
.replace(/^\s*\/\/.*$/gm, "");
|
||||||
|
|
||||||
|
function checkAuthBody() {
|
||||||
|
const start = source.indexOf("function checkAuth(");
|
||||||
|
expect(start).toBeGreaterThanOrEqual(0);
|
||||||
|
const rest = source.slice(start + 1);
|
||||||
|
const next = rest.search(/^(async\s+)?function\s/m);
|
||||||
|
return next === -1 ? rest : rest.slice(0, next);
|
||||||
|
}
|
||||||
|
|
||||||
|
test("no direct equality against a Bearer string anywhere in index.js", () => {
|
||||||
|
// Template literal (`Bearer ${x}`) or concatenation ("Bearer " + x), on
|
||||||
|
// either side of == / ===.
|
||||||
|
const bearerExpr = String.raw`(\`Bearer [^\`]*\`|["']Bearer ["']\s*\+\s*[\w.]+)`;
|
||||||
|
const direct = new RegExp(String.raw`[!=]==?\s*${bearerExpr}|${bearerExpr}\s*[!=]==?`);
|
||||||
|
expect(source).not.toMatch(direct);
|
||||||
|
});
|
||||||
|
|
||||||
|
test("checkAuth() calls tokenMatches exactly once, for both token kinds", () => {
|
||||||
|
const body = checkAuthBody();
|
||||||
|
expect(body.match(/tokenMatches\(/g)).toHaveLength(1);
|
||||||
|
// The comparison must not be chosen per token kind: `isIngest` may pick
|
||||||
|
// the expected token and the error, never the comparison itself.
|
||||||
|
// `typeof header === "string"` is the only allowed comparison on header.
|
||||||
|
expect(body).not.toMatch(/\bheader\s*[!=]==?(?!=)\s*[^"'\s]/);
|
||||||
|
expect(body).not.toMatch(/isIngest\s*\?[^;]*tokenMatches/);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe("auth gate — fail-closed when HEALTH_TOKEN is unset", () => {
|
describe("auth gate — fail-closed when HEALTH_TOKEN is unset", () => {
|
||||||
test("401 on GET /defenseurs even with a well-formed bearer token", async () => {
|
test("401 on GET /defenseurs even with a well-formed bearer token", async () => {
|
||||||
delete process.env.HEALTH_TOKEN;
|
delete process.env.HEALTH_TOKEN;
|
||||||
|
|
|
||||||
19
index.js
19
index.js
|
|
@ -257,13 +257,11 @@ function logRejection(req, status, reason, id) {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Constant-time bearer comparison for the ingestion path. Both sides are hashed
|
// Constant-time bearer comparison, used for BOTH tokens (read and ingest). Both
|
||||||
// first, so the buffers are always 32 bytes: timingSafeEqual never throws on
|
// sides are hashed first, so the buffers are always 32 bytes: timingSafeEqual
|
||||||
// mismatched lengths, and the length of the real token cannot leak.
|
// never throws on mismatched lengths, and the length of the real token cannot
|
||||||
//
|
// leak. Passing the raw strings to timingSafeEqual would leak it
|
||||||
// Deliberately scoped to the new write path. Realigning the existing
|
// (GHSA-376v-xgjx-7mfr).
|
||||||
// HEALTH_TOKEN comparison touches every read route in production and is tracked
|
|
||||||
// separately (issue #17).
|
|
||||||
function tokenMatches(provided, expected) {
|
function tokenMatches(provided, expected) {
|
||||||
if (typeof provided !== "string" || typeof expected !== "string") return false;
|
if (typeof provided !== "string" || typeof expected !== "string") return false;
|
||||||
const a = createHash("sha256").update(provided).digest();
|
const a = createHash("sha256").update(provided).digest();
|
||||||
|
|
@ -690,11 +688,10 @@ function checkAuth(req, route) {
|
||||||
}
|
}
|
||||||
|
|
||||||
const header = req.headers["authorization"];
|
const header = req.headers["authorization"];
|
||||||
const ok = isIngest
|
const ok =
|
||||||
? typeof header === "string" &&
|
typeof header === "string" &&
|
||||||
header.startsWith("Bearer ") &&
|
header.startsWith("Bearer ") &&
|
||||||
tokenMatches(header.slice("Bearer ".length), expected)
|
tokenMatches(header.slice("Bearer ".length), expected);
|
||||||
: header === `Bearer ${expected}`;
|
|
||||||
|
|
||||||
if (!ok) {
|
if (!ok) {
|
||||||
return { status: 401, body: { error: "Unauthorized" }, reason: "missing or invalid bearer token" };
|
return { status: 401, body: { error: "Unauthorized" }, reason: "missing or invalid bearer token" };
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue