fix(relay-auth): close 5 pre-production security findings (F1–F5)
F1 (HIGH): bind step-up freshness to the required method (stepUpMethod) so a login-time or weaker (TOTP) factor can no longer satisfy a passkey requirement. F2 (HIGH): make the step-up gate host-driven and fail-closed on connect+reattach (deny when policy.required and the principal is missing/stale/wrong-method). F3 (MED): bind the WebAuthn assertion to the stored credential (credentialId cross-check + forward publicKey/credentialId to the verifier). F4 (LOW): make verifyDpopProof totally fail-safe (no unhandled throw), wrap the authz boundary into a clean audited 401, and validate cnfJkt format at issue. F5 (LOW): return newSignCount from finishAuthentication so the caller can persist the advanced counter (WebAuthn clone detection). All fixes ship regression tests. relay-auth: 122 tests green, tsc clean.
This commit is contained in:
@@ -11,6 +11,7 @@ import {
|
||||
} from '../src/capability/verify.js'
|
||||
import { jwkThumbprint } from '../src/crypto/thumbprint.js'
|
||||
import { CapabilityError } from '../src/capability/errors.js'
|
||||
import { encodeBase64UrlBytes } from 'relay-contracts'
|
||||
import { setupP5SigningKey, makeEphemeral, principal, uuid } from './_helpers.js'
|
||||
|
||||
const NOW = 1_700_000_000
|
||||
@@ -150,5 +151,31 @@ describe('capability token (§4.3)', () => {
|
||||
expect(await verifyDpopProof(tok, { proofJws: proof, htu, htm: 'GET' }, NOW)).toBe(true)
|
||||
expect(await verifyDpopProof(tok, { proofJws: proof, htu, htm: 'GET' }, NOW)).toBe(false)
|
||||
})
|
||||
|
||||
// F4: a proof whose jwk.x decodes to a NON-32-byte blob makes importEd25519PublicRaw throw.
|
||||
// verifyDpopProof must be TOTALLY fail-safe and RESOLVE to false, never reject.
|
||||
it('resolves false (never throws) when the proof key is not a valid 32-byte Ed25519 key', async () => {
|
||||
// A 16-byte "key" — importEd25519PublicRaw will reject this raw length.
|
||||
const shortBlob = new Uint8Array(16).fill(7)
|
||||
// cnf.jkt is computed over the SAME 16-byte blob so the thumbprint check passes and
|
||||
// execution reaches the risky importEd25519PublicRaw call.
|
||||
const cnfJkt = await jwkThumbprint(shortBlob)
|
||||
const raw = await issueFor(signingKey, { cnfJkt })
|
||||
const tok = await verifyCapabilityToken(raw, AUD, NOW)
|
||||
|
||||
const htu = 'https://alice.term.example.com/ws'
|
||||
const enc = (o: unknown) => encodeBase64UrlBytes(new TextEncoder().encode(JSON.stringify(o)))
|
||||
const h = enc({ typ: 'dpop+ed25519', jwk: { crv: 'Ed25519', kty: 'OKP', x: encodeBase64UrlBytes(shortBlob) } })
|
||||
const p = enc({ htu, htm: 'GET', jti: uuid(), iat: NOW })
|
||||
const s = encodeBase64UrlBytes(new Uint8Array(64)) // any signature bytes
|
||||
const proofJws = `${h}.${p}.${s}`
|
||||
|
||||
const verify = verifyDpopProof(tok, { proofJws, htu, htm: 'GET' }, NOW)
|
||||
await expect(verify).resolves.toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
it('rejects a malformed cnf.jkt at issue (not a 43-char base64url thumbprint)', async () => {
|
||||
await expect(issueFor(signingKey, { cnfJkt: 'short' })).rejects.toMatchObject({ reason: 'bad_cnf' })
|
||||
})
|
||||
})
|
||||
|
||||
@@ -20,7 +20,7 @@ const NOW = 1_700_000_000
|
||||
const AUD_A = 'alice.term.example.com'
|
||||
const ORIGIN_A = `https://${AUD_A}`
|
||||
const ALLOWED = [ORIGIN_A]
|
||||
const NEVER_REQUIRED: StepUpPolicy = { maxAgeSeconds: Number.MAX_SAFE_INTEGER, requiredMethod: 'passkey' }
|
||||
const NEVER_REQUIRED: StepUpPolicy = { required: false, maxAgeSeconds: Number.MAX_SAFE_INTEGER, requiredMethod: 'passkey' }
|
||||
|
||||
describe('enforcement onUpgrade/onReattach (T12)', () => {
|
||||
let signingKey: CryptoKey
|
||||
@@ -126,8 +126,8 @@ describe('enforcement onUpgrade/onReattach (T12)', () => {
|
||||
expect(out).toMatchObject({ ok: false, status: 403, reason: 'cross_tenant_session' })
|
||||
})
|
||||
|
||||
describe('v0.10 step-up augmentation (Finding-3)', () => {
|
||||
const STRICT: StepUpPolicy = { maxAgeSeconds: 300, requiredMethod: 'passkey' }
|
||||
describe('v0.10 step-up augmentation (Finding-3, F1/F2)', () => {
|
||||
const STRICT: StepUpPolicy = { required: true, maxAgeSeconds: 300, requiredMethod: 'passkey' }
|
||||
|
||||
it('fresh login but stale step-up → 403 step_up_required at onUpgrade', async () => {
|
||||
deps = { ...deps, stepUpPolicyFor: () => STRICT }
|
||||
@@ -141,10 +141,26 @@ describe('enforcement onUpgrade/onReattach (T12)', () => {
|
||||
it('after a fresh step-up the same request → ok:true', async () => {
|
||||
deps = { ...deps, stepUpPolicyFor: () => STRICT }
|
||||
const b = await issueWithDpop(signingKey, { accountId: 'acct-A', host: hostA, aud: AUD_A, now: NOW })
|
||||
const steppedUp = principal('acct-A', { authAt: NOW, stepUpAt: NOW, amr: ['passkey', 'stepup'] })
|
||||
const steppedUp = principal('acct-A', { authAt: NOW, stepUpAt: NOW, amr: ['passkey', 'stepup'], stepUpMethod: 'passkey' })
|
||||
expect(needsStepUp(steppedUp, STRICT, NOW)).toBe(false)
|
||||
const out = await onUpgrade(ctxFor(b, { principal: steppedUp }), deps, ALLOWED, NOW)
|
||||
expect(out.ok).toBe(true)
|
||||
})
|
||||
|
||||
it('STRICT (required) policy + principal:null → 403 step_up_required (F2 fail-closed)', async () => {
|
||||
deps = { ...deps, stepUpPolicyFor: () => STRICT }
|
||||
const b = await issueWithDpop(signingKey, { accountId: 'acct-A', host: hostA, aud: AUD_A, now: NOW })
|
||||
const out = await onUpgrade(ctxFor(b, { principal: null }), deps, ALLOWED, NOW)
|
||||
expect(out).toMatchObject({ ok: false, status: 403, reason: 'step_up_required' })
|
||||
expect(audit.events.some((e) => e.action === 'stepup' && e.outcome === 'deny')).toBe(true)
|
||||
})
|
||||
|
||||
it('required:false policy + principal:null → allow (non-step-up host preserved)', async () => {
|
||||
deps = { ...deps, stepUpPolicyFor: () => NEVER_REQUIRED }
|
||||
const b = await issueWithDpop(signingKey, { accountId: 'acct-A', host: hostA, aud: AUD_A, now: NOW })
|
||||
const out = await onUpgrade(ctxFor(b, { principal: null }), deps, ALLOWED, NOW)
|
||||
expect(out.ok).toBe(true)
|
||||
expect(audit.events.some((e) => e.outcome === 'allow')).toBe(true)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,10 +1,10 @@
|
||||
import { describe, it, expect } from 'vitest'
|
||||
import { needsStepUp, recordStepUp, policyForHost } from '../src/human/stepup/stepup.js'
|
||||
import { needsStepUp, recordStepUp, policyForHost, NO_STEPUP_POLICY } from '../src/human/stepup/stepup.js'
|
||||
import type { StepUpPolicy } from '../src/types.js'
|
||||
import { principal, makeHost, uuid } from './_helpers.js'
|
||||
|
||||
const NOW = 1_700_000_000
|
||||
const POLICY: StepUpPolicy = { maxAgeSeconds: 300, requiredMethod: 'passkey' }
|
||||
const POLICY: StepUpPolicy = { required: true, maxAgeSeconds: 300, requiredMethod: 'passkey' }
|
||||
|
||||
describe('step-up before session open (T8, Finding-3)', () => {
|
||||
it('fresh login but stepUpAt=null → needsStepUp true (login ≠ step-up)', () => {
|
||||
@@ -13,34 +13,61 @@ describe('step-up before session open (T8, Finding-3)', () => {
|
||||
})
|
||||
|
||||
it('stale step-up (older than maxAgeSeconds) → true', () => {
|
||||
const p = principal('acct-A', { stepUpAt: NOW - 301, amr: ['passkey', 'stepup'] })
|
||||
const p = principal('acct-A', { stepUpAt: NOW - 301, amr: ['passkey', 'stepup'], stepUpMethod: 'passkey' })
|
||||
expect(needsStepUp(p, POLICY, NOW)).toBe(true)
|
||||
})
|
||||
|
||||
it('required method absent from amr → true', () => {
|
||||
const p = principal('acct-A', { stepUpAt: NOW, amr: ['totp'] })
|
||||
it('required method not the LAST step-up method → true', () => {
|
||||
const p = principal('acct-A', { stepUpAt: NOW, amr: ['totp'], stepUpMethod: 'totp' })
|
||||
expect(needsStepUp(p, POLICY, NOW)).toBe(true)
|
||||
})
|
||||
|
||||
it('fresh passkey step-up → false', () => {
|
||||
const p = principal('acct-A', { stepUpAt: NOW, amr: ['passkey'] })
|
||||
it('fresh passkey step-up satisfies passkey policy → false (F1)', () => {
|
||||
const p = principal('acct-A', { stepUpAt: NOW, amr: ['passkey'], stepUpMethod: 'passkey' })
|
||||
expect(needsStepUp(p, POLICY, NOW)).toBe(false)
|
||||
})
|
||||
|
||||
it('fresh TOTP step-up does NOT satisfy passkey policy → true (F1 factor-downgrade regression)', () => {
|
||||
// A weaker (TOTP) step-up performed just now must not satisfy a passkey requirement, even though
|
||||
// it is fresh — the required method itself must be the LAST step-up.
|
||||
const p = principal('acct-A', { stepUpAt: NOW, amr: ['passkey', 'totp'], stepUpMethod: 'totp' })
|
||||
expect(needsStepUp(p, POLICY, NOW)).toBe(true)
|
||||
})
|
||||
|
||||
it('login-time passkey in amr but no step-up (stepUpMethod null) → true (login ≠ step-up)', () => {
|
||||
const p = principal('acct-A', { stepUpAt: NOW, amr: ['passkey'], stepUpMethod: null })
|
||||
expect(needsStepUp(p, POLICY, NOW)).toBe(true)
|
||||
})
|
||||
|
||||
it('recordStepUp returns a NEW principal (immutable), original untouched', () => {
|
||||
const p = principal('acct-A', { stepUpAt: null, amr: ['totp'] })
|
||||
const p = principal('acct-A', { stepUpAt: null, amr: ['totp'], stepUpMethod: 'totp' })
|
||||
const stepped = recordStepUp(p, 'passkey', NOW)
|
||||
expect(stepped).not.toBe(p)
|
||||
expect(p.stepUpAt).toBeNull()
|
||||
expect(p.amr).toEqual(['totp'])
|
||||
expect(p.stepUpMethod).toBe('totp')
|
||||
expect(stepped.stepUpAt).toBe(NOW)
|
||||
expect(stepped.amr).toContain('passkey')
|
||||
expect(needsStepUp(stepped, POLICY, NOW)).toBe(false)
|
||||
})
|
||||
|
||||
it('policyForHost yields a passkey freshness requirement', () => {
|
||||
it('recordStepUp stamps stepUpMethod with the method used (F1)', () => {
|
||||
const p = principal('acct-A', { stepUpAt: null, amr: ['passkey'], stepUpMethod: null })
|
||||
const stepped = recordStepUp(p, 'passkey', NOW)
|
||||
expect(stepped.stepUpMethod).toBe('passkey')
|
||||
expect(stepped.stepUpAt).toBe(NOW)
|
||||
})
|
||||
|
||||
it('policyForHost yields a REQUIRED passkey freshness requirement', () => {
|
||||
const policy = policyForHost(makeHost('acct-A', uuid()))
|
||||
expect(policy.required).toBe(true)
|
||||
expect(policy.requiredMethod).toBe('passkey')
|
||||
expect(policy.maxAgeSeconds).toBeGreaterThan(0)
|
||||
})
|
||||
|
||||
it('NO_STEPUP_POLICY is the v0.9 never-required default', () => {
|
||||
expect(NO_STEPUP_POLICY.required).toBe(false)
|
||||
expect(NO_STEPUP_POLICY.requiredMethod).toBe('passkey')
|
||||
expect(NO_STEPUP_POLICY.maxAgeSeconds).toBeGreaterThan(0)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -29,7 +29,7 @@ const NOW = 1_700_000_000
|
||||
const AUD_A = 'alice.term.example.com'
|
||||
const AUD_B = 'bob.term.example.com'
|
||||
const ORIGIN_A = `https://${AUD_A}`
|
||||
const NEVER: StepUpPolicy = { maxAgeSeconds: Number.MAX_SAFE_INTEGER, requiredMethod: 'passkey' }
|
||||
const NEVER: StepUpPolicy = { required: false, maxAgeSeconds: Number.MAX_SAFE_INTEGER, requiredMethod: 'passkey' }
|
||||
|
||||
describe('PERMANENT TRIPWIRE — device A can never reach host B (INV1)', () => {
|
||||
let signingKey: CryptoKey
|
||||
|
||||
60
relay-auth/test/types.test.ts
Normal file
60
relay-auth/test/types.test.ts
Normal file
@@ -0,0 +1,60 @@
|
||||
import { describe, it, expect } from 'vitest'
|
||||
import {
|
||||
AuthenticatedPrincipalSchema,
|
||||
StepUpPolicySchema,
|
||||
type AuthenticatedPrincipal,
|
||||
type StepUpPolicy,
|
||||
} from '../src/types.js'
|
||||
|
||||
const basePrincipal = {
|
||||
kind: 'human' as const,
|
||||
accountId: 'acct-A',
|
||||
principalId: 'cred-1',
|
||||
amr: ['passkey' as const],
|
||||
authAt: 1_700_000_000,
|
||||
stepUpAt: null,
|
||||
}
|
||||
|
||||
describe('StepUpPolicy.required (host-driven, fail-closed)', () => {
|
||||
it('parses a policy with required=true', () => {
|
||||
const policy: StepUpPolicy = { required: true, maxAgeSeconds: 300, requiredMethod: 'passkey' }
|
||||
expect(StepUpPolicySchema.parse(policy)).toEqual(policy)
|
||||
})
|
||||
|
||||
it('parses a policy with required=false', () => {
|
||||
const policy: StepUpPolicy = { required: false, maxAgeSeconds: 300, requiredMethod: 'passkey' }
|
||||
expect(StepUpPolicySchema.parse(policy)).toEqual(policy)
|
||||
})
|
||||
|
||||
it('rejects a policy missing required', () => {
|
||||
const bad = { maxAgeSeconds: 300, requiredMethod: 'passkey' }
|
||||
expect(StepUpPolicySchema.safeParse(bad).success).toBe(false)
|
||||
})
|
||||
|
||||
it('rejects a non-boolean required', () => {
|
||||
const bad = { required: 'yes', maxAgeSeconds: 300, requiredMethod: 'passkey' }
|
||||
expect(StepUpPolicySchema.safeParse(bad).success).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('AuthenticatedPrincipal.stepUpMethod (F1 method-binding, optional)', () => {
|
||||
it('parses a principal that omits stepUpMethod (backward compatible)', () => {
|
||||
const parsed = AuthenticatedPrincipalSchema.parse(basePrincipal)
|
||||
expect(parsed.stepUpMethod).toBeUndefined()
|
||||
})
|
||||
|
||||
it('parses a principal with an explicit stepUpMethod', () => {
|
||||
const p: AuthenticatedPrincipal = { ...basePrincipal, stepUpAt: 1_700_000_100, stepUpMethod: 'passkey' }
|
||||
expect(AuthenticatedPrincipalSchema.parse(p).stepUpMethod).toBe('passkey')
|
||||
})
|
||||
|
||||
it('parses a principal with stepUpMethod=null (no step-up)', () => {
|
||||
const parsed = AuthenticatedPrincipalSchema.parse({ ...basePrincipal, stepUpMethod: null })
|
||||
expect(parsed.stepUpMethod).toBeNull()
|
||||
})
|
||||
|
||||
it('rejects an invalid stepUpMethod value', () => {
|
||||
const bad = { ...basePrincipal, stepUpMethod: 'sms' }
|
||||
expect(AuthenticatedPrincipalSchema.safeParse(bad).success).toBe(false)
|
||||
})
|
||||
})
|
||||
@@ -21,14 +21,14 @@ const verifier: WebAuthnVerifier = {
|
||||
signCount: 0,
|
||||
transports: ['internal'],
|
||||
}),
|
||||
verifyAuthentication: async (_r, _c, _rp, _o, stored) => ({ verified: true, newSignCount: stored + 1 }),
|
||||
verifyAuthentication: async (_r, _c, _rp, _o, _pk, _cid, stored) => ({ verified: true, newSignCount: stored + 1 }),
|
||||
}
|
||||
|
||||
function regResp(over: Partial<RegistrationResponse> = {}): RegistrationResponse {
|
||||
return { clientChallenge: 'chal', origin: ORIGIN, rpId: RP_ID, ...over }
|
||||
}
|
||||
function authResp(over: Partial<AuthenticationResponse> = {}): AuthenticationResponse {
|
||||
return { clientChallenge: 'chal', origin: ORIGIN, rpId: RP_ID, ...over }
|
||||
return { clientChallenge: 'chal', origin: ORIGIN, rpId: RP_ID, credentialId: 'cred-1', ...over }
|
||||
}
|
||||
const cred: WebAuthnCredential = {
|
||||
credentialId: 'cred-1',
|
||||
@@ -68,16 +68,32 @@ describe('WebAuthn / Passkey (T5, primary)', () => {
|
||||
it('happy path auth yields a principal with amr:[passkey]', async () => {
|
||||
const startOpts = await startAuthentication('acct-A', RP_ID)
|
||||
expect(startOpts.rpId).toBe(RP_ID)
|
||||
const p = await finishAuthentication(cred, authResp(), 'chal', RP_ID, ORIGIN, verifier, NOW)
|
||||
const { principal: p } = await finishAuthentication(cred, authResp(), 'chal', RP_ID, ORIGIN, verifier, NOW)
|
||||
expect(p.accountId).toBe('acct-A')
|
||||
expect(p.amr).toEqual(['passkey'])
|
||||
expect(p.authAt).toBe(NOW)
|
||||
})
|
||||
|
||||
it('F3: rejects an assertion whose credentialId does not match the stored credential', async () => {
|
||||
const forgiving: WebAuthnVerifier = {
|
||||
...verifier,
|
||||
verifyAuthentication: async (_r, _c, _rp, _o, _pk, _cid, stored) => ({ verified: true, newSignCount: stored + 1 }),
|
||||
}
|
||||
await expect(
|
||||
finishAuthentication(cred, authResp({ credentialId: 'other-cred' }), 'chal', RP_ID, ORIGIN, forgiving, NOW),
|
||||
).rejects.toThrow('credential id mismatch')
|
||||
})
|
||||
|
||||
it('F5: returns the verifier-advanced newSignCount so the caller can persist it', async () => {
|
||||
const { principal: p, newSignCount } = await finishAuthentication(cred, authResp(), 'chal', RP_ID, ORIGIN, verifier, NOW)
|
||||
expect(p.accountId).toBe('acct-A')
|
||||
expect(newSignCount).toBe(cred.signCount + 1)
|
||||
})
|
||||
|
||||
it('rejects a signCount regression (cloned authenticator signal)', async () => {
|
||||
const regressing: WebAuthnVerifier = {
|
||||
...verifier,
|
||||
verifyAuthentication: async () => ({ verified: true, newSignCount: 3 }), // < stored 5
|
||||
verifyAuthentication: async () => ({ verified: true, newSignCount: 3 }), // < stored 5 (adapts to {principal,newSignCount} return)
|
||||
}
|
||||
await expect(
|
||||
finishAuthentication(cred, authResp(), 'chal', RP_ID, ORIGIN, regressing, NOW),
|
||||
|
||||
Reference in New Issue
Block a user