Merge pull request #12399 from rabbitmq/deprecate-oauth2-settings

Deprecate two OAuth2 settings: auth_oauth2.jwks_url and management.metadata_url
rabbitmq · Oct 9, 2024 · 9893a2b · 9893a2b
2 parents 6c50ce2 + 0f1b876
commit 9893a2b
Show file tree

Hide file tree

Showing 12 changed files with 350 additions and 254 deletions.
diff --git a/deps/oauth2_client/src/oauth2_client.erl b/deps/oauth2_client/src/oauth2_client.erl
@@ -215,12 +215,10 @@ do_update_oauth_provider_endpoints_configuration(OAuthProvider) when
         undefined -> do_nothing;
         EndSessionEndpoint -> set_env(end_session_endpoint, EndSessionEndpoint)
     end,
-    List = get_env(key_config, []),
-    ModifiedList = case OAuthProvider#oauth_provider.jwks_uri of
-        undefined ->  List;
-        JwksEndPoint -> [{jwks_url, JwksEndPoint} | proplists:delete(jwks_url, List)]
+    case OAuthProvider#oauth_provider.jwks_uri of
+        undefined -> do_nothing;
+        JwksUri -> set_env(jwks_uri, JwksUri)
     end,
-    set_env(key_config, ModifiedList),
     rabbit_log:debug("Updated oauth_provider details: ~p ", 
         [format_oauth_provider(OAuthProvider)]),
     OAuthProvider;
@@ -271,7 +269,7 @@ unlock(LockId) ->
 -spec get_oauth_provider(list()) -> {ok, oauth_provider()} | {error, any()}.
 get_oauth_provider(ListOfRequiredAttributes) ->
     case get_env(default_oauth_provider) of
-        undefined -> get_oauth_provider_from_keyconfig(ListOfRequiredAttributes);
+        undefined -> get_root_oauth_provider(ListOfRequiredAttributes);
         DefaultOauthProviderId ->
             rabbit_log:debug("Using default_oauth_provider ~p", 
                 [DefaultOauthProviderId]),
@@ -303,9 +301,9 @@ ensure_oauth_provider_has_attributes(OAuthProvider, ListOfRequiredAttributes) ->
             {error, {missing_oauth_provider_attributes, Attrs}}
     end.
 
