Add a setting to enforce minimum DSA key length defaulting to 2048

Signed-off-by: Rémy Coutable <remy@rymai.me>

12 files changed, 41 insertions, 13 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 1818e724944..24078afa075 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -126,6 +126,10 @@ class ApplicationSetting < ActiveRecord::Base
             presence: true,
             inclusion: { in: Gitlab::SSHPublicKey.allowed_sizes('ecdsa') }
 
+  validates :minimum_dsa_bits,
+            presence: true,
+            inclusion: { in: Gitlab::SSHPublicKey.allowed_sizes('dsa') }
+
   validates_each :restricted_visibility_levels do |record, attr, value|
     value&.each do |level|
       unless Gitlab::VisibilityLevel.options.has_value?(level)
@@ -206,6 +210,7 @@ class ApplicationSetting < ActiveRecord::Base
       koding_url: nil,
       max_artifacts_size: Settings.artifacts['max_size'],
       max_attachment_size: Settings.gitlab['max_attachment_size'],
+      minimum_dsa_bits: 2048,
       minimum_ecdsa_bits: 256,
       minimum_rsa_bits: 2048,
       plantuml_enabled: false,
diff --git a/app/models/key.rb b/app/models/key.rb
index d8ba15da8ea..295680d1ae4 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -76,7 +76,7 @@ class Key < ActiveRecord::Base
     GitlabShellWorker.perform_async(
       :remove_key,
       shell_id,
-      key,
+      key
     )
   end
 
@@ -104,12 +104,18 @@ class Key < ActiveRecord::Base
       if public_key.size < current_application_settings.minimum_rsa_bits
         errors.add(:key, "length must be at least #{current_application_settings.minimum_rsa_bits} bits")
       end
+    when :dsa
+      if public_key.size < current_application_settings.minimum_dsa_bits
+        errors.add(:key, "length must be at least #{current_application_settings.minimum_dsa_bits} bits")
+      end
     end
   end
 
   def key_type_is_allowed
     unless current_application_settings.allowed_key_types.include?(public_key.type.to_s)
-      allowed_types = current_application_settings.allowed_key_types.map(&:upcase).to_sentence(last_word_connector: ', or ', two_words_connector: ' or ')
+      allowed_types = current_application_settings.allowed_key_types.
+        map(&:upcase).
+        to_sentence(last_word_connector: ', or ', two_words_connector: ' or ')
       errors.add(:key, "type is not allowed. Must be #{allowed_types}")
     end
   end
diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml
index f98022297a8..2df1dd4c615 100644
--- a/app/views/admin/application_settings/_form.html.haml
+++ b/app/views/admin/application_settings/_form.html.haml
@@ -68,6 +68,13 @@
         .help-block
           The minimum elliptic curve size for user ECDSA SSH keys (in bits)
 
+    .form-group
+      = f.label :minimum_dsa_bits, 'Minimum DSA key length', class: 'control-label col-sm-2'
+      .col-sm-10
+        = f.select :minimum_dsa_bits, Gitlab::SSHPublicKey.allowed_sizes('dsa'), {}, class: 'form-control'
+        .help-block
+          The minimum length for user DSA SSH keys (in bits)
+
   %fieldset
     %legend Account and Limit Settings
     .form-group
diff --git a/db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb b/db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb
index a7b12501e28..fb916a48335 100644
--- a/db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb
+++ b/db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb
@@ -9,12 +9,14 @@ class AddMinimumKeyLengthToApplicationSettings < ActiveRecord::Migration
   def up
     add_column_with_default :application_settings, :minimum_rsa_bits, :integer, default: 2048
     add_column_with_default :application_settings, :minimum_ecdsa_bits, :integer, default: 256
