From 5a7fd6b3022fd5a9e1e4b6e342e2cc9f2a73e7de Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 18 Jan 2024 11:53:00 -0800 Subject: [PATCH] Inspect guest for supported key types Updates the SSH communicator to inspect the guest sshd configuration for supported key types when creating a new key to replace the default insecure public key. If the guest cannot be inspected, the connection will be inspected. If the connection cannot be inspected, it will simply fall back to the original behavior of using an rsa type key. --- plugins/communicators/ssh/communicator.rb | 59 +++++-- .../communicators/ssh/communicator_test.rb | 159 ++++++++++++++++-- 2 files changed, 188 insertions(+), 30 deletions(-) diff --git a/plugins/communicators/ssh/communicator.rb b/plugins/communicators/ssh/communicator.rb index 1ba3d0f21..db5403b8e 100644 --- a/plugins/communicators/ssh/communicator.rb +++ b/plugins/communicators/ssh/communicator.rb @@ -838,26 +838,59 @@ module VagrantPlugins end def supported_key_types + return @supported_key_types if @supported_key_types + if @connection.nil? raise Vagrant::Errors::SSHNotReady end - server_data = @connection. - transport&. - algorithms&. - instance_variable_get(:@server_data) - if server_data.nil? - @logger.warn("No server data available for key type support check") - raise ServerDataError, "no data available" - end - if !server_data.is_a?(Hash) - @logger.warn("Server data is not expected type (expecting Hash, got #{server_data.class})") - raise ServerDataError, "unexpected type encountered (expecting Hash, got #{server_data.class})" + list = "" + result = sudo("sshd -T | grep key", {error_check: false}) do |type, data| + list << data end - @logger.debug("server supported key type list: #{server_data[:host_key]}") + # If the command failed, attempt to extract some supported + # key information from within net-ssh + if result != 0 + server_data = @connection. + transport&. + algorithms&. + instance_variable_get(:@server_data) + if server_data.nil? + @logger.warn("No server data available for key type support check") + raise ServerDataError, "no data available" + end + if !server_data.is_a?(Hash) + @logger.warn("Server data is not expected type (expecting Hash, got #{server_data.class})") + raise ServerDataError, "unexpected type encountered (expecting Hash, got #{server_data.class})" + end - server_data[:host_key] + @logger.debug("server supported key type list (extracted from connection server info using host key): #{server_data[:host_key]}") + return @supported_key_types = server_data[:host_key] + end + + # Convert the options into a Hash for easy access + opts = Hash[*list.split("\n").map{|line| line.split(" ", 2)}.flatten] + + # Define the option names to check for in preferred order + # NOTE: pubkeyacceptedkeytypes has been renamed to pubkeyacceptedalgorithms + # ref: https://github.com/openssh/openssh-portable/commit/ee9c0da8035b3168e8e57c1dedc2d1b0daf00eec + ["pubkeyacceptedalgorithms", "pubkeyacceptedkeytypes", "hostkeyalgorithms"].each do |opt_name| + next if !opts.key?(opt_name) + + @supported_key_types = opts[opt_name].split(",") + @logger.debug("server supported key type list (using #{opt_name}): #{@supported_key_types}") + + return @supported_key_types + end + + # Still here means unable to determine key types + # so log what information was returned and toss + # and error + @logger.warn("failed to determine supported key types from remote inspection") + @logger.debug("data returned for supported key types remote inspection: #{list.inspect}") + + raise ServerDataError, "no data available" end end end diff --git a/test/unit/plugins/communicators/ssh/communicator_test.rb b/test/unit/plugins/communicators/ssh/communicator_test.rb index f77006264..bc5cf979b 100644 --- a/test/unit/plugins/communicators/ssh/communicator_test.rb +++ b/test/unit/plugins/communicators/ssh/communicator_test.rb @@ -69,7 +69,8 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do let(:command_stderr_data) { '' } # Mock for net-ssh scp let(:scp) { double("scp") } - + # Value returned from remote ssh supported key check + let(:sudo_supported_key_list) { "pubkeyacceptedalgorithms ssh-rsa" } # Setup for commands using the net-ssh connection. This can be reused where needed # by providing to `before` @@ -93,13 +94,16 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do and_yield(nil, exit_data) # Return mocked net-ssh connection during setup allow(communicator).to receive(:retryable).and_return(connection) + # Stub in a response for supported key types check + allow(communicator).to receive(:sudo).with("sshd -T | grep key", any_args). + and_yield(:stdout, sudo_supported_key_list).and_return(0) end before do allow(host).to receive(:capability?).and_return(false) end - describe ".wait_for_ready" do + describe "#wait_for_ready" do before(&connection_setup) context "with no static config (default scenario)" do context "when ssh_info requires a multiple tries before it is ready" do @@ -162,7 +166,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end - describe "reset!" do + describe "#reset!" do let(:connection) { double("connection") } before do @@ -182,7 +186,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end - describe ".ready?" do + describe "#ready?" do before(&connection_setup) it "returns true if shell test is successful" do expect(communicator.ready?).to be(true) @@ -248,8 +252,6 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do 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). @@ -264,7 +266,6 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do 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) allow(communicator).to receive(:supported_key_types).and_raise(described_class.const_get(:ServerDataError)) end @@ -297,7 +298,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do context "with server algorithm support data" do before do - allow(communicator).to receive(:supported_key_types).and_call_original + allow(communicator).to receive(:supported_key_types).and_return(valid_key_types) end context "when rsa is the only match" do @@ -371,8 +372,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do context "when an error is encountered getting server data" do before do - expect(communicator).to receive(:supported_key_types).and_call_original - expect(connection).to receive(:transport).and_raise(StandardError) + expect(communicator).to receive(:supported_key_types).and_raise(StandardError) end it "should default to rsa key" do @@ -385,7 +385,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end - describe ".execute" do + describe "#execute" do before(&connection_setup) it "runs valid command and returns successful status code" do expect(command_channel).to receive(:send_data).with(/ls \/\n/) @@ -579,7 +579,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end - describe ".test" do + describe "#test" do before(&connection_setup) context "with exit code as zero" do it "returns true" do @@ -598,7 +598,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end - describe ".upload" do + describe "#upload" do before do expect(communicator).to receive(:scp_connect).and_yield(scp) allow(communicator).to receive(:create_remote_directory) @@ -704,7 +704,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end - describe ".download" do + describe "#download" do before do expect(communicator).to receive(:scp_connect).and_yield(scp) end @@ -715,7 +715,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end - describe ".connect" do + describe "#connect" do it "cannot be called directly" do expect{ communicator.connect }.to raise_error(NoMethodError) @@ -1030,7 +1030,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end - describe ".insecure_key?" do + describe "#insecure_key?" do let(:key_data) { "" } let(:key_file) { if !@key_file @@ -1069,7 +1069,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end - describe ".generate_environment_export" do + 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") end @@ -1082,4 +1082,129 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end end + + describe "#supported_key_types" do + let(:sudo_result) { 0 } + let(:sudo_data) { "" } + let(:server_data_error) { VagrantPlugins::CommunicatorSSH::Communicator::ServerDataError } + let(:transport) { double("transport", algorithms: algorithms) } + let(:algorithms) { double("algorithms") } + + before do + allow(communicator).to receive(:ready?).and_return(true) + expect(communicator).to receive(:sudo). + with("sshd -T | grep key", any_args). + and_yield(:stdout, sudo_data). + and_return(sudo_result) + # The @connection value is checked to determine if supported key types + # can be checked. To facilitate this, set it to a non-nil value + communicator.instance_variable_set(:@connection, connection) + allow(connection).to receive(:transport).and_return(transport) + end + + it "should raise an error when no data is returned" do + expect { communicator.send(:supported_key_types) }.to raise_error(server_data_error) + end + + context "when sudo command is unsuccessful" do + let(:sudo_result) { 1 } + + it "should inspect the net-ssh connection" do + expect(algorithms).to receive(:instance_variable_get). + with(:@server_data).and_return({}) + communicator.send(:supported_key_types) + end + end + + context "when data includes pubkeyacceptedalgorithms" do + let(:sudo_data) do + "pubkeyauthentication yes +gssapikeyexchange no +gssapistorecredentialsonrekey no +trustedusercakeys none +revokedkeys none +authorizedkeyscommand none +authorizedkeyscommanduser none +hostkeyagent none +hostbasedacceptedkeytypes ecdsa-sha2-nistp521,ssh-ed25519,rsa-sha2-512,rsa-sha2-256,ssh-rsa +hostkeyalgorithms ssh-ed25519,rsa-sha2-512,rsa-sha2-256,ssh-rsa +pubkeyacceptedalgorithms rsa-sha2-512,rsa-sha2-256,ssh-rsa +authorizedkeysfile .ssh/authorized_keys +hostkey /etc/ssh/ssh_host_rsa_key +rekeylimit 0 0" + end + + it "should return expected values" do + expect(communicator.send(:supported_key_types)).to eq(["rsa-sha2-512", "rsa-sha2-256", "ssh-rsa"]) + end + end + + context "when data includes pubkeyacceptedkeytypes" do + let(:sudo_data) do + "pubkeyauthentication yes +gssapikeyexchange no +gssapistorecredentialsonrekey no +trustedusercakeys none +revokedkeys none +authorizedkeyscommand none +authorizedkeyscommanduser none +hostkeyagent none +hostbasedacceptedkeytypes ecdsa-sha2-nistp521,ssh-ed25519,rsa-sha2-512,rsa-sha2-256,ssh-rsa +hostkeyalgorithms ssh-ed25519,rsa-sha2-512,rsa-sha2-256,ssh-rsa +pubkeyacceptedkeytypes rsa-sha2-512,rsa-sha2-256,ssh-rsa +authorizedkeysfile .ssh/authorized_keys +hostkey /etc/ssh/ssh_host_rsa_key +rekeylimit 0 0" + end + + it "should return expected values" do + expect(communicator.send(:supported_key_types)). + to eq(["rsa-sha2-512", "rsa-sha2-256", "ssh-rsa"]) + end + end + + context "when data does not include pubkeyacceptedalgorithms or pubkeyacceptedkeytypes" do + let(:sudo_data) do + "pubkeyauthentication yes +gssapikeyexchange no +gssapistorecredentialsonrekey no +trustedusercakeys none +revokedkeys none +authorizedkeyscommand none +authorizedkeyscommanduser none +hostkeyagent none +hostbasedacceptedkeytypes ecdsa-sha2-nistp521,ssh-ed25519,rsa-sha2-512,rsa-sha2-256,ssh-rsa +hostkeyalgorithms ssh-ed25519,rsa-sha2-512,rsa-sha2-256,ssh-rsa +authorizedkeysfile .ssh/authorized_keys +hostkey /etc/ssh/ssh_host_rsa_key +rekeylimit 0 0" + end + + it "should use hostkeyalgorithms" do + expect(communicator.send(:supported_key_types)). + to eq(["ssh-ed25519", "rsa-sha2-512", "rsa-sha2-256", "ssh-rsa"]) + end + end + + context "when data does not include defined config options" do + let(:sudo_data) do + "pubkeyauthentication yes +gssapikeyexchange no +gssapistorecredentialsonrekey no +trustedusercakeys none +revokedkeys none +authorizedkeyscommand none +authorizedkeyscommanduser none +hostkeyagent none +authorizedkeysfile .ssh/authorized_keys +hostkey /etc/ssh/ssh_host_rsa_key +rekeylimit 0 0" + end + + it "should raise error" do + expect { communicator.send(:supported_key_types) }. + to raise_error(server_data_error) + end + end + end end