Merge pull request #13432 from JimMoen/0705-fix-jwt-pem-check

fix: create authn jwt with bad public key

apps/emqx_auth/test/data/bad_public_key_file.pem

@@ -0,0 +1,3 @@
+-----BEGIN PUBLIC KEY-----
+XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
+-----END PUBLIC KEY-----

apps/emqx_auth_jwt/src/emqx_authn_jwt.erl

@@ -18,6 +18,7 @@
 
 -include_lib("emqx_auth/include/emqx_authn.hrl").
 -include_lib("emqx/include/logger.hrl").
+-include_lib("jose/include/jose_jwk.hrl").
 
 -export([
     create/2,
@@ -38,7 +39,7 @@ create(_AuthenticatorID, Config) ->
     create(Config).
 
 create(#{verify_claims := VerifyClaims} = Config) ->
-    create2(Config#{verify_claims => handle_verify_claims(VerifyClaims)}).
+    do_create(Config#{verify_claims => handle_verify_claims(VerifyClaims)}).
 
 update(
     #{use_jwks := false} = Config,
@@ -83,6 +84,7 @@ authenticate(
     }
 ) ->
     JWT = maps:get(From, Credential),
+    %% XXX: Only supports single public key
     JWKs = [JWK],
     VerifyClaims = replace_placeholder(VerifyClaims0, Credential),
     verify(JWT, JWKs, VerifyClaims, AclClaimName, DisconnectAfterExpire);
@@ -119,7 +121,7 @@ destroy(_) ->
 %% Internal functions
 %%--------------------------------------------------------------------
 
-create2(#{
+do_create(#{
     use_jwks := false,
     algorithm := 'hmac-based',
     secret := Secret0,
@@ -142,24 +144,35 @@ create2(#{
                 from => From
             }}
     end;
-create2(#{
-    use_jwks := false,
-    algorithm := 'public-key',
-    public_key := PublicKey,
-    verify_claims := VerifyClaims,
-    disconnect_after_expire := DisconnectAfterExpire,
-    acl_claim_name := AclClaimName,
-    from := From
-}) ->
-    JWK = create_jwk_from_public_key(PublicKey),
-    {ok, #{
-        jwk => JWK,
-        verify_claims => VerifyClaims,
-        disconnect_after_expire => DisconnectAfterExpire,
-        acl_claim_name => AclClaimName,
-        from => From
-    }};
-create2(
+do_create(
+    #{
+        use_jwks := false,
+        algorithm := 'public-key',
+        public_key := PublicKey,
+        verify_claims := VerifyClaims,
+        disconnect_after_expire := DisconnectAfterExpire,
+        acl_claim_name := AclClaimName,
+        from := From
+    } = Config
+) ->
+    case
+        create_jwk_from_public_key(
+            maps:get(enable, Config, false),
+            PublicKey
+        )
+    of
+        {ok, JWK} ->
+            {ok, #{
+                jwk => JWK,
+                verify_claims => VerifyClaims,
+                disconnect_after_expire => DisconnectAfterExpire,
+                acl_claim_name => AclClaimName,
+                from => From
+            }};
+        {error, _Reason} = Err ->
+            Err
+    end;
+do_create(
     #{
         use_jwks := true,
         verify_claims := VerifyClaims,
@@ -183,9 +196,23 @@ create2(
         from => From
     }}.
 
-create_jwk_from_public_key(PublicKey) when
+create_jwk_from_public_key(true, PublicKey) when
     is_binary(PublicKey); is_list(PublicKey)
 ->
+    try do_create_jwk_from_public_key(PublicKey) of
+        %% XXX: Only supports single public key
+        #jose_jwk{} = Res ->
+            {ok, Res};
+        _ ->
+            {error, invalid_public_key}
+    catch
+        _:_ ->
+            {error, invalid_public_key}
+    end;
+create_jwk_from_public_key(false, _PublicKey) ->
+    {ok, []}.
+
+do_create_jwk_from_public_key(PublicKey) ->
     case filelib:is_file(PublicKey) of
         true ->
             jose_jwk:from_pem_file(PublicKey);

apps/emqx_auth_jwt/test/emqx_authn_jwt_SUITE.erl

@@ -185,6 +185,7 @@ t_public_key(_) ->
         from => password,
         acl_claim_name => <<"acl">>,
         use_jwks => false,
+        enable => true,
         algorithm => 'public-key',
         public_key => PublicKey,
         verify_claims => [],
@@ -206,6 +207,51 @@ t_public_key(_) ->
     ?assertEqual(ok, emqx_authn_jwt:destroy(State)),
     ok.
 
+t_bad_public_keys(_) ->
+    BaseConfig = #{
+        mechanism => jwt,
+        from => password,
+        acl_claim_name => <<"acl">>,
+        use_jwks => false,
+        algorithm => 'public-key',
+        verify_claims => [],
+        disconnect_after_expire => false
+    },
+
+    %% try create with invalid public key
+    ?assertMatch(
+        {error, invalid_public_key},
+        emqx_authn_jwt:create(?AUTHN_ID, BaseConfig#{
+            enable => true,
+            public_key => <<"bad_public_key">>
+        })
+    ),
+
+    %% no such file
+    ?assertMatch(
+        {error, invalid_public_key},
+        emqx_authn_jwt:create(?AUTHN_ID, BaseConfig#{
+            enable => true,
+            public_key => data_file("bad_flie_path.pem")
+        })
+    ),
+
+    %% bad public key file content
+    ?assertMatch(
+        {error, invalid_public_key},
+        emqx_authn_jwt:create(?AUTHN_ID, BaseConfig#{
+            enable => true,
+            public_key => data_file("bad_public_key_file.pem")
+        })
+    ),
+
+    %% assume jwk authenticator is disabled
+    {ok, State} =
+        emqx_authn_jwt:create(?AUTHN_ID, BaseConfig#{public_key => <<"bad_public_key">>}),
+
+    ?assertEqual(ok, emqx_authn_jwt:destroy(State)),
+    ok.
+
 t_jwt_in_username(_) ->
     Secret = <<"abcdef">>,
     Config = #{

changes/fix-13432.en.md

@@ -0,0 +1 @@
+Fixed the issue where JWT authentication was silently bypassed when an invalid public key (or invalid public key file path) was used.