Merge branch 'hans/ssh/retry_pwd_patch/OTP-13674' into maint-18

* hans/ssh/retry_pwd_patch/OTP-13674:
  ssh: update vsn.mk
  ssh: polishing of password prompt's linefeed
  ssh: Fix a hazard bug in ssh_auth
  ssh: Some code cuddling in ssh_io
  ssh: Fix type error in args of ssh_auth:sort_selected_mthds
  ssh: Make client send a faulty pwd only once, ssh_connection_handler part
  ssh: Make client send a faulty pwd only once, ssh_auth part
  ssh: test cases for no repetition of bad passwords

5 files changed, 264 insertions, 133 deletions

diff --git a/lib/ssh/src/ssh_auth.erl b/lib/ssh/src/ssh_auth.erl
index b71bed033a..0c378d084b 100644
--- a/lib/ssh/src/ssh_auth.erl
+++ b/lib/ssh/src/ssh_auth.erl
@@ -31,12 +31,111 @@
 -export([publickey_msg/1, password_msg/1, keyboard_interactive_msg/1,
 	 service_request_msg/1, init_userauth_request_msg/1,
 	 userauth_request_msg/1, handle_userauth_request/3,
-	 handle_userauth_info_request/3, handle_userauth_info_response/2
+	 handle_userauth_info_request/2, handle_userauth_info_response/2
 	]).
 
 %%--------------------------------------------------------------------
 %%% Internal application API
 %%--------------------------------------------------------------------
