Merge branch 'hans/ssh/pref_public_key_algs/OTP-13158' into maint

* hans/ssh/pref_public_key_algs/OTP-13158:
  ssh: tests skips if not supported crypto
  ssh: ssh_auth checks support for user pubkey alg
  ssh: client pub key opt implemented
  ssh: client pub key testcase
  ssh: client pub key documentation

7 files changed, 190 insertions, 62 deletions

diff --git a/lib/ssh/doc/src/ssh.xml b/lib/ssh/doc/src/ssh.xml
index 18bced2d1d..b3f850fc38 100644
--- a/lib/ssh/doc/src/ssh.xml
+++ b/lib/ssh/doc/src/ssh.xml
@@ -206,26 +206,25 @@
 	  <tag><c><![CDATA[{public_key_alg, 'ssh-rsa' | 'ssh-dss'}]]></c></tag>
 	  <item>
 	    <note>
-	      <p>This option is kept for compatibility. It is ignored if the <c>preferred_algorithms</c>
-	      option is used. The equivalence of <c>{public_key_alg,'ssh-dss'}</c> is 
-	      <c>{preferred_algorithms, [{public_key,['ssh-dss','ssh-rsa']}]}</c>.</p>
+	      <p>This option will be removed in OTP 20, but is kept for compatibility. It is ignored if
+	      the preferred <c>pref_public_key_algs</c> option is used.</p>
 	    </note>
             <p>Sets the preferred public key algorithm to use for user
 	    authentication. If the preferred algorithm fails,
-	    the other algorithm is tried. The default is
-	    to try <c><![CDATA['ssh-rsa']]></c> first.</p>
+	    the other algorithm is tried. If <c>{public_key_alg, 'ssh-rsa'}</c> is set, it is translated
+	    to <c>{pref_public_key_algs, ['ssh-rsa','ssh-dss']}</c>.  If it is 
+	    <c>{public_key_alg, 'ssh-dss'}</c>, it is translated
+	    to <c>{pref_public_key_algs, ['ssh-dss','ssh-rsa']}</c>.
+	    </p>
 	  </item>
 
 	  <tag><c><![CDATA[{pref_public_key_algs, list()}]]></c></tag>
 	  <item>
-	    <note>
-	      <p>This option is kept for compatibility. It is ignored if the <c>preferred_algorithms</c>
-	      option is used. The equivalence of <c>{pref_public_key_algs,['ssh-dss']}</c> is 
-	      <c>{preferred_algorithms, [{public_key,['ssh-dss']}]}</c>.</p>
-	    </note>
-            <p>List of public key algorithms to try to use.
-	    <c>'ssh-rsa'</c> and <c>'ssh-dss'</c> are available.
-	    Overrides <c><![CDATA[{public_key_alg, 'ssh-rsa' | 'ssh-dss'}]]></c></p>
+            <p>List of user (client) public key algorithms to try to use.</p>
+	    <p>The default value is 
+	    <c><![CDATA[['ssh-rsa','ssh-dss','ecdsa-sha2-nistp256','ecdsa-sha2-nistp384','ecdsa-sha2-nistp521'] ]]></c>
+	    </p>
+	    <p>If there is no public key of a specified type available, the corresponding entry is ignored.</p>
 	  </item>
 
 	  <tag><c><![CDATA[{preferred_algorithms, algs_list()}]]></c></tag>
@@ -233,6 +232,7 @@
             <p>List of algorithms to use in the algorithm negotiation. The default <c>algs_list()</c> can
 	    be obtained from <seealso marker="#default_algorithms/0">default_algorithms/0</seealso>.
 	    </p>
+	    <p>If an alg_entry() is missing in the algs_list(), the default value is used for that entry.</p>
 	    <p>Here is an example of this option:</p>
 	<code>
 {preferred_algorithms, 
@@ -243,9 +243,9 @@
   {compression,[none,zlib]}
 }
 </code>
