From 72602df5111af55ebffd08c34d7b50bcc6810a15 Mon Sep 17 00:00:00 2001
From: JianBo He <heeejianbo@163.com>
Date: Thu, 3 Jun 2021 14:16:58 +0800
Subject: [PATCH] fix(svr): avoid crashed on undefined remote jwks server
 (#4916)

---
 apps/emqx_auth_jwt/src/emqx_auth_jwt.app.src  |  2 +-
 .../emqx_auth_jwt/src/emqx_auth_jwt.appup.src | 15 +++++++++++++++
 apps/emqx_auth_jwt/src/emqx_auth_jwt_svr.erl  | 12 +++++++-----
 .../test/emqx_auth_jwt_SUITE.erl              | 19 +++++++++++++++++++
 4 files changed, 42 insertions(+), 6 deletions(-)
 create mode 100644 apps/emqx_auth_jwt/src/emqx_auth_jwt.appup.src

diff --git a/apps/emqx_auth_jwt/src/emqx_auth_jwt.app.src b/apps/emqx_auth_jwt/src/emqx_auth_jwt.app.src
index bf0ba78aa..8db4ffe84 100644
--- a/apps/emqx_auth_jwt/src/emqx_auth_jwt.app.src
+++ b/apps/emqx_auth_jwt/src/emqx_auth_jwt.app.src
@@ -1,6 +1,6 @@
 {application, emqx_auth_jwt,
  [{description, "EMQ X Authentication with JWT"},
-  {vsn, "4.3.0"}, % strict semver, bump manually!
+  {vsn, "4.3.1"}, % strict semver, bump manually!
   {modules, []},
   {registered, [emqx_auth_jwt_sup]},
   {applications, [kernel,stdlib,jose]},
diff --git a/apps/emqx_auth_jwt/src/emqx_auth_jwt.appup.src b/apps/emqx_auth_jwt/src/emqx_auth_jwt.appup.src
new file mode 100644
index 000000000..b9831bb6f
--- /dev/null
+++ b/apps/emqx_auth_jwt/src/emqx_auth_jwt.appup.src
@@ -0,0 +1,15 @@
+%% -*-: erlang -*-
+{VSN,
+ [
+    {"4.3.0", [
+      {load_module, emqx_auth_jwt_svr, brutal_purge, soft_purge, []}
+    ]},
+    {<<".*">>, []}
+ ],
+ [
+    {"4.3.0", [
+      {load_module, emqx_auth_jwt_svr, brutal_purge, soft_purge, []}
+    ]},
+    {<<".*">>, []}
+ ]
+}.
diff --git a/apps/emqx_auth_jwt/src/emqx_auth_jwt_svr.erl b/apps/emqx_auth_jwt/src/emqx_auth_jwt_svr.erl
index 2d00903e3..b9d19bf57 100644
--- a/apps/emqx_auth_jwt/src/emqx_auth_jwt_svr.erl
+++ b/apps/emqx_auth_jwt/src/emqx_auth_jwt_svr.erl
@@ -140,7 +140,7 @@ handle_verify(JwsCompacted,
               State = #state{static = Static, remote = Remote}) ->
     try
         Jwks = case emqx_json:decode(jose_jws:peek_protected(JwsCompacted), [return_maps]) of
-                   #{<<"kid">> := Kid} ->
+                   #{<<"kid">> := Kid} when Remote /= undefined ->
                        [J || J <- Remote, maps:get(<<"kid">>, J#jose_jwk.fields, undefined) =:= Kid];
                    _ -> Static
                end,
@@ -150,7 +150,9 @@ handle_verify(JwsCompacted,
                 {reply, do_verify(JwsCompacted, Jwks), State}
         end
     catch
-        _:_ ->
+        Class : Reason : Stk ->
+            ?LOG(error, "Handle JWK crashed: ~p, ~p, stacktrace: ~p~n",
+                        [Class, Reason, Stk]),
             {reply, {error, invalid_signature}, State}
     end.
 
@@ -186,8 +188,8 @@ do_verify(JwsCompacted, [Jwk|More]) ->
         {true, Payload, _Jws} ->
             Claims = emqx_json:decode(Payload, [return_maps]),
             case check_claims(Claims) of
-                false ->
-                    {error, invalid_signature};
+                {false, <<"exp">>} ->
+                    {error, {invalid_signature, expired}};
                 NClaims ->
                     {ok, NClaims}
             end;
@@ -217,6 +219,6 @@ do_check_claim([{K, F}|More], Claims) ->
         {V, NClaims} ->
             case F(V) of
                 true -> do_check_claim(More, NClaims);
-                _ -> false
+                _ -> {false, K}
             end
     end.
diff --git a/apps/emqx_auth_jwt/test/emqx_auth_jwt_SUITE.erl b/apps/emqx_auth_jwt/test/emqx_auth_jwt_SUITE.erl
index d0a4a34a0..d4f562b6f 100644
--- a/apps/emqx_auth_jwt/test/emqx_auth_jwt_SUITE.erl
+++ b/apps/emqx_auth_jwt/test/emqx_auth_jwt_SUITE.erl
@@ -33,6 +33,7 @@ groups() ->
                                  , t_check_claims
                                  , t_check_claims_clientid
                                  , t_check_claims_username
+                                 , t_check_claims_kid_in_header
                                  ]}
     ].
 
@@ -61,6 +62,12 @@ set_special_configs(emqx_auth_jwt) ->
 set_special_configs(_) ->
     ok.
 
+sign(Payload, Header, Key) when is_map(Header) ->
+    Jwk = jose_jwk:from_oct(Key),
+    Jwt = emqx_json:encode(Payload),
+    {_, Token} = jose_jws:compact(jose_jwt:sign(Jwk, Header, Jwt)),
+    Token;
+
 sign(Payload, Alg, Key) ->
     Jwk = jose_jwk:from_oct(Key),
     Jwt = emqx_json:encode(Payload),
@@ -145,3 +152,15 @@ t_check_claims_username(_) ->
     Result3 = emqx_access_control:authenticate(Plain#{password => Jwt_Error}),
     ct:pal("Auth result for the invalid jwt: ~p~n", [Result3]),
     ?assertEqual({error, invalid_signature}, Result3).
+
+t_check_claims_kid_in_header(_) ->
+    application:set_env(emqx_auth_jwt, verify_claims, []),
+    Plain = #{clientid => <<"client23">>, username => <<"plain">>, zone => external},
+    Jwt = sign([{clientid, <<"client23">>},
+                {username, <<"plain">>},
+                {exp, os:system_time(seconds) + 3}],
+               #{<<"alg">> => <<"HS256">>,
+                 <<"kid">> => <<"a_kid_str">>}, <<"emqxsecret">>),
+    Result0 = emqx_access_control:authenticate(Plain#{password => Jwt}),
+    ct:pal("Auth result: ~p~n", [Result0]),
+    ?assertMatch({ok, #{auth_result := success, jwt_claims := _}}, Result0).