fix: use oidcc_jwt_util:verify_signature/3, not jose_jwt:verify/2 directly - #10
Conversation
…ectly
validate_token/3's JWKS lookup (oidcc_provider_configuration_worker:get_jwks/1)
always returns a #jose_jwk{keys = {jose_jwk_set, _}} - a provider can
rotate keys, so it's always a set, even with one key. jose_jwt:verify/2
has no clause matching a key set (only a single #jose_jwk{} with a
concrete kty), so it raised an uncaught function_clause on every
genuinely-signed token. This function had zero test coverage of its
actual signature-verification path (only validate_claims/3 was tested
in isolation), so it shipped broken: any consumer calling
validate_token/3 against a real OIDC provider got a crash, not a
validated identity.
Replaced with oidcc_jwt_util:verify_signature/3 (already in the oidcc
dependency), which:
- folds over a key set correctly, matching by `kid` when the token's
header carries one
- enforces an explicit algorithm allowlist (added here: RS256/384/512,
ES256/384/512 - the algorithms every major OIDC provider actually
signs ID tokens with). HMAC variants are deliberately excluded: an
RSA public key is, by definition, public (published at the
provider's JWKS endpoint), and if HS* were allowed an attacker could
replay it as an HMAC secret to forge a token ("alg confusion",
RFC 8725)
- rejects alg:none explicitly rather than relying on it happening to
fall through to a non-match
Added end-to-end signature-verification tests (previously the only
coverage was validate_claims/3 in isolation): a genuinely RS256-signed
token from a single- and multi-key JWKS set both validate; alg:none and
a token signed by a key not in the JWKS are both rejected. Verified
non-vacuous by reverting to jose_jwt:verify/2 and confirming 3 of the 4
new tests crash with the exact function_clause this fix closes, then
restored.
Test plan:
- rebar3 fmt --check, xref, dialyzer - clean
- rebar3 eunit - 17 tests, 0 failures
- rebar3 ct - 12 tests, 0 failures
…able
Security review of the previous commit (the key-set signature-verify
fix) found 0 High findings against the crypto path itself - alg
confusion, kid steering, alg:none, and wrong-key attacks all correctly
fail closed. But the fix also makes validate_claims/3 reachable for the
first time in practice (every real token used to crash before reaching
it), and the review found three real gaps there:
- No azp (authorized party) check. OIDC Core 3.1.3.7: on an IdP shared
with other registered clients, a token minted for a DIFFERENT client
that also lists our client_id in a multi-value aud would pass the
existing audience check - a confused-deputy risk. Added
validate_azp/2: when azp is present it must equal our client_id;
absent is fine (not every IdP sets it for single-audience tokens).
- exp was compared with a bare `Exp =< Now`, no type guard. Erlang term
order puts binaries/lists above every number, so a non-numeric exp
(e.g. a string) would have been treated as "not expired". Now
requires is_number/1, with `invalid_exp` for anything else.
- No sub validation. nova_auth:actor()'s `id` field is mandatory, and
build_actor/3's default claims mapping reads it straight from `sub`
with no fallback - a signature-valid token with correct exp/iss/aud
but no sub produced an actor with no identity at all, silently
violating validate_token/3's own declared spec. Added
validate_subject/3, checked right after exp.
Also normalized the error contract at the signature-verification
boundary: oidcc_jwt_util:verify_signature/3's error terms are its own
internal vocabulary, and two variants ({none_alg_used, Jwt, Jws}) carry
decoded token/JWS records - i.e. attacker-supplied claims - that a
consumer logging Reason verbatim would write straight into logs. Every
failure now normalizes to {error, invalid_signature} (this module's
existing public contract, unchanged from before the previous commit),
with only a classified atom logged at debug level.
Test hardening prompted by the same review:
- The two existing negative signature tests (alg:none, wrong key)
asserted only {error, _} - weak enough to pass even if the meck
expectation silently failed to apply and the real (unstarted) worker
path took over instead. Now assert the concrete {error,
invalid_signature}.
- Dropped `non_strict` from meck:new(oidcc_provider_configuration_worker,
...) - this suite's whole job is pinning the contract against the
REAL module, and non_strict would let a renamed/removed get_jwks/1
pass silently instead of failing the mock setup.
- Added an alg-confusion regression test (RSA public key replayed as an
HS256 secret) - verified it does NOT turn red if HS256 is added back
to ?ALLOWED_ALGORITHMS (jose's own key-type matching independently
rejects it too), so the test comment states what it actually pins
rather than overclaiming the allowlist is the sole defense.
- Added an integration test proving validate_token/3 rejects a
cryptographically-valid-but-expired token, not just validate_claims/3
in isolation.
- New claims tests for azp match/absent/mismatch, missing/empty sub,
non-numeric exp.
Every new claims-validation branch verified non-vacuous the same way as
the previous commit: reverted it, confirmed the corresponding test (or,
for the azp check, a compile-time "function is unused" error once its
only call sites were removed) fails, then restored.
signature_error_class/1's catch-all clause is dialyzer-provably dead
against oidcc_jwt_util's current error() type, but deliberately kept
(with a documented -dialyzer nowarn_function) - this classifier has no
try/catch of its own to fall back on inside this module, so removing
it would mean a future oidcc error variant crashes validate_token/3
uncaught, which is the exact bug class this whole fix exists to close.
Test plan:
- rebar3 fmt --check, xref, dialyzer - clean
- rebar3 eunit - 25 tests, 0 failures
- rebar3 ct - 12 tests, 0 failures
|
Security review came back with 0 High findings on the core fix - alg confusion, kid steering, alg:none, and wrong-key attacks all correctly fail closed, verified with real RSA/EC keys and signatures. Fixed the 3 Medium findings in 6c0a59a, all in claims validation that this fix makes reachable for the first time (every real token used to crash before reaching it):
Test hardening: the two existing negative tests now assert the concrete error instead of a weak Follow-ups filed, not blocking: #11 (algorithm allowlist missing PS*/EdDSA, no per-consumer override), #12 (consider migrating to the public Toolchain still clean: fmt/xref/dialyzer, eunit 25/25, ct 12/12. |
Summary
Closes #9.
validate_token/3's JWKS lookup always returns a#jose_jwk{keys = {jose_jwk_set, _}}(a provider can rotate keys, so it's always a set, even with one key).jose_jwt:verify/2has no clause matching a key set, so it raised an uncaughtfunction_clauseon every genuinely-signed token. This had zero test coverage of its actual signature-verification path (onlyvalidate_claims/3was tested in isolation), so it shipped broken.Replaced with
oidcc_jwt_util:verify_signature/3(already in theoidccdependency), which:kidwhen the token's header carries onealg:noneexplicitlyTest plan
rebar3 fmt --check,xref,dialyzer- cleanrebar3 eunit- 17 tests, 0 failures. New: end-to-end signature verification (previously untested at any layer) - a genuinely RS256-signed token from a single- and multi-key JWKS set both validate;alg:noneand a token signed by a key not in the JWKS are both rejected. Verified non-vacuous by reverting tojose_jwt:verify/2and confirming 3 of the 4 new tests crash with the exactfunction_clausethis fix closes, then restored.rebar3 ct- 12 tests, 0 failures