Merge pull request #2732 from apache/jwt-controls-2backport-jwt-3.x

Enhance JWT controls (again)

5 files changed, 72 insertions, 41 deletions

diff --git a/rel/overlay/etc/default.ini b/rel/overlay/etc/default.ini
index 16387f2f3..4654d55ee 100644
--- a/rel/overlay/etc/default.ini
+++ b/rel/overlay/etc/default.ini
@@ -142,9 +142,7 @@ max_db_number_for_dbs_info_req = 100
 
 ;[jwt_auth]
 ; List of claims to validate
-; required_claims = exp
-; List of algorithms to accept during checks
-; allowed_algorithms = HS256
+; required_claims =
 ;
 ; [jwt_keys]
 ; Configure at least one key here if using the JWT auth handler.
diff --git a/src/couch/src/couch_httpd_auth.erl b/src/couch/src/couch_httpd_auth.erl
index 5cabda34e..b7abf2a01 100644
--- a/src/couch/src/couch_httpd_auth.erl
+++ b/src/couch/src/couch_httpd_auth.erl
@@ -192,8 +192,7 @@ jwt_authentication_handler(Req) ->
     case header_value(Req, "Authorization") of
         "Bearer " ++ Jwt ->
             RequiredClaims = get_configured_claims(),
-            AllowedAlgorithms = get_configured_algorithms(),
-            case jwtf:decode(?l2b(Jwt), [{alg, AllowedAlgorithms} | RequiredClaims], fun jwtf_keystore:get/2) of
+            case jwtf:decode(?l2b(Jwt), [alg | RequiredClaims], fun jwtf_keystore:get/2) of
                 {ok, {Claims}} ->
                     case lists:keyfind(<<"sub">>, 1, Claims) of
                         false -> throw({unauthorized, <<"Token missing sub claim.">>});
@@ -208,11 +207,14 @@ jwt_authentication_handler(Req) ->
         _ -> Req
     end.
 
-get_configured_algorithms() ->
-    re:split(config:get("jwt_auth", "allowed_algorithms", "HS256"), "\s*,\s*", [{return, binary}]).
-
 get_configured_claims() ->
-    re:split(config:get("jwt_auth", "required_claims", ""), "\s*,\s*", [{return, binary}]).
+    Claims = config:get("jwt_auth", "required_claims", ""),
+    case re:split(Claims, "\s*,\s*", [{return, list}]) of
+        [[]] ->
+            []; %% if required_claims is the empty string.
+        List ->
+            [list_to_existing_atom(C) || C <- List]
+    end.
 
 cookie_authentication_handler(Req) ->
     cookie_authentication_handler(Req, couch_auth_cache).
diff --git a/src/jwtf/src/jwtf.erl b/src/jwtf/src/jwtf.erl
index b558bdc63..247f2b508 100644
--- a/src/jwtf/src/jwtf.erl
+++ b/src/jwtf/src/jwtf.erl
@@ -123,8 +123,15 @@ validate(Header0, Payload0, Signature, Checks, KS) ->
     Key = key(Header1, Checks, KS),
     verify(Alg, Header0, Payload0, Signature, Key).
 
+
 validate_checks(Checks) when is_list(Checks) ->
-    UnknownChecks = proplists:get_keys(Checks) -- ?CHECKS,
+    case {lists:usort(Checks), lists:sort(Checks)} of
+        {L, L} ->
+            ok;
+        {L1, L2} ->
+            error({duplicate_checks, L2 -- L1})
+    end,
+    {_, UnknownChecks} = lists:partition(fun valid_check/1, Checks),
     case UnknownChecks of
         [] ->
             ok;
@@ -132,6 +139,17 @@ validate_checks(Checks) when is_list(Checks) ->
             error({unknown_checks, UnknownChecks})
     end.
 
