ssh: Improve 'server-sig-algs' handling in client

2 files changed, 95 insertions, 52 deletions

diff --git a/lib/ssh/src/ssh_auth.erl b/lib/ssh/src/ssh_auth.erl
index 6cf659f830..ac64a7bf14 100644
--- a/lib/ssh/src/ssh_auth.erl
+++ b/lib/ssh/src/ssh_auth.erl
@@ -28,7 +28,8 @@
 -include("ssh_auth.hrl").
 -include("ssh_transport.hrl").
 
--export([publickey_msg/1, password_msg/1, keyboard_interactive_msg/1,
+-export([get_public_key/2,
+         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/2, handle_userauth_info_response/2
@@ -136,41 +137,49 @@ keyboard_interactive_msg([#ssh{user = User,
 	      Ssh)
     end.
 
-publickey_msg([SigAlg, #ssh{user = User,
-		       session_id = SessionId,
-		       service = Service,
-		       opts = Opts} = Ssh]) ->
-    Hash = ssh_transport:sha(SigAlg),
+
+get_public_key(SigAlg, #ssh{opts = Opts}) ->
     KeyAlg = key_alg(SigAlg),
     {KeyCb,KeyCbOpts} = ?GET_OPT(key_cb, Opts),
     UserOpts = ?GET_OPT(user_options, Opts),
     case KeyCb:user_key(KeyAlg, [{key_cb_private,KeyCbOpts}|UserOpts]) of
-	{ok, PrivKey} ->
-	    SigAlgStr = atom_to_list(SigAlg),
+        {ok, PrivKey} ->
             try
                 Key = ssh_transport:extract_public_key(PrivKey),
                 public_key:ssh_encode(Key, ssh2_pubkey)
             of
-		PubKeyBlob ->
-		    SigData = build_sig_data(SessionId, User, Service,
-                                             PubKeyBlob, SigAlgStr),
-		    Sig = ssh_transport:sign(SigData, Hash, PrivKey),
-		    SigBlob = list_to_binary([?string(SigAlgStr),
-                                              ?binary(Sig)]),
-		    ssh_transport:ssh_packet(
-		      #ssh_msg_userauth_request{user = User,
-						service = Service,
-						method = "publickey",
-						data = [?TRUE,
-							?string(SigAlgStr),
-							?binary(PubKeyBlob),
-							?binary(SigBlob)]},
-		      Ssh)
+                PubKeyBlob -> {ok,{PrivKey,PubKeyBlob}}
             catch
                 _:_ -> 
-		    {not_ok, Ssh}
+		    not_ok
             end;
-     	_Error ->
+	_Error ->
+	    not_ok
+    end.
+
+
+publickey_msg([SigAlg, #ssh{user = User,
+		       session_id = SessionId,
+		       service = Service} = Ssh]) ->
+    case get_public_key(SigAlg, Ssh) of
+	{ok, {PrivKey,PubKeyBlob}} ->
+            SigAlgStr = atom_to_list(SigAlg),
+            SigData = build_sig_data(SessionId, User, Service,
+                                     PubKeyBlob, SigAlgStr),
+            Hash = ssh_transport:sha(SigAlg),
+            Sig = ssh_transport:sign(SigData, Hash, PrivKey),
+            SigBlob = list_to_binary([?string(SigAlgStr),
+                                      ?binary(Sig)]),
+            ssh_transport:ssh_packet(
+              #ssh_msg_userauth_request{user = User,
+                                        service = Service,
+                                        method = "publickey",
+                                        data = [?TRUE,
+                                                ?string(SigAlgStr),
+                                                ?binary(PubKeyBlob),
+                                                ?binary(SigBlob)]},
+              Ssh);
+     	_ ->
 	    {not_ok, Ssh}
     end.
 
diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl
index f1ce337947..4c6aff5c24 100644
--- a/lib/ssh/src/ssh_connection_handler.erl
+++ b/lib/ssh/src/ssh_connection_handler.erl
@@ -453,16 +453,20 @@ init_ssh_record(Role, _Socket, PeerAddr, Opts) ->
                            PeerName0 when is_list(PeerName0) ->
                                PeerName0
                        end,
-	    S0#ssh{c_vsn = Vsn,
-		   c_version = Version,
-		   io_cb = case ?GET_OPT(user_interaction, Opts) of
-			       true ->  ssh_io;
-			       false -> ssh_no_io
-			   end,
-                   userauth_pubkeys = ?GET_OPT(pref_public_key_algs, Opts),
-		   userauth_quiet_mode = ?GET_OPT(quiet_mode, Opts),
-		   peer = {PeerName, PeerAddr}
-		  };
+            S1 =
+                S0#ssh{c_vsn = Vsn,
+                       c_version = Version,
+                       io_cb = case ?GET_OPT(user_interaction, Opts) of
+                                   true ->  ssh_io;
+                                   false -> ssh_no_io
+                               end,
+                       userauth_quiet_mode = ?GET_OPT(quiet_mode, Opts),
+                       peer = {PeerName, PeerAddr}
+                      },
+            S1#ssh{userauth_pubkeys = [K || K <- ?GET_OPT(pref_public_key_algs, Opts),
+                                            is_usable_user_pubkey(K, S1)
+                                      ]
+                  };
 
 	server ->
 	    S0#ssh{s_vsn = Vsn,
