diff --git a/lib/kitchen/driver/cloudstack/credentials.rb b/lib/kitchen/driver/cloudstack/credentials.rb index 75bcfc1..42ad144 100644 --- a/lib/kitchen/driver/cloudstack/credentials.rb +++ b/lib/kitchen/driver/cloudstack/credentials.rb @@ -27,9 +27,11 @@ class Credentials # Credential sources, in the order they take precedence. SOURCES = %i{keypair generated_password configured_password}.freeze - # A pem file starting with one of these is a public key, which will - # never authenticate. Users hit this by exporting the wrong half. - PUBLIC_KEY_PREFIXES = %w{ssh-rsa ssh-dsa ssh-ed25519 ecdsa-sha2-nistp256}.freeze + # Every private key format an SSH transport accepts -- PKCS#1, PKCS#8 + # and OpenSSH's own -- is PEM, so it opens with this. Anything else is + # a file that will never authenticate: an exported public key, a PuTTY + # .ppk, or the wrong file entirely. + PRIVATE_KEY_HEADER = "-----BEGIN".freeze attr_reader :warnings @@ -125,16 +127,21 @@ def search_directories [config[:keypair_search_directory], working_dir, home, File.join(home.to_s, ".ssh")].compact end - # Warns when the located .pem is a public key. + # Warns when the located .pem is not a private key. # # Exporting the wrong half of a keypair is a common mistake, and the - # resulting authentication failure is otherwise hard to read. + # resulting authentication failure is otherwise hard to read. This + # asks whether the file is a PEM rather than whether it looks like one + # of a handful of public key types, so an unusual public key type, a + # PuTTY .ppk and an empty file are all caught. # # @param path [String] the key file to inspect # @return [void] def warn_unless_private_key(path) - first_token = File.read(path).split.first - return unless PUBLIC_KEY_PREFIXES.include?(first_token) + first_line = File.open(path) do |file| + file.each_line.find { |line| !line.strip.empty? } + end + return if first_line&.start_with?(PRIVATE_KEY_HEADER) warnings << "SSH key #{path} is not a private key. Please check your kitchen.yml." end diff --git a/lib/kitchen/driver/cloudstack/server_options.rb b/lib/kitchen/driver/cloudstack/server_options.rb index 0a7d658..67d4485 100644 --- a/lib/kitchen/driver/cloudstack/server_options.rb +++ b/lib/kitchen/driver/cloudstack/server_options.rb @@ -47,7 +47,22 @@ class ServerOptions # Matches a string that is already valid base64, so user data supplied # pre-encoded is passed through rather than double-encoded. - BASE64_PATTERN = %r{^(?:[A-Za-z0-9+/]{4}\n?)*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$} + # + # Anchored with \A and \z rather than ^ and $, which in Ruby anchor to + # a line: a single blank line anywhere in the user data used to satisfy + # the whole pattern, and cloud-config and shell scripts are full of + # blank lines. At least one group is required so the empty string does + # not count as pre-encoded either. + # + # A short word made only of base64 characters is genuinely ambiguous + # and is still passed through; there is no way to tell it apart from + # data someone encoded themselves. + BASE64_PATTERN = %r{ + \A + (?:[A-Za-z0-9+/]{4}\n?)* + (?:[A-Za-z0-9+/]{4}|[A-Za-z0-9+/]{3}=|[A-Za-z0-9+/]{2}==) + \n?\z + }x # @param config [Hash] the driver configuration # @param instance_name [String] the Test Kitchen instance name @@ -128,12 +143,14 @@ def truncate_to_budget(parts, budget) # The user data to send, base64 encoded. # # Data that is already valid base64 is passed through untouched rather - # than being encoded a second time. + # than being encoded a second time. Encoding is strict so the result + # is one line: Base64.encode64 wraps at 60 characters, and those line + # breaks would go out inside an API query parameter. # # @return [String] base64-encoded user data def userdata data = config[:cloudstack_userdata] - data.match(BASE64_PATTERN) ? data : Base64.encode64(data) + data.match?(BASE64_PATTERN) ? data : Base64.strict_encode64(data) end end end diff --git a/spec/kitchen/driver/cloudstack/credentials_spec.rb b/spec/kitchen/driver/cloudstack/credentials_spec.rb index 05505a0..5c82e1c 100644 --- a/spec/kitchen/driver/cloudstack/credentials_spec.rb +++ b/spec/kitchen/driver/cloudstack/credentials_spec.rb @@ -115,6 +115,53 @@ def state_for(config, server_info = {}) expect(creds.warnings.join).to match(/not a private key/i) end + # The old check compared the first token against a short list of public + # key types, so anything outside that list -- and the list did not even + # name ssh-dss correctly -- was accepted as a private key. + it "warns about a public key type the old prefix list did not name" do + write_key(@search, "TestKey", "ecdsa-sha2-nistp521 AAAAE2VjZHNh user@host\n") + creds = described_class.new( + { cloudstack_ssh_keypair_name: "TestKey", keypair_search_directory: @search }, + home: @home, working_dir: @cwd + ) + creds.to_state({}) + + expect(creds.warnings.join).to match(/not a private key/i) + end + + it "warns when the file is a PuTTY key rather than a PEM" do + write_key(@search, "TestKey", "PuTTY-User-Key-File-3: ssh-ed25519\nEncryption: none\n") + creds = described_class.new( + { cloudstack_ssh_keypair_name: "TestKey", keypair_search_directory: @search }, + home: @home, working_dir: @cwd + ) + creds.to_state({}) + + expect(creds.warnings.join).to match(/not a private key/i) + end + + it "warns when the file is empty" do + write_key(@search, "TestKey", "") + creds = described_class.new( + { cloudstack_ssh_keypair_name: "TestKey", keypair_search_directory: @search }, + home: @home, working_dir: @cwd + ) + creds.to_state({}) + + expect(creds.warnings.join).to match(/not a private key/i) + end + + it "stays quiet about an OpenSSH format private key" do + write_key(@search, "TestKey", "-----BEGIN OPENSSH PRIVATE KEY-----\nb3BlbnNzaC1rZXk=\n") + creds = described_class.new( + { cloudstack_ssh_keypair_name: "TestKey", keypair_search_directory: @search }, + home: @home, working_dir: @cwd + ) + creds.to_state({}) + + expect(creds.warnings).to be_empty + end + it "warns when a keypair is named but no matching file is found" do creds = described_class.new( { cloudstack_ssh_keypair_name: "Missing" }, home: @home, working_dir: @cwd diff --git a/spec/kitchen/driver/cloudstack/server_options_spec.rb b/spec/kitchen/driver/cloudstack/server_options_spec.rb index 2c7f425..6191108 100644 --- a/spec/kitchen/driver/cloudstack/server_options_spec.rb +++ b/spec/kitchen/driver/cloudstack/server_options_spec.rb @@ -55,6 +55,35 @@ def options_for(config, instance_name: "default-ubuntu", login: "tsmith", hostna expect(opts[:userdata]).to eq(already_encoded) end + # A blank line used to satisfy the "already base64" check, because ^ and $ + # anchor to a line rather than to the whole string and every branch of the + # pattern is optional, so an empty line matched. Cloud-config and shell + # scripts are full of blank lines, so the ordinary case went to CloudStack + # unencoded. + it "base64-encodes userdata containing a blank line" do + data = "#cloud-config\n\npackages:\n - htop\n" + opts = options_for(base_config.merge(cloudstack_userdata: data)) + + expect(opts[:userdata]).not_to eq(data) + expect(Base64.decode64(opts[:userdata])).to eq(data) + end + + it "base64-encodes a shell script whose sections are separated by blank lines" do + data = "#!/bin/bash\n\necho hello\n" + opts = options_for(base_config.merge(cloudstack_userdata: data)) + + expect(opts[:userdata]).not_to eq(data) + expect(Base64.decode64(opts[:userdata])).to eq(data) + end + + it "sends userdata on one line so no line break reaches the query string" do + data = "#cloud-config\npackages:\n" + (" - htop\n" * 20) + opts = options_for(base_config.merge(cloudstack_userdata: data)) + + expect(opts[:userdata]).not_to include("\n") + expect(Base64.decode64(opts[:userdata])).to eq(data) + end + it "generates a display name from the instance name when none is configured" do opts = options_for(base_config, instance_name: "default-ubuntu")