+%%%----------------------------------------------------------------
+userauth_request_msg(#ssh{userauth_methods = ServerMethods,
+			  userauth_supported_methods = UserPrefMethods, % Note: this is not documented as supported for clients
+			  userauth_preference = ClientMethods0
+			 } = Ssh0) ->
+    case sort_select_mthds(ClientMethods0, UserPrefMethods, ServerMethods) of
+	[] ->
+	    Msg = #ssh_msg_disconnect{code = ?SSH_DISCONNECT_NO_MORE_AUTH_METHODS_AVAILABLE,
+				      description = "Unable to connect using the available authentication methods",
+				      language = "en"},
+	    {disconnect, Msg, ssh_transport:ssh_packet(Msg, Ssh0)};
+	
+	[{Pref,Module,Function,Args} | Prefs] ->
+	    Ssh = case Pref of
+		      "keyboard-interactive" -> Ssh0;
+		      _ -> Ssh0#ssh{userauth_preference = Prefs}
+		  end,
+	    case Module:Function(Args ++ [Ssh]) of
+		{not_ok, Ssh1} ->
+		    userauth_request_msg(Ssh1#ssh{userauth_preference = Prefs});
+		Result ->
+		    {Pref,Result}
+	    end
+    end.
+
+
+
+sort_select_mthds(Clients, undefined, Servers) ->
+    %% User has not expressed an opinion via option "auth_methods", use the server's prefs
+    sort_select_mthds1(Clients, Servers, string:tokens(?SUPPORTED_AUTH_METHODS,","));
+
+sort_select_mthds(Clients, Users0, Servers0) ->
+    %% The User has an opinion, use the intersection of that and the Servers whishes but
+    %% in the Users order
+    sort_select_mthds1(Clients, string:tokens(Users0,","), Servers0).
+
+
+sort_select_mthds1(Clients, Users0, Servers0) ->
+    Servers = unique(Servers0),
+    Users = unique(Users0),
+    [C || Key <- Users,
+	  lists:member(Key, Servers),
+	  C <- Clients,
+	  element(1,C) == Key].
+
+unique(L) -> 
+    lists:reverse(
+      lists:foldl(fun(E,Acc) -> 
+			  case lists:member(E,Acc) of
+			      true -> Acc;
+			      false -> [E|Acc]
+			  end
+		  end, [], L)).
+    
+
+%%%---- userauth_request_msg "callbacks"
+password_msg([#ssh{opts = Opts, io_cb = IoCb,
+		   user = User, service = Service} = Ssh0]) ->
+    {Password,Ssh} = 
+	case proplists:get_value(password, Opts) of
+	    undefined when IoCb == ssh_no_io ->
+		{not_ok, Ssh0};
+	    undefined -> 
+		{IoCb:read_password("ssh password: ",Ssh0), Ssh0};
+	    PW ->
+		%% If "password" option is given it should not be tried again
+		{PW, Ssh0#ssh{opts = lists:keyreplace(password,1,Opts,{password,not_ok})}}
+	end,
+    case Password of
+	not_ok ->
+	    {not_ok, Ssh};
+	_  ->
+	    ssh_transport:ssh_packet(
+	      #ssh_msg_userauth_request{user = User,
+					service = Service,
+					method = "password",
+					data =
+					    <<?BOOLEAN(?FALSE),
+					      ?STRING(unicode:characters_to_binary(Password))>>},
+	      Ssh)
+    end.
+
+%% See RFC 4256 for info on keyboard-interactive
+keyboard_interactive_msg([#ssh{user = User,
+			       opts = Opts,
+			       service = Service} = Ssh]) ->
+    case proplists:get_value(password, Opts) of
+	not_ok ->
+	    {not_ok,Ssh};       % No need to use a failed pwd once more
+	_ ->
+	    ssh_transport:ssh_packet(
+	      #ssh_msg_userauth_request{user = User,
+					service = Service,
+					method = "keyboard-interactive",
+					data = << ?STRING(<<"">>),
+						  ?STRING(<<>>) >> },
+	      Ssh)
+    end.
+
 publickey_msg([Alg, #ssh{user = User,
 		       session_id = SessionId,
 		       service = Service,
@@ -48,7 +147,7 @@ publickey_msg([Alg, #ssh{user = User,
 	    StrAlgo = atom_to_list(Alg),
             case encode_public_key(StrAlgo, ssh_transport:extract_public_key(PrivKey)) of
 		not_ok ->
-		    not_ok;
+		    {not_ok, Ssh};
 		PubKeyBlob ->
 		    SigData = build_sig_data(SessionId, 
 					     User, Service, PubKeyBlob, StrAlgo),
@@ -65,52 +164,15 @@ publickey_msg([Alg, #ssh{user = User,
 		      Ssh)
 	    end;
      	_Error ->
-	    not_ok
-    end.
-
-password_msg([#ssh{opts = Opts, io_cb = IoCb,
-		   user = User, service = Service} = Ssh]) ->
-    Password = case proplists:get_value(password, Opts) of
-		   undefined -> 
-		       user_interaction(IoCb, Ssh);
-		   PW -> 
-		       PW
-	       end,
-    case Password of
-	not_ok ->
-	    not_ok;
-	_  ->
-	    ssh_transport:ssh_packet(
-	      #ssh_msg_userauth_request{user = User,
-					service = Service,
-					method = "password",
-					data =
-					    <<?BOOLEAN(?FALSE),
-					      ?STRING(unicode:characters_to_binary(Password))>>},
-	      Ssh)
+	    {not_ok, Ssh}
     end.
 
-user_interaction(ssh_no_io, _) ->
-    not_ok;
-user_interaction(IoCb, Ssh) ->
-    IoCb:read_password("ssh password: ", Ssh).
-
-
-%% See RFC 4256 for info on keyboard-interactive
-keyboard_interactive_msg([#ssh{user = User,
-			       service = Service} = Ssh]) ->
-    ssh_transport:ssh_packet(
-      #ssh_msg_userauth_request{user = User,
-				service = Service,
-				method = "keyboard-interactive",
-				data = << ?STRING(<<"">>),
-					  ?STRING(<<>>) >> },
-      Ssh).
-
+%%%----------------------------------------------------------------
 service_request_msg(Ssh) ->
     ssh_transport:ssh_packet(#ssh_msg_service_request{name = "ssh-userauth"},
 			   Ssh#ssh{service = "ssh-userauth"}).
 
+%%%----------------------------------------------------------------
 init_userauth_request_msg(#ssh{opts = Opts} = Ssh) ->
     case user_name(Opts) of
 	{ok, User} ->
@@ -140,34 +202,9 @@ init_userauth_request_msg(#ssh{opts = Opts} = Ssh) ->
 				      language = "en"})
     end.
 
-userauth_request_msg(#ssh{userauth_preference = []} = Ssh) ->    
-    Msg = #ssh_msg_disconnect{code = 
-			      ?SSH_DISCONNECT_NO_MORE_AUTH_METHODS_AVAILABLE,
-			      description = "Unable to connect using the available"
-			      " authentication methods",
-			      language = "en"},
-    {disconnect, Msg, ssh_transport:ssh_packet(Msg, Ssh)};
-
-userauth_request_msg(#ssh{userauth_methods = Methods, 
-			  userauth_preference = [{Pref, Module,
-					      Function, Args} | Prefs]} 
-		     = Ssh0) ->
-    Ssh = Ssh0#ssh{userauth_preference = Prefs},
-    case lists:member(Pref, Methods) of
-	true ->
-	    case Module:Function(Args ++ [Ssh]) of
-		not_ok ->
-		    userauth_request_msg(Ssh);
-		Result ->
-		    {Pref,Result}
-	    end;
-	false ->
-	    userauth_request_msg(Ssh)
-    end.
-
-
-handle_userauth_request(#ssh_msg_service_request{name = 
-						 Name = "ssh-userauth"},
+%%%----------------------------------------------------------------
+%%% called by server
+handle_userauth_request(#ssh_msg_service_request{name = Name = "ssh-userauth"},
 			_, Ssh) ->
     {ok, ssh_transport:ssh_packet(#ssh_msg_service_accept{name = Name},
 				  Ssh#ssh{service = "ssh-connection"})};
@@ -319,21 +356,28 @@ handle_userauth_request(#ssh_msg_userauth_request{user = User,
 				 partial_success = false}, Ssh)}.
 
 
-
-handle_userauth_info_request(
-  #ssh_msg_userauth_info_request{name = Name,
-				 instruction = Instr,
-				 num_prompts = NumPrompts,
-				 data  = Data}, IoCb, 
-  #ssh{opts = Opts} = Ssh) ->
+%%%----------------------------------------------------------------
+%%% keyboard-interactive client
+handle_userauth_info_request(#ssh_msg_userauth_info_request{name = Name,
+							    instruction = Instr,
+							    num_prompts = NumPrompts,
+							    data  = Data},
+			     #ssh{opts = Opts,
+				  io_cb = IoCb
+				 } = Ssh) ->
     PromptInfos = decode_keyboard_interactive_prompts(NumPrompts,Data),
