Fix HS256 Key Checking (#241)

erlef · Sep 19, 2023 · aeb872e · aeb872e
1 parent f042424
commit aeb872e
Show file tree

Hide file tree

Showing 6 changed files with 167 additions and 3 deletions.
diff --git a/priv/test/fixtures/example-metadata.json b/priv/test/fixtures/example-metadata.json
@@ -18,7 +18,7 @@
   "none"
  ],
  "subject_types_supported": ["public"],
- "id_token_signing_alg_values_supported": ["none", "RS256"],
+ "id_token_signing_alg_values_supported": ["none", "HS256", "RS256"],
  "scopes_supported": ["openid", "email", "profile"],
  "token_endpoint_auth_methods_supported": [
   "client_secret_post",

diff --git a/priv/test/fixtures/openid-certification-jwks.json b/priv/test/fixtures/openid-certification-jwks.json
@@ -0,0 +1,66 @@
+{
+  "keys": [
+    {
+      "kty": "RSA",
+      "e": "AQAB",
+      "use": "sig",
+      "kid": "39c8b673-8f3f-4588-a0b6-752f63d33b74",
+      "n": "n2ROFb13OU-f1dQXbPsfDTk1r55f52_3H-nOzGtzXiLagDkMCPp1JDu-CLaw-mS5jwZIeYfKAKaHZyeXHevJ68suodmQmY3IlUVHWfbWABZD1vo8cRXyK7EtqSCVDnWvDAx9A1QKe8H7wD86Q6MoTAP3BD4cLEEkJfq0WcLp-jnpRXdLzMTT-STn5r1NmGTJh5klAR80WFo1d14ihdE9LQXv2dovhl5WgZB08rN2WgKyhDoeHbgaGF5d-YHDS_1WmrYq8ZybHjSninPR-dpy3FqJU5aOZxKUEdSj31Iwf7FlPgUoK7Q7XkjYHdD-8I7UShKgmr-XCO7FkA4AiZBcTQ"
+    },
+    {
+      "kty": "RSA",
+      "e": "AQAB",
+      "use": "sig",
+      "kid": "09c882ae-aa63-49cb-82ba-80279e2e253c",
+      "n": "iasSpBEtH4yiig4r-oZ9D9keINBloBYyUqDdzZYAl1xiPmDr_hpmPjGIJCsSCF7rx-GF-iDn7y_ymbPClpJACRzVOKYKXRlcAg6N5mqoguZumNZyEKSBk7SNXq_wtPz02P3NtKYZI2Aa04qyBRQGCxZlPzK9jnFlyaku3f1bd28xLPU3UEv7Fs11HG8-Mk6SR_mipRjkafsV4ntwrk19XuPbL7VfbKbaPUreW3yYQRtKvWm5YgfZzr2Jiu6UNJSoZcmxZAXL-xpK_hu05ySJj8LVpLzxYoBK5q0R5jXCF4swL186PvoXVToAnFFyiNs-wMsiIuefJA3LHOtndYWF5Q"
+    },
+    {
+      "kty": "EC",
+      "use": "sig",
+      "crv": "P-256",
+      "kid": "5c0b9041-8f48-4e09-b520-e12414433d43",
+      "x": "_xZGFpNUQBjA80JULxRhEc3Ei3dRPHnVW8LAW-6T-Vk",
+      "y": "S8C5Yt9mYDEZWUw5Xa53idBNIREaCIBxshLpVINQbt8"
+    },
+    {
+      "kty": "EC",
+      "use": "sig",
+      "crv": "P-256",
+      "kid": "8cafed9d-88a7-4e7d-8cd2-88090c7d90e1",
+      "x": "sXUy3AF9HQkaf2W3EstcO-y7mVlnIi-wDtF557IREt0",
+      "y": "_uZrRLS2Hbh18orB5j3GHXY9Mw8ddgS0ML9dKY5zD3U"
+    },
+    {
+      "kty": "EC",
+      "use": "sig",
+      "crv": "secp256k1",
+      "kid": "8090008e-6361-4a43-87a0-b5e587452bf5",
+      "x": "mJR1JPZyAzF6PMJ-8Lz0busmuT0GM4PbPX9e28NzKFQ",
+      "y": "qPjD74719cdg2CrBjTh38PHFhvYT01L43IVEuNhHhd8"
+    },
+    {
+      "kty": "OKP",
+      "use": "sig",
+      "crv": "Ed25519",
+      "kid": "0e596a8c-7996-4d0c-9570-3c9b0b24cfce",
+      "x": "MJjZicUI5ikHR4U6JicUbfLdiSjwOmbYAJ8rjq5KzvE"
+    },
+    {
+      "kty": "RSA",
+      "e": "AQAB",
+      "use": "enc",
+      "kid": "ff82bc1a-249d-4beb-9544-1e133b566480",
+      "alg": "RSA-OAEP",
+      "n": "wsRfFIQKLHxNRvkJk6DdQDnf5PKvN_DoAOVYWIar945ZIOETe8HfNx40Wg3ZJLw_KhdUhmaDq78Qcoby_Jilc5K9Teru_es4uUvu4tvdQ_l77WPrHtxPgd5J983VvD0a-jy3B74Srh1ZGbYJn5kG9n3ichELjTcegc7Xagx5tGwXevyqNMoVI8pwUq2kGlYUMWtw4pu5NiUupbkAZJTif347NzNC2mzN-BOGboHfOBg3EXgaSlQTCSpUzHSYta4q1-yHERP0hwIf_qPsJbmzC9eJjwq_NyhgxPM3uwIC9xa6L10LlZmTDwcimq-gRmQPSs4ekEhdQwjgrM3-6vqXlQ"
+    },
+    {
+      "kty": "EC",
+      "use": "enc",
+      "crv": "P-256",
+      "kid": "ac0b707f-702f-49c9-8a08-55cff93864f0",
+      "x": "ok6k2pvgo7NKAPEtY7p6NQSYSVPGdFKWAJGh46BMZhA",
+      "y": "wJDRuaN7yzfeIHi90KuTfhochiMBHKU6DpYQ5G2BgIE",
+      "alg": "ECDH-ES"
+    }
+  ]
+}
diff --git a/src/oidcc_jwt_util.erl b/src/oidcc_jwt_util.erl
@@ -9,6 +9,7 @@
 -include_lib("jose/include/jose_jwt.hrl").
 
 -export([client_secret_oct_keys/2]).
+-export([merge_jwks/2]).
 -export([refresh_jwks_fun/1]).
 -export([verify_claims/2]).
 -export([verify_signature/3]).
@@ -84,6 +85,9 @@ verify_signature(Token, AllowAlgorithms, #jose_jwk{} = Jwks) ->
         end
     catch
         error:{badarg, [_Token]} ->
+            {error, invalid_jwt_token};
+        %% Some Keys crash if a non matching alg is provided
+        error:function_clause ->
             {error, invalid_jwt_token}
     end.
 
@@ -138,3 +142,16 @@ refresh_jwks_fun(ProviderConfigurationWorkerName) ->
         ),
         {ok, oidcc_provider_configuration_worker:get_jwks(ProviderConfigurationWorkerName)}
     end.
+
+%% @private
+-spec merge_jwks(Left :: jose_jwk:key(), Right :: jose_jwk:key()) -> jose_jwk:key().
+merge_jwks(#jose_jwk{keys = {jose_jwk_set, LeftKeys}, fields = LeftFields}, #jose_jwk{
+    keys = {jose_jwk_set, RightKeys}, fields = RightFields
+}) ->
+    #jose_jwk{
+        keys = {jose_jwk_set, LeftKeys ++ RightKeys}, fields = maps:merge(LeftFields, RightFields)
+    };
+merge_jwks(#jose_jwk{} = Left, #jose_jwk{keys = {jose_jwk_set, _RightKeys}} = Right) ->
+    merge_jwks(#jose_jwk{keys = {jose_jwk_set, [Left]}}, Right);
+merge_jwks(Left, Right) ->
+    merge_jwks(Left, #jose_jwk{keys = {jose_jwk_set, [Right]}}).
diff --git a/src/oidcc_token.erl b/src/oidcc_token.erl
@@ -675,7 +675,7 @@ validate_id_token(IdToken, ClientContext, Nonce) ->
                 none ->
                     Jwks;
                 OctJwk ->
-                    jose_jwk:merge(OctJwk, Jwks)
+                    oidcc_jwt_util:merge_jwks(Jwks, OctJwk)
             end,
         {ok, {#jose_jwt{fields = Claims}, Jws}} ?=
             oidcc_jwt_util:verify_signature(IdToken, AllowAlgorithms, JwksInclOct),

diff --git a/src/oidcc_userinfo.erl b/src/oidcc_userinfo.erl
@@ -199,7 +199,7 @@ validate_userinfo_token(UserinfoToken, ClientContext, Opts) ->
                 none ->
                     Jwks;
                 OctJwk ->
-                    jose_jwk:merge(OctJwk, Jwks)
+                    oidcc_jwt_util:merge_jwks(Jwks, OctJwk)
             end,
         {ok, {#jose_jwt{fields = Claims}, _Jws}} ?=
             oidcc_jwt_util:verify_signature(UserinfoToken, AllowAlgorithms, JwksInclOct),

diff --git a/test/oidcc_token_test.erl b/test/oidcc_token_test.erl
@@ -213,3 +213,84 @@ retrieve_rs256_with_rotation_test() ->
     meck:unload(httpc),
 
     ok.
+
+retrieve_hs256_test() ->
+    PrivDir = code:priv_dir(oidcc),
+
+    {ok, _} = application:ensure_all_started(oidcc),
+
+    {ok, ConfigurationBinary} = file:read_file(PrivDir ++ "/test/fixtures/example-metadata.json"),
+    {ok,
+        #oidcc_provider_configuration{token_endpoint = TokenEndpoint, issuer = Issuer} =
+            Configuration} =
+        oidcc_provider_configuration:decode_configuration(jose:decode(ConfigurationBinary)),
+
+    ClientId = <<"client_id">>,
+    ClientSecret = <<"at_least_32_character_client_secret">>,
+    LocalEndpoint = <<"https://my.server/auth">>,
+    AuthCode = <<"1234567890">>,
+    AccessToken = <<"access_token">>,
+    RefreshToken = <<"refresh_token">>,
+    Claims =
+        #{
+            <<"iss">> => Issuer,
+            <<"sub">> => <<"sub">>,
+            <<"aud">> => ClientId,
+            <<"iat">> => erlang:system_time(second),
+            <<"exp">> => erlang:system_time(second) + 10,
+            <<"at_hash">> => <<"hrOQHuo3oE6FR82RIiX1SA">>
+        },
+
+    Jwk = jose_jwk:from_oct(<<"at_least_32_character_client_secret">>),
+
+    Jwt = jose_jwt:from(Claims),
+    Jws = #{<<"alg">> => <<"HS256">>},
+    {_Jws, Token} = jose_jws:compact(jose_jwt:sign(Jwk, Jws, Jwt)),
+
+    OtherJwk = jose_jwk:from_file(PrivDir ++ "/test/fixtures/openid-certification-jwks.json"),
+
+    TokenData =
+        jsx:encode(#{
+            <<"access_token">> => AccessToken,
+            <<"token_type">> => <<"Bearer">>,
+            <<"id_token">> => Token,
+            <<"scope">> => <<"profile openid">>,
+            <<"refresh_token">> => RefreshToken
+        }),
+
+    ClientContext = oidcc_client_context:from_manual(
+        Configuration, OtherJwk, ClientId, ClientSecret
+    ),
+
+    ok = meck:new(httpc, [no_link]),
+    HttpFun =
+        fun(
+            post,
+            {ReqTokenEndpoint, _Header, "application/x-www-form-urlencoded", _Body},
+            _HttpOpts,
+            _Opts
+        ) ->
+            TokenEndpoint = ReqTokenEndpoint,
+            {ok, {{"HTTP/1.1", 200, "OK"}, [{"content-type", "application/json"}], TokenData}}
+        end,
+    ok = meck:expect(httpc, request, HttpFun),
+
+    ?assertMatch(
+        {ok, #oidcc_token{
+            id = #oidcc_token_id{token = Token, claims = Claims},
+            access = #oidcc_token_access{token = AccessToken},
+            refresh = #oidcc_token_refresh{token = RefreshToken},
+            scope = [<<"profile">>, <<"openid">>]
+        }},
+        oidcc_token:retrieve(
+            AuthCode,
+            ClientContext,
+            #{redirect_uri => LocalEndpoint}
+        )
+    ),
+
+    true = meck:validate(httpc),
+
+    meck:unload(httpc),
+
+    ok.