fix: user data with a blank line was sent unencoded, and only four public key types were recognised

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

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

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

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")