-get_oauth_provider_from_keyconfig(ListOfRequiredAttributes) ->
-    OAuthProvider = lookup_oauth_provider_from_keyconfig(),
-    rabbit_log:debug("Using oauth_provider ~p from keyconfig", 
+get_root_oauth_provider(ListOfRequiredAttributes) ->
+    OAuthProvider = lookup_root_oauth_provider(),
+    rabbit_log:debug("Using root oauth_provider ~p", 
         [format_oauth_provider(OAuthProvider)]),
     case find_missing_attributes(OAuthProvider, ListOfRequiredAttributes) of
         [] ->
@@ -384,7 +382,7 @@ find_missing_attributes(#oauth_provider{} = OAuthProvider, RequiredAttributes) -
     Filtered = filter_undefined_props(PropList),
     intersection(Filtered, RequiredAttributes).
 
-lookup_oauth_provider_from_keyconfig() ->
+lookup_root_oauth_provider() ->
     Map = maps:from_list(get_env(key_config, [])),
     Issuer = get_env(issuer),
     DiscoverEndpoint = build_openid_discovery_endpoint(Issuer, 
@@ -393,7 +391,7 @@ lookup_oauth_provider_from_keyconfig() ->
         id = root,
         issuer = Issuer,
         discovery_endpoint = DiscoverEndpoint,
-        jwks_uri = maps:get(jwks_url, Map, undefined), %% jwks_url not uri . _url is the legacy name
+        jwks_uri = get_env(jwks_uri, maps:get(jwks_url, Map, undefined)),
         token_endpoint = get_env(token_endpoint),
         authorization_endpoint = get_env(authorization_endpoint),
         end_session_endpoint = get_env(end_session_endpoint),
@@ -437,15 +435,17 @@ extract_ssl_options_as_list(Map) ->
     ++
     case maps:get(hostname_verification, Map, none) of
         wildcard ->
-            [{customize_hostname_check, [{match_fun, public_key:pkix_verify_hostname_match_fun(https)}]}];
+            [{customize_hostname_check, [{match_fun, 
+                public_key:pkix_verify_hostname_match_fun(https)}]}];
         none ->
             []
     end.
 
 % Replace peer_verification with verify to make it more consistent with other
 % ssl_options in RabbitMQ and Erlang's ssl options
 % Eventually, peer_verification will be removed. For now, both are allowed
--spec get_verify_or_peer_verification(#{atom() => any()}, verify_none | verify_peer ) -> verify_none | verify_peer.
+-spec get_verify_or_peer_verification(#{atom() => 
+    any()}, verify_none | verify_peer ) -> verify_none | verify_peer.
 get_verify_or_peer_verification(Ssl_options, Default) ->
     case maps:get(verify, Ssl_options, undefined) of
         undefined ->
@@ -464,7 +464,8 @@ lookup_oauth_provider_config(OAuth2ProviderId) ->
                 undefined ->
                     {error, {oauth_provider_not_found, OAuth2ProviderId}};
                 OAuthProvider ->
-                    ensure_oauth_provider_has_id_property(OAuth2ProviderId, OAuthProvider)
+                    ensure_oauth_provider_has_id_property(OAuth2ProviderId, 
+                        OAuthProvider)
             end;
         _ ->  {error, invalid_oauth_provider_configuration}
     end.
@@ -535,8 +536,9 @@ get_timeout_of_default(Timeout) ->
 is_json(?CONTENT_JSON) -> true;
 is_json(_) -> false.
 
--spec decode_body(string(), string() | binary() | term()) -> 'false' | 'null' | 'true' |
-                                                              binary() | [any()] | number() | map() | {error, term()}.
+-spec decode_body(string(), string() | binary() | term()) -> 
+    'false' | 'null' | 'true' | binary() | [any()] | number() | map() | 
+    {error, term()}.
 
 decode_body(_, []) -> [];
 decode_body(?CONTENT_JSON, Body) ->
@@ -615,7 +617,8 @@ format_ssl_options(TlsOptions) ->
         [] -> 0;
         Certs -> length(Certs)
     end,
-    lists:flatten(io_lib:format("{verify: ~p, fail_if_no_peer_cert: ~p, crl_check: ~p, depth: ~p, cacertfile: ~p, cacerts(count): ~p }", [
+    lists:flatten(io_lib:format("{verify: ~p, fail_if_no_peer_cert: ~p, " ++
+        "crl_check: ~p, depth: ~p, cacertfile: ~p, cacerts(count): ~p }", [
         proplists:get_value(verify, TlsOptions),
         proplists:get_value(fail_if_no_peer_cert, TlsOptions),
         proplists:get_value(crl_check, TlsOptions),

diff --git a/deps/oauth2_client/test/system_SUITE.erl b/deps/oauth2_client/test/system_SUITE.erl
@@ -12,7 +12,8 @@
 
 -include_lib("oauth2_client.hrl").
 -import(oauth2_client, [
-    build_openid_discovery_endpoint/3]).
+    build_openid_discovery_endpoint/3
+]).
 
 -compile(export_all).
 
@@ -35,10 +36,12 @@ groups() ->
 [
 
     {with_all_oauth_provider_settings, [], [
-        {group, verify_get_oauth_provider}
+        {group, verify_get_oauth_provider},
+        jwks_uri_takes_precedence_over_jwks_url,
+        jwks_url_is_used_in_absense_of_jwks_uri
     ]},
     {without_all_oauth_providers_settings, [], [
-        {group, verify_get_oauth_provider}
+        {group, verify_get_oauth_provider}        
     ]},
     {verify_openid_configuration, [], [
         get_openid_configuration,
@@ -57,7 +60,7 @@ groups() ->
         expiration_time_in_token
     ]},
     {verify_get_oauth_provider, [], [
-        get_oauth_provider,
+        get_oauth_provider,        
         {with_default_oauth_provider, [], [
             get_oauth_provider
         ]},
@@ -78,10 +81,16 @@ groups() ->
 
 init_per_suite(Config) ->
     [
-        {denies_access_token, [ {token_endpoint, denies_access_token_expectation()} ]},
-        {auth_server_error, [ {token_endpoint, auth_server_error_when_access_token_request_expectation()} ]},
-        {non_json_payload, [ {token_endpoint, non_json_payload_when_access_token_request_expectation()} ]},
-        {grants_refresh_token, [ {token_endpoint, grants_refresh_token_expectation()} ]}
+        {jwks_url, build_jwks_uri("https", "/certs4url")},
+        {jwks_uri, build_jwks_uri("https")},
+        {denies_access_token, [ 
+            {token_endpoint, denies_access_token_expectation()} ]},
+        {auth_server_error, [ 
+            {token_endpoint, auth_server_error_when_access_token_request_expectation()} ]},
+        {non_json_payload, [ 
+            {token_endpoint, non_json_payload_when_access_token_request_expectation()} ]},
+        {grants_refresh_token, [ 
+            {token_endpoint, grants_refresh_token_expectation()} ]}
       | Config].
 
 end_per_suite(Config) ->
@@ -95,7 +104,7 @@ init_per_group(https, Config) ->
     CertsDir = ?config(rmq_certsdir, Config0),
     CaCertFile = filename:join([CertsDir, "testca", "cacert.pem"]),
     WrongCaCertFile = filename:join([CertsDir, "server", "server.pem"]),
-    [{group, https},
+    [{group, https},        
         {oauth_provider_id, <<"uaa">>},
         {oauth_provider, build_https_oauth_provider(<<"uaa">>, CaCertFile)},
         {oauth_provider_with_issuer, keep_only_issuer_and_ssl_options(
@@ -198,21 +207,38 @@ configure_all_oauth_provider_settings(Config) ->
         OAuthProvider#oauth_provider.end_session_endpoint),
     application:set_env(rabbitmq_auth_backend_oauth2, authorization_endpoint,
         OAuthProvider#oauth_provider.authorization_endpoint),
-    KeyConfig = [ { jwks_url, OAuthProvider#oauth_provider.jwks_uri } ] ++
+    KeyConfig0 = 
         case OAuthProvider#oauth_provider.ssl_options of
             undefined ->
                 [];
             _ ->
                 [ {peer_verification, proplists:get_value(verify,
                     OAuthProvider#oauth_provider.ssl_options) },
-                  {cacertfile, proplists:get_value(cacertfile,
+                    {cacertfile, proplists:get_value(cacertfile,
                         OAuthProvider#oauth_provider.ssl_options) }
                 ]
         end,
+    KeyConfig = 
+        case ?config(jwks_uri_type_of_config, Config) of 
+            undefined ->
+                application:set_env(rabbitmq_auth_backend_oauth2, jwks_uri,
+                    OAuthProvider#oauth_provider.jwks_uri),
+                KeyConfig0;
+            only_jwks_uri -> 
+                application:set_env(rabbitmq_auth_backend_oauth2, jwks_uri,
+                    OAuthProvider#oauth_provider.jwks_uri),
+                KeyConfig0; 
+            only_jwks_url -> 
+                [ { jwks_url, ?config(jwks_url, Config) } | KeyConfig0 ];
+            both -> 
+                application:set_env(rabbitmq_auth_backend_oauth2, jwks_uri,
+                    OAuthProvider#oauth_provider.jwks_uri),
+                [ { jwks_url, ?config(jwks_url, Config) } | KeyConfig0 ]
+        end,
     application:set_env(rabbitmq_auth_backend_oauth2, key_config, KeyConfig).
 
 configure_minimum_oauth_provider_settings(Config) ->
-    OAuthProvider = ?config(oauth_provider_with_issuer, Config),
+    OAuthProvider = ?config(oauth_provider, Config),
     OAuthProviders = #{ ?config(oauth_provider_id, Config) =>
         oauth_provider_to_proplist(OAuthProvider) },
     application:set_env(rabbitmq_auth_backend_oauth2, oauth_providers,
@@ -232,9 +258,18 @@ configure_minimum_oauth_provider_settings(Config) ->
         end,
     application:set_env(rabbitmq_auth_backend_oauth2, key_config, KeyConfig).
 
-init_per_testcase(TestCase, Config) ->
+init_per_testcase(TestCase, Config0) ->
     application:set_env(rabbitmq_auth_backend_oauth2, use_global_locks, false),
 
+    Config = [case TestCase of 
+        jwks_url_is_used_in_absense_of_jwks_uri -> 
+            {jwks_uri_type_of_config, only_jwks_url};
+        jwks_uri_takes_precedence_over_jwks_url -> 
+            {jwks_uri_type_of_config, both};
+        _ -> 
+            {jwks_uri_type_of_config, only_jwks_uri}
+        end | Config0],
+
     case ?config(with_all_oauth_provider_settings, Config) of
         false -> configure_minimum_oauth_provider_settings(Config);
         true -> configure_all_oauth_provider_settings(Config);
@@ -248,6 +283,9 @@ init_per_testcase(TestCase, Config) ->
         https ->
             start_https_oauth_server(?AUTH_PORT, ?config(rmq_certsdir, Config),
                 ListOfExpectations);
+        without_all_oauth_providers_settings ->
+            start_https_oauth_server(?AUTH_PORT, ?config(rmq_certsdir, Config),
+                ListOfExpectations);    
         _ ->
             do_nothing
     end,
@@ -256,13 +294,16 @@ init_per_testcase(TestCase, Config) ->
 end_per_testcase(_, Config) ->
     application:unset_env(rabbitmq_auth_backend_oauth2, oauth_providers),
     application:unset_env(rabbitmq_auth_backend_oauth2, issuer),
+    application:unset_env(rabbitmq_auth_backend_oauth2, jwks_uri),
     application:unset_env(rabbitmq_auth_backend_oauth2, token_endpoint),
     application:unset_env(rabbitmq_auth_backend_oauth2, authorization_endpoint),
     application:unset_env(rabbitmq_auth_backend_oauth2, end_session_endpoint),
     application:unset_env(rabbitmq_auth_backend_oauth2, key_config),
     case ?config(group, Config) of
         https ->
             stop_https_auth_server();
+        without_all_oauth_providers_settings ->
+            stop_https_auth_server();
         _ ->
             do_nothing
     end,
@@ -466,16 +507,15 @@ ssl_connection_error(Config) ->
     {error, {failed_connect, _} } = oauth2_client:get_access_token(
         ?config(oauth_provider_with_wrong_ca, Config), build_access_token_request(Parameters)).
 
-verify_get_oauth_provider_returns_oauth_provider_from_key_config() ->
+verify_get_oauth_provider_returns_root_oauth_provider() ->
     {ok, #oauth_provider{id = Id,
                         issuer = Issuer,
                         token_endpoint = TokenEndPoint,
                         jwks_uri = Jwks_uri}} =
         oauth2_client:get_oauth_provider([issuer, token_endpoint, jwks_uri]),
-    ExpectedIssuer = application:get_env(rabbitmq_auth_backend_oauth2, issuer, undefined),
-    ExpectedTokenEndPoint = application:get_env(rabbitmq_auth_backend_oauth2, token_endpoint, undefined),
-    ExpectedJwks_uri = proplists:get_value(jwks_url,
-        application:get_env(rabbitmq_auth_backend_oauth2, key_config, [])),
+    ExpectedIssuer = get_env(issuer),
+    ExpectedTokenEndPoint = get_env(token_endpoint),
+    ExpectedJwks_uri = get_env(jwks_uri),
     ?assertEqual(root, Id),
     ?assertEqual(ExpectedIssuer, Issuer),
     ?assertEqual(ExpectedTokenEndPoint, TokenEndPoint),
@@ -492,9 +532,9 @@ verify_get_oauth_provider_returns_default_oauth_provider(DefaultOAuthProviderId)
 get_oauth_provider(Config) ->
     case ?config(with_all_oauth_provider_settings, Config) of
         true ->
-            case application:get_env(rabbitmq_auth_backend_oauth2, default_oauth_provider, undefined) of
+            case get_env(default_oauth_provider) of
                 undefined ->
-                    verify_get_oauth_provider_returns_oauth_provider_from_key_config();
+                    verify_get_oauth_provider_returns_root_oauth_provider();
                 DefaultOAuthProviderId ->
                     verify_get_oauth_provider_returns_default_oauth_provider(DefaultOAuthProviderId)
             end;
@@ -525,8 +565,7 @@ get_oauth_provider_given_oauth_provider_id(Config) ->
                     [issuer, token_endpoint, jwks_uri, authorization_endpoint,
                         end_session_endpoint]),
 
-            OAuthProviders = application:get_env(rabbitmq_auth_backend_oauth2,
-                oauth_providers, #{}),
+            OAuthProviders = get_env(oauth_providers, #{}),
             ExpectedProvider = maps:get(Id, OAuthProviders, []),
             ?assertEqual(proplists:get_value(issuer, ExpectedProvider),
                 Issuer),
@@ -564,6 +603,17 @@ get_oauth_provider_given_oauth_provider_id(Config) ->
                 Jwks_uri)
     end.
 
+jwks_url_is_used_in_absense_of_jwks_uri(Config) ->
+    {ok, #oauth_provider{
+        jwks_uri = Jwks_uri}} = oauth2_client:get_oauth_provider([jwks_uri]),                
+    ?assertEqual(
+        proplists:get_value(jwks_url, get_env(key_config, []), undefined), 
+        Jwks_uri).
+
+jwks_uri_takes_precedence_over_jwks_url(Config) ->
+    {ok, #oauth_provider{
+        jwks_uri = Jwks_uri}} = oauth2_client:get_oauth_provider([jwks_uri]),
+    ?assertEqual(get_env(jwks_uri), Jwks_uri).
 
 
 %%% HELPERS
@@ -584,10 +634,13 @@ build_token_endpoint_uri(Scheme) ->
                          path => "/token"}).
 
 build_jwks_uri(Scheme) ->
+    build_jwks_uri(Scheme, "/certs").
+
+build_jwks_uri(Scheme, Path) ->
     uri_string:recompose(#{scheme => Scheme,
                          host => "localhost",
                          port => rabbit_data_coercion:to_integer(?AUTH_PORT),
-                         path => "/certs"}).
+                         path => Path}).
 
 build_access_token_request(Request) ->
     #access_token_request {
@@ -623,11 +676,11 @@ oauth_provider_to_proplist(#oauth_provider{
         authorization_endpoint = AuthorizationEndpoint,
         ssl_options = SslOptions,
         jwks_uri = Jwks_uri}) ->
-    [ { issuer, Issuer},
+    [   {issuer, Issuer},
         {token_endpoint, TokenEndpoint},
         {end_session_endpoint, EndSessionEndpoint},
         {authorization_endpoint, AuthorizationEndpoint},
-        { https,
+        {https,
             case SslOptions of
                 undefined -> [];
                 Value -> Value
@@ -677,6 +730,11 @@ token(ExpiresIn) ->
     EncodedToken.
 
 
+get_env(Par) ->
+    application:get_env(rabbitmq_auth_backend_oauth2, Par, undefined).
+get_env(Par, Default) ->
+    application:get_env(rabbitmq_auth_backend_oauth2, Par, Default).
+
 
 build_http_mock_behaviour(Request, Response) ->
     #{request => Request, response => Response}.