diff --git a/src/nova_auth_oidc_jwt.erl b/src/nova_auth_oidc_jwt.erl index f3fcc1b..066c0d5 100644 --- a/src/nova_auth_oidc_jwt.erl +++ b/src/nova_auth_oidc_jwt.erl @@ -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. @@ -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 -> @@ -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 -> @@ -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, @@ -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}, diff --git a/test/nova_auth_oidc_jwt_tests.erl b/test/nova_auth_oidc_jwt_tests.erl index 93c3b43..d13daa4 100644 --- a/test/nova_auth_oidc_jwt_tests.erl +++ b/test/nova_auth_oidc_jwt_tests.erl @@ -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"). @@ -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"}) @@ -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). @@ -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))), + <
>. + +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()}.