-    add_column_with_default :application_settings, :allowed_key_types, :string, default: %w[rsa dsa ecdsa].to_yaml
+    add_column_with_default :application_settings, :minimum_dsa_bits, :integer, default: 2048
+    add_column_with_default :application_settings, :allowed_key_types, :string, default: %w[rsa ecdsa dsa].to_yaml
   end
 
   def down
     remove_column :application_settings, :minimum_rsa_bits
     remove_column :application_settings, :minimum_ecdsa_bits
+    remove_column :application_settings, :minimum_dsa_bits
     remove_column :application_settings, :allowed_key_types
   end
 end
diff --git a/db/schema.rb b/db/schema.rb
index 60d5ce5e31b..001f7b2123a 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -99,8 +99,9 @@ ActiveRecord::Schema.define(version: 20170215200045) do
     t.text "shared_runners_text_html"
     t.text "after_sign_up_text_html"
     t.integer "minimum_ecdsa_bits", default: 256, null: false
-    t.string "allowed_key_types", default: "---\n- rsa\n- dsa\n- ecdsa\n", null: false
     t.integer "minimum_rsa_bits", default: 2048, null: false
+    t.integer "minimum_dsa_bits", default: 2048, null: false
+    t.string "allowed_key_types", default: "---\n- ecdsa\n- rsa\n- dsa\n", null: false
     t.boolean "housekeeping_enabled", default: true, null: false
     t.boolean "housekeeping_bitmaps_enabled", default: true, null: false
     t.integer "housekeeping_incremental_repack_period", default: 10, null: false
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index dca5a4a18bb..85eb3b8ced9 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -575,6 +575,7 @@ module API
       expose :terminal_max_session_time
       expose :minimum_rsa_bits
       expose :minimum_ecdsa_bits
+      expose :minimum_dsa_bits
       expose :allowed_key_types
     end
 
diff --git a/lib/api/settings.rb b/lib/api/settings.rb
index f4a4cc30a5a..1e3bbe02e43 100644
--- a/lib/api/settings.rb
+++ b/lib/api/settings.rb
@@ -111,6 +111,7 @@ module API
       optional :terminal_max_session_time, type: Integer, desc: 'Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time.'
       optional :minimum_rsa_bits, type: Integer, values: Gitlab::SSHPublicKey.allowed_sizes('rsa'), desc: 'The minimum allowed bit length of an uploaded RSA key.'
       optional :minimum_ecdsa_bits, type: Integer, values: Gitlab::SSHPublicKey.allowed_sizes('ecdsa'), desc: 'The minimum allowed curve size (in bits) of an uploaded ECDSA key.'
+      optional :minimum_dsa_bits, type: Integer, values: Gitlab::SSHPublicKey.allowed_sizes('dsa'), desc: 'The minimum allowed bit length of an uploaded DSA key.'
       optional :allowed_key_types, type: Array[String], values: Gitlab::SSHPublicKey.technology_names, desc: 'The SSH key types accepted by the application (`rsa`, `dsa`, or `ecdsa`).'
 
       at_least_one_of :default_branch_protection, :default_project_visibility, :default_snippet_visibility,
diff --git a/lib/gitlab/ssh_public_key.rb b/lib/gitlab/ssh_public_key.rb
index 2680b2456f8..76087912913 100644
--- a/lib/gitlab/ssh_public_key.rb
+++ b/lib/gitlab/ssh_public_key.rb
@@ -63,7 +63,7 @@ module Gitlab
         when :rsa
           key.n.num_bits
         when :dsa
-          1024
+          key.p.num_bits
         end
     end
 
diff --git a/spec/factories/keys.rb b/spec/factories/keys.rb
index b8d3c26127d..c8ceca9fea9 100644
--- a/spec/factories/keys.rb
+++ b/spec/factories/keys.rb
@@ -30,9 +30,9 @@ FactoryGirl.define do
       end
     end
 
-    factory :dsa_key do
+    factory :dsa_key_2048 do
       key do
