Rework the permissions model for SSH key restrictions

`allowed_key_types` is removed and the `minimum_<type>_bits` fields are
renamed to `<tech>_key_restriction`. A special sentinel value (`-1`) signifies
that the key type is disabled.

This also feeds through to the UI - checkboxes per key type are out, inline
selection of "forbidden" and "allowed" (i.e., no restrictions) are in.

As with the previous model, unknown key types are disallowed, even if the
underlying ssh daemon happens to support them. The defaults have also been
changed from the lowest known bit size to "no restriction". So if someone
does happen to have a 768-bit RSA key, it will continue to work on upgrade, at
least until the administrator restricts them.

7 files changed, 109 insertions, 124 deletions

diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb
index 9698dead67a..563818e8761 100644
--- a/spec/features/admin/admin_settings_spec.rb
+++ b/spec/features/admin/admin_settings_spec.rb
@@ -80,23 +80,19 @@ feature 'Admin updates settings' do
   end
 
   scenario 'Change Keys settings' do
-    uncheck 'RSA'
-    uncheck 'DSA'
-    uncheck 'ED25519'
-    select '384', from: 'Minimum ECDSA key length'
+    select 'Are forbidden', from: 'RSA SSH keys'
+    select 'Are allowed', from: 'DSA SSH keys'
+    select 'Must be at least 384 bits', from: 'ECDSA SSH keys'
+    select 'Are forbidden', from: 'ED25519 SSH keys'
     click_on 'Save'
 
-    expect(page).to have_content 'Application settings saved successfully'
-
-    expect(find_field('RSA', checked: false)).not_to be_checked
-    expect(find_field('DSA', checked: false)).not_to be_checked
-    expect(find_field('ED25519', checked: false)).not_to be_checked
-    expect(find_field('Minimum ECDSA key length').value).to eq '384'
+    forbidden = ApplicationSetting::FORBIDDEN_KEY_VALUE.to_s
 
-    uncheck 'ECDSA'
-    click_on 'Save'
-
-    expect(page).to have_content "Allowed key types can't be blank"
+    expect(page).to have_content 'Application settings saved successfully'
+    expect(find_field('RSA SSH keys').value).to eq(forbidden)
+    expect(find_field('DSA SSH keys').value).to eq('0')
+    expect(find_field('ECDSA SSH keys').value).to eq('384')
+    expect(find_field('ED25519 SSH keys').value).to eq(forbidden)
   end
 
   def check_all_events
diff --git a/spec/features/profiles/keys_spec.rb b/spec/features/profiles/keys_spec.rb
index a4ccf222b50..aa71c4dbba4 100644
--- a/spec/features/profiles/keys_spec.rb
+++ b/spec/features/profiles/keys_spec.rb
@@ -31,7 +31,8 @@ feature 'Profile > SSH Keys' do
 
     context 'when only DSA and ECDSA keys are allowed' do
       before do
-        stub_application_setting(allowed_key_types: %w[dsa ecdsa])
+        forbidden = ApplicationSetting::FORBIDDEN_KEY_VALUE
+        stub_application_setting(rsa_key_restriction: forbidden, ed25519_key_restriction: forbidden)
       end
 
       scenario 'shows a validation error' do
@@ -41,7 +42,7 @@ feature 'Profile > SSH Keys' do
         fill_in('Title', with: attrs[:title])
         click_button('Add key')
 
-        expect(page).to have_content('Key type is not allowed. Must be DSA or ECDSA')
+        expect(page).to have_content('Key type is forbidden. Must be DSA or ECDSA')
       end
     end
   end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index a67902c7209..9e4174ecdca 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -162,28 +162,28 @@ describe Gitlab::GitAccess do
 
     context 'key is too small' do
       before do
-        stub_application_setting(minimum_rsa_bits: 4096)
+        stub_application_setting(rsa_key_restriction: 4096)
       end
 
       it 'does not allow keys which are too small' do
         aggregate_failures do
           expect(actor).not_to be_valid