@@ -1700,29 +1704,59 @@ handle_ssh_msg_ext_info(#ssh_msg_ext_info{data=Data}, D0) ->
     lists:foldl(fun ext_info/2, D0, Data).
 
 
-ext_info({"server-sig-algs",SigAlgs}, D0 = #data{ssh_params=#ssh{role=client,
-                                                                 userauth_pubkeys=ClientSigAlgs}=Ssh0}) ->
-    %% Make strings to eliminate risk of beeing bombed with odd strings that fills the atom table:
-    SupportedAlgs = lists:map(fun erlang:atom_to_list/1, ssh_transport:supported_algorithms(public_key)),
-    ServerSigAlgs = [list_to_atom(SigAlg) || SigAlg <- string:tokens(SigAlgs,","),
-                                             %% length of SigAlg is implicitly checked by the comparison
-                                             %% in member/2:
-                                             lists:member(SigAlg, SupportedAlgs)
-                    ],
-    CommonAlgs = [Alg || Alg <- ServerSigAlgs,
-                         lists:member(Alg, ClientSigAlgs)],
-    SelectedAlgs =
-        case CommonAlgs of
-            [] -> ClientSigAlgs; % server-sig-algs value is just an advice
-            _ -> CommonAlgs
-        end,
-    D0#data{ssh_params = Ssh0#ssh{userauth_pubkeys = SelectedAlgs} };
+ext_info({"server-sig-algs",SigAlgsStr},
+         D0 = #data{ssh_params=#ssh{role=client,
+                                    userauth_pubkeys=ClientSigAlgs}=Ssh0}) ->
+    %% ClientSigAlgs are the pub_key algortithms that:
+    %%  1) is usable, that is, the user has such a public key and
+    %%  2) is either the default list or set by the caller
+    %%     with the client option 'pref_public_key_algs'
+    %%
+    %% The list is already checked for duplicates.
+
+    SigAlgs = [A || Astr <- string:tokens(SigAlgsStr, ","),
+                    A <- try [list_to_existing_atom(Astr)]
+                              %% list_to_existing_atom will fail for unknown algorithms
+                         catch _:_ -> []
+                         end],
+
+    CommonAlgs = [A || A <- SigAlgs,
+                       lists:member(A, ClientSigAlgs)],
+
+    %% Re-arrange the client supported public-key algorithms so that the server
+    %% preferred ones are tried first.
+    %% Trying algorithms not mentioned by the server is ok, since the server can't know
+    %% if the client supports 'server-sig-algs' or not.
+
+    D0#data{
+      ssh_params =
+          Ssh0#ssh{
+            userauth_pubkeys =
+                CommonAlgs ++ (ClientSigAlgs -- CommonAlgs)
+           }};
+
+    %% If there are algorithms common to the client and the server, use them.
+    %% Otherwise try with ones that the client supports.  The server-sig-alg
+    %% list is a suggestion, not an order.
+    %% case CommonAlgs of
+    %%     [_|_] ->
+    %%         D0#data{ssh_params = Ssh0#ssh{userauth_pubkeys = CommonAlgs}};
+    %%     [] ->
+    %%         D0
+    %% end;
 
 ext_info(_, D0) ->
     %% Not implemented
     D0.
 
 %%%----------------------------------------------------------------
+is_usable_user_pubkey(A, Ssh) ->
+    case ssh_auth:get_public_key(A, Ssh) of
+        {ok,_} -> true;
+        _ -> false
+    end.
+
+%%%----------------------------------------------------------------
 handle_request(ChannelPid, ChannelId, Type, Data, WantReply, From, D) ->
     case ssh_channel:cache_lookup(cache(D), ChannelId) of
 	#channel{remote_id = Id} = Channel ->