Merge pull request #26 from musybite/lh30

Do not prompt any passphrases before trying all identities from agent.

5 files changed, 114 insertions, 39 deletions

diff --git a/lib/net/ssh/authentication/key_manager.rb b/lib/net/ssh/authentication/key_manager.rb
index 7a0692f..6c1704e 100644
--- a/lib/net/ssh/authentication/key_manager.rb
+++ b/lib/net/ssh/authentication/key_manager.rb
@@ -95,12 +95,14 @@ module Net
         # from ssh-agent will be ignored unless it present in key_files or
         # key_data.
         def each_identity
-          user_identities = load_identities_from_files + load_identities_from_data
+          prepared_identities = prepare_identities_from_files + prepare_identities_from_data
+
+          user_identities = load_identities(prepared_identities, false)
 
           if agent
             agent.identities.each do |key|
               corresponding_user_identity = user_identities.detect { |identity|
-                identity[:public_key].to_pem == key.to_pem
+                identity[:public_key] && identity[:public_key].to_pem == key.to_pem
               }
               user_identities.delete(corresponding_user_identity) if corresponding_user_identity
 
@@ -111,6 +113,8 @@ module Net
             end
           end
 
+          user_identities = load_identities(user_identities, true)
+
           user_identities.each do |identity|
             key = identity.delete(:public_key)
             known_identities[key] = identity
@@ -134,7 +138,7 @@ module Net
 
           if info[:key].nil? && info[:from] == :file
             begin
-              info[:key] = KeyFactory.load_private_key(info[:file], options[:passphrase])
+              info[:key] = KeyFactory.load_private_key(info[:file], options[:passphrase], true)
             rescue Exception, OpenSSL::OpenSSLError => e 
               raise KeyManagerError, "the given identity is known, but the private key could not be loaded: #{e.class} (#{e.message})"
             end
@@ -179,37 +183,67 @@ module Net
 
         private
 
-        # Extracts identities from user key_files, preserving their order and sources.
-        def load_identities_from_files
+        # Prepares identities from user key_files for loading, preserving their order and sources.
+        def prepare_identities_from_files
           key_files.map do |file|
             public_key_file = file + ".pub"
             if File.readable?(public_key_file)
-              begin
-                key = KeyFactory.load_public_key(public_key_file)
-                { :public_key => key, :from => :file, :file => file }
-              rescue Exception => e
-                error { "could not load public key file `#{public_key_file}': #{e.class} (#{e.message})" }
-                nil
-              end
+              { :load_from => :pubkey_file, :file => file }
             elsif File.readable?(file)
-              begin
-                private_key = KeyFactory.load_private_key(file, options[:passphrase])
+              { :load_from => :privkey_file, :file => file }
+            end
+          end.compact
+        end
+
+        # Prepared identities from user key_data, preserving their order and sources.
+        def prepare_identities_from_data
+          key_data.map do |data|
+            { :load_from => :data, :data => data }
+          end
+        end
+
+        # Load prepared identities. Private key decryption errors ignored if passphrase was not prompted.
+        def load_identities(identities, ask_passphrase)
+          identities.map do |identity|
+            begin
+              case identity[:load_from]
+              when :pubkey_file
+                key = KeyFactory.load_public_key(identity[:file] + ".pub")
+                { :public_key => key, :from => :file, :file => identity[:file] }
+              when :privkey_file
+                private_key = KeyFactory.load_private_key(identity[:file], options[:passphrase], ask_passphrase)
                 key = private_key.send(:public_key)
-                { :public_key => key, :from => :file, :file => file, :key => private_key }
-              rescue Exception => e
-                error { "could not load private key file `#{file}': #{e.class} (#{e.message})" }
+                { :public_key => key, :from => :file, :file => identity[:file], :key => private_key }
+              when :data
+                private_key = KeyFactory.load_data_private_key(identity[:data], options[:passphrase], ask_passphrase)
+                key = private_key.send(:public_key)
+                { :public_key => key, :from => :key_data, :data => identity[:data], :key => private_key }
+              else
+                identity
+              end
+
+            rescue OpenSSL::PKey::RSAError, OpenSSL::PKey::DSAError => e
+              if ask_passphrase
+                process_identity_loading_error(identity, e)
                 nil
+              else
+                identity
               end
+            rescue Exception => e
+              process_identity_loading_error(identity, e)
+              nil
             end
           end.compact
         end
 
