Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Merged
merged 5 commits into from

2 participants

@musybite

This pull request fixes LH#30.

That bug had following causes. To use IdentitesOnly option, one have to know what IdentityFile corresponds to what identity (i.e. public key) and vice versa. Because of that KeyManager loads all identity files before listing agent identities. But it is impossible to get public key from encrypted private key file without passphrase; so, KeyManager prompted passphrase even before exchange with agent.

Fix: identities load process was separated to two phases. First phase trying to load all identities without prompting passphrase. It runs before listing agent identities. Second phase runs afterwards and loads all remained identities with passphrase prompt.

@delano delano merged commit 613a7e1 into from
@delano
Collaborator

Thanks for the fix. It's in release 2.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 22, 2011
  1. @musybite
  2. @musybite

    LH30: Failing test

    musybite authored
Commits on Aug 23, 2011
  1. @musybite
  2. @musybite
  3. @musybite

    LH30 fixed

    musybite authored
    KeyManager#each_identity now trying to load all identites without
    promting passphrase before listing agent identites.
This page is out of date. Refresh to see the latest.
View
80 lib/net/ssh/authentication/key_manager.rb
@@ -95,12 +95,14 @@ def finish
# 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 @@ def each_identity
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 @@ def sign(identity, data)
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 @@ def agent
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
View
8 lib/net/ssh/key_factory.rb
@@ -34,9 +34,9 @@ def get(name)
# 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 @@ def load_private_key(filename, passphrase=nil)
# 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 @@ def load_data_private_key(data, passphrase=nil, filename="")
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)
View
53 test/authentication/test_key_manager.rb
@@ -31,8 +31,8 @@ def test_use_agent_should_be_set_to_false_if_agent_could_not_be_found
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 @@ def test_identities_should_load_from_agent
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 @@ def test_only_identities_with_key_files_should_load_from_agent_of_keys_only_set
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 @@ def test_sign_with_agent_originated_key_should_request_signature_from_agent
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 @@ def test_sign_with_file_originated_key_should_raise_key_manager_error_if_unloada
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)
View
8 test/test_key_factory.rb
@@ -40,6 +40,12 @@ def test_load_encrypted_private_key_should_give_three_tries_for_the_password_and
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 @@ def public(key)
result << [Net::SSH::Buffer.from(:key, key).to_s].pack("m*").strip.tr("\n\r\t ", "")
result << " joe@host.test"
end
-end
+end
View
4 test/transport/test_algorithms.rb
@@ -146,7 +146,7 @@ def test_key_exchange_when_server_does_not_support_any_preferred_kex_should_rais
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 @@ def transport
end
end
-end
+end
Something went wrong with that request. Please try again.