Merge pull request #12831 from rabbitmq/mergify/bp/v4.0.x/pr-12818

michaelklishin · web-flow · commit 5b90d1aacab3 · 2024-11-29T12:18:48.000-05:00
OAuth 2: JWT token refresh should preserve user identity displayed in the UI (backport #12818)
diff --git a/deps/rabbitmq_auth_backend_oauth2/src/rabbit_auth_backend_oauth2.erl b/deps/rabbitmq_auth_backend_oauth2/src/rabbit_auth_backend_oauth2.erl
@@ -20,8 +20,9 @@
 
 % for testing
 -export([post_process_payload/2, get_expanded_scopes/2]).
-
+-import(uaa_jwt, [resolve_resource_server_id/1]).
 -import(rabbit_data_coercion, [to_map/1]).
+-import(rabbit_oauth2_config, [get_preferred_username_claims/1]).
 
 -ifdef(TEST).
 -compile(export_all).
@@ -98,19 +99,28 @@ check_topic_access(#auth_user{impl = DecodedTokenFun},
         end).
 
 update_state(AuthUser, NewToken) ->
-  case check_token(NewToken) of
-      %% avoid logging the token
-      {error, _} = E  -> E;
-      {refused, {error, {invalid_token, error, _Err, _Stacktrace}}} ->
-        {refused, "Authentication using an OAuth 2/JWT token failed: provided token is invalid"};
-      {refused, Err} ->
-        {refused, rabbit_misc:format("Authentication using an OAuth 2/JWT token failed: ~tp", [Err])};
-      {ok, DecodedToken} ->
-          Tags = tags_from(DecodedToken),
-
-          {ok, AuthUser#auth_user{tags = Tags,
-                                  impl = fun() -> DecodedToken end}}
-  end.
+    case check_token(NewToken) of
+        %% avoid logging the token
+        {error, _} = E  -> E;
+        {refused, {error, {invalid_token, error, _Err, _Stacktrace}}} ->
+            {refused, "Authentication using an OAuth 2/JWT token failed: provided token is invalid"};
+        {refused, Err} ->
+            {refused, rabbit_misc:format("Authentication using an OAuth 2/JWT token failed: ~tp", [Err])};
+        {ok, DecodedToken} ->
+            ResourceServerId = resolve_resource_server_id(DecodedToken),        
+            CurToken = AuthUser#auth_user.impl,
+            case ensure_same_username(
+                            get_preferred_username_claims(ResourceServerId),
+                            CurToken(), DecodedToken) of           
+                ok -> 
+                    Tags = tags_from(DecodedToken),
+                    {ok, AuthUser#auth_user{tags = Tags,
+                                            impl = fun() -> DecodedToken end}};
+                {error, mismatch_username_after_token_refresh} -> 
+                    {refused, 
+                        "Not allowed to change username on refreshed token"}
+            end
+    end.
 
 expiry_timestamp(#auth_user{impl = DecodedTokenFun}) ->
     case DecodedTokenFun() of
@@ -135,13 +145,15 @@ authenticate(_, AuthProps0) ->
           {refused, "Authentication using an OAuth 2/JWT token failed: ~tp", [Err]};
         {ok, DecodedToken} ->
             Func = fun(Token0) ->
-                        Username = username_from(rabbit_oauth2_config:get_preferred_username_claims(), Token0),
-                        Tags     = tags_from(Token0),
-
-                        {ok, #auth_user{username = Username,
-                                        tags = Tags,
-                                        impl = fun() -> Token0 end}}
-                   end,
+                ResourceServerId = resolve_resource_server_id(Token0),
+                Username = username_from(
+                    get_preferred_username_claims(ResourceServerId), 
+                    Token0),
+                Tags     = tags_from(Token0),
+                {ok, #auth_user{username = Username,
+                                tags = Tags,
+                                impl = fun() -> Token0 end}}
+            end,
             case with_decoded_token(DecodedToken, Func) of
                 {error, Err} ->
                     {refused, "Authentication using an OAuth 2/JWT token failed: ~tp", [Err]};
@@ -157,6 +169,12 @@ with_decoded_token(DecodedToken, Fun) ->
             rabbit_log:error(Msg),
             Err
     end.