-        "ssh-dss AAAAB3NzaC1kc3MAAACBAI5NHnRIaP9j0wJeQWsSbIHCK/UvBI9adxzL7XeCfFmO6uNjv5i2kMYBcBM3UBJk1TDEy4A7R7eRsWt3BQHevOlfqiweyoviVKAeEaBLYdTcxhP1r1Yegrka9pS2sg6LXJtdSm/UV1ve3Gmy8Al4JOR+9oGgqRDpK4T5XxOAf08FAAAAFQCj46CT1IRffz2u1Us4icO/POCLlwAAAIAguwAG2Gj8v2CRG0N8EIY0TyXIBI8X+Nokr6aDsYXEi9zDLqd1QNM3alJ9r1ARqz8VvB/rtlinoZ5ZTZl01zdwgDw50/73ufgxiBkIbeeerissJ0SOnCdbwgGbyKoImOtlA1ImTbS5uHo8vJifqmBlhNbHzU/5fuBGtvDV1JbTwwAAAIBhodvryJ55SX4wa29yOdTOgFJhlFVVoSciE4FfjBAAfmr8InzYx3ZgJ87ohNPmevnq9lC2S2eJVP5/h9mdJzrpP3jwieipxD4NazlhUEVcTRywz3b2swMbN9I+GR/dgzLkWXtkg/XTFftC8YPIgz+eQOTP7tJrutE4pOrDiSWSyg== dummy@gitlab.com"
+        "ssh-dss AAAAB3NzaC1kc3MAAAEBAO/3/NPLA/zSFkMOCaTtGo+uos1flfQ5f038Uk+GY9AeLGzX+Srhw59GdVXmOQLYBrOt5HdGwqYcmLnE2VurUGmhtfeO5H+3p5pGJbkS0GxpYH1HRO9lWsncF3Hh1w4lYsDjkclDiSTdfTuN8F4Kb3DXNnVSCieeonp+B25F/CXagyTQ/pvNmHFeYgGCVdnBtFdi+xfxaZ8NKdPrGggzokbKHElDZQ4Xo5EpdcyLajgM7nB2r2RzOrmeaevKi5lV68ehRa9Yyrb7vxvwiwBwOgqR/mnN7Gnaq1jUdmJY+ct04Qwx37f5jvhv5gA4U40SGMoiHM8RFIN7Ksz0jsyX73MAAAAVALRWOfjfzHpK7KLz4iqDvvTUAevJAAABAEa9NZ+6y9iQ5erGsdfLTXFrhSefTG0NhghoO/5IFkSGfd8V7kzTvCHaFrcfpEA5kP8tpoeOG0TASB6tgGOxm1Bq4Wncry5RORBPJlAVpDGRcvZ931ddH7IgltEInS6za2uH6F/1M1QfKePSLr6xJ1ZLYfP0Og5KTp1x6yMQvfwV0a+XdA+EPgaJWLWp/pWwKWa0oLUgjsIHMYzuOGh5c708uZrmkzqvgtW2NgXhcIroRgynT3IfI2lP2rqqb3uuuE/qH5UCUFO+Dc3HnAFNeQDT/M25AERdPYBAY5a+iPjIgO+jT7BfmfByT+AZTqZySrCyc7nNZL3YgGLK0l6A1GgAAAEBAN9FpFOdIXE+YEZhKl1vPmbcn+b1y5zOl6N4x1B7Q8pD/pLMziWROIS8uLzbaZ0sMIWezHIkxuo1iROMeT+jtCubn7ragaN6AX7nMpxYUH9+mYZZs/fyElt6wCviVhTIzM+u7VdQsnZttOOlQfogHdL+SpeAft0DsfJjlcgQnsLlHQKv6aPqCPYUST2nE7RyW/ExPrMxLtOWt0/j8RYHbwwqvyeZqBz3ESBgrS9c5tBdBfauwYUV/E7gPLOU3OZFw9ue7o+zwzoTZqW6Xouy5wtWvSLQSLT5XwOslmQz8QMBxD0AQyDfEFGsBCWzmbTgKv9uqrBjubsSTaja+Cf9kMo== dummy@gitlab.com"
       end
     end
   end
