Merge pull request rabbitmq#1466 from rabbitmq/rabbitmq-server-story-153435857

Internal authN backend: make it impossible to successfully log in with a blank password (for 3.6.x)

Author: michaelklishin

src/rabbit_auth_backend_internal.erl

@@ -93,6 +93,11 @@ hashing_module_for_user(#internal_user{
     hashing_algorithm = ModOrUndefined}) ->
         rabbit_password:hashing_mod(ModOrUndefined).
 
+-define(BLANK_PASSWORD_REJECTION_MESSAGE,
+        "user '~s' attempted to log in with a blank password, which is prohibited by the internal authN backend. "
+        "To use TLS/x509 certificate-based authentication, see the rabbitmq_auth_mechanism_ssl plugin and configure the client to use the EXTERNAL authentication mechanism. "
+        "Alternatively change the password for the user to be non-blank.").
+
 %% For cases when we do not have a set of credentials,
 %% namely when x509 (TLS) certificates are used. This should only be
 %% possible when the EXTERNAL authentication mechanism is used, see
@@ -103,6 +108,14 @@ user_login_authentication(Username, []) ->
 %% performs initial validation.
 user_login_authentication(Username, AuthProps) ->
     case lists:keyfind(password, 1, AuthProps) of
+        %% Passwordless users are not supposed to be used with
+        %% this backend (and PLAIN authentication mechanism in general).
+        {password, <<"">>} ->
+            {refused, ?BLANK_PASSWORD_REJECTION_MESSAGE,
+             [Username]};
+        {password, ""} ->
+            {refused, ?BLANK_PASSWORD_REJECTION_MESSAGE,
+             [Username]};
         {password, Cleartext} ->
             internal_check_user_login(
               Username,
@@ -111,6 +124,13 @@ user_login_authentication(Username, AuthProps) ->
                     } = U) ->
                   Hash =:= rabbit_password:salted_hash(
                       hashing_module_for_user(U), Salt, Cleartext);
+                  %% rabbitmqctl clear_password will set password_hash to an empty
+                  %% binary.
+                  %% 
+                  %% See the comment on passwordless users above.
+                  (#internal_user{
+                        password_hash = <<"">>}) ->
+                      false;
                   (#internal_user{}) ->
                       false
               end);

src/rabbit_auth_mechanism_plain.erl

@@ -28,12 +28,6 @@
                     {requires,    rabbit_registry},
                     {enables,     kernel_ready}]}).
 
-%% SASL PLAIN, as used by the Qpid Java client and our clients. Also,
-%% apparently, by OpenAMQ.
-
-%% TODO: reimplement this using the binary module? - that makes use of
-%% BIFs to do binary matching and will thus be much faster.
-
 description() ->
     [{description, <<"SASL PLAIN authentication mechanism">>}].
 

test/unit_inbroker_parallel_SUITE.erl

@@ -19,6 +19,7 @@
 -include_lib("common_test/include/ct.hrl").
 -include_lib("kernel/include/file.hrl").
 -include_lib("amqp_client/include/amqp_client.hrl").
+-include_lib("eunit/include/eunit.hrl").
 
 -compile(export_all).
 
@@ -47,6 +48,10 @@ groups() ->
               password_hashing,
               change_password
             ]},
+          {auth_backend_internal, [parallel], [
+              login_with_credentials_but_no_password,
+              login_of_passwordless_user
+            ]},
           {policy_validation, [parallel, {repeat, 20}], [
               ha_policy_validation,
               policy_validation,
@@ -684,9 +689,12 @@ user_management1(_Config) ->
     %% user authentication
     ok = control_action(authenticate_user,
       ["user_management-user", "user_management-newpassword"]),
-    {refused, _User, _Format, _Params} =
+    ?assertMatch({refused, _User, _Format, _Params},
         control_action(authenticate_user,
-          ["user_management-user", "user_management-password"]),
+          ["user_management-user", "user_management-password"])),
+    ?assertMatch({refused, _User, _Format, _Params},
+        control_action(authenticate_user,
+          ["user_management-user", ""])),
 
     %% vhost creation
     ok = control_action(add_vhost,
@@ -748,6 +756,60 @@ user_management1(_Config) ->
 
     passed.
 
+
+%% -------------------------------------------------------------------
+%% rabbit_auth_backend_internal
+%% -------------------------------------------------------------------
+
+login_with_credentials_but_no_password(Config) ->
+    passed = rabbit_ct_broker_helpers:rpc(Config, 0,
+      ?MODULE, login_with_credentials_but_no_password1, [Config]).
+
+login_with_credentials_but_no_password1(_Config) ->
+    Username = "login_with_credentials_but_no_password-user",
+    Password = "login_with_credentials_but_no_password-password",
+    ok = control_action(add_user, [Username, Password]),
+
+    try
+        rabbit_auth_backend_internal:user_login_authentication(Username,
+                                                              [{key, <<"value">>}]),
+        ?assert(false)
+    catch exit:{unknown_auth_props, Username, [{key, <<"value">>}]} ->
+            ok
+    end,
+
+    ok = control_action(delete_user,
+      [Username]),
+
+    passed.
+
+%% passwordless users are not supposed to be used with
+%% this backend (and PLAIN authentication mechanism in general)
+login_of_passwordless_user(Config) ->
+    passed = rabbit_ct_broker_helpers:rpc(Config, 0,
+      ?MODULE, login_of_passwordless_user1, [Config]).
+
+login_of_passwordless_user1(_Config) ->
+    Username = "login_of_passwordless_user-user",
+    Password = "",
+    ok = control_action(add_user, [Username, Password]),
+
+    ?assertMatch(
+       {refused, _Message, [Username]},
+       rabbit_auth_backend_internal:user_login_authentication(Username,
+                                                              [{password, <<"">>}])),
+
+    ?assertMatch(
+       {refused, _Format, [Username]},
+       rabbit_auth_backend_internal:user_login_authentication(Username,
+                                                              [{password, ""}])),
+
+    ok = control_action(delete_user,
+      [Username]),
+
+    passed.
+
+
 runtime_parameters(Config) ->
     passed = rabbit_ct_broker_helpers:rpc(Config, 0,
       ?MODULE, runtime_parameters1, [Config]).