From 602d42bbc883bc8d7d9bb3f40b082e27cded4abc Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 26 Jun 2023 15:47:32 -0700 Subject: [PATCH] Add and update tests for insecure private keys Updates existing test coverage to use insecure private key collection and adds testing for behavior changes within the communicator and the keypair utility. --- .../communicators/ssh/communicator_test.rb | 92 +++++++++++++++++++ test/unit/vagrant/machine_test.rb | 8 +- test/unit/vagrant/util/keypair_test.rb | 58 ++++++++---- 3 files changed, 139 insertions(+), 19 deletions(-) diff --git a/test/unit/plugins/communicators/ssh/communicator_test.rb b/test/unit/plugins/communicators/ssh/communicator_test.rb index 314bceb66..a40f7331c 100644 --- a/test/unit/plugins/communicators/ssh/communicator_test.rb +++ b/test/unit/plugins/communicators/ssh/communicator_test.rb @@ -242,6 +242,10 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do let(:openssh){ :openssh } let(:private_key_file){ double("private_key_file") } let(:path_joiner){ double("path_joiner") } + let(:algorithms) { double(:algorithms) } + let(:transport) { double(:transport, algorithms: algorithms) } + let(:valid_key_types) { [] } + let(:server_data) { { host_key: valid_key_types} } before do allow(Vagrant::Util::Keypair).to receive(:create). @@ -255,6 +259,8 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do allow(path_joiner).to receive(:join).and_return(private_key_file) allow(guest).to receive(:capability).with(:insert_public_key) allow(guest).to receive(:capability).with(:remove_public_key) + allow(connection).to receive(:transport).and_return(transport) + allow(algorithms).to receive(:instance_variable_get).with(:@server_data).and_return(server_data) end after{ communicator.ready? } @@ -280,6 +286,53 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do it "should remove the default public key" do expect(guest).to receive(:capability).with(:remove_public_key, any_args) end + + context "with server algorithm support data" do + context "when no key type matches are found" do + it "should default to rsa type" do + expect(Vagrant::Util::Keypair).to receive(:create). + with(type: :rsa).and_call_original + end + end + + context "when rsa is the only match" do + let(:valid_key_types) { ["ssh-edsca", "ssh-rsa"] } + + it "should use rsa type" do + expect(Vagrant::Util::Keypair).to receive(:create). + with(type: :rsa).and_call_original + end + end + + context "when ed25519 and rsa are both available" do + let(:valid_key_types) { ["ssh-ed25519", "ssh-rsa"] } + + it "should use ed25519 type" do + expect(Vagrant::Util::Keypair).to receive(:create). + with(type: :ed25519).and_call_original + end + end + + context "when ed25519 is the only match" do + let(:valid_key_types) { ["ssh-edsca", "ssh-ed25519"] } + + it "should use ed25519 type" do + expect(Vagrant::Util::Keypair).to receive(:create). + with(type: :ed25519).and_call_original + end + end + end + + context "when an error is encountered getting server data" do + before do + expect(connection).to receive(:transport).and_raise(StandardError) + end + + it "should default to rsa key" do + expect(Vagrant::Util::Keypair).to receive(:create). + with(type: :rsa).and_call_original + end + end end end end @@ -929,6 +982,45 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end + describe ".insecure_key?" do + let(:key_data) { "" } + let(:key_file) { + if !@key_file + f = Tempfile.new + f.write(key_data) + f.close + @key_file = f.path + end + @key_file + } + + after { File.delete(key_file) } + + context "when using rsa private key" do + let(:key_data) { File.read(Vagrant.source_root.join("keys", "vagrant.key.rsa")) } + + it "should match as insecure key" do + expect(communicator.send(:insecure_key?, key_file)).to be_truthy + end + end + + context "when using ed25519 private key" do + let(:key_data) { File.read(Vagrant.source_root.join("keys", "vagrant.key.ed25519")) } + + it "should match as insecure key" do + expect(communicator.send(:insecure_key?, key_file)).to be_truthy + end + end + + context "when using unknown private key" do + let(:key_data) { "invalid data" } + + it "should not match as insecure key" do + expect(communicator.send(:insecure_key?, key_file)).to be_falsey + end + end + end + describe ".generate_environment_export" do it "should generate bourne shell compatible export" do expect(communicator.send(:generate_environment_export, "TEST", "value")).to eq("export TEST=\"value\"\n") diff --git a/test/unit/vagrant/machine_test.rb b/test/unit/vagrant/machine_test.rb index 6b4dd1531..be160b235 100644 --- a/test/unit/vagrant/machine_test.rb +++ b/test/unit/vagrant/machine_test.rb @@ -729,7 +729,9 @@ describe Vagrant::Machine do provider_ssh_info[:private_key_path] = nil instance.config.ssh.private_key_path = nil - expect(ssh_klass).to receive(:check_key_permissions).once.with(Pathname.new(instance.env.default_private_key_path.to_s)) + instance.env.default_private_key_paths.each do |key_path| + expect(ssh_klass).to receive(:check_key_permissions).once.with(Pathname.new(key_path.to_s)) + end instance.ssh_info end @@ -773,7 +775,7 @@ describe Vagrant::Machine do instance.config.ssh.private_key_path = nil expect(instance.ssh_info[:private_key_path]).to eq( - [instance.env.default_private_key_path.to_s] + instance.env.default_private_key_paths ) end @@ -783,7 +785,7 @@ describe Vagrant::Machine do instance.config.ssh.keys_only = false expect(instance.ssh_info[:private_key_path]).to eq( - [instance.env.default_private_key_path.to_s] + instance.env.default_private_key_paths ) end diff --git a/test/unit/vagrant/util/keypair_test.rb b/test/unit/vagrant/util/keypair_test.rb index 30ab76eba..972131823 100644 --- a/test/unit/vagrant/util/keypair_test.rb +++ b/test/unit/vagrant/util/keypair_test.rb @@ -1,34 +1,60 @@ require "openssl" +require "ed25519" +require "net/ssh" require File.expand_path("../../../base", __FILE__) require "vagrant/util/keypair" describe Vagrant::Util::Keypair do - describe ".create" do - it "generates a usable keypair with no password" do - # I don't know how to validate the final return value yet... - pubkey, privkey, _ = described_class.create + describe Vagrant::Util::Keypair::Rsa do + describe ".create" do + it "generates a usable keypair with no password" do + # I don't know how to validate the final return value yet... + pubkey, privkey, _ = described_class.create - pubkey = OpenSSL::PKey::RSA.new(pubkey) - privkey = OpenSSL::PKey::RSA.new(privkey) + pubkey = OpenSSL::PKey::RSA.new(pubkey) + privkey = OpenSSL::PKey::RSA.new(privkey) - encrypted = pubkey.public_encrypt("foo") - decrypted = privkey.private_decrypt(encrypted) + encrypted = pubkey.public_encrypt("foo") + decrypted = privkey.private_decrypt(encrypted) - expect(decrypted).to eq("foo") + expect(decrypted).to eq("foo") + end + + it "generates a keypair that requires a password" do + pubkey, privkey, _ = described_class.create("password") + + pubkey = OpenSSL::PKey::RSA.new(pubkey) + privkey = OpenSSL::PKey::RSA.new(privkey, "password") + + encrypted = pubkey.public_encrypt("foo") + decrypted = privkey.private_decrypt(encrypted) + + expect(decrypted).to eq("foo") + end end + end - it "generates a keypair that requires a password" do - pubkey, privkey, _ = described_class.create("password") + describe Vagrant::Util::Keypair::Ed25519 do + describe ".create" do + it "generates a usable keypair with no password" do + pubkey, ossh_privkey, _ = described_class.create - pubkey = OpenSSL::PKey::RSA.new(pubkey) - privkey = OpenSSL::PKey::RSA.new(privkey, "password") - encrypted = pubkey.public_encrypt("foo") - decrypted = privkey.private_decrypt(encrypted) + privkey = Net::SSH::Authentication::ED25519::PrivKey.read(ossh_privkey, "").sign_key + pubkey = Ed25519::VerifyKey.new(pubkey) - expect(decrypted).to eq("foo") + message = "vagrant test" + signature = privkey.sign(message) + expect(pubkey.verify(signature, message)).to be_truthy + end + + it "does not generate a keypair that requires a password" do + expect { + described_class.create("my password") + }.to raise_error(NotImplementedError) + end end end end