-        # Extraccts identities from user key_data, preserving their order and sources.
-        def load_identities_from_data
-          key_data.map do |data|
-            private_key = KeyFactory.load_data_private_key(data)
-            key = private_key.send(:public_key)
-            { :public_key => key, :from => :key_data, :data => data, :key => private_key }
+        def process_identity_loading_error(identity, e)
+          case identity[:load_from]
+          when :pubkey_file
+            error { "could not load public key file `#{identity[:file]}': #{e.class} (#{e.message})" }
+          when :privkey_file
+            error { "could not load private key file `#{identity[:file]}': #{e.class} (#{e.message})" }
+          else
+            raise e
           end
         end
 
diff --git a/lib/net/ssh/key_factory.rb b/lib/net/ssh/key_factory.rb
index 5879810..d9d48d0 100644
--- a/lib/net/ssh/key_factory.rb
+++ b/lib/net/ssh/key_factory.rb
@@ -34,9 +34,9 @@ module Net; module SSH
       # appropriately. The new key is returned. If the key itself is
       # encrypted (requiring a passphrase to use), the user will be
       # prompted to enter their password unless passphrase works. 
-      def load_private_key(filename, passphrase=nil)
+      def load_private_key(filename, passphrase=nil, ask_passphrase=true)
         data = File.read(File.expand_path(filename))
-        load_data_private_key(data, passphrase, filename)
+        load_data_private_key(data, passphrase, ask_passphrase, filename)
       end
 
       # Loads a private key. It will correctly determine
@@ -44,7 +44,7 @@ module Net; module SSH
       # appropriately. The new key is returned. If the key itself is
       # encrypted (requiring a passphrase to use), the user will be
       # prompted to enter their password unless passphrase works. 
-      def load_data_private_key(data, passphrase=nil, filename="")
+      def load_data_private_key(data, passphrase=nil, ask_passphrase=true, filename="")
         if data.match(/-----BEGIN DSA PRIVATE KEY-----/)
           key_type = OpenSSL::PKey::DSA
         elsif data.match(/-----BEGIN RSA PRIVATE KEY-----/)
@@ -61,7 +61,7 @@ module Net; module SSH
         begin
           return key_type.new(data, passphrase || 'invalid')
         rescue OpenSSL::PKey::RSAError, OpenSSL::PKey::DSAError => e
-          if encrypted_key
+          if encrypted_key && ask_passphrase
             tries += 1
             if tries <= 3
               passphrase = prompt("Enter passphrase for #{filename}:", false)
diff --git a/test/authentication/test_key_manager.rb b/test/authentication/test_key_manager.rb
index 3e8c9b0..0c6f585 100644
--- a/test/authentication/test_key_manager.rb
+++ b/test/authentication/test_key_manager.rb
@@ -31,8 +31,8 @@ module Authentication
     def test_each_identity_should_load_from_key_files
       manager.stubs(:agent).returns(nil)
 
-      stub_file_key "/first", rsa
-      stub_file_key "/second", dsa
+      stub_file_private_key "/first", rsa
+      stub_file_private_key "/second", dsa
 
       identities = []
       manager.each_identity { |identity| identities << identity }
@@ -62,7 +62,7 @@ module Authentication
     def test_only_identities_with_key_files_should_load_from_agent_of_keys_only_set
       manager(:keys_only => true).stubs(:agent).returns(agent)
 
-      stub_file_key "/first", rsa
+      stub_file_private_key "/first", rsa
 
       identities = []
       manager.each_identity { |identity| identities << identity }
@@ -73,6 +73,22 @@ module Authentication
       assert_equal({:from => :agent}, manager.known_identities[rsa])
     end
 
+    def test_identities_without_public_key_files_should_not_be_touched_if_identity_loaded_from_agent
+      manager.stubs(:agent).returns(agent)
+
+      stub_file_public_key  "/first", rsa
+      stub_file_private_key "/second", dsa, :passphrase => :should_not_be_asked
+
+      identities = []
+      manager.each_identity do |identity|
+        identities << identity
+        break if manager.known_identities[identity][:from] == :agent
+      end
+
+      assert_equal 1, identities.length
+      assert_equal rsa.to_blob, identities.first.to_blob
+    end
+
     def test_sign_with_agent_originated_key_should_request_signature_from_agent
       manager.stubs(:agent).returns(agent)
       manager.each_identity { |identity| } # preload the known_identities