+ensure_same_username(PreferredUsernameClaims, CurrentDecodedToken, NewDecodedToken) ->
+    CurUsername = username_from(PreferredUsernameClaims, CurrentDecodedToken),
+    case {CurUsername, username_from(PreferredUsernameClaims, NewDecodedToken)} of 
+        {CurUsername, CurUsername} -> ok;
+        _ -> {error, mismatch_username_after_token_refresh}
+    end. 
 
 validate_token_expiry(#{<<"exp">> := Exp}) when is_integer(Exp) ->
     Now = os:system_time(seconds),
diff --git a/deps/rabbitmq_auth_backend_oauth2/src/uaa_jwt.erl b/deps/rabbitmq_auth_backend_oauth2/src/uaa_jwt.erl
@@ -9,6 +9,7 @@
 -export([add_signing_key/3,
          decode_and_verify/1,
          get_jwk/2,
+         resolve_resource_server_id/1,
          verify_signing_key/2]).
 
 -export([client_id/1, sub/1, client_id/2, sub/2]).
@@ -79,6 +80,14 @@ decode_and_verify(Token) ->
           end
     end.
 
+-spec resolve_resource_server_id(binary()|map()) -> binary() | {error, term()}.
+resolve_resource_server_id(Token) when is_map(Token) ->
+    case maps:get(<<"aud">>, Token, undefined) of
+        undefined ->
+            {error, audience_not_found_in_token};
+        Audience ->
+            rabbit_oauth2_config:get_resource_server_id_for_audience(Audience)
+    end;
 resolve_resource_server_id(Token) ->
     case uaa_jwt_jwt:get_aud(Token) of
         {error, _} = Error ->
@@ -87,6 +96,7 @@ resolve_resource_server_id(Token) ->
             rabbit_oauth2_config:get_resource_server_id_for_audience(Audience)
     end.
 
+
 -spec get_jwk(binary(), oauth_provider_id()) -> {ok, map()} | {error, term()}.
 get_jwk(KeyId, OAuthProviderId) ->
     get_jwk(KeyId, OAuthProviderId, true).
diff --git a/deps/rabbitmq_auth_backend_oauth2/test/jwks_SUITE.erl b/deps/rabbitmq_auth_backend_oauth2/test/jwks_SUITE.erl
@@ -17,6 +17,10 @@
                                    open_unmanaged_connection/4, open_unmanaged_connection/5,
                                    close_connection_and_channel/2]).
 -import(rabbit_mgmt_test_util, [amqp_port/1]).
