Merge branch 'use-ssh_data-gem' into 'master'

Use `ssh_data` gem instead of `net-ssh` and `sshkey` where possible

See merge request gitlab-org/gitlab!77424

Gemfile

@@ -468,7 +468,7 @@ gem 'net-ntp'
 
 # SSH host key support
 gem 'net-ssh', '~> 6.0'
-gem 'sshkey', '~> 2.0'
+gem 'ssh_data', '~> 1.2'
 
 # Required for ED25519 SSH host key support
 group :ed25519 do

Gemfile.lock

@@ -1232,7 +1232,7 @@ GEM
       activesupport (>= 4.0)
       sprockets (>= 3.0.0)
     sqlite3 (1.3.13)
-    sshkey (2.0.0)
+    ssh_data (1.2.0)
     ssrf_filter (1.0.7)
     stackprof (0.2.15)
     state_machines (0.5.0)
@@ -1639,7 +1639,7 @@ DEPENDENCIES
   spring-commands-rspec (~> 1.0.4)
   sprite-factory (~> 1.7)
   sprockets (~> 3.7.0)
-  sshkey (~> 2.0)
+  ssh_data (~> 1.2)
   stackprof (~> 0.2.15)
   state_machines-activerecord (~> 0.8.0)
   sys-filesystem (~> 1.4.3)

app/models/concerns/mirror_authentication.rb

@@ -4,11 +4,6 @@
 # implements support for persisting the necessary data in a `credentials`
 # serialized attribute. It also needs an `url` method to be defined
 module MirrorAuthentication
-  SSH_PRIVATE_KEY_OPTS = {
-    type: 'RSA',
-    bits: 4096
-  }.freeze
-
   extend ActiveSupport::Concern
 
   included do
@@ -84,10 +79,10 @@ module MirrorAuthentication
     return if ssh_private_key.blank?
 
     comment = "git@#{::Gitlab.config.gitlab.host}"
-    ::SSHKey.new(ssh_private_key, comment: comment).ssh_public_key
+    SSHData::PrivateKey.parse(ssh_private_key).first.public_key.openssh(comment: comment)
   end
 
   def generate_ssh_private_key!
-    self.ssh_private_key = ::SSHKey.generate(SSH_PRIVATE_KEY_OPTS).private_key
+    self.ssh_private_key = SSHData::PrivateKey::RSA.generate(4096).openssl.to_pem
   end
 end

app/models/instance_configuration.rb

@@ -147,7 +147,7 @@ class InstanceConfiguration
   end
 
   def ssh_algorithm_sha256(ssh_file_content)
-    Gitlab::SSHPublicKey.new(ssh_file_content).fingerprint('SHA256')
+    Gitlab::SSHPublicKey.new(ssh_file_content).fingerprint_sha256
   end
 
   def application_settings

app/models/key.rb

@@ -130,7 +130,7 @@ class Key < ApplicationRecord
     return unless public_key.valid?
 
     self.fingerprint_md5 = public_key.fingerprint
-    self.fingerprint_sha256 = public_key.fingerprint("SHA256").gsub("SHA256:", "")
+    self.fingerprint_sha256 = public_key.fingerprint_sha256.gsub("SHA256:", "")
   end
 
   def key_meets_restrictions

ee/spec/features/projects/mirror_spec.rb

@@ -202,7 +202,7 @@ RSpec.describe 'Project mirror', :js do
     end
 
     describe 'host key management', :use_clean_rails_memory_store_caching do
-      let(:key) { Gitlab::SSHPublicKey.new(SSHKeygen.generate) }
+      let(:key) { Gitlab::SSHPublicKey.new(SSHData::PrivateKey::RSA.generate(2048).public_key.openssh) }
       let(:cache) { SshHostKey.new(project: project, url: "ssh://example.com:22") }
 
       it 'fills fingerprints and host keys when detecting' do

ee/spec/models/project_import_data_spec.rb

@@ -143,9 +143,9 @@ RSpec.describe ProjectImportData do
 
       it 'returns the public counterpart of the SSH private key' do
         comment = "git@#{::Gitlab.config.gitlab.host}"
-        expected = SSHKey.new(import_data.ssh_private_key, comment: comment)
+        public_counterpart = SSHData::PrivateKey.parse(import_data.ssh_private_key).first.public_key.openssh(comment: comment)
 
-        is_expected.to eq(expected.ssh_public_key)
+        is_expected.to eq(public_counterpart)
       end
     end
   end

ee/spec/serializers/project_mirror_entity_spec.rb