-          expect { pull_access_check }.to raise_unauthorized('Your SSH key length must be at least 4096 bits.')
-          expect { push_access_check }.to raise_unauthorized('Your SSH key length must be at least 4096 bits.')
+          expect { pull_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.')
+          expect { push_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.')
         end
       end
     end
 
     context 'key type is not allowed' do
       before do
-        stub_application_setting(allowed_key_types: ['ecdsa'])
+        stub_application_setting(rsa_key_restriction: ApplicationSetting::FORBIDDEN_KEY_VALUE)
       end
 
       it 'does not allow keys which are too small' do
         aggregate_failures do
           expect(actor).not_to be_valid
-          expect { pull_access_check }.to raise_unauthorized('Your SSH key type is not allowed. Must be ECDSA.')
-          expect { push_access_check }.to raise_unauthorized('Your SSH key type is not allowed. Must be ECDSA.')
+          expect { pull_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/)
+          expect { push_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/)
         end
       end
     end
diff --git a/spec/lib/gitlab/ssh_public_key_spec.rb b/spec/lib/gitlab/ssh_public_key_spec.rb
index d3314552d31..93d538141ce 100644
--- a/spec/lib/gitlab/ssh_public_key_spec.rb
+++ b/spec/lib/gitlab/ssh_public_key_spec.rb
@@ -4,32 +4,36 @@ describe Gitlab::SSHPublicKey, lib: true do
   let(:key) { attributes_for(:rsa_key_2048)[:key] }
   let(:public_key) { described_class.new(key) }
 
-  describe '.technology_names' do
-    it 'returns the available technology names' do
-      expect(described_class.technology_names).to eq(%w[rsa dsa ecdsa ed25519])
+  describe '.technology(name)' do
+    it 'returns nil for an unrecognised name' do
+      expect(described_class.technology(:foo)).to be_nil
+    end
+
+    where(:name) do
+      [:rsa, :dsa, :ecdsa, :ed25519]
+    end
+
+    with_them do
+      it { expect(described_class.technology(name).name).to eq(name) }
+      it { expect(described_class.technology(name.to_s).name).to eq(name) }
     end
   end
 
-  describe '.allowed_sizes(name)' do
+  describe '.supported_sizes(name)' do
     where(:name, :sizes) do
       [
-        ['rsa', [1024, 2048, 3072, 4096]],
-        ['dsa', [1024, 2048, 3072]],
-        ['ecdsa', [256, 384, 521]],
-        ['ed25519', [256]]
+        [:rsa, [1024, 2048, 3072, 4096]],
+        [:dsa, [1024, 2048, 3072]],
+        [:ecdsa, [256, 384, 521]],
+        [:ed25519, [256]]
       ]
     end
 
-    subject { described_class.allowed_sizes(name) }
+    subject { described_class.supported_sizes(name) }
 
     with_them do
-      it { is_expected.to eq(sizes) }
-    end
-  end
-
-  describe '.allowed_type?' do
-    it 'determines the key type' do
-      expect(described_class.allowed_type?('foo')).to be(false)
+      it { expect(described_class.supported_sizes(name)).to eq(sizes) }
+      it { expect(described_class.supported_sizes(name.to_s)).to eq(sizes) }
     end
   end
 
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 44d473db07d..0435aa9dfe1 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -72,25 +72,22 @@ describe ApplicationSetting do
         .is_greater_than(0)
     end
 
-    it { is_expected.to validate_presence_of(:minimum_rsa_bits) }
-    it { is_expected.to allow_value(*Gitlab::SSHPublicKey.allowed_sizes('rsa')).for(:minimum_rsa_bits) }
-    it { is_expected.not_to allow_value(128).for(:minimum_rsa_bits) }
-
-    it { is_expected.to validate_presence_of(:minimum_dsa_bits) }
-    it { is_expected.to allow_value(*Gitlab::SSHPublicKey.allowed_sizes('dsa')).for(:minimum_dsa_bits) }
-    it { is_expected.not_to allow_value(128).for(:minimum_dsa_bits) }
+    context 'key restrictions' do
+      it 'supports all key types' do
+        expect(described_class::SUPPORTED_KEY_TYPES).to contain_exactly(:rsa, :dsa, :ecdsa, :ed25519)
+      end
 
-    it { is_expected.to validate_presence_of(:minimum_ecdsa_bits) }
-    it { is_expected.to allow_value(*Gitlab::SSHPublicKey.allowed_sizes('ecdsa')).for(:minimum_ecdsa_bits) }
-    it { is_expected.not_to allow_value(128).for(:minimum_ecdsa_bits) }
+      where(:type) do
+        described_class::SUPPORTED_KEY_TYPES
+      end
 
-    it { is_expected.to validate_presence_of(:minimum_ed25519_bits) }
-    it { is_expected.to allow_value(*Gitlab::SSHPublicKey.allowed_sizes('ed25519')).for(:minimum_ed25519_bits) }
-    it { is_expected.not_to allow_value(128).for(:minimum_ed25519_bits) }
+      with_them do
+        let(:field) { :"#{type}_key_restriction" }
 
-    describe 'allowed_key_types validations' do
-      it { is_expected.to allow_value(Gitlab::SSHPublicKey.technology_names).for(:allowed_key_types) }
-      it { is_expected.not_to allow_value(['foo']).for(:allowed_key_types) }
+        it { is_expected.to validate_presence_of(field) }
+        it { is_expected.to allow_value(*described_class.supported_key_restrictions(type)).for(field) }
+        it { is_expected.not_to allow_value(128).for(field) }
+      end
     end
 
     it_behaves_like 'an object with email-formated attributes', :admin_notification_email do
@@ -463,15 +460,35 @@ describe ApplicationSetting do
     end
   end
 
-  context 'allowed key types attribute' do
-    it 'set value with array of symbols' do
-      setting.allowed_key_types = [:rsa]
-      expect(setting.allowed_key_types).to contain_exactly(:rsa)
+  describe '#allowed_key_types' do
+    it 'includes all key types by default' do
+      expect(setting.allowed_key_types).to contain_exactly(*described_class::SUPPORTED_KEY_TYPES)
+    end
+
+    it 'excludes disabled key types' do
+      expect(setting.allowed_key_types).to include(:ed25519)
+
+      setting.ed25519_key_restriction = described_class::FORBIDDEN_KEY_VALUE
+
+      expect(setting.allowed_key_types).not_to include(:ed25519)
+    end
+  end
+
+  describe '#key_restriction_for' do
+    it 'returns the restriction value for recognised types' do
+      setting.rsa_key_restriction = 1024
+
+      expect(setting.key_restriction_for(:rsa)).to eq(1024)
+    end
+
+    it 'allows types to be passed as a string' do
+      setting.rsa_key_restriction = 1024
+
+      expect(setting.key_restriction_for('rsa')).to eq(1024)
     end
 
-    it 'get value as array of symbols' do
-      setting.allowed_key_types = ['rsa']
-      expect(setting.allowed_key_types).to eq(['rsa'])
+    it 'returns forbidden for unrecognised type' do
+      expect(setting.key_restriction_for(:foo)).to eq(described_class::FORBIDDEN_KEY_VALUE)
     end
   end
 end
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index 83b11baa371..96baeaff0a4 100644
--- a/spec/models/key_spec.rb
+++ b/spec/models/key_spec.rb
@@ -104,19 +104,34 @@ describe Key, :mailer do
     end
   end
 
-  context 'validate it meets minimum bit length' do
+  context 'validate it meets key restrictions' do
     where(:factory, :minimum, :result) do
+      forbidden = ApplicationSetting::FORBIDDEN_KEY_VALUE
+
       [
+        [:rsa_key_2048,    0, true],
+        [:dsa_key_2048,    0, true],
+        [:ecdsa_key_256,   0, true],
+        [:ed25519_key_256, 0, true],
+
         [:rsa_key_2048, 1024, true],
         [:rsa_key_2048, 2048, true],
         [:rsa_key_2048, 4096, false],
+
         [:dsa_key_2048, 1024, true],
         [:dsa_key_2048, 2048, true],
         [:dsa_key_2048, 4096, false],
+
         [:ecdsa_key_256, 256, true],
         [:ecdsa_key_256, 384, false],
+
         [:ed25519_key_256, 256, true],
-        [:ed25519_key_256, 384, false]
+        [:ed25519_key_256, 384, false],
+
+        [:rsa_key_2048,    forbidden, false],
+        [:dsa_key_2048,    forbidden, false],
+        [:ecdsa_key_256,   forbidden, false],
+        [:ed25519_key_256, forbidden, false]
       ]
     end
 
@@ -124,58 +139,13 @@ describe Key, :mailer do
       subject(:key) { build(factory) }
 
       before do
-        stub_application_setting("minimum_#{key.public_key.type}_bits" => minimum)
+        stub_application_setting("#{key.public_key.type}_key_restriction" => minimum)
       end
 
       it { expect(key.valid?).to eq(result) }
     end
   end
 
-  context 'validate the key type is allowed' do
-    it 'accepts RSA, DSA, ECDSA and ED25519 keys by default' do
-      expect(build(:rsa_key_2048)).to be_valid
-      expect(build(:dsa_key_2048)).to be_valid
-      expect(build(:ecdsa_key_256)).to be_valid
-      expect(build(:ed25519_key_256)).to be_valid
-    end
-
-    it 'rejects RSA, ECDSA and ED25519 keys if DSA is the only allowed type' do
-      stub_application_setting(allowed_key_types: ['dsa'])
-
-      expect(build(:rsa_key_2048)).not_to be_valid
-      expect(build(:dsa_key_2048)).to be_valid
-      expect(build(:ecdsa_key_256)).not_to be_valid
-      expect(build(:ed25519_key_256)).not_to be_valid
-    end
-
-    it 'rejects RSA, DSA and ED25519 keys if ECDSA is the only allowed type' do
-      stub_application_setting(allowed_key_types: ['ecdsa'])
-
-      expect(build(:rsa_key_2048)).not_to be_valid
-      expect(build(:dsa_key_2048)).not_to be_valid
-      expect(build(:ecdsa_key_256)).to be_valid
-      expect(build(:ed25519_key_256)).not_to be_valid
-    end
-
-    it 'rejects DSA, ECDSA and ED25519 keys if RSA is the only allowed type' do
-      stub_application_setting(allowed_key_types: ['rsa'])
-
-      expect(build(:rsa_key_2048)).to be_valid
-      expect(build(:dsa_key_2048)).not_to be_valid
-      expect(build(:ecdsa_key_256)).not_to be_valid
-      expect(build(:ed25519_key_256)).not_to be_valid
-    end
-
-    it 'rejects RSA, DSA and ECDSA keys if ED25519 is the only allowed type' do
-      stub_application_setting(allowed_key_types: ['ed25519'])
-
-      expect(build(:rsa_key_2048)).not_to be_valid
-      expect(build(:dsa_key_2048)).not_to be_valid
-      expect(build(:ecdsa_key_256)).not_to be_valid
-      expect(build(:ed25519_key_256)).to be_valid
-    end
-  end
-
   context 'callbacks' do
     it 'adds new key to authorized_file' do
       key = build(:personal_key, id: 7)
diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb
index 60e7c2d0da3..0b9a4b5c3db 100644
--- a/spec/requests/api/settings_spec.rb
+++ b/spec/requests/api/settings_spec.rb
@@ -19,11 +19,10 @@ describe API::Settings, 'Settings' do
       expect(json_response['default_project_visibility']).to be_a String
       expect(json_response['default_snippet_visibility']).to be_a String
       expect(json_response['default_group_visibility']).to be_a String
-      expect(json_response['minimum_rsa_bits']).to eq(1024)
-      expect(json_response['minimum_dsa_bits']).to eq(1024)
-      expect(json_response['minimum_ecdsa_bits']).to eq(256)
-      expect(json_response['minimum_ed25519_bits']).to eq(256)
-      expect(json_response['allowed_key_types']).to contain_exactly('rsa', 'dsa', 'ecdsa', 'ed25519')
+      expect(json_response['rsa_key_restriction']).to eq(0)
+      expect(json_response['dsa_key_restriction']).to eq(0)
+      expect(json_response['ecdsa_key_restriction']).to eq(0)
+      expect(json_response['ed25519_key_restriction']).to eq(0)
     end
   end
 
@@ -50,11 +49,10 @@ describe API::Settings, 'Settings' do
           help_page_hide_commercial_content: true,
           help_page_support_url: 'http://example.com/help',
           project_export_enabled: false,
-          minimum_rsa_bits: 2048,
-          minimum_dsa_bits: 2048,
-          minimum_ecdsa_bits: 384,
-          minimum_ed25519_bits: 256,
-          allowed_key_types: ['rsa']
+          rsa_key_restriction: ApplicationSetting::FORBIDDEN_KEY_VALUE,
+          dsa_key_restriction: 2048,
+          ecdsa_key_restriction: 384,
+          ed25519_key_restriction: 256
 
         expect(response).to have_http_status(200)
         expect(json_response['default_projects_limit']).to eq(3)
@@ -71,11 +69,10 @@ describe API::Settings, 'Settings' do
         expect(json_response['help_page_hide_commercial_content']).to be_truthy
         expect(json_response['help_page_support_url']).to eq('http://example.com/help')
         expect(json_response['project_export_enabled']).to be_falsey
-        expect(json_response['minimum_rsa_bits']).to eq(2048)
-        expect(json_response['minimum_dsa_bits']).to eq(2048)
-        expect(json_response['minimum_ecdsa_bits']).to eq(384)
-        expect(json_response['minimum_ed25519_bits']).to eq(256)
-        expect(json_response['allowed_key_types']).to eq(['rsa'])
+        expect(json_response['rsa_key_restriction']).to eq(ApplicationSetting::FORBIDDEN_KEY_VALUE)
+        expect(json_response['dsa_key_restriction']).to eq(2048)
+        expect(json_response['ecdsa_key_restriction']).to eq(384)
+        expect(json_response['ed25519_key_restriction']).to eq(256)
       end
     end