Skip to content

fix: use oidcc_jwt_util:verify_signature/3, not jose_jwt:verify/2 directly #10

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.

Sign up for GitHub

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

Merged
Taure merged 2 commits into from
Aug 1, 2026
Merged

fix: use oidcc_jwt_util:verify_signature/3, not jose_jwt:verify/2 directly #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69df8cc
fix: use oidcc_jwt_util:verify_signature/3, not jose_jwt:verify/2 dir…
Taure Aug 1, 2026
6c0a59a
fix: close claims-validation gaps a security review found newly reach…
Taure Aug 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 108 additions & 7 deletions src/nova_auth_oidc_jwt.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,37 @@ end
""".

-include_lib("jose/include/jose_jwt.hrl").
-include_lib("kernel/include/logger.hrl").

-export([validate_bearer/2, validate_bearer/3, validate_token/3]).
%% Exported for tests: standard-claim validation (exp/iss/aud) in isolation.
-export([validate_claims/3]).

%% signature_error_class/1's catch-all is unreachable against
%% oidcc_jwt_util:verify_signature/3's CURRENT error() type, and dialyzer
%% proves it - deliberately kept anyway, since this classifier is what
%% makes an external dependency's error terms safe to log, with no
%% try/catch of its own to fall back on inside this module. See the
%% function for the full reasoning.
-dialyzer({nowarn_function, signature_error_class/1}).

%% ID tokens are always asymmetrically signed (the private key never
%% leaves the IdP) - RSA and ECDSA cover every major provider (Google,
%% Apple, Microsoft, Discord all sign with RS256). HMAC variants are
%% deliberately excluded: if this list included an HS* algorithm, a
%% raw RSA public key (which IS public, published at the provider's
%% JWKS endpoint) could be replayed as an HMAC secret to forge a token
%% ("alg confusion") - see oidcc_jwt_util's own default and the RFC
%% 8725 discussion of this class of vulnerability.
-define(ALLOWED_ALGORITHMS, [
~"RS256",
~"RS384",
~"RS512",
~"ES256",
~"ES384",
~"ES512"
]).

-doc """
Validate a JWT bearer token from the request's Authorization header.
Uses the first configured provider for validation.
Expand Down Expand Up @@ -63,8 +89,16 @@ validate_token(AuthMod, Provider, Token) ->
WorkerName = nova_auth_oidc:provider_worker_name(AuthMod, Provider),
case get_jwks(WorkerName) of
{ok, Jwks} ->
case jose_jwt:verify(Jwks, Token) of
{true, Jwt, _Jws} ->
%% 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 for a key
%% set, it only matches a single #jose_jwk{} with a concrete
%% kty, so it always raised function_clause here. oidcc's own
%% verify_signature/3 folds over the set, matches by `kid` when
%% present, and enforces the algorithm allowlist itself (also
%% closing off alg:none and cross-algorithm confusion).
case oidcc_jwt_util:verify_signature(Token, ?ALLOWED_ALGORITHMS, Jwks) of
{ok, {Jwt, _Jws}} ->
Claims = Jwt#jose_jwt.fields,
case validate_claims(AuthMod, Provider, Claims) of
ok ->
Expand All @@ -73,7 +107,22 @@ validate_token(AuthMod, Provider, Token) ->
{error, _} = Err ->
Err
end;
{false, _, _} ->
{error, Reason} ->
%% oidcc_jwt_util's error terms are its own internal
%% vocabulary (no_matching_key, {no_matching_key_with_kid,
%% Kid}, {none_alg_used, Jwt, Jws} - the latter two
%% carrying decoded token/JWS records, i.e. attacker-
%% supplied claims, straight into a Reason a consumer
%% might log verbatim). Normalize at this boundary so
%% this module's own public error contract
%% (invalid_signature, unchanged since before this fix)
%% doesn't silently change shape whenever oidcc's
%% internal vocabulary does, and so nothing attacker-
%% controlled crosses it.
?LOG_DEBUG(#{
msg => ~"jwt signature verification failed",
reason_class => signature_error_class(Reason)
}),
{error, invalid_signature}
end;
{error, _} = Err ->
Expand All @@ -91,15 +140,53 @@ get_jwks(WorkerName) ->
_:_ -> {error, provider_not_available}
end.

%% Classify oidcc_jwt_util:verify_signature/3's error term into a bare
%% atom for logging - two of its variants ({none_alg_used, Jwt, Jws})
%% carry decoded token/JWS records, i.e. attacker-supplied claims;
%% logging Reason verbatim would put those in the log.
signature_error_class({no_matching_key_with_kid, _Kid}) -> no_matching_key_with_kid;
signature_error_class({none_alg_used, _Jwt, _Jws}) -> none_alg_used;
signature_error_class(Reason) when is_atom(Reason) -> Reason;
%% Dialyzer proves this is unreachable against oidcc_jwt_util's CURRENT
%% error() type - deliberately kept anyway. validate_token/3 has no
%% try/catch of its own in this module (unlike a caller such as
%% asobi_oauth_controller, which wraps the whole call), so if a future
%% oidcc version added an error variant this function doesn't recognize,
%% removing this clause would mean THIS classifier - the thing meant to
%% make an external dependency's behavior safe to log - is what crashes
%% validate_token/3 uncaught. That is the exact bug class this whole fix
%% exists to close, one level deeper. See the -dialyzer attribute at the
%% top of this module.
signature_error_class(_Other) -> unknown.

validate_claims(AuthMod, Provider, Claims) ->
Now = erlang:system_time(second),
case maps:get(~"exp", Claims, undefined) of
Exp when is_number(Exp), Exp > Now ->
validate_subject(AuthMod, Provider, Claims);
undefined ->
{error, missing_exp};
Exp when Exp =< Now ->
Exp when is_number(Exp) ->
{error, token_expired};
_ ->
validate_issuer(AuthMod, Provider, Claims)
%% A non-numeric exp (a string, a list, ...) sorts above every
%% number in Erlang term order, so `Exp =< Now` would have
%% treated it as "not expired" - reject the shape outright
%% rather than silently accept a claim that isn't a timestamp.
{error, invalid_exp}
end.

%% sub identifies the end user. nova_auth:actor()'s `id` field is
%% mandatory, and build_actor/3's default claims mapping reads it
%% straight from this claim with no fallback - a token without one would
%% otherwise produce an actor with no identity at all, rather than
%% failing validation up front.
validate_subject(AuthMod, Provider, Claims) ->
case maps:get(~"sub", Claims, undefined) of
Sub when is_binary(Sub), Sub =/= ~"" ->
validate_issuer(AuthMod, Provider, Claims);
_ ->
{error, missing_sub}
end.

%% The token's `iss` must match the provider's configured issuer. Without this,
Expand All @@ -116,16 +203,30 @@ validate_audience(AuthMod, Provider, Claims) ->
Aud = maps:get(~"aud", Claims, undefined),
case Aud of
ExpectedAud ->
ok;
validate_azp(ExpectedAud, Claims);
AudList when is_list(AudList) ->
case lists:member(ExpectedAud, AudList) of
true -> ok;
true -> validate_azp(ExpectedAud, Claims);
false -> {error, invalid_audience}
end;
_ ->
{error, invalid_audience}
end.

%% OIDC Core 3.1.3.7: when a token carries azp (authorized party), it
%% must equal our client_id. Without this, on an IdP shared with other
%% registered clients, a token minted for a DIFFERENT client that also
%% happens to list our client_id in `aud` would pass the audience check
%% above - a confused-deputy risk. A token with no azp at all has
%% nothing to check here (not every IdP sets it for single-audience
%% tokens), so this only rejects a token that names a different party.
validate_azp(ExpectedAud, Claims) ->
case maps:get(~"azp", Claims, undefined) of
undefined -> ok;
ExpectedAud -> ok;
_ -> {error, invalid_azp}
end.

build_actor(AuthMod, Provider, Claims) ->
Mapping = nova_auth_oidc:config(AuthMod, claims_mapping),
Base = #{provider => Provider},
Expand Down
181 changes: 178 additions & 3 deletions test/nova_auth_oidc_jwt_tests.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
-module(nova_auth_oidc_jwt_tests).
-include_lib("eunit/include/eunit.hrl").

%% Standard-claim validation: exp -> iss -> aud. Provider config comes from
%% test_oidc_config (authentik: issuer https://auth.example.com/application/o/myapp,
%% client_id test-client-id).
%% Standard-claim validation: exp -> sub -> iss -> aud -> azp. Provider
%% config comes from test_oidc_config (authentik: issuer
%% https://auth.example.com/application/o/myapp, client_id test-client-id).

-define(ISS, ~"https://auth.example.com/application/o/myapp").
-define(AUD, ~"test-client-id").
Expand All @@ -17,6 +17,21 @@ expired_rejected_test() ->
missing_exp_rejected_test() ->
?assertEqual({error, missing_exp}, validate(maps:remove(~"exp", base()))).

%% Erlang term order puts binaries/lists above every number, so a naive
%% `Exp =< Now` would treat a non-numeric exp as "not expired" - this
%% must be its own rejection, not silently accepted.
non_numeric_exp_rejected_test() ->
?assertEqual({error, invalid_exp}, validate((base())#{~"exp" => ~"not-a-number"})).

missing_sub_rejected_test() ->
%% nova_auth:actor()'s `id` is mandatory and comes straight from `sub`
%% with no fallback - a token without one must fail validation, not
%% produce an actor with no identity.
?assertEqual({error, missing_sub}, validate(maps:remove(~"sub", base()))).

empty_sub_rejected_test() ->
?assertEqual({error, missing_sub}, validate((base())#{~"sub" => ~""})).

wrong_issuer_rejected_test() ->
?assertEqual(
{error, invalid_issuer}, validate((base())#{~"iss" => ~"https://evil.example.com"})
Expand All @@ -31,6 +46,21 @@ wrong_audience_rejected_test() ->
audience_list_pass_test() ->
?assertEqual(ok, validate((base())#{~"aud" => [~"other", ?AUD]})).

%% OIDC Core 3.1.3.7: azp, when present, must equal our client_id - aud
%% alone isn't proof the token was issued for us on a shared IdP where
%% another client could list us as an additional audience.
azp_matching_client_id_accepted_test() ->
?assertEqual(ok, validate((base())#{~"azp" => ?AUD})).

azp_absent_accepted_test() ->
?assertEqual(ok, validate(base())).

azp_mismatch_rejected_test() ->
?assertEqual(
{error, invalid_azp},
validate((base())#{~"aud" => [~"other-client", ?AUD], ~"azp" => ~"other-client"})
).

validate(Claims) ->
nova_auth_oidc_jwt:validate_claims(test_oidc_config, authentik, Claims).

Expand All @@ -39,3 +69,148 @@ base() ->

future() -> erlang:system_time(second) + 3600.
past() -> erlang:system_time(second) - 3600.

%% End-to-end signature verification: validate_token/3's real
%% get_jwks -> verify_signature -> validate_claims -> build_actor path,
%% against genuinely-signed tokens. Previously untested at any layer -
%% the only coverage was validate_claims/3 in isolation above, which is
%% exactly why jose_jwt:verify/2 shipped with a function_clause crash on
%% any real, correctly-signed token: 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 a single key), and jose_jwt:verify/2 has
%% no clause matching a set - only oidcc_jwt_util:verify_signature/3
%% (what validate_token/3 now calls) folds over one.

setup() ->
%% No non_strict: this suite's entire job is pinning the contract
%% against the REAL oidcc_provider_configuration_worker module - the
%% bug being fixed was a dependency API mismatch, and non_strict
%% would let a renamed/removed get_jwks/1 pass silently instead of
%% failing the meck:expect below.
meck:new(oidcc_provider_configuration_worker, [passthrough]),
ok.

cleanup(_) ->
meck:unload(oidcc_provider_configuration_worker).

validate_token_test_() ->
{foreach, fun setup/0, fun cleanup/1, [
{"a genuinely RS256-signed token from a single-key JWKS set validates",
fun signed_token_validates/0},
{"a genuinely signed token from a multi-key JWKS set still validates",
fun signed_token_from_multi_key_set_validates/0},
{"an alg:none token is rejected, not treated as valid", fun alg_none_rejected/0},
{"a token signed by a key that isn't in the JWKS is rejected",
fun wrong_key_signature_rejected/0},
{"an RSA public key replayed as an HMAC secret (alg confusion) is rejected",
fun hmac_alg_confusion_rejected/0},
{"a signature-valid token with an expired exp is still rejected by validate_token/3",
fun signature_valid_but_expired_claims_rejected/0}
]}.

signed_token_validates() ->
Priv = rsa_key(),
mock_jwks(jwks_of([Priv])),
Token = sign_rs256(Priv, claims()),
?assertMatch(
{ok, #{id := ~"user-1"}},
nova_auth_oidc_jwt:validate_token(test_oidc_config, authentik, Token)
).

signed_token_from_multi_key_set_validates() ->
Priv1 = rsa_key(),
Priv2 = rsa_key(),
mock_jwks(jwks_of([Priv1, Priv2])),
Token = sign_rs256(Priv1, claims()),
?assertMatch(
{ok, _},
nova_auth_oidc_jwt:validate_token(test_oidc_config, authentik, Token)
).

alg_none_rejected() ->
Priv = rsa_key(),
mock_jwks(jwks_of([Priv])),
Token = none_alg_token(claims()),
?assertEqual(
{error, invalid_signature},
nova_auth_oidc_jwt:validate_token(test_oidc_config, authentik, Token)
).

wrong_key_signature_rejected() ->
Priv = rsa_key(),
OtherPriv = rsa_key(),
mock_jwks(jwks_of([Priv])),
Token = sign_rs256(OtherPriv, claims()),
?assertEqual(
{error, invalid_signature},
nova_auth_oidc_jwt:validate_token(test_oidc_config, authentik, Token)
).

%% ?ALLOWED_ALGORITHMS excludes HS* precisely to close this off: an RSA
%% public key IS public (published at the provider's JWKS endpoint), so
%% if HMAC were allowed, an attacker could sign their own token with it
%% as the HMAC secret and have it accepted as if the IdP had signed it.
%% This attempt is rejected regardless (jose's own key-type matching
%% also refuses to treat an RSA-family JWK as HMAC-compatible), so this
%% pins the end-to-end property - a forged HS256 token using the public
%% key is never accepted - rather than isolating the allowlist as the
%% sole cause; adding ~"HS256" to ?ALLOWED_ALGORITHMS does not turn this
%% specific test red, confirmed by trying it.
hmac_alg_confusion_rejected() ->
Priv = rsa_key(),
mock_jwks(jwks_of([Priv])),
PubPem = element(2, jose_jwk:to_pem(jose_jwk:to_public(Priv))),
HmacJwk = jose_jwk:from_oct(PubPem),
Jwt = jose_jwt:from(claims()),
{_, Signed} = jose_jwt:sign(HmacJwk, #{~"alg" => ~"HS256"}, Jwt),
{_, Token} = jose_jws:compact(Signed),
?assertEqual(
{error, invalid_signature},
nova_auth_oidc_jwt:validate_token(test_oidc_config, authentik, Token)
).

%% validate_claims/3 above is tested in isolation - this proves the two
%% stages are actually wired together through validate_token/3: a
%% cryptographically valid signature must not short-circuit claims
%% validation.
signature_valid_but_expired_claims_rejected() ->
Priv = rsa_key(),
mock_jwks(jwks_of([Priv])),
Token = sign_rs256(Priv, (claims())#{~"exp" => past()}),
?assertEqual(
{error, token_expired},
nova_auth_oidc_jwt:validate_token(test_oidc_config, authentik, Token)
).

%% ---- signature-test helpers ----

mock_jwks(Jwks) ->
meck:expect(oidcc_provider_configuration_worker, get_jwks, fun(_) -> Jwks end).

rsa_key() ->
jose_jwk:generate_key({rsa, 2048}).

jwks_of(PrivKeys) ->
PubMaps = [element(2, jose_jwk:to_map(jose_jwk:to_public(K))) || K <- PrivKeys],
jose_jwk:from_map(#{~"keys" => PubMaps}).

sign_rs256(PrivKey, Claims) ->
Jwt = jose_jwt:from(Claims),
{_, Signed} = jose_jwt:sign(PrivKey, #{~"alg" => ~"RS256"}, Jwt),
{_, Compact} = jose_jws:compact(Signed),
Compact.

%% jose refuses to sign an alg:none token itself (jose_jwa:unsecured_signing()
%% gates it, off by default) - build the raw compact form directly, matching
%% how an attacker actually constructs this forgery.
none_alg_token(Claims) ->
Header = b64url(iolist_to_binary(json:encode(#{~"alg" => ~"none", ~"typ" => ~"JWT"}))),
Payload = b64url(iolist_to_binary(json:encode(Claims))),
<<Header/binary, ".", Payload/binary, ".">>.

b64url(Bin) ->
NoPad = binary:replace(base64:encode(Bin), <<"=">>, <<"">>, [global]),
binary:replace(binary:replace(NoPad, <<"+">>, <<"-">>, [global]), <<"/">>, <<"_">>, [global]).

claims() ->
#{~"iss" => ?ISS, ~"aud" => ?AUD, ~"sub" => ~"user-1", ~"exp" => future()}.
Loading