+
+valid_check(Check) when is_atom(Check) ->
+    lists:member(Check, ?CHECKS);
+
+valid_check({Check, _}) when is_atom(Check) ->
+    lists:member(Check, ?CHECKS);
+
+valid_check(_) ->
+    false.
+
+
 validate_header(Props, Checks) ->
     validate_typ(Props, Checks),
     validate_alg(Props, Checks).
@@ -141,11 +159,11 @@ validate_typ(Props, Checks) ->
     Required = prop(typ, Checks),
     TYP = prop(<<"typ">>, Props),
     case {Required, TYP} of
-        {undefined, _} ->
+        {undefined, undefined} ->
             ok;
         {true, undefined} ->
             throw({bad_request, <<"Missing typ header parameter">>});
-        {true, <<"JWT">>} ->
+        {_, <<"JWT">>} ->
             ok;
         {true, _} ->
             throw({bad_request, <<"Invalid typ header parameter">>})
@@ -156,13 +174,12 @@ validate_alg(Props, Checks) ->
     Required = prop(alg, Checks),
     Alg = prop(<<"alg">>, Props),
     case {Required, Alg} of
-        {undefined, _} ->
+        {undefined, undefined} ->
             ok;
-        {Required, undefined} when Required /= undefined ->
+        {true, undefined} ->
             throw({bad_request, <<"Missing alg header parameter">>});
-        {Required, Alg} when Required == true; is_list(Required) ->
-            AllowedAlg = if Required == true -> true; true -> lists:member(Alg, Required) end,
-            case AllowedAlg andalso lists:member(Alg, valid_algorithms()) of
+        {_, Alg} ->
+            case lists:member(Alg, valid_algorithms()) of
                 true ->
                     ok;
                 false ->
@@ -185,9 +202,9 @@ validate_iss(Props, Checks) ->
     ActualISS = prop(<<"iss">>, Props),
 
     case {ExpectedISS, ActualISS} of
-        {undefined, _} ->
+        {undefined, undefined} ->
             ok;
-        {_ISS, undefined} ->
+        {ISS, undefined} when ISS /= undefined ->
             throw({bad_request, <<"Missing iss claim">>});
         {ISS, ISS} ->
             ok;
@@ -201,11 +218,11 @@ validate_iat(Props, Checks) ->
     IAT = prop(<<"iat">>, Props),
 
     case {Required, IAT} of
-        {undefined, _} ->
+        {undefined, undefined} ->
             ok;
         {true, undefined} ->
             throw({bad_request, <<"Missing iat claim">>});
-        {true, IAT} when is_integer(IAT) ->
+        {_, IAT} when is_integer(IAT) ->
             ok;
         {true, _} ->
             throw({bad_request, <<"Invalid iat claim">>})
@@ -217,11 +234,11 @@ validate_nbf(Props, Checks) ->
     NBF = prop(<<"nbf">>, Props),
 
     case {Required, NBF} of
-        {undefined, _} ->
+        {undefined, undefined} ->
             ok;
         {true, undefined} ->
             throw({bad_request, <<"Missing nbf claim">>});
-        {true, IAT} ->
+        {_, IAT} ->
             assert_past(<<"nbf">>, IAT)
     end.
 
@@ -231,11 +248,11 @@ validate_exp(Props, Checks) ->
     EXP = prop(<<"exp">>, Props),
 
     case {Required, EXP} of
-        {undefined, _} ->
+        {undefined, undefined} ->
             ok;
         {true, undefined} ->
             throw({bad_request, <<"Missing exp claim">>});
-        {true, EXP} ->
+        {_, EXP} ->
             assert_future(<<"exp">>, EXP)
     end.
 
diff --git a/src/jwtf/test/jwtf_tests.erl b/src/jwtf/test/jwtf_tests.erl
index e445e5fc9..ba944f7c7 100644
--- a/src/jwtf/test/jwtf_tests.erl
+++ b/src/jwtf/test/jwtf_tests.erl
@@ -82,16 +82,6 @@ invalid_alg_test() ->
     ?assertEqual({error, {bad_request,<<"Invalid alg header parameter">>}},
         jwtf:decode(Encoded, [alg], nil)).
 
