Commit a58f8c32 authored by Douwe Maan's avatar Douwe Maan

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

Sanitize extra blank spaces used when uploading a SSH key

Closes #40552

See merge request gitlab-org/gitlab-ce!16821
parents cd5d75c3 972f564d
...@@ -33,9 +33,8 @@ class Key < ActiveRecord::Base ...@@ -33,9 +33,8 @@ class Key < ActiveRecord::Base
after_destroy :refresh_user_cache after_destroy :refresh_user_cache
def key=(value) def key=(value)
value&.delete!("\n\r") write_attribute(:key, value.present? ? Gitlab::SSHPublicKey.sanitize(value) : nil)
value.strip! unless value.blank?
write_attribute(:key, value)
@public_key = nil @public_key = nil
end end
...@@ -97,7 +96,7 @@ class Key < ActiveRecord::Base ...@@ -97,7 +96,7 @@ class Key < ActiveRecord::Base
def generate_fingerprint def generate_fingerprint
self.fingerprint = nil self.fingerprint = nil
return unless self.key.present? return unless public_key.valid?
self.fingerprint = public_key.fingerprint self.fingerprint = public_key.fingerprint
end end
......
---
title: Sanitize extra blank spaces used when uploading a SSH key
merge_request: 40552
author:
type: fixed
...@@ -21,6 +21,22 @@ module Gitlab ...@@ -21,6 +21,22 @@ module Gitlab
technology(name)&.supported_sizes technology(name)&.supported_sizes
end 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 attr_reader :key_text, :key
# Unqualified MD5 fingerprint for compatibility # Unqualified MD5 fingerprint for compatibility
...@@ -37,23 +53,23 @@ module Gitlab ...@@ -37,23 +53,23 @@ module Gitlab
end end
def valid? def valid?
key.present? key.present? && bits && technology.supported_sizes.include?(bits)
end end
def type def type
technology.name if valid? technology.name if key.present?
end end
def bits def bits
return unless valid? return if key.blank?
case type case type
when :rsa when :rsa
key.n.num_bits key.n&.num_bits
when :dsa when :dsa
key.p.num_bits key.p&.num_bits
when :ecdsa when :ecdsa
key.group.order.num_bits key.group.order&.num_bits
when :ed25519 when :ed25519
256 256
else else
......
...@@ -5,6 +5,10 @@ FactoryBot.define do ...@@ -5,6 +5,10 @@ FactoryBot.define do
title title
key { Spec::Support::Helpers::KeyGeneratorHelper.new(1024).generate + ' dummy@gitlab.com' } 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 :deploy_key, class: 'DeployKey'
factory :personal_key do factory :personal_key do
......
...@@ -37,6 +37,41 @@ describe Gitlab::SSHPublicKey, lib: true do ...@@ -37,6 +37,41 @@ describe Gitlab::SSHPublicKey, lib: true do
end end
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 describe '#valid?' do
subject { public_key } subject { public_key }
......
...@@ -72,15 +72,52 @@ describe Key, :mailer do ...@@ -72,15 +72,52 @@ describe Key, :mailer do
expect(build(:key)).to be_valid expect(build(:key)).to be_valid
end end
it 'accepts a key with newline charecters after stripping them' do it 'rejects the unfingerprintable key (not a key)' do
key = build(:key) expect(build(:key, key: 'ssh-rsa an-invalid-key==')).not_to be_valid
key.key = key.key.insert(100, "\n") end
key.key = key.key.insert(40, "\r\n")
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).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 end
it 'rejects the unfingerprintable key (not a key)' do context 'validate size' do
expect(build(:key, key: 'ssh-rsa an-invalid-key==')).not_to be_valid 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
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment