Address review comments

app/helpers/form_helper.rb

 module FormHelper
   prepend ::EE::FormHelper
 
-  def form_errors(model, headline = 'The form contains the following')
+  def form_errors(model, type: 'form')
     return unless model.errors.any?
 
     pluralized = 'error'.pluralize(model.errors.count)
-
-    headline = headline + ' ' + pluralized + ':'
+    headline   = "The #{type} contains the following #{pluralized}:"
 
     content_tag(:div, class: 'alert alert-danger', id: 'error_explanation') do
       content_tag(:h4, headline) <<

app/models/application_setting.rb

@@ -16,13 +16,9 @@ class ApplicationSetting < ActiveRecord::Base
 
   # Setting a key restriction to `-1` means that all keys of this type are
   # forbidden.
-  FORBIDDEN_KEY_VALUE = -1
+  FORBIDDEN_KEY_VALUE = KeyRestrictionValidator::FORBIDDEN
   SUPPORTED_KEY_TYPES = %i[rsa dsa ecdsa ed25519].freeze
 
-  def self.supported_key_restrictions(type)
-    [0, *Gitlab::SSHPublicKey.supported_sizes(type), FORBIDDEN_KEY_VALUE]
-  end
-
   serialize :restricted_visibility_levels # rubocop:disable Cop/ActiveRecordSerialize
   serialize :import_sources # rubocop:disable Cop/ActiveRecordSerialize
   serialize :disabled_oauth_sign_in_sources, Array # rubocop:disable Cop/ActiveRecordSerialize
@@ -169,11 +165,11 @@ class ApplicationSetting < ActiveRecord::Base
             numericality: { greater_than_or_equal_to: 0 }
 
   SUPPORTED_KEY_TYPES.each do |type|
-    validates :"#{type}_key_restriction",
-              presence: true,
-              inclusion: { in: ApplicationSetting.supported_key_restrictions(type) }
+    validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type }
   end
 
+  validates :allowed_key_types, presence: true
+
   validates_each :restricted_visibility_levels do |record, attr, value|
     value&.each do |level|
       unless Gitlab::VisibilityLevel.options.value?(level)
@@ -489,8 +485,7 @@ class ApplicationSetting < ActiveRecord::Base
   def key_restriction_for(type)
     attr_name = "#{type}_key_restriction"
 
-    # rubocop:disable GitlabSecurity/PublicSend
-    has_attribute?(attr_name) ? public_send(attr_name) : FORBIDDEN_KEY_VALUE
+    has_attribute?(attr_name) ? public_send(attr_name) : FORBIDDEN_KEY_VALUE # rubocop:disable GitlabSecurity/PublicSend
   end
 
   private

app/models/key.rb

 require 'digest/md5'
 
 class Key < ActiveRecord::Base
-  include AfterCommitQueue
   include Gitlab::CurrentSettings
   include Sortable
 

app/validators/key_restriction_validator.rb

+class KeyRestrictionValidator < ActiveModel::EachValidator
+  FORBIDDEN = -1
+
+  def self.supported_sizes(type)
+    Gitlab::SSHPublicKey.supported_sizes(type)
+  end
+
+  def self.supported_key_restrictions(type)
+    [0, *supported_sizes(type), FORBIDDEN]
+  end
+
+  def validate_each(record, attribute, value)
+    unless valid_restriction?(value)
+      record.errors.add(attribute, "must be forbidden, allowed, or one of these sizes: #{supported_sizes_message}")
+    end
+  end
+
+  private
+
+  def supported_sizes_message
+    sizes = self.class.supported_sizes(options[:type])
+    sizes.to_sentence(last_word_connector: ', or ', two_words_connector: ' or ')
+  end
+
+  def valid_restriction?(value)
+    choices = self.class.supported_key_restrictions(options[:type])
+    choices.include?(value)
+  end
+end

app/views/profiles/keys/_key.html.haml

@@ -3,8 +3,8 @@
     - if key.valid?
       = icon 'key', class: 'settings-list-icon hidden-xs'
     - else
-      = icon 'exclamation-triangle', class: 'settings-list-icon hidden-xs',
-        title: 'The key is disabled because it is invalid'
+      = icon 'exclamation-triangle', class: 'settings-list-icon hidden-xs has-tooltip',
+        title: key.errors.full_messages.join(', ')
 
 
   .key-list-item-info

app/views/profiles/keys/_key_details.html.haml

@@ -16,7 +16,7 @@
           %strong= @key.last_used_at.try(:to_s, :medium) || 'N/A'
 
   .col-md-8
-    = form_errors(@key, 'The key has the following') unless @key.valid?
+    = form_errors(@key, type: 'key') unless @key.valid?
     %p
       %span.light Fingerprint:
       %code.key-fingerprint= @key.fingerprint

changelogs/unreleased/17849-allow-admin-to-restrict-min-key-length-and-techno.yml

 ---
-title: Add settings for minimum key strength and allowed key type
+title: Add settings for minimum SSH key strength and allowed key type
 merge_request: 13712
 author: Cory Hinshaw
 type: added

db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb

@@ -7,12 +7,13 @@ class AddMinimumKeyLengthToApplicationSettings < ActiveRecord::Migration
   disable_ddl_transaction!
 
   def up
-    # A key restriction has two possible states:
+    # A key restriction has these possible states:
     #
     #   * -1 means "this key type is completely disabled"