@@ -82,7 +98,7 @@ module Authentication
 
     def test_sign_with_file_originated_key_should_load_private_key_and_sign_with_it
       manager.stubs(:agent).returns(nil)
-      stub_file_key "/first", rsa(512)
+      stub_file_private_key "/first", rsa(512)
       rsa.expects(:ssh_do_sign).with("hello, world").returns("abcxyz123")
       manager.each_identity { |identity| } # preload the known_identities
       assert_equal "\0\0\0\assh-rsa\0\0\0\011abcxyz123", manager.sign(rsa, "hello, world")
@@ -100,12 +116,31 @@ module Authentication
 
     private
 
-      def stub_file_key(name, key)
+      def stub_file_private_key(name, key, options = {})
         manager.add(name)
-        File.expects(:readable?).with(name).returns(true)
-        File.expects(:readable?).with(name + ".pub").returns(false)
-        Net::SSH::KeyFactory.expects(:load_private_key).with(name, nil).returns(key).at_least_once
-        key.expects(:public_key).returns(key)
+        File.stubs(:readable?).with(name).returns(true)
+        File.stubs(:readable?).with(name + ".pub").returns(false)
+
+        case options.fetch(:passphrase, :indifferently)
+        when :should_be_asked
+          Net::SSH::KeyFactory.expects(:load_private_key).with(name, nil, false).raises(OpenSSL::PKey::RSAError).at_least_once
+          Net::SSH::KeyFactory.expects(:load_private_key).with(name, nil, true).returns(key).at_least_once
+        when :should_not_be_asked
+          Net::SSH::KeyFactory.expects(:load_private_key).with(name, nil, false).raises(OpenSSL::PKey::RSAError).at_least_once
+          Net::SSH::KeyFactory.expects(:load_private_key).with(name, nil, true).never
+        else # :indifferently
+          Net::SSH::KeyFactory.expects(:load_private_key).with(name, nil, any_of(true, false)).returns(key).at_least_once
+        end
+
+        key.stubs(:public_key).returns(key)
+      end
+
+      def stub_file_public_key(name, key)
+        manager.add(name)
+        File.stubs(:readable?).with(name).returns(false)
+        File.stubs(:readable?).with(name + ".pub").returns(true)
+
+        Net::SSH::KeyFactory.expects(:load_public_key).with(name + ".pub").returns(key).at_least_once
       end
 
       def rsa(size=512)
diff --git a/test/test_key_factory.rb b/test/test_key_factory.rb
index 5ca574f..caaa899 100644
--- a/test/test_key_factory.rb
+++ b/test/test_key_factory.rb
@@ -40,6 +40,12 @@ class TestKeyFactory < Test::Unit::TestCase
     assert_raises(OpenSSL::PKey::RSAError) { Net::SSH::KeyFactory.load_private_key("/key-file") }
   end
 
+  def test_load_encrypted_private_key_should_raise_exception_without_asking_passphrase
+    File.expects(:read).with("/key-file").returns(encrypted(rsa_key, "password"))
+    Net::SSH::KeyFactory.expects(:prompt).never
+    assert_raises(OpenSSL::PKey::RSAError) { Net::SSH::KeyFactory.load_private_key("/key-file", nil, false) }
+  end
+
   def test_load_public_rsa_key_should_return_key
     File.expects(:read).with("/key-file").returns(public(rsa_key))
     assert_equal rsa_key.to_blob, Net::SSH::KeyFactory.load_public_key("/key-file").to_blob
@@ -66,4 +72,4 @@ class TestKeyFactory < Test::Unit::TestCase
       result << [Net::SSH::Buffer.from(:key, key).to_s].pack("m*").strip.tr("\n\r\t ", "")
       result << " joe@host.test"
     end
-end
\ No newline at end of file
+end
diff --git a/test/transport/test_algorithms.rb b/test/transport/test_algorithms.rb
index 4a864ec..b58b4df 100644
--- a/test/transport/test_algorithms.rb
+++ b/test/transport/test_algorithms.rb
@@ -146,7 +146,7 @@ module Transport
     def test_allow_when_not_pending_should_be_true_for_all_packets
       (0..255).each do |type|
         packet = stub("packet", :type => type)
-        assert algorithms.allow?(packet), type
+        assert algorithms.allow?(packet), type.to_s
       end
     end
 
@@ -299,4 +299,4 @@ module Transport
       end
   end
 
-end
\ No newline at end of file
+end