@@ -45,7 +45,7 @@ RSpec.describe ProjectMirrorEntity do
     context 'SSH public-key authentication' do
       before do
         project.import_url = "ssh://example.com"
-        import_data.update!(auth_method: 'ssh_public_key', ssh_known_hosts: "example.com #{SSHKeygen.generate}")
+        import_data.update!(auth_method: 'ssh_public_key', ssh_known_hosts: "example.com #{SSHData::PrivateKey::RSA.generate(2048).public_key.openssh}")
       end
 
       it 'represents the pull mirror' do

ee/spec/support/ssh_keygen.rb

-# frozen_string_literal: true
-
-require 'openssl'
-require 'base64'
-
-#
-# Copyright:: Copyright (c) 2015 Chris Marchesi
-# Copyright:: Copyright (c) 2016 GitLab Inc
-# License:: Apache License, Version 2.0
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-#
-class SSHKeygen
-  class << self
-    def generate
-      "ssh-rsa #{openssh_rsa_public_key(generate_private_key)}"
-    end
-
-    private
-
-    def generate_private_key
-      ::OpenSSL::PKey::RSA.new(2048)
-    end
-
-    # Encode an OpenSSH RSA public key.
-    # Key format is PEM-encoded - size (big-endian), then data:
-    #  * Type (ie: len: 7 (size of string), data: ssh-rsa)
-    #  * Exponent (len/data)
-    #  * Modulus (len+1/NUL+data)
-    def openssh_rsa_public_key(private_key)
-      enc_type = "#{[7].pack('N')}ssh-rsa"
-      enc_exponent = "#{[private_key.public_key.e.num_bytes].pack('N')}#{private_key.public_key.e.to_s(2)}"
-      enc_modulus = "#{[private_key.public_key.n.num_bytes + 1].pack('N')}\0#{private_key.public_key.n.to_s(2)}"
-      Base64.strict_encode64("#{enc_type}#{enc_exponent}#{enc_modulus}")
-    end
-  end
-end

lib/gitlab/ssh_public_key.rb

@@ -7,10 +7,10 @@ module Gitlab
     # See https://man.openbsd.org/sshd#AUTHORIZED_KEYS_FILE_FORMAT for the list of
     # supported algorithms.
     TECHNOLOGIES = [
-      Technology.new(:rsa, OpenSSL::PKey::RSA, [1024, 2048, 3072, 4096], %w(ssh-rsa)),
-      Technology.new(:dsa, OpenSSL::PKey::DSA, [1024, 2048, 3072], %w(ssh-dss)),
-      Technology.new(:ecdsa, OpenSSL::PKey::EC, [256, 384, 521], %w(ecdsa-sha2-nistp256 ecdsa-sha2-nistp384 ecdsa-sha2-nistp521)),
-      Technology.new(:ed25519, Net::SSH::Authentication::ED25519::PubKey, [256], %w(ssh-ed25519))
+      Technology.new(:rsa, SSHData::PublicKey::RSA, [1024, 2048, 3072, 4096], %w(ssh-rsa)),
+      Technology.new(:dsa, SSHData::PublicKey::DSA, [1024, 2048, 3072], %w(ssh-dss)),
+      Technology.new(:ecdsa, SSHData::PublicKey::ECDSA, [256, 384, 521], %w(ecdsa-sha2-nistp256 ecdsa-sha2-nistp384 ecdsa-sha2-nistp521)),
+      Technology.new(:ed25519, SSHData::PublicKey::ED25519, [256], %w(ssh-ed25519))
     ].freeze
 
     def self.technology(name)
@@ -18,7 +18,7 @@ module Gitlab
     end
 
     def self.technology_for_key(key)
-      TECHNOLOGIES.find { |tech| key.is_a?(tech.key_class) }
+      TECHNOLOGIES.find { |tech| key.instance_of?(tech.key_class) }
     end
 
     def self.supported_types
@@ -45,7 +45,7 @@ module Gitlab
       parts.each_with_object(+"#{ssh_type} ").with_index do |(part, content), index|
         content << part
 
-        if Gitlab::SSHPublicKey.new(content).valid?
+        if self.new(content).valid?
           break [content, parts[index + 1]].compact.join(' ') # Add the comment part if present
         elsif parts.size == index + 1 # return original content if we've reached the last element
           break key_content
@@ -55,41 +55,49 @@ module Gitlab
 
     attr_reader :key_text, :key
 