-    #   * >= 0 means "keys must have at least this many bits to be valid"
+    #   * 0 means "all keys of this type are valid"
+    #   * > 0 means "keys must have at least this many bits to be valid"
     #
-    # A value of 0 is equivalent to "there are no restrictions on keys of this type"
+    # The default is 0, for backward compatibility
     add_column_with_default :application_settings, :rsa_key_restriction, :integer, default: 0
     add_column_with_default :application_settings, :dsa_key_restriction, :integer, default: 0
     add_column_with_default :application_settings, :ecdsa_key_restriction, :integer, default: 0

doc/security/README.md

 # Security
 
 - [Password length limits](password_length_limits.md)
-- [Restrict allowed SSH key technologies and minimum length](ssh_keys_restrictions.md)
+- [Restrict SSH key technologies and minimum length](ssh_keys_restrictions.md)
 - [Rack attack](rack_attack.md)
 - [Webhooks and insecure internal web services](webhooks.md)
 - [Information exclusivity](information_exclusivity.md)

doc/security/ssh_keys_restrictions.md

@@ -2,12 +2,13 @@
 
 `ssh-keygen` allows users to create RSA keys with as few as 768 bits, which
 falls well below recommendations from certain standards groups (such as the US
-NIST). Some organizations deploying Gitlab will need to enforce minimum key
+NIST). Some organizations deploying GitLab will need to enforce minimum key
 strength, either to satisfy internal security policy or for regulatory
 compliance.
 
-Similarly, certain standards groups recommend using RSA or ECDSA over the older
-DSA and administrators may need to limit the allowed SSH key algorithms.
+Similarly, certain standards groups recommend using RSA, ECDSA, or ED25519 over
+the older DSA, and administrators may need to limit the allowed SSH key
+algorithms.
 
 GitLab allows you to restrict the allowed SSH key technology as well as specify
 the minimum key length for each technology.

lib/api/settings.rb

@@ -125,7 +125,7 @@ module API
       ApplicationSetting::SUPPORTED_KEY_TYPES.each do |type|
         optional :"#{type}_key_restriction",
                  type: Integer,
-                 values: ApplicationSetting.supported_key_restrictions(type),
+                 values: KeyRestrictionValidator.supported_key_restrictions(type),
                  desc: "Restrictions on the complexity of uploaded #{type.upcase} keys. A value of #{ApplicationSetting::FORBIDDEN_KEY_VALUE} disables all #{type.upcase} keys."
       end
 

lib/gitlab/git_access.rb

@@ -37,8 +37,8 @@ module Gitlab
     end
 
     def check(cmd, changes)
-      check_valid_actor!
       check_protocol!
+      check_valid_actor!
       check_active_user!
       check_project_accessibility!
       check_project_moved!

lib/gitlab/ssh_public_key.rb

@@ -13,6 +13,10 @@ module Gitlab
       Technologies.find { |tech| tech.name.to_s == name.to_s }
     end
 
+    def self.technology_for_key(key)
+      Technologies.find { |tech| key.is_a?(tech.key_class) }
+    end
+
     def self.supported_sizes(name)
       technology(name)&.supported_sizes
     end
@@ -37,9 +41,7 @@ module Gitlab
     end
 
     def type
-      return unless valid?
-
-      technology.name
+      technology.name if valid?
     end
 
     def bits
@@ -63,12 +65,7 @@ module Gitlab
 
     def technology
       @technology ||=
-        begin
-          tech = Technologies.find { |tech| key.is_a?(tech.key_class) }
-          raise "Unsupported key type: #{key.class}" unless tech
-
-          tech
-        end
+        self.class.technology_for_key(key) || raise("Unsupported key type: #{key.class}")
     end
   end
 end

spec/lib/gitlab/git_access_spec.rb

@@ -165,12 +165,10 @@ describe Gitlab::GitAccess do
         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 must be at least 4096 bits.')
-          expect { push_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.')
-        end
+      it 'does not allow keys which are too small', aggregate_failures: true do
+        expect(actor).not_to be_valid
+        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
 
@@ -179,12 +177,10 @@ describe Gitlab::GitAccess do
         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 forbidden/)
-          expect { push_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/)
-        end
+      it 'does not allow keys which are too small', aggregate_failures: true do
+        expect(actor).not_to be_valid
+        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

spec/models/application_setting_spec.rb

@@ -77,6 +77,15 @@ describe ApplicationSetting do
         expect(described_class::SUPPORTED_KEY_TYPES).to contain_exactly(:rsa, :dsa, :ecdsa, :ed25519)
       end
 
+      it 'does not allow all key types to be disabled' do
+        described_class::SUPPORTED_KEY_TYPES.each do |type|
+          setting["#{type}_key_restriction"] = described_class::FORBIDDEN_KEY_VALUE
+        end
+
+        expect(setting).not_to be_valid
+        expect(setting.errors.messages).to have_key(:allowed_key_types)
+      end
+
       where(:type) do
         described_class::SUPPORTED_KEY_TYPES
       end
@@ -85,7 +94,7 @@ describe ApplicationSetting do
         let(:field) { :"#{type}_key_restriction" }
 
         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.to allow_value(*KeyRestrictionValidator.supported_key_restrictions(type)).for(field) }
         it { is_expected.not_to allow_value(128).for(field) }
       end
     end