-not_allowed_alg_test() ->
-    Encoded = encode({[{<<"alg">>, <<"HS256">>}]}, []),
-    ?assertEqual({error, {bad_request,<<"Invalid alg header parameter">>}},
-        jwtf:decode(Encoded, [{alg, [<<"RS256">>]}], nil)).
-
-reject_unknown_alg_test() ->
-    Encoded = encode({[{<<"alg">>, <<"NOPE">>}]}, []),
-    ?assertEqual({error, {bad_request,<<"Invalid alg header parameter">>}},
-        jwtf:decode(Encoded, [{alg, [<<"NOPE">>]}], nil)).
-
 
 missing_iss_test() ->
     Encoded = encode(valid_header(), {[]}),
@@ -178,9 +168,17 @@ malformed_token_test() ->
     ?assertEqual({error, {bad_request, <<"Malformed token">>}},
         jwtf:decode(<<"a.b.c.d">>, [], nil)).
 
-unknown_check_test() ->
-    ?assertError({unknown_checks, [bar, foo]},
-        jwtf:decode(<<"a.b.c">>, [exp, foo, iss, bar, exp], nil)).
+unknown_atom_check_test() ->
+    ?assertError({unknown_checks, [foo, bar]},
+        jwtf:decode(<<"a.b.c">>, [exp, foo, iss, bar], nil)).
+
+unknown_binary_check_test() ->
+    ?assertError({unknown_checks, [<<"bar">>]},
+        jwtf:decode(<<"a.b.c">>, [exp, iss, <<"bar">>], nil)).
+
+duplicate_check_test() ->
+    ?assertError({duplicate_checks, [exp]},
+        jwtf:decode(<<"a.b.c">>, [exp, exp], nil)).
 
 
 %% jwt.io generated
@@ -190,7 +188,7 @@ hs256_test() ->
                      "6MTAwMDAwMDAwMDAwMDAsImtpZCI6ImJhciJ9.iS8AH11QHHlczkBn"
                      "Hl9X119BYLOZyZPllOVhSBZ4RZs">>,
     KS = fun(<<"HS256">>, <<"123456">>) -> <<"secret">> end,
-    Checks = [{iss, <<"https://foo.com">>}, iat, exp, typ, {alg, [<<"HS256">>]}, kid],
+    Checks = [{iss, <<"https://foo.com">>}, iat, exp, typ, alg, kid],
     ?assertMatch({ok, _}, catch jwtf:decode(EncodedToken, Checks, KS)).
 
 
@@ -277,7 +275,7 @@ header(Alg) ->
 
 
 claims() ->
-    EpochSeconds = 1496205841,
+    EpochSeconds = os:system_time(second),
     {[
         {<<"iat">>, EpochSeconds},
         {<<"exp">>, EpochSeconds + 3600}
diff --git a/test/elixir/test/jwtauth_test.exs b/test/elixir/test/jwtauth_test.exs
index 9d43c23e2..55e075cb2 100644
--- a/test/elixir/test/jwtauth_test.exs
+++ b/test/elixir/test/jwtauth_test.exs
@@ -103,7 +103,23 @@ defmodule JwtAuthTest do
   end
 
   def test_fun(alg, key) do
-    {:ok, token} = :jwtf.encode({[{"alg", alg}, {"typ", "JWT"}]}, {[{"sub", "couch@apache.org"}, {"_couchdb.roles", ["testing"]}]}, key)
+    now = DateTime.to_unix(DateTime.utc_now())
+    {:ok, token} = :jwtf.encode(
+      {
+        [
+          {"alg", alg},
+          {"typ", "JWT"}
+        ]
+      },
+      {
+        [
+          {"nbf", now - 60},
+          {"exp", now + 60},
+          {"sub", "couch@apache.org"},
+          {"_couchdb.roles", ["testing"]
+          }
+        ]
+      }, key)
 
     resp = Couch.get("/_session",
       headers: [authorization: "Bearer #{token}"]