-    # Unqualified MD5 fingerprint for compatibility
-    delegate :fingerprint, to: :key, allow_nil: true
-
     def initialize(key_text)
       @key_text = key_text
 
+      # We need to strip options to parse key with options or in known_hosts
+      # format. See https://man.openbsd.org/sshd#AUTHORIZED_KEYS_FILE_FORMAT
+      # and https://man.openbsd.org/sshd#SSH_KNOWN_HOSTS_FILE_FORMAT
+      key_text_without_options = @key_text.to_s.match(/(\A|\s)(#{self.class.supported_algorithms.join('|')}).*/).to_s
+
       @key =
         begin
-          Net::SSH::KeyFactory.load_data_public_key(key_text)
-        rescue StandardError, NotImplementedError
+          SSHData::PublicKey.parse_openssh(key_text_without_options)
+        rescue SSHData::DecodeError
         end
     end
 
     def valid?
-      SSHKey.valid_ssh_public_key?(key_text)
+      key.present?
     end
 
     def type
-      technology.name if key.present?
+      technology.name if valid?
+    end
+
+    def fingerprint
+      key.fingerprint(md5: true) if valid?
+    end
+
+    def fingerprint_sha256
+      'SHA256:' + key.fingerprint(md5: false) if valid?
     end
 
     def bits
-      return if key.blank?
+      return unless valid?
 
       case type
       when :rsa
-        key.n&.num_bits
+        key.n.num_bits
       when :dsa
-        key.p&.num_bits
+        key.p.num_bits
       when :ecdsa
-        key.group.order&.num_bits
+        key.openssl.group.order.num_bits
       when :ed25519
         256
-      else
-        raise "Unsupported key type: #{type}"
       end
     end
 
@@ -97,7 +105,11 @@ module Gitlab
 
     def technology
       @technology ||=
-        self.class.technology_for_key(key) || raise("Unsupported key type: #{key.class}")
+        self.class.technology_for_key(key) || raise_unsupported_key_type_error
+    end
+
+    def raise_unsupported_key_type_error
+      raise("Unsupported key type: #{key.class}")
     end
   end
 end

spec/lib/gitlab/ssh_public_key_spec.rb

@@ -129,6 +129,26 @@ RSpec.describe Gitlab::SSHPublicKey, lib: true do
         let(:key) { attributes_for(factory)[:key] }
 
         it { is_expected.to be_valid }
+
+        context 'when key begins with options' do
+          let(:key) { "restrict,command='dump /home' #{attributes_for(factory)[:key]}" }
+
+          it { is_expected.to be_valid }
+        end
+
+        context 'when key is in known_hosts format' do
+          context "when key begins with 'example.com'" do
+            let(:key) { "example.com #{attributes_for(factory)[:key]}" }
+
+            it { is_expected.to be_valid }
+          end
+
+          context "when key begins with '@revoked other.example.com'" do
+            let(:key) { "@revoked other.example.com #{attributes_for(factory)[:key]}" }
+
+            it { is_expected.to be_valid }
+          end
+        end
       end
     end
 
@@ -137,6 +157,40 @@ RSpec.describe Gitlab::SSHPublicKey, lib: true do
 
       it { is_expected.not_to be_valid }
     end
+
+    context 'when an unsupported SSH key algorithm' do
+      let(:key) { "unsupported-#{attributes_for(:rsa_key_2048)[:key]}" }
+
+      it { is_expected.not_to be_valid }
+    end
+  end
+
+  shared_examples 'raises error when the key is represented by a class that is not in the list of supported technologies' do
+    context 'when the key is represented by a class that is not in the list of supported technologies' do
+      it 'raises error' do
+        klass = Class.new
+        key = klass.new
+
+        allow(public_key).to receive(:key).and_return(key)
+
+        expect { subject }.to raise_error("Unsupported key type: #{key.class}")
+      end
+    end
+
+    context 'when the key is represented by a subclass of the class that is in the list of supported technologies' do
+      it 'raises error' do
+        rsa_subclass = Class.new(described_class.technology(:rsa).key_class) do
+          def initialize
+          end
+        end
+
+        key = rsa_subclass.new
+
+        allow(public_key).to receive(:key).and_return(key)
+
+        expect { subject }.to raise_error("Unsupported key type: #{key.class}")
+      end
+    end
   end
 
   describe '#type' do
@@ -162,6 +216,8 @@ RSpec.describe Gitlab::SSHPublicKey, lib: true do
 
       it { is_expected.to be_nil }
     end
+
+    include_examples 'raises error when the key is represented by a class that is not in the list of supported technologies'
   end
 
   describe '#bits' do
@@ -190,6 +246,8 @@ RSpec.describe Gitlab::SSHPublicKey, lib: true do
 
       it { is_expected.to be_nil }
     end
+
+    include_examples 'raises error when the key is represented by a class that is not in the list of supported technologies'
   end
 
   describe '#fingerprint' do
@@ -220,18 +278,18 @@ RSpec.describe Gitlab::SSHPublicKey, lib: true do
     end
   end
 
-  describe '#fingerprint in SHA256 format' do
-    subject { public_key.fingerprint("SHA256").gsub("SHA256:", "") if public_key.fingerprint("SHA256") }
+  describe '#fingerprint_sha256' do
+    subject { public_key.fingerprint_sha256 }
 
     where(:factory, :fingerprint_sha256) do
       [
-        [:rsa_key_2048, 'GdtgO0eHbwLB+mK47zblkoXujkqKRZjgMQrHH6Kks3E'],
-        [:rsa_key_4096, 'ByDU7hQ1JB95l6p53rHrffc4eXvEtqGUtQhS+Dhyy7g'],
-        [:rsa_key_5120, 'PCCupLbFHScm4AbEufbGDvhBU27IM0MVAor715qKQK8'],
-        [:rsa_key_8192, 'CtHFQAS+9Hb8z4vrv4gVQPsHjNN0WIZhWODaB1mQLs4'],
-        [:dsa_key_2048, '+a3DQ7cU5GM+gaYOfmc0VWNnykHQSuth3VRcCpWuYNI'],
-        [:ecdsa_key_256, 'C+I5k3D+IGeM6k5iBR1ZsphqTKV+7uvL/XZ5hcrTr7g'],
-        [:ed25519_key_256, 'DCKAjzxWrdOTjaGKBBjtCW8qY5++GaiAJflrHPmp6W0']
+        [:rsa_key_2048, 'SHA256:GdtgO0eHbwLB+mK47zblkoXujkqKRZjgMQrHH6Kks3E'],
+        [:rsa_key_4096, 'SHA256:ByDU7hQ1JB95l6p53rHrffc4eXvEtqGUtQhS+Dhyy7g'],
+        [:rsa_key_5120, 'SHA256:PCCupLbFHScm4AbEufbGDvhBU27IM0MVAor715qKQK8'],
+        [:rsa_key_8192, 'SHA256:CtHFQAS+9Hb8z4vrv4gVQPsHjNN0WIZhWODaB1mQLs4'],
+        [:dsa_key_2048, 'SHA256:+a3DQ7cU5GM+gaYOfmc0VWNnykHQSuth3VRcCpWuYNI'],
+        [:ecdsa_key_256, 'SHA256:C+I5k3D+IGeM6k5iBR1ZsphqTKV+7uvL/XZ5hcrTr7g'],
+        [:ed25519_key_256, 'SHA256:DCKAjzxWrdOTjaGKBBjtCW8qY5++GaiAJflrHPmp6W0']
       ]
     end
 
@@ -249,10 +307,19 @@ RSpec.describe Gitlab::SSHPublicKey, lib: true do
   end
 
   describe '#key_text' do
-    let(:key) { 'this is not a key' }
+    where(:key_value) do
+      [
+        'this is not a key',
+        nil
+      ]
+    end
 
-    it 'carries the unmodified key data' do
-      expect(public_key.key_text).to eq(key)
+    with_them do
+      let(:key) { key_value }
+
+      it 'carries the unmodified key data' do
+        expect(public_key.key_text).to eq(key)
+      end
     end
   end
 end

spec/validators/x509_certificate_credentials_validator_spec.rb

@@ -55,6 +55,14 @@ RSpec.describe X509CertificateCredentialsValidator do
       expect(record.errors[:private_key]).to include('could not read private key, is the passphrase correct?')
     end
 
+    it 'adds an error when private key does not match certificate' do
+      record.private_key = SSHData::PrivateKey::RSA.generate(4096).openssl.to_pem
+
+      validator.validate(record)
+
+      expect(record.errors[:private_key]).to include('private key does not match certificate.')
+    end
+
     it 'has no error when the private key is correct' do
       record.private_key = pkey_data
 
@@ -85,7 +93,7 @@ RSpec.describe X509CertificateCredentialsValidator do
 
       validator.validate(record)
 
-      expect(record.errors[:private_key]).not_to be_empty
+      expect(record.errors[:private_key]).to include('could not read private key, is the passphrase correct?')
     end
   end
 end