-        <p>The example specifies different algorithms in the two directions (client2server and server2client), for cipher but specifies the same
-algorithms for mac and compression in both directions. The kex (key exchange) and public key algorithms are set to their default values,
-kex is implicit but public_key is set explicitly.</p>
+        <p>The example specifies different algorithms in the two directions (client2server and server2client),
+	for cipher but specifies the same algorithms for mac and compression in both directions.
+	The kex (key exchange) is implicit but public_key is set explicitly.</p>
 
         <warning>
 	  <p>Changing the values can make a connection less secure. Do not change unless you
@@ -451,6 +451,7 @@ kex is implicit but public_key is set explicitly.</p>
             <p>List of algorithms to use in the algorithm negotiation. The default <c>algs_list()</c> can
 	    be obtained from <seealso marker="#default_algorithms/0">default_algorithms/0</seealso>.
 	    </p>
+	    <p>If an alg_entry() is missing in the algs_list(), the default value is used for that entry.</p>
 	    <p>Here is an example of this option:</p>
 	<code>
 {preferred_algorithms, 
@@ -461,9 +462,9 @@ kex is implicit but public_key is set explicitly.</p>
   {compression,[none,zlib]}
 }
 </code>
-        <p>The example specifies different algorithms in the two directions (client2server and server2client), for cipher but specifies the same
-algorithms for mac and compression in both directions. The kex (key exchange) and public key algorithms are set to their default values,
-kex is implicit but public_key is set explicitly.</p>
+        <p>The example specifies different algorithms in the two directions (client2server and server2client),
+	for cipher but specifies the same algorithms for mac and compression in both directions.
+	The kex (key exchange) is implicit but public_key is set explicitly.</p>
 
         <warning>
 	  <p>Changing the values can make a connection less secure. Do not change unless you
diff --git a/lib/ssh/src/ssh.erl b/lib/ssh/src/ssh.erl
index 1d29c95229..54f94acbdc 100644
--- a/lib/ssh/src/ssh.erl
+++ b/lib/ssh/src/ssh.erl
@@ -297,13 +297,6 @@ find_hostport(Fd) ->
     ok = inet:close(S),
     HostPort.
     
-%% find_port(Fd) ->
-%%     %% Hack....
-%%     {ok,TmpSock} = gen_tcp:listen(0,[{fd,Fd}]),
-%%     {ok, {_,ThePort}} = inet:sockname(TmpSock),
-%%     gen_tcp:close(TmpSock),
-%%     ThePort.
-
 
 handle_options(Opts) ->
     try handle_option(algs_compatibility(proplists:unfold(Opts)), [], []) of
@@ -315,32 +308,27 @@ handle_options(Opts) ->
     end.
 
 
-algs_compatibility(Os) ->
+algs_compatibility(Os0) ->
     %% Take care of old options 'public_key_alg' and 'pref_public_key_algs'
-    comp_pk(proplists:get_value(preferred_algorithms,Os),
-	    proplists:get_value(pref_public_key_algs,Os),
-	    proplists:get_value(public_key_alg, Os),
-	    [{K,V} || {K,V} <- Os,
-		      K =/= public_key_alg,
-		      K =/= pref_public_key_algs]
-	   ).
-
-comp_pk(undefined, undefined, undefined, Os) -> Os;
-comp_pk( PrefAlgs,         _,         _, Os) when PrefAlgs =/= undefined -> Os;
-
-comp_pk(undefined, undefined,   ssh_dsa, Os) -> comp_pk(undefined, undefined, 'ssh-dss', Os);
-comp_pk(undefined, undefined,   ssh_rsa, Os) -> comp_pk(undefined, undefined, 'ssh-rsa', Os);
-comp_pk(undefined, undefined,        PK, Os) -> 
-    PKs = [PK | ssh_transport:supported_algorithms(public_key)--[PK]],
-    [{preferred_algorithms, [{public_key,PKs}] } | Os];
-
-comp_pk(undefined, PrefPKs, _, Os) when PrefPKs =/= undefined ->
-    PKs = [case PK of
-	       ssh_dsa -> 'ssh-dss';
-	       ssh_rsa -> 'ssh-rsa';
-	       _ -> PK
-	   end || PK <- PrefPKs],
-    [{preferred_algorithms, [{public_key,PKs}]} | Os].
+    case proplists:get_value(public_key_alg, Os0) of
+	undefined -> 
+	    Os0;
+	A when is_atom(A) -> 
+	    %% Skip public_key_alg if pref_public_key_algs is defined:
+	    Os = lists:keydelete(public_key_alg, 1, Os0),
+	    case proplists:get_value(pref_public_key_algs,Os) of
+		undefined when A == 'ssh-rsa' ; A==ssh_rsa ->
+		    [{pref_public_key_algs,['ssh-rsa','ssh-dss']} | Os];
+		undefined when A == 'ssh-dss' ; A==ssh_dsa ->
+		    [{pref_public_key_algs,['ssh-dss','ssh-rsa']} | Os];
+		undefined ->
+		    throw({error, {eoptions, {public_key_alg,A} }});
+		_ -> 
+		    Os
+	    end;
+	V ->
+	    throw({error, {eoptions, {public_key_alg,V} }})
+    end.
 
 
 handle_option([], SocketOptions, SshOptions) ->