-    Responses = keyboard_interact_get_responses(IoCb, Opts,
-					    Name, Instr, PromptInfos),
-    {ok, 
-     ssh_transport:ssh_packet(
-       #ssh_msg_userauth_info_response{num_responses = NumPrompts,
-				       data = Responses}, Ssh)}.
+    case keyboard_interact_get_responses(IoCb, Opts, Name, Instr, PromptInfos) of
+	not_ok ->
+	    not_ok;
+	Responses ->
+	    {ok, 
+	     ssh_transport:ssh_packet(
+	       #ssh_msg_userauth_info_response{num_responses = NumPrompts,
+					       data = Responses}, Ssh)}
+    end.
 
+%%%----------------------------------------------------------------
+%%% keyboard-interactive server
 handle_userauth_info_response(#ssh_msg_userauth_info_response{num_responses = 1,
 							      data = <<?UINT32(Sz), Password:Sz/binary>>},
 			      #ssh{opts = Opts,
@@ -369,11 +413,6 @@ method_preference(Algs) ->
 		       [{"publickey", ?MODULE, publickey_msg, [A]} | Acc]
 	       end, 
 	       [{"password", ?MODULE, password_msg, []},
-		{"keyboard-interactive", ?MODULE, keyboard_interactive_msg, []},
-		{"keyboard-interactive", ?MODULE, keyboard_interactive_msg, []},
-		{"keyboard-interactive", ?MODULE, keyboard_interactive_msg, []},
-		{"keyboard-interactive", ?MODULE, keyboard_interactive_msg, []},
-		{"keyboard-interactive", ?MODULE, keyboard_interactive_msg, []},
 		{"keyboard-interactive", ?MODULE, keyboard_interactive_msg, []}
 	       ],
 	       Algs).
@@ -473,6 +512,9 @@ keyboard_interact_get_responses(IoCb, Opts, Name, Instr, PromptInfos) ->
 				    proplists:get_value(password, Opts, undefined), IoCb, Name,
 				    Instr, PromptInfos, Opts, NumPrompts).
 
