fix: Handle failing pubkey refresh (#15)

Author: moritzploss-k

.github/workflows/erlang.yml

@@ -11,13 +11,14 @@ jobs:
     name: OTP ${{matrix.otp}}
     strategy:
       matrix:
-        otp: ["23.2", "24.0"]
+        otp: ["24.0", "25.0"]
 
     steps:
     - uses: actions/[email protected]
-    - uses: gleam-lang/setup-erlang@v1.1.2
+    - uses: erlef/setup-beam@v1.16.0
       with:
-        otp-version: ${{matrix.otp}}
+        otp-version: ${{ matrix.otp }}
+        rebar3-version: '3.16.1'
     - name: Compile
       run: make
     - name: Run xref

elvis.config

@@ -8,7 +8,7 @@
           ignore => [],
           ruleset => [{elvis_style, state_record_and_type, disable}],
           rules => [ {elvis_text_style, line_length,
-                      #{ limit => 80,
+                      #{ limit => 100,
                          skip_comments => false
                        }}
                    , {elvis_text_style, no_tabs}
@@ -57,7 +57,7 @@
                   ],
           filter => "*.erl",
           rules => [ {elvis_text_style, line_length,
-                      #{ limit => 80,
+                      #{ limit => 100,
                          skip_comments => false
                        }}
                    , {elvis_text_style, no_tabs}

rebar.config

@@ -1,7 +1,7 @@
 {erl_opts, [debug_info]}.
