Merge pull request #2727 from apache/jwt-kty-check

Only trust the servers declaration of JWT key type

4 files changed, 134 insertions, 33 deletions

diff --git a/rel/overlay/etc/default.ini b/rel/overlay/etc/default.ini
index 25daa4813..25f1027d2 100644
--- a/rel/overlay/etc/default.ini
+++ b/rel/overlay/etc/default.ini
@@ -151,14 +151,15 @@ max_db_number_for_dbs_info_req = 100
 ; If your JWT tokens do not include a "kid" attribute, use "_default"
 ; as the config key, otherwise use the kid as the config key.
 ; Examples
-; _default = aGVsbG8=
-; foo = aGVsbG8=
+; hmac:_default = aGVsbG8=
+; hmac:foo = aGVsbG8=
 ; The config values can represent symmetric and asymmetrics keys.
 ; For symmetrics keys, the value is base64 encoded;
-; _default = aGVsbG8= # base64-encoded form of "hello"
+; hmac:_default = aGVsbG8= # base64-encoded form of "hello"
 ; For asymmetric keys, the value is the PEM encoding of the public
 ; key with newlines replaced with the escape sequence \n.
-; foo = -----BEGIN PUBLIC KEY-----\nMHYwEAYHKoZIzj0CAQYFK4EEACIDYgAEDsr0lz/Dg3luarb+Kua0Wcj9WrfR23os\nwHzakglb8GhWRDn+oZT0Bt/26sX8uB4/ij9PEOLHPo+IHBtX4ELFFVr5GTzlqcJe\nyctaTDd1OOAPXYuc67EWtGZ3pDAzztRs\n-----END PUBLIC KEY-----\n\n
+; rsa:foo = -----BEGIN PUBLIC KEY-----\nMIIBIjAN...IDAQAB\n-----END PUBLIC KEY-----\n
+; ec:bar = -----BEGIN PUBLIC KEY-----\nMHYwEAYHK...AzztRs\n-----END PUBLIC KEY-----\n
 
 [couch_peruser]
 ; If enabled, couch_peruser ensures that a private per-user database
diff --git a/src/jwtf/src/jwtf_keystore.erl b/src/jwtf/src/jwtf_keystore.erl
index 2f2f24744..be261e67c 100644
--- a/src/jwtf/src/jwtf_keystore.erl
+++ b/src/jwtf/src/jwtf_keystore.erl
@@ -14,6 +14,8 @@
 -behaviour(gen_server).
 -behaviour(config_listener).
 
