Merge branch...

Merge branch 'rd-43185-revert-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key' into 'master'

Revert "Merge branch 'rd-40552-gitlab-should-check-if-keys-are-valid-before-saving' into 'master'"

See merge request gitlab-org/gitlab-ce!17062

app/models/key.rb

@@ -33,8 +33,9 @@ class Key < ActiveRecord::Base
   after_destroy :refresh_user_cache
 
   def key=(value)
-    write_attribute(:key, value.present? ? Gitlab::SSHPublicKey.sanitize(value) : nil)
-
+    value&.delete!("\n\r")
+    value.strip! unless value.blank?
+    write_attribute(:key, value)
     @public_key = nil
   end
 
@@ -96,7 +97,7 @@ class Key < ActiveRecord::Base
   def generate_fingerprint
     self.fingerprint = nil
 
-    return unless public_key.valid?
+    return unless self.key.present?
 
     self.fingerprint = public_key.fingerprint
   end

changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml

----
-title: Sanitize extra blank spaces used when uploading a SSH key
-merge_request: 40552
-author:
-type: fixed

lib/gitlab/ssh_public_key.rb

@@ -21,22 +21,6 @@ module Gitlab
       technology(name)&.supported_sizes
     end
 
-    def self.sanitize(key_content)
-      ssh_type, *parts = key_content.strip.split
-
-      return key_content if parts.empty?
-
-      parts.each_with_object("#{ssh_type} ").with_index do |(part, content), index|
-        content << part
-
-        if Gitlab::SSHPublicKey.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
-        end
-      end
-    end
-
     attr_reader :key_text, :key
 
     # Unqualified MD5 fingerprint for compatibility
@@ -53,23 +37,23 @@ module Gitlab
     end
 
     def valid?
-      key.present? && bits && technology.supported_sizes.include?(bits)
+      key.present?
     end
 
     def type
-      technology.name if key.present?
+      technology.name 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.group.order.num_bits
       when :ed25519
         256
       else

spec/factories/keys.rb

@@ -5,10 +5,6 @@ FactoryBot.define do
     title
     key { Spec::Support::Helpers::KeyGeneratorHelper.new(1024).generate + ' dummy@gitlab.com' }
 
-    factory :key_without_comment do
-      key { Spec::Support::Helpers::KeyGeneratorHelper.new(1024).generate }
-    end
-
     factory :deploy_key, class: 'DeployKey'
 
     factory :personal_key do

spec/lib/gitlab/ssh_public_key_spec.rb

@@ -37,41 +37,6 @@ describe Gitlab::SSHPublicKey, lib: true do
     end
   end
 
-  describe '.sanitize(key_content)' do
-    let(:content) { build(:key).key }
-
-    context 'when key has blank space characters' do
-      it 'removes the extra blank space characters' do
-        unsanitized = content.insert(100, "\n")
-          .insert(40, "\r\n")
-          .insert(30, ' ')
-
-        sanitized = described_class.sanitize(unsanitized)
-        _, body = sanitized.split
-
-        expect(sanitized).not_to eq(unsanitized)
-        expect(body).not_to match(/\s/)
-      end
-    end
-
-    context "when key doesn't have blank space characters" do
-      it "doesn't modify the content" do
-        sanitized = described_class.sanitize(content)
-
-        expect(sanitized).to eq(content)
-      end
-    end
-
-    context "when key is invalid" do
-      it 'returns the original content' do
-        unsanitized = "ssh-foo any content=="
-        sanitized = described_class.sanitize(unsanitized)
-
-        expect(sanitized).to eq(unsanitized)
-      end
-    end
-  end
-
   describe '#valid?' do
     subject { public_key }
 

spec/models/key_spec.rb

@@ -72,52 +72,15 @@ describe Key, :mailer do
       expect(build(:key)).to be_valid
     end
 
-    it 'rejects the unfingerprintable key (not a key)' do
-      expect(build(:key, key: 'ssh-rsa an-invalid-key==')).not_to be_valid
-    end
-
-    where(:factory, :chars, :expected_sections) do
-      [
-        [:key,                 ["\n", "\r\n"], 3],
-        [:key,                 [' ', ' '],     3],
-        [:key_without_comment, [' ', ' '],     2]
-      ]
-    end
-
-    with_them do
-      let!(:key) { create(factory) }
-      let!(:original_fingerprint) { key.fingerprint }
-
-      it 'accepts a key with blank space characters after stripping them' do
-        modified_key = key.key.insert(100, chars.first).insert(40, chars.last)
-        _, content = modified_key.split
-
-        key.update!(key: modified_key)
-
+    it 'accepts a key with newline charecters after stripping them' do
+      key = build(:key)
+      key.key = key.key.insert(100, "\n")
+      key.key = key.key.insert(40, "\r\n")
       expect(key).to be_valid
-        expect(key.key.split.size).to eq(expected_sections)
-
-        expect(content).not_to match(/\s/)
-        expect(original_fingerprint).to eq(key.fingerprint)
-      end
-    end
-  end
-
-  context 'validate size' do
-    where(:key_content, :result) do
-      [
-        [Spec::Support::Helpers::KeyGeneratorHelper.new(512).generate,  false],
-        [Spec::Support::Helpers::KeyGeneratorHelper.new(8192).generate, false],
-        [Spec::Support::Helpers::KeyGeneratorHelper.new(1024).generate, true]
-      ]
     end
 
-    with_them do
-      it 'validates the size of the key' do
-        key = build(:key, key: key_content)
-
-        expect(key.valid?).to eq(result)
-      end
+    it 'rejects the unfingerprintable key (not a key)' do
+      expect(build(:key, key: 'ssh-rsa an-invalid-key==')).not_to be_valid
     end
   end