-{deps, [ {jsx, {git, "https://github.com/talentdeficit/jsx.git"  , {tag, "v2.11.0"}}}
-       , {jose, {git, "https://github.com/potatosalad/erlang-jose.git" , {tag, "1.11.1"}}}
-       , {hackney, {git, "https://github.com/benoitc/hackney.git"  , {tag, "1.17.0"}}}
+{deps, [ {jsx, {git, "https://github.com/talentdeficit/jsx.git"  , {tag, "v3.1.0"}}}
+       , {jose, {git, "https://github.com/potatosalad/erlang-jose.git" , {tag, "1.11.5"}}}
+       , {hackney, {git, "https://github.com/benoitc/hackney.git"  , {tag, "1.18.0"}}}
        ]}.
 
 {shell, [{config, "config/sys.config"},

rebar.lock

@@ -1,41 +1,38 @@
 {"1.2.0",
-[{<<"base64url">>,{pkg,<<"base64url">>,<<"0.0.1">>},1},
- {<<"certifi">>,{pkg,<<"certifi">>,<<"2.5.3">>},1},
+[{<<"certifi">>,{pkg,<<"certifi">>,<<"2.8.0">>},1},
  {<<"hackney">>,
   {git,"https://github.com/benoitc/hackney.git",
-       {ref,"f642ee0eccaff9aa15707ae3a3effa29f2920e41"}},
+       {ref,"c8f3a6c5a837e4554be219f7c21ae8fdb3d01c40"}},
   0},
  {<<"idna">>,{pkg,<<"idna">>,<<"6.1.1">>},1},
  {<<"jose">>,
   {git,"https://github.com/potatosalad/erlang-jose.git",
-       {ref,"090a2ed054304ecc012d6c2d9d10d2a294d835b1"}},
+       {ref,"e0110a1afacde3011c8bb2a6f1c4ab9ea94bd4f4"}},
   0},
  {<<"jsx">>,
   {git,"https://github.com/talentdeficit/jsx.git",
-       {ref,"e8d2e01b608e0670a4f82e35ccb5ef3f86115423"}},
+       {ref,"bb9b3e570a7efe331eed0900c3a5188043a850d7"}},
   0},
  {<<"metrics">>,{pkg,<<"metrics">>,<<"1.0.1">>},1},
  {<<"mimerl">>,{pkg,<<"mimerl">>,<<"1.2.0">>},1},
- {<<"parse_trans">>,{pkg,<<"parse_trans">>,<<"3.3.0">>},1},
+ {<<"parse_trans">>,{pkg,<<"parse_trans">>,<<"3.3.1">>},1},
  {<<"ssl_verify_fun">>,{pkg,<<"ssl_verify_fun">>,<<"1.1.6">>},1},
  {<<"unicode_util_compat">>,{pkg,<<"unicode_util_compat">>,<<"0.7.0">>},1}]}.
 [
 {pkg_hash,[
- {<<"base64url">>, <<"36A90125F5948E3AFD7BE97662A1504B934DD5DAC78451CA6E9ABF85A10286BE">>},
- {<<"certifi">>, <<"70BDD7E7188C804F3A30EE0E7C99655BC35D8AC41C23E12325F36AB449B70651">>},
+ {<<"certifi">>, <<"D4FB0A6BB20B7C9C3643E22507E42F356AC090A1DCEA9AB99E27E0376D695EBA">>},
  {<<"idna">>, <<"8A63070E9F7D0C62EB9D9FCB360A7DE382448200FBBD1B106CC96D3D8099DF8D">>},
  {<<"metrics">>, <<"25F094DEA2CDA98213CECC3AEFF09E940299D950904393B2A29D191C346A8486">>},
  {<<"mimerl">>, <<"67E2D3F571088D5CFD3E550C383094B47159F3EEE8FFA08E64106CDF5E981BE3">>},
- {<<"parse_trans">>, <<"09765507A3C7590A784615CFD421D101AEC25098D50B89D7AA1D66646BC571C1">>},
+ {<<"parse_trans">>, <<"16328AB840CC09919BD10DAB29E431DA3AF9E9E7E7E6F0089DD5A2D2820011D8">>},
  {<<"ssl_verify_fun">>, <<"CF344F5692C82D2CD7554F5EC8FD961548D4FD09E7D22F5B62482E5AEAEBD4B0">>},
  {<<"unicode_util_compat">>, <<"BC84380C9AB48177092F43AC89E4DFA2C6D62B40B8BD132B1059ECC7232F9A78">>}]},
 {pkg_hash_ext,[
- {<<"base64url">>, <<"FAB09B20E3F5DB886725544CBCF875B8E73EC93363954EB8A1A9ED834AA8C1F9">>},
- {<<"certifi">>, <<"ED516ACB3929B101208A9D700062D520F3953DA3B6B918D866106FFA980E1C10">>},
+ {<<"certifi">>, <<"6AC7EFC1C6F8600B08D625292D4BBF584E14847CE1B6B5C44D983D273E1097EA">>},
  {<<"idna">>, <<"92376EB7894412ED19AC475E4A86F7B413C1B9FBB5BD16DCCD57934157944CEA">>},
  {<<"metrics">>, <<"69B09ADDDC4F74A40716AE54D140F93BEB0FB8978D8636EADED0C31B6F099F16">>},
  {<<"mimerl">>, <<"F278585650AA581986264638EBF698F8BB19DF297F66AD91B18910DFC6E19323">>},
- {<<"parse_trans">>, <<"17EF63ABDE837AD30680EA7F857DD9E7CED9476CDD7B0394432AF4BFC241B960">>},
+ {<<"parse_trans">>, <<"07CD9577885F56362D414E8C4C4E6BDF10D43A8767ABB92D24CBE8B24C54888B">>},
  {<<"ssl_verify_fun">>, <<"BDB0D2471F453C88FF3908E7686F86F9BE327D065CC1EC16FA4540197EA04680">>},
  {<<"unicode_util_compat">>, <<"25EEE6D67DF61960CF6A794239566599B09E17E668D3700247BC498638152521">>}]}
 ].

src/id_token.erl

@@ -16,8 +16,8 @@ validate(Provider, IdToken) ->
     true  ->
       case id_token_jws:validate(IdToken, Keys) of
         {error, no_public_key_matches} ->
-          refresh_and_validate(Provider, IdToken,
-                               #{force_refresh => true});
+          Kid = id_token_jws:extract_kid(IdToken),
+          refresh_and_validate(Provider, IdToken, #{kid => Kid});
         Result ->
           Result
       end;

src/id_token_jws.erl

@@ -3,7 +3,8 @@
 -define(API_CALLS,
         [generate_key_for/1, generate_key_for/2,
          sign/2, sign/3,
-         validate/1, validate/2]). 
+         validate/1, validate/2,
+         extract_kid/1]). 
 -ignore_xref(?API_CALLS).
 -export(?API_CALLS).
 
@@ -78,6 +79,11 @@ generate_key_for(Alg, Options) ->
   {_, PublicKeyMap} = jose_jwk:to_public_map(Jwk),
   {Jwk, PublicKeyMap}.
 
+extract_kid(IdToken) ->
+  Protected = jose_jwt:peek_protected(IdToken),
+  {_M, #{<<"kid">> := Kid}} = jose_jws:to_map(Protected),
+  Kid.
+
 %%%=============================================================================
 %%% Internal functions
 %%%=============================================================================
@@ -120,11 +126,6 @@ kid(_) -> jose_base64url:encode(crypto:strong_rand_bytes(16)).
 iat(#{iat := Iat}) -> Iat;
 iat(_) -> erlang:system_time(seconds).
 
-extract_kid(IdToken) ->
-  Protected = jose_jwt:peek_protected(IdToken),
-  {_M, #{<<"kid">> := Kid}} = jose_jws:to_map(Protected),
-  Kid.
-
 %%%_* Emacs ====================================================================
 %%% Local Variables:
 %%% allout-layout: t

src/id_token_provider.erl

@@ -2,6 +2,8 @@
 
 -behaviour(gen_server).
 
+-include_lib("kernel/include/logger.hrl").
+
 %% API
 -define(API, [start_link/0, get_cached_keys/1,
               refresh_keys/1, refresh_keys/2, add_provider/2]).
@@ -14,7 +16,7 @@
 
 -define(REVALIDATE_DELAY, 7).
 
--type refresh_keys_opts() :: #{force_refresh => boolean()}.
+-type refresh_keys_opts() :: #{ kid => binary(), force_refresh => boolean() }.
 
 %%%===================================================================
 %%% API
@@ -68,19 +70,29 @@ handle_info({refresh, Provider}, State) ->
   #{exp_at := ExpAt} = refresh(Provider),
   Now = id_token_util:now_gregorian_seconds(),
   RevalidateTime = ExpAt - ?REVALIDATE_DELAY,
-  case Now < RevalidateTime of
-    true ->
-      timer:send_after((RevalidateTime - Now) * 1000,
-                       self(), {refresh, Provider});
-    false ->
-      %% Not enough time for revalidation,
-      %% let the first request pay the price
-      ok
-  end,
+  Delay =
+    case Now < RevalidateTime of
+      true ->
+        (RevalidateTime - Now) * 1000;
+      false ->
+        %% Not enough time for revalidation, let the first request pay
+        %% the price and re-initiate async_revalidate-loop after 60 seconds
+        60_000
+    end,
+  timer:send_after(Delay, self(), {refresh, Provider}),
   {noreply, State}.
 
 maybe_refresh(Provider, #{force_refresh := true}) ->
   refresh(Provider);
+maybe_refresh(Provider, #{kid := Kid}) ->
+  %% check whether Kid has been added to the cache already while the
+  %% refresh request was queued in the gen_server inbox
+  [{Provider, CacheEntry}] = ets:lookup(?ID_TOKEN_CACHE, Provider),
+  #{keys := Keys}          = CacheEntry,
+  case lists:search(fun(#{<<"kid">> := K}) -> K =:= Kid end, Keys) of
+    {value, _} -> CacheEntry;
+    false      -> refresh(Provider)
+  end;
 maybe_refresh(Provider, _Opts) ->
   [{Provider, CacheEntry}] = ets:lookup(?ID_TOKEN_CACHE, Provider),
   #{exp_at := ExpAt, well_known_uri := WellKnownUri} = CacheEntry,
@@ -95,11 +107,19 @@ refresh(Provider) ->
   refresh(Provider, WellKnownUri).
 
 refresh(Provider, WellKnownUri) ->
-  {ok, KeysUrl} = id_token_jwks:get_jwks_uri(WellKnownUri),
-  {ok, NewKeys} = id_token_jwks:get_pub_keys(KeysUrl),
-  NewCacheEntry = NewKeys#{well_known_uri => WellKnownUri},
-  ets:insert(?ID_TOKEN_CACHE, {Provider, NewCacheEntry}),
-  NewKeys.
+  case id_token_jwks:get_jwks_uri(WellKnownUri) of
+    {ok, KeysUrl} ->
+      case id_token_jwks:get_pub_keys(KeysUrl) of
+        {ok, NewKeys} ->
+          NewCacheEntry = NewKeys#{well_known_uri => WellKnownUri},
+          ets:insert(?ID_TOKEN_CACHE, {Provider, NewCacheEntry}),
+          NewKeys;
+        {error, Reason} ->
+          handle_error("failed to refresh pubkey cache", Provider, Reason)
+      end;
+    {error, Reason} ->
+      handle_error("failed to fetch jwks uri", Provider, Reason)
+  end.
 
 %%%===================================================================
 %%% Internal functions
@@ -112,6 +132,13 @@ add_provider({Name, Uri}) ->
   true = ets:insert(?ID_TOKEN_CACHE, EtsEntry),
   ok.
 
+handle_error(Message, Provider, Reason) ->
+  ?LOG_ERROR(#{ message => Message,
+                reason => Reason,
+                provider => Provider }),
+  [{Provider, CacheEntry}] = ets:lookup(?ID_TOKEN_CACHE, Provider),
+  CacheEntry.
+
 %%%_* Emacs ============================================================
 %%% Local Variables:
 %%% allout-layout: t

test/id_token_SUITE.erl

@@ -10,7 +10,8 @@
 
 groups() -> [].
 
-all() -> [validate_jwt, keys_are_cached].
+all() -> [ validate_jwt,
+           keys_are_only_refreshed_once_per_kid ].
 
 init_per_suite(Config) ->
   application:ensure_all_started(jose),
@@ -22,9 +23,9 @@ init_per_testcase(_TestCase, Config) ->
     id_token_jws:generate_key_for(<<"RS256">>, #{key_size => 1024}),
   Claims = #{ <<"exp">> => erlang:system_time(second) + 10},
   Jwt = id_token_jws:sign(Claims, Jwk),
-  mock_id_provider(PublicKeyMap),
+  mock_id_provider(PublicKeyMap, 0),
   application:ensure_all_started(id_token),
-  [{jwt, Jwt} | Config].
+  [{jwt, Jwt}, {pubkeys, [PublicKeyMap]} | Config].
 end_per_testcase(_TestCase, Config) ->
   application:stop(id_token),
   meck:unload([id_token_jwks, hackney]),
@@ -36,19 +37,47 @@ validate_jwt(Config) ->
 
 keys_are_cached(Config) ->
   Jwt = ?config(jwt, Config),
-  id_token:validate(?ID_PROVIDER, Jwt),
-  id_token:validate(?ID_PROVIDER, Jwt),
+  ?assertMatch({ok, _}, id_token:validate(?ID_PROVIDER, Jwt)),
+  ?assertMatch({ok, _}, id_token:validate(?ID_PROVIDER, Jwt)),
   1 = meck:num_calls(id_token_jwks, get_pub_keys, 1),
   ok.
 
-mock_id_provider(PublicKeyMap) ->
-  meck:expect(id_token_jwks, get_providers, 0,
-              [{?ID_PROVIDER, ?WELL_KNOWN_URI}]),
+keys_are_only_refreshed_once_per_kid(Config) ->
+  %% init pubkey cache with config from init_per_testcase
+  CurrentKeyCache = #{ exp_at => id_token_util:now_gregorian_seconds() + 10,
+                       keys => ?config(pubkeys, Config)},
+  ok = meck:expect(id_token_provider, get_cached_keys, 1, CurrentKeyCache),
+
+  %% create JWT with kid that's not yet in the pubkey cache
+  {Jwk, NewPubkey} = id_token_jws:generate_key_for(<<"RS256">>, #{key_size => 1024}),
+  Claims = #{ <<"exp">> => erlang:system_time(second) + 10 },
+  Jwt = id_token_jws:sign(Claims, Jwk),
+
+  %% set up provider to return new pubkeys with a 50 ms delay
+  HttpReponseDelay = 50,
+  mock_id_provider(NewPubkey, HttpReponseDelay),
+  ?assertEqual(0, meck:num_calls(id_token_jwks, get_pub_keys, 1)),
+
+  %% try to validate multiple JWTs based on kid that's not yet in the key cache
+  spawn(fun() -> id_token:validate(?ID_PROVIDER, Jwt) end),
+  spawn(fun() -> id_token:validate(?ID_PROVIDER, Jwt) end),
+  spawn(fun() -> id_token:validate(?ID_PROVIDER, Jwt) end),
+  timer:sleep(10 * HttpReponseDelay),
+
+  %% ensure that the pubkey cache was only refreshed once
+  ?assertEqual(1, meck:num_calls(id_token_jwks, get_pub_keys, 1)),
+  ?assertMatch({ok, _}, id_token:validate(?ID_PROVIDER, Jwt)),
+
+  meck:unload([id_token_provider]).
+
+mock_id_provider(PublicKeyMap, HttpReponseDelay) ->
+  meck:expect(id_token_jwks, get_providers, 0, [{?ID_PROVIDER, ?WELL_KNOWN_URI}]),
   meck:expect(hackney, request,
               fun(get, ?WELL_KNOWN_URI, _, _, _) ->
                   Body = jsx:encode(#{<<"jwks_uri">> => ?JWKS_URI}),
                   {ok, 200, [], Body};
                  (get, ?JWKS_URI, _, _, _) ->
+                  timer:sleep(HttpReponseDelay),
                   Body = jsx:encode(#{<<"keys">> => [PublicKeyMap]}),
                   MaxAge = <<"max-age=3600">>,
                   Headers = [{<<"Cache-Control">>, MaxAge}],