+-include_lib("public_key/include/public_key.hrl").
+
 % public api.
 -export([
     get/2,
@@ -29,19 +31,18 @@
 
 % public functions
 
-get(Alg, undefined) ->
-    get(Alg, "_default");
-
-get(Alg, KID) when is_binary(KID) ->
-    get(Alg, binary_to_list(KID));
+get(Alg, undefined) when is_binary(Alg) ->
+    get(Alg, <<"_default">>);
 
-get(Alg, KID) ->
-    case ets:lookup(?MODULE, KID) of
+get(Alg, KID0) when is_binary(Alg), is_binary(KID0) ->
+    Kty = kty(Alg),
+    KID = binary_to_list(KID0),
+    case ets:lookup(?MODULE, {Kty, KID}) of
         [] ->
-            Key = get_from_config(Alg, KID),
-            ok = gen_server:call(?MODULE, {set, KID, Key}),
+            Key = get_from_config(Kty, KID),
+            ok = gen_server:call(?MODULE, {set, Kty, KID, Key}),
             Key;
-        [{KID, Key}] ->
+        [{{Kty, KID}, Key}] ->
              Key
     end.
 
@@ -57,13 +58,13 @@ init(_) ->
     {ok, nil}.
 
 
-handle_call({set, KID, Key}, _From, State) ->
-    true = ets:insert(?MODULE, {KID, Key}),
+handle_call({set, Kty, KID, Key}, _From, State) ->
+    true = ets:insert(?MODULE, {{Kty, KID}, Key}),
     {reply, ok, State}.
 
 
-handle_cast({delete, KID}, State) ->
-    true = ets:delete(?MODULE, KID),
+handle_cast({delete, Kty, KID}, State) ->
+    true = ets:delete(?MODULE, {Kty, KID}),
     {noreply, State};
 
 handle_cast(_Msg, State) ->
@@ -88,8 +89,14 @@ code_change(_OldVsn, State, _Extra) ->
 
 % config listener callback
 
-handle_config_change("jwt_keys", KID, _Value, _, _) ->
-    {ok, gen_server:cast(?MODULE, {delete, KID})};
+handle_config_change("jwt_keys", ConfigKey, _ConfigValue, _, _) ->
+    case string:split(ConfigKey, ":") of
+        [Kty, KID] ->
+            gen_server:cast(?MODULE, {delete, Kty, KID});
+        _ ->
+            ignored
+    end,
+    {ok, nil};
 
 handle_config_change(_, _, _, _, _) ->
     {ok, nil}.
@@ -102,17 +109,53 @@ handle_config_terminate(_Server, _Reason, _State) ->
 
 % private functions
 
-get_from_config(Alg, KID) ->
-    case config:get("jwt_keys", KID) of
+get_from_config(Kty, KID) ->
+    case config:get("jwt_keys", string:join([Kty, KID], ":")) of
         undefined ->
             throw({bad_request, <<"Unknown kid">>});
-        Key ->
-            case jwtf:verification_algorithm(Alg) of
-                {hmac, _} ->
-                    base64:decode(Key);
-                {public_key, _} ->
-                    BinKey = iolist_to_binary(string:replace(Key, "\\n", "\n", all)),
-                    [PEMEntry] = public_key:pem_decode(BinKey),
-                    public_key:pem_entry_decode(PEMEntry)
+        Encoded ->
+            case Kty of
+                "hmac" ->
+                    try
+                        base64:decode(Encoded)
+                    catch
+                        error:_ ->
+                            throw({bad_request, <<"Not a valid key">>})
+                    end;
+                "rsa" ->
+                    case pem_decode(Encoded) of
+                        #'RSAPublicKey'{} = Key ->
+                            Key;
+                        _ ->
+                            throw({bad_request, <<"not an RSA public key">>})
+                    end;
+                "ec" ->
+                    case pem_decode(Encoded) of
+                        {#'ECPoint'{}, _} = Key ->
+                            Key;
+                        _ ->
+                            throw({bad_request, <<"not an EC public key">>})
+                    end
             end
     end.
+
+pem_decode(PEM) ->
+    BinPEM = iolist_to_binary(string:replace(PEM, "\\n", "\n", all)),
+    case public_key:pem_decode(BinPEM) of
+        [PEMEntry] ->
+            public_key:pem_entry_decode(PEMEntry);
+        [] ->
+            throw({bad_request, <<"Not a valid key">>})
+    end.
+
+kty(<<"HS", _/binary>>) ->
+    "hmac";
+
+kty(<<"RS", _/binary>>) ->
+    "rsa";
+
+kty(<<"ES", _/binary>>) ->
+    "ec";
+
+kty(_) ->
+    throw({bad_request, <<"Unknown kty">>}).
diff --git a/src/jwtf/test/jwtf_keystore_tests.erl b/src/jwtf/test/jwtf_keystore_tests.erl
new file mode 100644
index 000000000..9ec943653
--- /dev/null
+++ b/src/jwtf/test/jwtf_keystore_tests.erl
@@ -0,0 +1,57 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+%   http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(jwtf_keystore_tests).
+
+-include_lib("eunit/include/eunit.hrl").
+-include_lib("public_key/include/public_key.hrl").
+
+-define(HMAC_SECRET, "aGVsbG8=").
+-define(RSA_SECRET, "-----BEGIN PUBLIC KEY-----\\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAztanwQtIx0sms+x7m1SF\\nh7EHJHkM2biTJ41jR89FsDE2gd3MChpaqxemS5GpNvfFKRvuHa4PUZ3JtRCBG1KM\\n/7EWIVTy1JQDr2mb8couGlQNqz4uXN2vkNQ0XszgjU4Wn6ZpvYxmqPFbmkRe8QSn\\nAy2Wf8jQgjsbez8eaaX0G9S1hgFZUN3KFu7SVmUDQNvWpQdaJPP+ms5Z0CqF7JLa\\nvJmSdsU49nlYw9VH/XmwlUBMye6HgR4ZGCLQS85frqF0xLWvi7CsMdchcIjHudXH\\nQK1AumD/VVZVdi8Q5Qew7F6VXeXqnhbw9n6Px25cCuNuh6u5+E6GUzXRrMpqo9vO\\nqQIDAQAB\\n-----END PUBLIC KEY-----\\n").
+-define(EC_SECRET, "-----BEGIN PUBLIC KEY-----\\nMHYwEAYHKoZIzj0CAQYFK4EEACIDYgAEDsr0lz/Dg3luarb+Kua0Wcj9WrfR23os\\nwHzakglb8GhWRDn+oZT0Bt/26sX8uB4/ij9PEOLHPo+IHBtX4ELFFVr5GTzlqcJe\\nyctaTDd1OOAPXYuc67EWtGZ3pDAzztRs\\n-----END PUBLIC KEY-----\\n").
+
+setup() ->
+    test_util:start_applications([config, jwtf]),
+    config:set("jwt_keys", "hmac:hmac", ?HMAC_SECRET),
+    config:set("jwt_keys", "rsa:hmac", ?HMAC_SECRET),
+    config:set("jwt_keys", "ec:hmac", ?HMAC_SECRET),
+
+    config:set("jwt_keys", "hmac:rsa", ?RSA_SECRET),
+    config:set("jwt_keys", "rsa:rsa", ?RSA_SECRET),
+    config:set("jwt_keys", "ec:rsa", ?RSA_SECRET),
+
+    config:set("jwt_keys", "hmac:ec", ?EC_SECRET),
+    config:set("jwt_keys", "rsa:ec", ?EC_SECRET),
+    config:set("jwt_keys", "ec:ec", ?EC_SECRET).
+
+teardown(_) ->
+    test_util:stop_applications([config, jwtf]).
+
+jwtf_keystore_test_() ->
+    {
+     setup,
+     fun setup/0,
+     fun teardown/1,
+     [
+      ?_assertEqual(<<"hello">>,       jwtf_keystore:get(<<"HS256">>, <<"hmac">>)),
+      ?_assertThrow({bad_request, _},  jwtf_keystore:get(<<"RS256">>, <<"hmac">>)),
+      ?_assertThrow({bad_request, _},  jwtf_keystore:get(<<"ES256">>, <<"hmac">>)),
+
+      ?_assertThrow({bad_request, _},  jwtf_keystore:get(<<"HS256">>, <<"rsa">>)),
+      ?_assertMatch(#'RSAPublicKey'{}, jwtf_keystore:get(<<"RS256">>, <<"rsa">>)),
+      ?_assertThrow({bad_request, _},  jwtf_keystore:get(<<"ES256">>, <<"rsa">>)),
+
+      ?_assertThrow({bad_request, _},  jwtf_keystore:get(<<"HS256">>, <<"ec">>)),
+      ?_assertThrow({bad_request, _},  jwtf_keystore:get(<<"RS256">>, <<"ec">>)),
+      ?_assertMatch({#'ECPoint'{}, _}, jwtf_keystore:get(<<"ES256">>, <<"ec">>))
+     ]
+    }.
diff --git a/test/elixir/test/jwtauth_test.exs b/test/elixir/test/jwtauth_test.exs
index de5b3e65d..c50225cbd 100644
--- a/test/elixir/test/jwtauth_test.exs
+++ b/test/elixir/test/jwtauth_test.exs
@@ -10,7 +10,7 @@ defmodule JwtAuthTest do
     server_config = [
       %{
         :section => "jwt_keys",
-        :key => "_default",
+        :key => "hmac:_default",
         :value => :base64.encode(secret)
       },
       %{
@@ -49,7 +49,7 @@ defmodule JwtAuthTest do
     server_config = [
       %{
         :section => "jwt_keys",
-        :key => "_default",
+        :key => "rsa:_default",
         :value => public_pem
       },
       %{
@@ -87,7 +87,7 @@ defmodule JwtAuthTest do
     server_config = [
       %{
         :section => "jwt_keys",
-        :key => "_default",
+        :key => "ec:_default",
         :value => public_pem
       },
       %{