+-import(rabbit_ct_helpers, [
+    set_config/2,
+    get_config/2, get_config/3
+]).
 
 all() ->
     [
@@ -45,7 +49,8 @@ groups() ->
         test_failed_connection_with_a_token_with_insufficient_resource_permission,
         test_failed_connection_with_algorithm_restriction,
         test_failed_token_refresh_case1,
-        test_failed_token_refresh_case2
+        test_failed_token_refresh_case2,
+        cannot_change_username_on_refreshed_token
         ]},
     {no_peer_verification, [], [
         {group, happy_path},
@@ -531,6 +536,11 @@ generate_valid_token(Config, Jwk, Scopes, Audience) ->
     IncludeKid = rabbit_ct_helpers:get_config(Config, include_kid, true),
     ?UTIL_MOD:sign_token_hs(Token, Jwk, IncludeKid).
 
+generate_valid_token_with_sub(Config, Jwk, Scopes, Sub) ->
+    Token = ?UTIL_MOD:token_with_sub(?UTIL_MOD:fixture_token_with_scopes(Scopes), Sub),
+    IncludeKid = rabbit_ct_helpers:get_config(Config, include_kid, true),
+    ?UTIL_MOD:sign_token_hs(Token, Jwk, IncludeKid).
+
 generate_valid_token_with_extra_fields(Config, ExtraFields) ->
     Jwk = case rabbit_ct_helpers:get_config(Config, fixture_jwk) of
               undefined -> ?UTIL_MOD:fixture_jwk();
@@ -912,6 +922,29 @@ test_failed_token_refresh_case2(Config) ->
 
     close_connection(Conn).
 
+cannot_change_username_on_refreshed_token(Config) ->
+    Jwk = 
+        case get_config(Config, fixture_jwk) of
+            undefined -> ?UTIL_MOD:fixture_jwk();
+            Value     -> Value
+        end,
+    {_, CurToken} = generate_valid_token(Config, Jwk, <<"oldUsername">>, [
+        <<"rabbitmq.configure:vhost4/*">>,
+        <<"rabbitmq.write:vhost4/*">>,
+        <<"rabbitmq.read:vhost4/*">>]),
+    Conn     = open_unmanaged_connection(Config, 0, <<"vhost4">>, 
+                <<"oldUsername">>, CurToken),
+   
+    {_, RefreshToken} = generate_valid_token_with_sub(Config, Jwk, <<"newUsername">>,
+        [<<"rabbitmq.configure:vhost4/*">>,
+         <<"rabbitmq.write:vhost4/*">>,
+         <<"rabbitmq.read:vhost4/*">>]),
+
+    %% the error is communicated asynchronously via a connection-level error
+    ?assertException(exit, _, amqp_connection:update_secret(Conn, RefreshToken,
+        <<"token refresh">>)).
+
+
 test_failed_connection_with_algorithm_restriction(Config) ->
     {_Algo, Token} = rabbit_ct_helpers:get_config(Config, fixture_jwt),
     ?assertMatch({error, {auth_failure, _}},
diff --git a/deps/rabbitmq_auth_backend_oauth2/test/system_SUITE.erl b/deps/rabbitmq_auth_backend_oauth2/test/system_SUITE.erl
@@ -52,7 +52,8 @@ groups() ->
 
      {token_refresh, [], [
                        test_failed_token_refresh_case1,
-                       test_failed_token_refresh_case2
+                       test_failed_token_refresh_case2,
+                       refreshed_token_cannot_change_username
      ]},
 
      {extra_scopes_source, [], [
@@ -312,21 +313,33 @@ preconfigure_node(Config) ->
 
     rabbit_ct_helpers:set_config(Config, {fixture_jwk, Jwk}).
 
+generate_valid_token_with_sub(Config, Sub) ->
+    generate_valid_token(Config, 
+        ?UTIL_MOD:full_permission_scopes(), undefined, Sub).
+
 generate_valid_token(Config) ->
     generate_valid_token(Config, ?UTIL_MOD:full_permission_scopes()).
 
 generate_valid_token(Config, Scopes) ->
-    generate_valid_token(Config, Scopes, undefined).
+    generate_valid_token(Config, Scopes, undefined, undefined).
 
 generate_valid_token(Config, Scopes, Audience) ->
+    generate_valid_token(Config, Scopes, Audience, undefined).
+
+generate_valid_token(Config, Scopes, Audience, Sub) ->
     Jwk = case rabbit_ct_helpers:get_config(Config, fixture_jwk) of
               undefined -> ?UTIL_MOD:fixture_jwk();
               Value     -> Value
           end,
-    Token = case Audience of
+    Token0 = case Audience of
         undefined -> ?UTIL_MOD:fixture_token_with_scopes(Scopes);
-        DefinedAudience -> maps:put(<<"aud">>, DefinedAudience, ?UTIL_MOD:fixture_token_with_scopes(Scopes))
+        DefinedAudience -> maps:put(<<"aud">>, DefinedAudience, 
+            ?UTIL_MOD:fixture_token_with_scopes(Scopes))
     end,
+    Token = case Sub of 
+        undefined -> Token0;
+        _ -> maps:put(<<"sub">>, Sub, Token0)
+    end,    
     ?UTIL_MOD:sign_token_hs(Token, Jwk).
 
 generate_valid_token_with_extra_fields(Config, ExtraFields) ->
@@ -693,6 +706,15 @@ test_failed_token_refresh_case1(Config) ->
 
     close_connection(Conn).
 
+refreshed_token_cannot_change_username(Config) ->
+    {_, Token} = generate_valid_token_with_sub(Config, <<"username">>),
+    Conn     = open_unmanaged_connection(Config, 0, <<"vhost4">>, <<"username">>, Token),
+    {_, RefreshedToken} = generate_valid_token_with_sub(Config, <<"username2">>),
+
+    %% the error is communicated asynchronously via a connection-level error
+    ?assertException(exit, {{nodedown,not_allowed},_}, amqp_connection:update_secret(Conn, RefreshedToken, <<"token refresh">>)).
+    
+
 test_failed_token_refresh_case2(Config) ->
     {_Algo, Token} = generate_valid_token(Config, [<<"rabbitmq.configure:vhost4/*">>,
                                                    <<"rabbitmq.write:vhost4/*">>,
