-
Notifications
You must be signed in to change notification settings - Fork 18
fix(mpp/ts): enforce 32-byte HMAC secret floor at the @solana/mpp boundary (audit #24) #170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /** | ||
| * Audit #24: the @solana/mpp boundary rejects a weak HMAC secret key before any | ||
| * challenge is signed. mppx@0.5.x accepts any non-empty secret, so the floor is | ||
| * enforced here in the `Mppx.create` wrapper (see server/secret-guard.ts). | ||
| */ | ||
| import { afterEach, expect, test } from 'vitest'; | ||
| import { charge } from '../server/Charge.js'; | ||
| import { MIN_SECRET_KEY_BYTES, Mppx } from '../server/secret-guard.js'; | ||
|
|
||
| const RECIPIENT = '9xAXssX9j7vuK99c7cFwqbixzL3bFrzPy9PUhCtDPAYJ'; | ||
| const methods = () => [charge({ recipient: RECIPIENT, network: 'devnet', rpcUrl: 'https://mock-rpc' })]; | ||
|
|
||
| // A 'a'.repeat(N) string is N UTF-8 bytes, so length maps directly to byte count. | ||
| const strong = 'a'.repeat(MIN_SECRET_KEY_BYTES); | ||
| const tooShort = 'a'.repeat(MIN_SECRET_KEY_BYTES - 1); | ||
|
|
||
| afterEach(() => { | ||
| delete process.env.MPP_SECRET_KEY; | ||
| }); | ||
|
|
||
| test('rejects an explicit secret key shorter than the floor', () => { | ||
| expect(() => Mppx.create({ methods: methods(), secretKey: tooShort })).toThrow(/at least 32 bytes/); | ||
| }); | ||
|
|
||
| test('accepts an explicit secret key at the floor', () => { | ||
| delete process.env.MPP_SECRET_KEY; | ||
| expect(() => Mppx.create({ methods: methods(), secretKey: strong })).not.toThrow(); | ||
| }); | ||
|
|
||
| test('rejects a short MPP_SECRET_KEY from the environment', () => { | ||
| process.env.MPP_SECRET_KEY = tooShort; | ||
| expect(() => Mppx.create({ methods: methods() })).toThrow(/at least 32 bytes/); | ||
| }); | ||
|
|
||
| test('accepts a strong MPP_SECRET_KEY from the environment', () => { | ||
| process.env.MPP_SECRET_KEY = strong; | ||
| expect(() => Mppx.create({ methods: methods() })).not.toThrow(); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| // Defensive secret-key strength guard for the MPP server (audit #24). | ||
| // | ||
| // pay-kit's TypeScript SDK delegates HMAC-bound challenge issuance/verification | ||
| // to `mppx` via `Mppx.create({ secretKey })`. mppx@0.5.x only rejects an empty | ||
| // secret (`if (!secretKey) throw`), so a weak key such as `"key"` is accepted as | ||
| // the HMAC-SHA256 key that binds challenge IDs — an attacker who guesses or | ||
| // brute-forces a low-entropy key can forge challenges. Per NIST SP 800-107 the | ||
| // HMAC-SHA256 key should be at least the hash output size (32 bytes). | ||
| // | ||
| // Until a length floor lands upstream in mppx, we gate at OUR `@solana/mpp` | ||
| // boundary, mirroring `challenge-guard.ts`: this module re-exports the `Mppx` | ||
| // namespace with `create` wrapped so a short secret is rejected before any | ||
| // challenge is signed. We do NOT fork or vendor mppx, and we touch only the | ||
| // secret-strength gate, never the signing or settlement path. | ||
|
|
||
| import { Mppx as MppxBase } from 'mppx/server'; | ||
|
|
||
| /** | ||
| * Minimum accepted MPP HMAC secret-key length in bytes (audit #24). | ||
| * | ||
| * 32 bytes matches the HMAC-SHA256 output size (NIST SP 800-107). Generate a | ||
| * conforming key with `openssl rand -base64 32`. | ||
| */ | ||
| export const MIN_SECRET_KEY_BYTES = 32; | ||
|
|
||
| function assertStrongSecret(config: Parameters<typeof MppxBase.create>[0]): void { | ||
| // Mirror mppx's own resolution order: explicit `secretKey`, else the | ||
| // `MPP_SECRET_KEY` environment variable. When neither is set we stay silent | ||
| // and let mppx raise its own "secret key required" error, so we own only the | ||
| // strength check and never change the missing-secret behavior. | ||
| const secret = config.secretKey ?? process.env.MPP_SECRET_KEY; | ||
| if (secret === undefined) return; | ||
|
|
||
| const byteLength = new TextEncoder().encode(secret).length; | ||
| if (byteLength < MIN_SECRET_KEY_BYTES) { | ||
| throw new Error( | ||
| `MPP secret key must be at least ${MIN_SECRET_KEY_BYTES} bytes (got ${byteLength}); ` + | ||
| 'generate one with `openssl rand -base64 32`', | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * `Mppx.create` with the audit #24 secret-strength gate applied. Identical to | ||
| * `mppx`'s `Mppx.create` except a secret shorter than {@link MIN_SECRET_KEY_BYTES} | ||
| * bytes (whether passed explicitly or via `MPP_SECRET_KEY`) is rejected. | ||
| */ | ||
| export const create: typeof MppxBase.create = (config => { | ||
| assertStrongSecret(config); | ||
| return MppxBase.create(config); | ||
| }) as typeof MppxBase.create; | ||
|
|
||
| /** | ||
| * The `Mppx` namespace as exposed by `@solana/mpp/server`: the upstream `mppx` | ||
| * surface with the secret-strength gate applied to `create`. Use this instead | ||
| * of importing `Mppx` directly from `mppx` so a weak HMAC secret is rejected at | ||
| * server construction. | ||
| */ | ||
| export const Mppx: typeof MppxBase = { | ||
| ...MppxBase, | ||
| create, | ||
| }; | ||
|
Comment on lines
+59
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two behaviours documented in the guard but not exercised by the suite: (1) when neither
secretKeynorMPP_SECRET_KEYis set the guard must stay silent (currently only an inline comment guarantees this); (2) when both are set and they differ in strength, onlysecretKeyis evaluated due to??short-circuiting — a short env var with a strong explicit key should be accepted, and a strong env var with a short explicit key should be rejected. Without these cases the guard's priority semantics are untested and a silent regression would be easy to miss.