@@ -411,6 +399,8 @@ handle_option([{auth_methods, _} = Opt | Rest], SocketOptions, SshOptions) ->
     handle_option(Rest, SocketOptions, [handle_ssh_option(Opt) | SshOptions]);
 handle_option([{auth_method_kb_interactive_data, _} = Opt | Rest], SocketOptions, SshOptions) ->
     handle_option(Rest, SocketOptions, [handle_ssh_option(Opt) | SshOptions]);
+handle_option([{pref_public_key_algs, _} = Opt | Rest], SocketOptions, SshOptions) ->
+    handle_option(Rest, SocketOptions, [handle_ssh_option(Opt) | SshOptions]);
 handle_option([{preferred_algorithms,_} = Opt | Rest], SocketOptions, SshOptions) ->
     handle_option(Rest, SocketOptions, [handle_ssh_option(Opt) | SshOptions]);
 handle_option([{dh_gex_groups,_} = Opt | Rest], SocketOptions, SshOptions) ->
@@ -522,6 +512,13 @@ handle_ssh_option({dh_gex_limits,{Min,I,Max}} = Opt) when is_integer(Min), Min>0
 							  is_integer(Max), Max>=I ->
     %% Client
     Opt;
+handle_ssh_option({pref_public_key_algs, Value} = Opt) when is_list(Value), length(Value) >= 1 ->
+    case handle_user_pref_pubkey_algs(Value, []) of
+	{true, NewOpts} ->
+	    {pref_public_key_algs, NewOpts};
+	_ ->
+	    throw({error, {eoptions, Opt}})
+    end;
 handle_ssh_option({connect_timeout, Value} = Opt) when is_integer(Value); Value == infinity ->
     Opt;
 handle_ssh_option({max_sessions, Value} = Opt) when is_integer(Value), Value>0 ->
@@ -780,3 +777,16 @@ read_moduli_file(D, I, Acc) ->
 	    end
     end.
 			   
+handle_user_pref_pubkey_algs([], Acc) ->
+    {true, lists:reverse(Acc)};
+handle_user_pref_pubkey_algs([H|T], Acc) ->
+    case lists:member(H, ?SUPPORTED_USER_KEYS) of
+	true ->
+	    handle_user_pref_pubkey_algs(T, [H| Acc]);
+
+	false when H==ssh_dsa -> handle_user_pref_pubkey_algs(T, ['ssh-dss'| Acc]);
+	false when H==ssh_rsa -> handle_user_pref_pubkey_algs(T, ['ssh-rsa'| Acc]);
+
+	false ->
+	    false
+    end.
diff --git a/lib/ssh/src/ssh.hrl b/lib/ssh/src/ssh.hrl
index 8efc743b67..f88098819d 100644
--- a/lib/ssh/src/ssh.hrl
+++ b/lib/ssh/src/ssh.hrl
@@ -33,6 +33,9 @@
 -define(REKEY_DATA_TIMOUT, 60000).
 -define(DEFAULT_PROFILE, default).
 
+-define(SUPPORTED_AUTH_METHODS, "publickey,keyboard-interactive,password").
+-define(SUPPORTED_USER_KEYS, ['ssh-rsa','ssh-dss','ecdsa-sha2-nistp256','ecdsa-sha2-nistp384','ecdsa-sha2-nistp521']).
+
 -define(FALSE, 0).
 -define(TRUE,  1).
 %% basic binary constructors