+
+keyboard_interact_get_responses(_, _, not_ok, _, _, _, _, _, _) ->
+    not_ok;
 keyboard_interact_get_responses(_, undefined, Password, _, _, _, _, _,
 				1) when Password =/= undefined ->
     [Password]; %% Password auth implemented with keyboard-interaction and passwd is known
@@ -486,17 +528,18 @@ keyboard_interact_get_responses(true, Fun, _Pwd, _IoCb, Name, Instr, PromptInfos
     keyboard_interact_fun(Fun, Name, Instr, PromptInfos, NumPrompts).
 
 keyboard_interact(IoCb, Name, Instr, Prompts, Opts) ->
-    if Name /= "" -> IoCb:format("~s~n", [Name]);
-       true       -> ok
-    end,
-    if Instr /= "" -> IoCb:format("~s~n", [Instr]);
-       true        -> ok
-    end,
+    write_if_nonempty(IoCb, Name),
+    write_if_nonempty(IoCb, Instr),
     lists:map(fun({Prompt, true})  -> IoCb:read_line(Prompt, Opts);
 		 ({Prompt, false}) -> IoCb:read_password(Prompt, Opts)
 	      end,
 	      Prompts).
 
+write_if_nonempty(_, "") -> ok;
+write_if_nonempty(_, <<>>) -> ok;
+write_if_nonempty(IoCb, Text) -> IoCb:format("~s~n",[Text]).
+
+
 keyboard_interact_fun(KbdInteractFun, Name, Instr,  PromptInfos, NumPrompts) ->
     Prompts = lists:map(fun({Prompt, _Echo}) -> Prompt end,
 			PromptInfos),
diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl
index ce1931e4f4..b73f8b23d2 100644
--- a/lib/ssh/src/ssh_connection_handler.erl
+++ b/lib/ssh/src/ssh_connection_handler.erl
@@ -612,11 +612,14 @@ userauth(#ssh_msg_userauth_banner{message = Msg},
 
 
 userauth_keyboard_interactive(#ssh_msg_userauth_info_request{} = Msg, 
-			      #state{ssh_params = #ssh{role = client, 
-						       io_cb = IoCb} = Ssh0} = State) ->
-    {ok, {Reply, Ssh}} = ssh_auth:handle_userauth_info_request(Msg, IoCb, Ssh0),
-    send_msg(Reply, State),
-    {next_state, userauth_keyboard_interactive_info_response, next_packet(State#state{ssh_params = Ssh})};
+			      #state{ssh_params = #ssh{role = client} = Ssh0} = State) ->
+    case ssh_auth:handle_userauth_info_request(Msg, Ssh0) of
+	{ok, {Reply, Ssh}} ->
+	    send_msg(Reply, State),
+	    {next_state, userauth_keyboard_interactive_info_response, next_packet(State#state{ssh_params = Ssh})};
+	not_ok ->
+	    userauth(Msg, State)
+    end;
 
 userauth_keyboard_interactive(#ssh_msg_userauth_info_response{} = Msg, 
 			      #state{ssh_params = #ssh{role = server,
@@ -646,7 +649,18 @@ userauth_keyboard_interactive(Msg = #ssh_msg_userauth_failure{},
 
 
 userauth_keyboard_interactive_info_response(Msg=#ssh_msg_userauth_failure{},
-					    #state{ssh_params = #ssh{role = client}} = State) ->
+					    #state{ssh_params = #ssh{role = client,
+								     opts = Opts} = Ssh0} = State0) ->
+
+    State = case proplists:get_value(password, Opts) of
+		undefined ->
+		    State0;
+		_ ->
+		    State0#state{ssh_params =
+				     Ssh0#ssh{opts =
+						  lists:keyreplace(password,1,Opts,
+								   {password,not_ok})}}
+	    end,
     userauth(Msg, State);
 userauth_keyboard_interactive_info_response(Msg=#ssh_msg_userauth_success{},
 					    #state{ssh_params = #ssh{role = client}} = State) ->
@@ -1247,7 +1261,7 @@ init_ssh(client = Role, Vsn, Version, Options, Socket) ->
 	   end,
 
     AuthMethods = proplists:get_value(auth_methods, Options, 
-				      ?SUPPORTED_AUTH_METHODS),
+				      undefined),
     {ok, PeerAddr} = inet:peername(Socket),
     
     PeerName =  proplists:get_value(host, Options),
diff --git a/lib/ssh/src/ssh_io.erl b/lib/ssh/src/ssh_io.erl
index a5e627fdb3..5e335c2063 100644
--- a/lib/ssh/src/ssh_io.erl
+++ b/lib/ssh/src/ssh_io.erl
@@ -31,56 +31,55 @@ read_line(Prompt, Ssh) ->
     format("~s", [listify(Prompt)]),
     proplists:get_value(user_pid, Ssh) ! {self(), question},
     receive
-	Answer ->
+	Answer when is_list(Answer) ->
 	    Answer
     end.
 
 yes_no(Prompt, Ssh) ->
-    io:format("~s [y/n]?", [Prompt]),
+    format("~s [y/n]?", [Prompt]),
     proplists:get_value(user_pid, Ssh#ssh.opts) ! {self(), question},
     receive
-	Answer ->
+	%% I can't see that the atoms y and n are ever received, but it must
+	%% be investigated before removing
+	y -> yes;
+	n -> no;
+
+	Answer when is_list(Answer) ->
 	    case trim(Answer) of
 		"y" -> yes;
 		"n" -> no;
 		"Y" -> yes;
 		"N" -> no;
-		y -> yes;
-		n -> no;
 		_ ->
-		    io:format("please answer y or n\n"),
+		    format("please answer y or n\n",[]),
 		    yes_no(Prompt, Ssh)
 	    end
     end.
 
 
-read_password(Prompt, Ssh) ->
+read_password(Prompt, #ssh{opts=Opts}) -> read_password(Prompt, Opts);
+read_password(Prompt, Opts) when is_list(Opts) ->
     format("~s", [listify(Prompt)]),
-    case is_list(Ssh) of
-	false ->
-	    proplists:get_value(user_pid, Ssh#ssh.opts) ! {self(), user_password};
-	_ ->
-	    proplists:get_value(user_pid, Ssh) ! {self(), user_password}
-    end,
+    proplists:get_value(user_pid, Opts) ! {self(), user_password},
     receive
-	Answer ->
-	    case Answer of
-		"" ->
-		    read_password(Prompt, Ssh);
-		Pass -> Pass
-	    end
+	Answer when is_list(Answer) ->
+	     case trim(Answer) of
+		 "" ->
+		     read_password(Prompt, Opts);
+		 Pwd ->
+		     Pwd
+	     end
     end.
 
-listify(A) when is_atom(A) ->
-    atom_to_list(A);
-listify(L) when is_list(L) ->
-    L;
-listify(B) when is_binary(B)  ->
-    binary_to_list(B).
 
 format(Fmt, Args) ->
     io:format(Fmt, Args).
 
+%%%================================================================
+listify(A) when is_atom(A)   -> atom_to_list(A);
+listify(L) when is_list(L)   -> L;
+listify(B) when is_binary(B) -> binary_to_list(B).
+
 
 trim(Line) when is_list(Line) ->
     lists:reverse(trim1(lists:reverse(trim1(Line))));
@@ -93,6 +92,3 @@ trim1([$\r|Cs]) -> trim(Cs);
 trim1([$\n|Cs]) -> trim(Cs);
 trim1([$\t|Cs]) -> trim(Cs);
 trim1(Cs) -> Cs.
-    
-
-
diff --git a/lib/ssh/test/ssh_basic_SUITE.erl b/lib/ssh/test/ssh_basic_SUITE.erl
index 094d28e879..96d424dc98 100644
--- a/lib/ssh/test/ssh_basic_SUITE.erl
+++ b/lib/ssh/test/ssh_basic_SUITE.erl
@@ -49,7 +49,12 @@
 	 inet6_option/1,
 	 inet_option/1,
 	 internal_error/1,
-	 known_hosts/1,  
+	 known_hosts/1,
+	 login_bad_pwd_no_retry1/1,
+	 login_bad_pwd_no_retry2/1,
+	 login_bad_pwd_no_retry3/1,
+	 login_bad_pwd_no_retry4/1,
+	 login_bad_pwd_no_retry5/1,
 	 misc_ssh_options/1,
 	 openssh_zlib_basic_test/1,  
 	 packet_size_zero/1, 
@@ -99,7 +104,8 @@ all() ->
      daemon_opt_fd,
      multi_daemon_opt_fd,
      packet_size_zero,
-     ssh_info_print
+     ssh_info_print,
+     {group, login_bad_pwd_no_retry}
     ].
 
 groups() ->
@@ -115,7 +121,13 @@ groups() ->
      {dsa_pass_key, [], [pass_phrase]},
      {rsa_pass_key, [], [pass_phrase]},
      {key_cb, [], [key_callback, key_callback_options]},
-     {internal_error, [], [internal_error]}
+     {internal_error, [], [internal_error]},
+     {login_bad_pwd_no_retry, [], [login_bad_pwd_no_retry1,
+				   login_bad_pwd_no_retry2,
+				   login_bad_pwd_no_retry3,
+				   login_bad_pwd_no_retry4,
+				   login_bad_pwd_no_retry5
+				  ]}
     ].
 
 
@@ -1089,6 +1101,72 @@ ssh_info_print(Config) ->
 
 
 %%--------------------------------------------------------------------
+%% Check that a basd pwd is not tried more times. Could cause lock-out
+%% on server
+
+login_bad_pwd_no_retry1(Config) ->
+    login_bad_pwd_no_retry(Config, "keyboard-interactive,password").
+
+login_bad_pwd_no_retry2(Config) ->
+    login_bad_pwd_no_retry(Config, "password,keyboard-interactive").
+
+login_bad_pwd_no_retry3(Config) ->
+    login_bad_pwd_no_retry(Config, "password,publickey,keyboard-interactive").
+
+login_bad_pwd_no_retry4(Config) ->
+    login_bad_pwd_no_retry(Config, "password,other,keyboard-interactive").
+
+login_bad_pwd_no_retry5(Config) ->
+    login_bad_pwd_no_retry(Config, "password,other,keyboard-interactive,password,password").
+
+
+
+
+
+login_bad_pwd_no_retry(Config, AuthMethods) ->
+    PrivDir = proplists:get_value(priv_dir, Config),
+    UserDir = filename:join(PrivDir, nopubkey), % to make sure we don't use public-key-auth
+    file:make_dir(UserDir),
+    SysDir = proplists:get_value(data_dir, Config),
+
+    Parent = self(),
+    PwdFun = fun(_, _, _, undefined) -> {false, 1};
+		(_, _, _,         _) -> Parent ! retry_bad_pwd,
+					false
+	     end,
+
+    {DaemonRef, _Host, Port} = 
+	ssh_test_lib:daemon([{system_dir, SysDir},
+			     {user_dir, UserDir},
+			     {auth_methods, AuthMethods},
+			     {user_passwords, [{"foo","somepwd"}]},
+			     {pwdfun, PwdFun}
+			    ]),
+
+    ConnRes = ssh:connect("localhost", Port,
+			  [{silently_accept_hosts, true},
+			   {user, "foo"},
+			   {password, "badpwd"},
+			   {user_dir, UserDir},
+			   {user_interaction, false}]),
+
+    receive
+	retry_bad_pwd ->
+	    ssh:stop_daemon(DaemonRef),
+	    {fail, "Retry bad password"}
+    after 0 ->
+	    case ConnRes of
+		{error,"Unable to connect using the available authentication methods"} ->
+		    ssh:stop_daemon(DaemonRef),
+		    ok;
+		{ok,Conn} ->
+		    ssh:close(Conn),
+		    ssh:stop_daemon(DaemonRef),
+		    {fail, "Connect erroneosly succeded"}
+	    end
+    end.
+
+%%--------------------------------------------------------------------
 %% Internal functions ------------------------------------------------
 %%--------------------------------------------------------------------
 %% Due to timing the error message may or may not be delivered to
diff --git a/lib/ssh/vsn.mk b/lib/ssh/vsn.mk
index 41b42d454b..dc5e7092f8 100644
--- a/lib/ssh/vsn.mk
+++ b/lib/ssh/vsn.mk
@@ -1,5 +1,5 @@
 #-*-makefile-*-   ; force emacs to enter makefile-mode
 
-SSH_VSN = 4.2.2
+SSH_VSN = 4.2.2.1
 
 APP_VSN    = "ssh-$(SSH_VSN)"