diff --git a/spec/lib/gitlab/ssh_public_key_spec.rb b/spec/lib/gitlab/ssh_public_key_spec.rb
index 61e6ef85a87..c5d722c870e 100644
--- a/spec/lib/gitlab/ssh_public_key_spec.rb
+++ b/spec/lib/gitlab/ssh_public_key_spec.rb
@@ -46,7 +46,7 @@ describe Gitlab::SSHPublicKey, lib: true do
 
   describe '#type' do
     context 'with a DSA key' do
-      let(:key) { attributes_for(:dsa_key)[:key] }
+      let(:key) { attributes_for(:dsa_key_2048)[:key] }
 
       it 'determines the key type' do
         expect(public_key.type).to eq(:dsa)
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index d07fc4c375d..51af5d1759b 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -45,6 +45,10 @@ describe ApplicationSetting, models: true do
     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) }
 
+    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) }
+
     describe 'allowed_key_types validations' do
       it { is_expected.to allow_value([:rsa], [:rsa, :dsa, :ecdsa]).for(:allowed_key_types) }
       it { is_expected.not_to allow_value([:foo]).for(:allowed_key_types) }
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index f0d4480d944..c45ce4e6780 100644
--- a/spec/models/key_spec.rb
+++ b/spec/models/key_spec.rb
@@ -19,10 +19,11 @@ describe Key, models: true do
 
     it { is_expected.to validate_presence_of(:key) }
     it { is_expected.to validate_length_of(:key).is_at_most(5000) }
-    it { is_expected.to allow_value(attributes_for(:dsa_key)[:key]).for(:key) }
     it { is_expected.to allow_value(attributes_for(:ecdsa_key)[:key]).for(:key) }
+    it { is_expected.to allow_value(attributes_for(:dsa_key_2048)[:key]).for(:key) }
     it { is_expected.to allow_value(attributes_for(:rsa_key_2048)[:key]).for(:key) }
     it { is_expected.not_to allow_value('foo-bar').for(:key) }
+    it { is_expected.not_to allow_value("#{attributes_for(:dsa_key_2048)[:key]}\nfoo").for(:key) }
   end
 
   describe "Methods" do
@@ -139,32 +140,32 @@ describe Key, models: true do
   context 'validate the key type is allowed' do
     it 'accepts RSA, ECDSA, and DSA keys by default' do
       expect(build(:key)).to be_valid
-      expect(build(:dsa_key)).to be_valid
       expect(build(:ecdsa_key)).to be_valid
+      expect(build(:dsa_key_2048)).to be_valid
     end
 
     it 'rejects RSA and ECDSA key if DSA is the only allowed type' do
       stub_application_setting(allowed_key_types: ['dsa'])
 
       expect(build(:key)).not_to be_valid
-      expect(build(:dsa_key)).to be_valid
       expect(build(:ecdsa_key)).not_to be_valid
+      expect(build(:dsa_key_2048)).to be_valid
     end
 
     it 'rejects RSA and DSA key if ECDSA is the only allowed type' do
       stub_application_setting(allowed_key_types: ['ecdsa'])
 
       expect(build(:key)).not_to be_valid
-      expect(build(:dsa_key)).not_to be_valid
       expect(build(:ecdsa_key)).to be_valid
+      expect(build(:dsa_key_2048)).not_to be_valid
     end
 
     it 'rejects DSA and ECDSA key if RSA is the only allowed type' do
       stub_application_setting(allowed_key_types: ['rsa'])
 
       expect(build(:key)).to be_valid
-      expect(build(:dsa_key)).not_to be_valid
       expect(build(:ecdsa_key)).not_to be_valid
+      expect(build(:dsa_key_2048)).not_to be_valid
     end
   end