diff --git a/lib/ssh/src/ssh_auth.erl b/lib/ssh/src/ssh_auth.erl
index 4967a2e4cd..fdbb5c152a 100644
--- a/lib/ssh/src/ssh_auth.erl
+++ b/lib/ssh/src/ssh_auth.erl
@@ -118,11 +118,16 @@ init_userauth_request_msg(#ssh{opts = Opts} = Ssh) ->
 					    service = "ssh-connection",
 					    method = "none",
 					    data = <<>>},
+	    Algs0 = proplists:get_value(pref_public_key_algs, Opts, ?SUPPORTED_USER_KEYS),
+	    %% The following line is not strictly correct. The call returns the
+	    %% supported HOST key types while we are interested in USER keys. However,
+	    %% they "happens" to be the same (for now).  This could change....
+	    %% There is no danger as long as the set of user keys is a subset of the set
+	    %% of host keys.
+	    CryptoSupported = ssh_transport:supported_algorithms(public_key),
+	    Algs = [A || A <- Algs0,
+			 lists:member(A, CryptoSupported)],
 
-
-	    Algs = proplists:get_value(public_key, 
-				       proplists:get_value(preferred_algorithms, Opts, []),
-				       ssh_transport:default_algorithms(public_key)),
 	    Prefs = method_preference(Algs),
 	    ssh_transport:ssh_packet(Msg, Ssh#ssh{user = User,
 						  userauth_preference = Prefs,
diff --git a/lib/ssh/src/ssh_auth.hrl b/lib/ssh/src/ssh_auth.hrl
index 5197a42fa4..449bc4fa45 100644
--- a/lib/ssh/src/ssh_auth.hrl
+++ b/lib/ssh/src/ssh_auth.hrl
@@ -22,7 +22,6 @@
 
 %%% Description: Ssh User Authentication Protocol
 
--define(SUPPORTED_AUTH_METHODS, "publickey,keyboard-interactive,password").
 
 -define(SSH_MSG_USERAUTH_REQUEST,  50).
 -define(SSH_MSG_USERAUTH_FAILURE,  51).
diff --git a/lib/ssh/test/ssh_basic_SUITE.erl b/lib/ssh/test/ssh_basic_SUITE.erl
index d4cb03f2f2..85a6bac972 100644
--- a/lib/ssh/test/ssh_basic_SUITE.erl
+++ b/lib/ssh/test/ssh_basic_SUITE.erl
@@ -41,6 +41,10 @@
 	 double_close/1, 
 	 exec/1,
 	 exec_compressed/1,  
+	 exec_key_differs1/1,
+	 exec_key_differs2/1,
+	 exec_key_differs3/1,
+	 exec_key_differs_fail/1,
 	 idle_time/1,
 	 inet6_option/1,
 	 inet_option/1,
@@ -86,6 +90,7 @@ all() ->
      {group, ecdsa_sha2_nistp521_key},
      {group, dsa_pass_key},
      {group, rsa_pass_key},
+     {group, host_user_key_differs},
      {group, key_cb},
      {group, internal_error},
      daemon_already_started,
@@ -102,6 +107,10 @@ groups() ->
      {ecdsa_sha2_nistp256_key, [], basic_tests()},
      {ecdsa_sha2_nistp384_key, [], basic_tests()},
      {ecdsa_sha2_nistp521_key, [], basic_tests()},
+     {host_user_key_differs, [], [exec_key_differs1,
+				  exec_key_differs2,
+				  exec_key_differs3,
+				  exec_key_differs_fail]},
      {dsa_pass_key, [], [pass_phrase]},
      {rsa_pass_key, [], [pass_phrase]},
      {key_cb, [], [key_callback, key_callback_options]},
@@ -184,6 +193,21 @@ init_per_group(dsa_pass_key, Config) ->
     PrivDir = ?config(priv_dir, Config),
     ssh_test_lib:setup_dsa_pass_pharse(DataDir, PrivDir, "Password"),
     [{pass_phrase, {dsa_pass_phrase, "Password"}}| Config];
+init_per_group(host_user_key_differs, Config) ->
+    Data = ?config(data_dir, Config),
+    Sys = filename:join(?config(priv_dir, Config), system_rsa),
+    SysUsr = filename:join(Sys, user),
+    Usr = filename:join(?config(priv_dir, Config), user_ecdsa_256),
+    file:make_dir(Sys),
+    file:make_dir(SysUsr),
+    file:make_dir(Usr),
+    file:copy(filename:join(Data, "ssh_host_rsa_key"),     filename:join(Sys, "ssh_host_rsa_key")),
+    file:copy(filename:join(Data, "ssh_host_rsa_key.pub"), filename:join(Sys, "ssh_host_rsa_key.pub")),
+    file:copy(filename:join(Data, "id_ecdsa256"),         filename:join(Usr, "id_ecdsa")),
+    file:copy(filename:join(Data, "id_ecdsa256.pub"),     filename:join(Usr, "id_ecdsa.pub")),
+    ssh_test_lib:setup_ecdsa_auth_keys("256", Usr, SysUsr),
+    ssh_test_lib:setup_rsa_known_host(Sys, Usr),
+    Config;
 init_per_group(key_cb, Config) ->
     DataDir = ?config(data_dir, Config),
     PrivDir = ?config(priv_dir, Config),
@@ -491,6 +515,80 @@ shell(Config) when is_list(Config) ->
     end.
     
 %%--------------------------------------------------------------------
+%%% Test that we could user different types of host pubkey and user pubkey
+exec_key_differs1(Config) -> exec_key_differs(Config, ['ecdsa-sha2-nistp256']).
+
+exec_key_differs2(Config) -> exec_key_differs(Config, ['ssh-dss','ecdsa-sha2-nistp256']).
+
+exec_key_differs3(Config) -> exec_key_differs(Config, ['ecdsa-sha2-nistp384','ecdsa-sha2-nistp256']).
+
+
+
+exec_key_differs(Config, UserPKAlgs) ->
+    case lists:usort(['ssh-rsa'|UserPKAlgs])
+	-- ssh_transport:supported_algorithms(public_key)
+    of
+	[] ->
+	    process_flag(trap_exit, true),
+	    SystemDir = filename:join(?config(priv_dir, Config), system_rsa),
+	    SystemUserDir = filename:join(SystemDir, user),
+	    UserDir = filename:join(?config(priv_dir, Config), user_ecdsa_256),
+   
+	    {_Pid, _Host, Port} = ssh_test_lib:daemon([{system_dir, SystemDir},
+						       {user_dir, SystemUserDir},
+						       {preferred_algorithms,
+							[{public_key,['ssh-rsa']}]}]),
+	    ct:sleep(500),
+
+	    IO = ssh_test_lib:start_io_server(),
+	    Shell = ssh_test_lib:start_shell(Port, IO, UserDir,
+					     [{preferred_algorithms,[{public_key,['ssh-rsa']}]},
+					      {pref_public_key_algs,UserPKAlgs}
+					     ]),
+
+
+	    receive
+		{'EXIT', _, _} ->
+		    ct:fail(no_ssh_connection);  
+		ErlShellStart ->
+		    ct:log("Erlang shell start: ~p~n", [ErlShellStart]),
+		    do_shell(IO, Shell)
+	    after 
+		30000 -> ct:fail("timeout ~p:~p",[?MODULE,?LINE])
+	    end;
+
+	UnsupportedPubKeys ->
+	    {skip, io_lib:format("~p unsupported",[UnsupportedPubKeys])}
+    end.
+    
+%%--------------------------------------------------------------------
+exec_key_differs_fail(Config) when is_list(Config) ->
+    process_flag(trap_exit, true),
+    SystemDir = filename:join(?config(priv_dir, Config), system_rsa),
+    SystemUserDir = filename:join(SystemDir, user),
+    UserDir = filename:join(?config(priv_dir, Config), user_ecdsa_256),
+   
+    {_Pid, _Host, Port} = ssh_test_lib:daemon([{system_dir, SystemDir},
+					       {user_dir, SystemUserDir},
+					       {preferred_algorithms,
+						[{public_key,['ssh-rsa']}]}]),
+    ct:sleep(500),
+
+    IO = ssh_test_lib:start_io_server(),
+    ssh_test_lib:start_shell(Port, IO, UserDir,
+			     [{preferred_algorithms,[{public_key,['ssh-rsa']}]},
+			      {pref_public_key_algs,['ssh-dss']}]),
+    receive
+	{'EXIT', _, _} ->
+	    ok;  
+	ErlShellStart ->
+	    ct:log("Erlang shell start: ~p~n", [ErlShellStart]),
+	    ct:fail(connection_not_rejected)
+    after 
+	30000 -> ct:fail("timeout ~p:~p",[?MODULE,?LINE])
+    end.
+    
+%%--------------------------------------------------------------------
 cli(Config) when is_list(Config) ->
     process_flag(trap_exit, true),
     SystemDir = filename:join(?config(priv_dir, Config), system),
diff --git a/lib/ssh/test/ssh_renegotiate_SUITE.erl b/lib/ssh/test/ssh_renegotiate_SUITE.erl
index 227dfcddcd..e5cfa58bad 100644
--- a/lib/ssh/test/ssh_renegotiate_SUITE.erl
+++ b/lib/ssh/test/ssh_renegotiate_SUITE.erl
@@ -57,9 +57,15 @@ end_per_suite(_Config) ->
 
 %%--------------------------------------------------------------------
 init_per_group(aes_gcm, Config) ->
-    [{preferred_algorithms, [{cipher,[{client2server,['[email protected]']},
-				      {server2client,['[email protected]']}]}]}
-     | Config];
+    case lists:member({client2server,['[email protected]']},
+		      ssh_transport:supported_algorithms(cipher)) of
+	true ->
+	    [{preferred_algorithms, [{cipher,[{client2server,['[email protected]']},
+					      {server2client,['[email protected]']}]}]}
+	     | Config];
+	false ->
+	    {skip, "aes_gcm not supported"}
+    end;
 init_per_group(_, Config) ->
     [{preferred_algorithms, ssh:default_algorithms()} | Config].
 
@@ -107,7 +113,9 @@ rekey_limit(Config) ->
     UserDir = ?config(priv_dir, Config),
     DataFile = filename:join(UserDir, "rekey.data"),
 
-    {Pid, Host, Port} = ssh_test_lib:std_daemon(Config,[{max_random_length_padding,0}]),
+    Algs = ?config(preferred_algorithms, Config),
+    {Pid, Host, Port} = ssh_test_lib:std_daemon(Config,[{max_random_length_padding,0},
+							{preferred_algorithms,Algs}]),
 
     ConnectionRef = ssh_test_lib:std_connect(Config, Host, Port, [{rekey_limit, 6000},
 								  {max_random_length_padding,0}]),
@@ -151,7 +159,9 @@ renegotiate1(Config) ->
     UserDir = ?config(priv_dir, Config),
     DataFile = filename:join(UserDir, "renegotiate1.data"),
 
-    {Pid, Host, DPort} = ssh_test_lib:std_daemon(Config,[{max_random_length_padding,0}]),
+    Algs = ?config(preferred_algorithms, Config),
+    {Pid, Host, DPort} = ssh_test_lib:std_daemon(Config,[{max_random_length_padding,0},
+							 {preferred_algorithms,Algs}]),
 
     RPort = ssh_test_lib:inet_port(),
     {ok,RelayPid} = ssh_relay:start_link({0,0,0,0}, RPort, Host, DPort),
@@ -189,7 +199,9 @@ renegotiate2(Config) ->
     UserDir = ?config(priv_dir, Config),
     DataFile = filename:join(UserDir, "renegotiate2.data"),
 
-    {Pid, Host, DPort} = ssh_test_lib:std_daemon(Config,[{max_random_length_padding,0}]),
+    Algs = ?config(preferred_algorithms, Config),
+    {Pid, Host, DPort} = ssh_test_lib:std_daemon(Config,[{max_random_length_padding,0},
+							 {preferred_algorithms,Algs}]),
 
     RPort = ssh_test_lib:inet_port(),
     {ok,RelayPid} = ssh_relay:start_link({0,0,0,0}, RPort, Host, DPort),