summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRubén Dávila <ruben@gitlab.com>2018-02-01 17:00:16 -0500
committerRubén Dávila <ruben@gitlab.com>2018-02-01 17:00:16 -0500
commit972f564dcd897ee66799c19f96ed22df588f8a18 (patch)
tree85e88470019c9e9fbce725f81d12dfef03872e66
parent078dac4268cb5e991cc0012f5ad01ca3cb9a39c8 (diff)
downloadgitlab-ce-972f564dcd897ee66799c19f96ed22df588f8a18.tar.gz
Sanitize extra blank spaces used when uploading a SSH key
-rw-r--r--app/models/key.rb7
-rw-r--r--changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml5
-rw-r--r--lib/gitlab/ssh_public_key.rb28
-rw-r--r--spec/factories/keys.rb4
-rw-r--r--spec/lib/gitlab/ssh_public_key_spec.rb35
-rw-r--r--spec/models/key_spec.rb51
6 files changed, 113 insertions, 17 deletions
diff --git a/app/models/key.rb b/app/models/key.rb
index a3f8a5d6dc7..cfa009f212f 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -34,9 +34,8 @@ class Key < ActiveRecord::Base
after_destroy :refresh_user_cache
def key=(value)
- value&.delete!("\n\r")
- value.strip! unless value.blank?
- write_attribute(:key, value)
+ write_attribute(:key, value.present? ? Gitlab::SSHPublicKey.sanitize(value) : nil)
+
@public_key = nil
end
@@ -98,7 +97,7 @@ class Key < ActiveRecord::Base
def generate_fingerprint
self.fingerprint = nil
- return unless self.key.present?
+ return unless public_key.valid?
self.fingerprint = public_key.fingerprint
end
diff --git a/changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml b/changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml
new file mode 100644
index 00000000000..9e4811ca308
--- /dev/null
+++ b/changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml
@@ -0,0 +1,5 @@
+---
+title: Sanitize extra blank spaces used when uploading a SSH key
+merge_request: 40552
+author:
+type: fixed
diff --git a/lib/gitlab/ssh_public_key.rb b/lib/gitlab/ssh_public_key.rb
index 89ca1298120..545e7c74f7e 100644
--- a/lib/gitlab/ssh_public_key.rb
+++ b/lib/gitlab/ssh_public_key.rb
@@ -21,6 +21,22 @@ 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
@@ -37,23 +53,23 @@ module Gitlab
end
def valid?
- key.present?
+ key.present? && bits && technology.supported_sizes.include?(bits)
end
def type
- technology.name if valid?
+ technology.name if key.present?
end
def bits
- return unless valid?
+ return if key.blank?
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
diff --git a/spec/factories/keys.rb b/spec/factories/keys.rb
index f0c43f3d6f5..23a98a899f1 100644
--- a/spec/factories/keys.rb
+++ b/spec/factories/keys.rb
@@ -5,6 +5,10 @@ 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
diff --git a/spec/lib/gitlab/ssh_public_key_spec.rb b/spec/lib/gitlab/ssh_public_key_spec.rb
index 93d538141ce..c15e29774b6 100644
--- a/spec/lib/gitlab/ssh_public_key_spec.rb
+++ b/spec/lib/gitlab/ssh_public_key_spec.rb
@@ -37,6 +37,41 @@ 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 }
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index 4cd9e3f4f1d..f620e13e16d 100644
--- a/spec/models/key_spec.rb
+++ b/spec/models/key_spec.rb
@@ -79,16 +79,53 @@ describe Key, :mailer do
expect(build(:key)).to be_valid
end
- 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
- 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)
+
+ 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
+ end
end
context 'validate it meets key restrictions' do