diff options
| -rw-r--r-- | app/controllers/admin/application_settings_controller.rb | 3 | ||||
| -rw-r--r-- | app/helpers/application_settings_helper.rb | 28 | ||||
| -rw-r--r-- | app/models/application_setting.rb | 62 | ||||
| -rw-r--r-- | app/models/key.rb | 37 | ||||
| -rw-r--r-- | app/views/admin/application_settings/_form.html.haml | 42 | ||||
| -rw-r--r-- | db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb | 24 | ||||
| -rw-r--r-- | db/schema.rb | 9 | ||||
| -rw-r--r-- | doc/api/settings.md | 27 | ||||
| -rw-r--r-- | doc/security/img/ssh_keys_restrictions_settings.png | bin | 41803 -> 13698 bytes | |||
| -rw-r--r-- | lib/api/entities.rb | 1 | ||||
| -rw-r--r-- | lib/api/settings.rb | 11 | ||||
| -rw-r--r-- | lib/gitlab/ssh_public_key.rb | 52 | ||||
| -rw-r--r-- | spec/features/admin/admin_settings_spec.rb | 24 | ||||
| -rw-r--r-- | spec/features/profiles/keys_spec.rb | 5 | ||||
| -rw-r--r-- | spec/lib/gitlab/git_access_spec.rb | 12 | ||||
| -rw-r--r-- | spec/lib/gitlab/ssh_public_key_spec.rb | 36 | ||||
| -rw-r--r-- | spec/models/application_setting_spec.rb | 63 | ||||
| -rw-r--r-- | spec/models/key_spec.rb | 66 | ||||
| -rw-r--r-- | spec/requests/api/settings_spec.rb | 27 |
19 files changed, 228 insertions, 301 deletions
diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 8cc7111033f..8367c22d1ca 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -66,8 +66,6 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController end end - params[:application_setting][:allowed_key_types]&.delete('') - enabled_oauth_sign_in_sources = params[:application_setting].delete(:enabled_oauth_sign_in_sources) params[:application_setting][:disabled_oauth_sign_in_sources] = @@ -85,7 +83,6 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController def visible_application_setting_attributes ApplicationSettingsHelper.visible_attributes + [ :domain_blacklist_file, - allowed_key_types: [], disabled_oauth_sign_in_sources: [], import_sources: [], repository_storages: [], diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 75d090359d0..0c74d464601 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -81,18 +81,16 @@ module ApplicationSettingsHelper end end - def allowed_key_types_checkboxes(help_block_id) - Gitlab::SSHPublicKey.technology_names.map do |type| - checked = current_application_settings.allowed_key_types.include?(type) - checkbox_id = "allowed_key_types-#{type}" - - label_tag(checkbox_id, class: checked ? 'active' : nil) do - check_box_tag('application_setting[allowed_key_types][]', type, checked, - autocomplete: 'off', - 'aria-describedby' => help_block_id, - id: checkbox_id) + type.upcase - end + def key_restriction_options_for_select(type) + bit_size_options = Gitlab::SSHPublicKey.supported_sizes(type).map do |bits| + ["Must be at least #{bits} bits", bits] end + + [ + ['Are allowed', 0], + *bit_size_options, + ['Are forbidden', ApplicationSetting::FORBIDDEN_KEY_VALUE] + ] end def repository_storages_options_for_select @@ -127,6 +125,9 @@ module ApplicationSettingsHelper :domain_blacklist_enabled, :domain_blacklist_raw, :domain_whitelist_raw, + :dsa_key_restriction, + :ecdsa_key_restriction, + :ed25519_key_restriction, :email_author_in_body, :enabled_git_access_protocol, :gravatar_enabled, @@ -155,10 +156,6 @@ module ApplicationSettingsHelper :metrics_port, :metrics_sample_interval, :metrics_timeout, - :minimum_dsa_bits, - :minimum_ecdsa_bits, - :minimum_ed25519_bits, - :minimum_rsa_bits, :password_authentication_enabled, :performance_bar_allowed_group_id, :performance_bar_enabled, @@ -174,6 +171,7 @@ module ApplicationSettingsHelper :repository_storages, :require_two_factor_authentication, :restricted_visibility_levels, + :rsa_key_restriction, :send_user_confirmation_email, :sentry_dsn, :sentry_enabled, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 988ee4802b9..0f9053262c2 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -13,6 +13,15 @@ class ApplicationSetting < ActiveRecord::Base [\r\n] # any number of newline characters }x + # Setting a key restriction to `-1` means that all keys of this type are + # forbidden. + FORBIDDEN_KEY_VALUE = -1 + 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 @@ -20,7 +29,6 @@ class ApplicationSetting < ActiveRecord::Base serialize :domain_blacklist, Array # rubocop:disable Cop/ActiveRecordSerialize serialize :repository_storages # rubocop:disable Cop/ActiveRecordSerialize serialize :sidekiq_throttling_queues, Array # rubocop:disable Cop/ActiveRecordSerialize - serialize :allowed_key_types, Array # rubocop:disable Cop/ActiveRecordSerialize cache_markdown_field :sign_in_text cache_markdown_field :help_page_text @@ -147,23 +155,11 @@ class ApplicationSetting < ActiveRecord::Base presence: true, numericality: { greater_than_or_equal_to: 0 } - validates :allowed_key_types, presence: true - - validates :minimum_rsa_bits, - presence: true, - inclusion: { in: Gitlab::SSHPublicKey.allowed_sizes('rsa') } - - validates :minimum_dsa_bits, - presence: true, - inclusion: { in: Gitlab::SSHPublicKey.allowed_sizes('dsa') } - - validates :minimum_ecdsa_bits, - presence: true, - inclusion: { in: Gitlab::SSHPublicKey.allowed_sizes('ecdsa') } - - validates :minimum_ed25519_bits, - presence: true, - inclusion: { in: Gitlab::SSHPublicKey.allowed_sizes('ed25519') } + SUPPORTED_KEY_TYPES.each do |type| + validates :"#{type}_key_restriction", + presence: true, + inclusion: { in: ApplicationSetting.supported_key_restrictions(type) } + end validates_each :restricted_visibility_levels do |record, attr, value| value&.each do |level| @@ -189,14 +185,6 @@ class ApplicationSetting < ActiveRecord::Base end end - validates_each :allowed_key_types do |record, attr, value| - value&.each do |type| - unless Gitlab::SSHPublicKey.allowed_type?(type) - record.errors.add(attr, "'#{type}' is not a valid SSH key type") - end - end - end - before_validation :ensure_uuid! before_save :ensure_runners_registration_token @@ -240,7 +228,6 @@ class ApplicationSetting < ActiveRecord::Base { after_sign_up_text: nil, akismet_enabled: false, - allowed_key_types: Gitlab::SSHPublicKey.technology_names, container_registry_token_expire_delay: 5, default_artifacts_expire_in: '30 days', default_branch_protection: Settings.gitlab['default_branch_protection'], @@ -250,6 +237,9 @@ class ApplicationSetting < ActiveRecord::Base default_group_visibility: Settings.gitlab.default_projects_features['visibility_level'], disabled_oauth_sign_in_sources: [], domain_whitelist: Settings.gitlab['domain_whitelist'], + dsa_key_restriction: 0, + ecdsa_key_restriction: 0, + ed25519_key_restriction: 0, gravatar_enabled: Settings.gravatar['enabled'], help_page_text: nil, help_page_hide_commercial_content: false, @@ -268,10 +258,7 @@ class ApplicationSetting < ActiveRecord::Base max_attachment_size: Settings.gitlab['max_attachment_size'], password_authentication_enabled: Settings.gitlab['password_authentication_enabled'], performance_bar_allowed_group_id: nil, - minimum_rsa_bits: 1024, - minimum_dsa_bits: 1024, - minimum_ecdsa_bits: 256, - minimum_ed25519_bits: 256, + rsa_key_restriction: 0, plantuml_enabled: false, plantuml_url: nil, project_export_enabled: true, @@ -446,6 +433,19 @@ class ApplicationSetting < ActiveRecord::Base usage_ping_can_be_configured? && super end + def allowed_key_types + SUPPORTED_KEY_TYPES.select do |type| + key_restriction_for(type) != FORBIDDEN_KEY_VALUE + end + end + + 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 + end + private def ensure_uuid! diff --git a/app/models/key.rb b/app/models/key.rb index 27c91679ec9..2334603b58b 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -24,7 +24,7 @@ class Key < ActiveRecord::Base uniqueness: true, presence: { message: 'cannot be generated' } - validate :key_meets_minimum_bit_length, :key_type_is_allowed + validate :key_meets_restrictions delegate :name, :email, to: :user, prefix: true @@ -100,37 +100,24 @@ class Key < ActiveRecord::Base self.fingerprint = public_key.fingerprint end - def key_meets_minimum_bit_length - case public_key.type - when :rsa - if public_key.bits < 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.bits < current_application_settings.minimum_dsa_bits - errors.add(:key, "length must be at least #{current_application_settings.minimum_dsa_bits} bits") - end - when :ecdsa - if public_key.bits < current_application_settings.minimum_ecdsa_bits - errors.add(:key, "elliptic curve size must be at least #{current_application_settings.minimum_ecdsa_bits} bits") - end - when :ed25519 - if public_key.bits < current_application_settings.minimum_ed25519_bits - errors.add(:key, "length must be at least #{current_application_settings.minimum_ed25519_bits} bits") - end + def key_meets_restrictions + restriction = current_application_settings.key_restriction_for(public_key.type) + + if restriction == ApplicationSetting::FORBIDDEN_KEY_VALUE + errors.add(:key, forbidden_key_type_message) + elsif public_key.bits < restriction + errors.add(:key, "must be at least #{restriction} bits") 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 + def forbidden_key_type_message + 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 + "type is forbidden. Must be #{allowed_types}" end def notify_user diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 1cda98ffea8..fd083c03633 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -57,42 +57,12 @@ %span.help-block#clone-protocol-help Allow only the selected protocols to be used for Git access. - .form-group - = f.label :allowed_key_types, 'Allowed SSH keys', class: 'control-label col-sm-2' - .col-sm-10 - = hidden_field_tag 'application_setting[allowed_key_types][]', nil, id: 'allowed_key_types-none' - - allowed_key_types_checkboxes('allowed-key-types-help').each do |key_type_checkbox| - .checkbox= key_type_checkbox - %span.help-block#allowed-key-types-help - Only SSH keys with allowed algorithms can be uploaded. - - .form-group - = f.label :minimum_rsa_bits, 'Minimum RSA key length', class: 'control-label col-sm-2' - .col-sm-10 - = f.select :minimum_rsa_bits, Gitlab::SSHPublicKey.allowed_sizes('rsa'), {}, class: 'form-control' - .help-block - The minimum length for user RSA 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) - - .form-group - = f.label :minimum_ecdsa_bits, 'Minimum ECDSA key length', class: 'control-label col-sm-2' - .col-sm-10 - = f.select :minimum_ecdsa_bits, Gitlab::SSHPublicKey.allowed_sizes('ecdsa'), {}, class: 'form-control' - .help-block - The minimum elliptic curve size for user ECDSA SSH keys (in bits) - - .form-group - = f.label :minimum_ed25519_bits, 'Minimum ED25519 key length', class: 'control-label col-sm-2' - .col-sm-10 - = f.select :minimum_ed25519_bits, Gitlab::SSHPublicKey.allowed_sizes('ed25519'), {}, class: 'form-control' - .help-block - The minimum length for user ED25519 SSH keys (in bits) + - ApplicationSetting::SUPPORTED_KEY_TYPES.each do |type| + - field_name = :"#{type}_key_restriction" + .form-group + = f.label field_name, "#{type.upcase} SSH keys", class: 'control-label col-sm-2' + .col-sm-10 + = f.select field_name, key_restriction_options_for_select(type), {}, class: 'form-control' %fieldset %legend Account and Limit Settings 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 ce87d8a26b6..2882e7f8b45 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 @@ -7,18 +7,22 @@ class AddMinimumKeyLengthToApplicationSettings < ActiveRecord::Migration disable_ddl_transaction! def up - add_column_with_default :application_settings, :minimum_rsa_bits, :integer, default: 1024 - add_column_with_default :application_settings, :minimum_dsa_bits, :integer, default: 1024 - add_column_with_default :application_settings, :minimum_ecdsa_bits, :integer, default: 256 - add_column_with_default :application_settings, :minimum_ed25519_bits, :integer, default: 256 - add_column_with_default :application_settings, :allowed_key_types, :string, default: %w[rsa dsa ecdsa ed25519].to_yaml + # A key restriction has two possible states: + # + # * -1 means "this key type is completely disabled" + # * >= 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" + 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 + add_column_with_default :application_settings, :ed25519_key_restriction, :integer, default: 0 end def down - remove_column :application_settings, :minimum_rsa_bits - remove_column :application_settings, :minimum_dsa_bits - remove_column :application_settings, :minimum_ecdsa_bits - remove_column :application_settings, :minimum_ed25519_bits - remove_column :application_settings, :allowed_key_types + remove_column :application_settings, :rsa_key_restriction + remove_column :application_settings, :dsa_key_restriction + remove_column :application_settings, :ecdsa_key_restriction + remove_column :application_settings, :ed25519_key_restriction end end diff --git a/db/schema.rb b/db/schema.rb index 49ae4b48627..434d1326419 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -129,11 +129,10 @@ ActiveRecord::Schema.define(version: 20170824162758) do t.boolean "password_authentication_enabled" t.boolean "project_export_enabled", default: true, null: false t.boolean "hashed_storage_enabled", default: false, null: false - t.integer "minimum_rsa_bits", default: 1024, null: false - t.integer "minimum_dsa_bits", default: 1024, null: false - t.integer "minimum_ecdsa_bits", default: 256, null: false - t.integer "minimum_ed25519_bits", default: 256, null: false - t.string "allowed_key_types", default: "---\n- rsa\n- dsa\n- ecdsa\n- ed25519\n", null: false + t.integer "rsa_key_restriction", default: 0, null: false + t.integer "dsa_key_restriction", default: 0, null: false + t.integer "ecdsa_key_restriction", default: 0, null: false + t.integer "ed25519_key_restriction", default: 0, null: false end create_table "audit_events", force: :cascade do |t| diff --git a/doc/api/settings.md b/doc/api/settings.md index a43e13e6217..b78f1252108 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -49,11 +49,10 @@ Example response: "plantuml_url": null, "terminal_max_session_time": 0, "polling_interval_multiplier": 1.0, - "minimum_rsa_bits": 1024, - "minimum_dsa_bits": 1024, - "minimum_ecdsa_bits": 256, - "minimum_ed25519_bits": 256, - "allowed_key_types": ["rsa", "dsa", "ecdsa", "ed25519"] + "rsa_key_restriction": 0, + "dsa_key_restriction": 0, + "ecdsa_key_restriction": 0, + "ed25519_key_restriction": 0, } ``` @@ -93,11 +92,10 @@ PUT /application/settings | `plantuml_url` | string | yes (if `plantuml_enabled` is `true`) | The PlantUML instance URL for integration. | | `terminal_max_session_time` | integer | no | Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time. | | `polling_interval_multiplier` | decimal | no | Interval multiplier used by endpoints that perform polling. Set to 0 to disable polling. | -| `minimum_rsa_bits` | integer | no | The minimum allowed bit length of an uploaded RSA key. Default is `1024`. -| `minimum_dsa_bits` | integer | no | The minimum allowed bit length of an uploaded DSA key. Default is `1024`. -| `minimum_ecdsa_bits` | integer | no | The minimum allowed curve size (in bits) of an uploaded ECDSA key. Default is `256`. -| `minimum_ed25519_bits` | integer | no | The minimum allowed curve size (in bits) of an uploaded ED25519 key. Default is `256`. -| `allowed_key_types` | array of strings | no | Array of SSH key types accepted by the application. Allowed values are: `rsa`, `dsa`, `ecdsa`, and `ed25519`. Default is `["rsa", "dsa", "ecdsa", "ed25519"]`. +| `rsa_key_restriction` | integer | no | The minimum allowed bit length of an uploaded RSA key. Default is `0` (no restriction). `-1` disables RSA keys. +| `dsa_key_restriction` | integer | no | The minimum allowed bit length of an uploaded DSA key. Default is `0` (no restriction). `-1` disables DSA keys. +| `ecdsa_key_restriction` | integer | no | The minimum allowed curve size (in bits) of an uploaded ECDSA key. Default is `0` (no restriction). `-1` disables ECDSA keys. +| `ed25519_key_restriction` | integer | no | The minimum allowed curve size (in bits) of an uploaded ED25519 key. Default is `0` (no restriction). `-1` disables ED25519 keys. ```bash curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/application/settings?signup_enabled=false&default_project_visibility=internal @@ -136,10 +134,9 @@ Example response: "plantuml_url": null, "terminal_max_session_time": 0, "polling_interval_multiplier": 1.0, - "minimum_rsa_bits": 1024, - "minimum_dsa_bits": 1024, - "minimum_ecdsa_bits": 256, - "minimum_ed25519_bits": 256, - "allowed_key_types": ["rsa", "dsa", "ecdsa", "ed25519"] + "rsa_key_restriction": 0, + "dsa_key_restriction": 0, + "ecdsa_key_restriction": 0, + "ed25519_key_restriction": 0, } ``` diff --git a/doc/security/img/ssh_keys_restrictions_settings.png b/doc/security/img/ssh_keys_restrictions_settings.png Binary files differindex b62bfc2f7e0..7b8bbb05bce 100644 --- a/doc/security/img/ssh_keys_restrictions_settings.png +++ b/doc/security/img/ssh_keys_restrictions_settings.png diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 8f766ba4f8d..803b48dd88a 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -744,7 +744,6 @@ module API expose(:default_snippet_visibility) { |setting, _options| Gitlab::VisibilityLevel.string_level(setting.default_snippet_visibility) } expose(:default_group_visibility) { |setting, _options| Gitlab::VisibilityLevel.string_level(setting.default_group_visibility) } expose :password_authentication_enabled, as: :signin_enabled - expose :allowed_key_types end class Release < Grape::Entity diff --git a/lib/api/settings.rb b/lib/api/settings.rb index 6ace0e1e390..01123e45ee0 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -122,11 +122,12 @@ 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 :polling_interval_multiplier, type: BigDecimal, desc: 'Interval multiplier used by endpoints that perform polling. Set to 0 to disable polling.' - 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_dsa_bits, type: Integer, values: Gitlab::SSHPublicKey.allowed_sizes('dsa'), desc: 'The minimum allowed bit length of an uploaded DSA 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_ed25519_bits, type: Integer, values: Gitlab::SSHPublicKey.allowed_sizes('ed25519'), desc: 'The minimum allowed curve size (in bits) of an uploaded ED25519 key.' - optional :allowed_key_types, type: Array[String], values: Gitlab::SSHPublicKey.technology_names, desc: 'The SSH key types accepted by the application (`rsa`, `dsa`, `ecdsa` or `ed25519`).' + ApplicationSetting::SUPPORTED_KEY_TYPES.each do |type| + optional :"#{type}_key_restriction", + type: Integer, + values: ApplicationSetting.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 optional(*::ApplicationSettingsHelper.visible_attributes) at_least_one_of(*::ApplicationSettingsHelper.visible_attributes) diff --git a/lib/gitlab/ssh_public_key.rb b/lib/gitlab/ssh_public_key.rb index 2df31bcc246..a3f8730fb04 100644 --- a/lib/gitlab/ssh_public_key.rb +++ b/lib/gitlab/ssh_public_key.rb @@ -1,31 +1,20 @@ module Gitlab class SSHPublicKey - TYPES = %w[rsa dsa ecdsa ed25519].freeze - - Technology = Struct.new(:name, :allowed_sizes) + Technology = Struct.new(:name, :key_class, :supported_sizes) Technologies = [ - Technology.new('rsa', [1024, 2048, 3072, 4096]), - Technology.new('dsa', [1024, 2048, 3072]), - Technology.new('ecdsa', [256, 384, 521]), - Technology.new('ed25519', [256]) + Technology.new(:rsa, OpenSSL::PKey::RSA, [1024, 2048, 3072, 4096]), + Technology.new(:dsa, OpenSSL::PKey::DSA, [1024, 2048, 3072]), + Technology.new(:ecdsa, OpenSSL::PKey::EC, [256, 384, 521]), + Technology.new(:ed25519, Net::SSH::Authentication::ED25519::PubKey, [256]) ].freeze - def self.technology_names - Technologies.map(&:name) - end - def self.technology(name) - Technologies.find { |ssh_key_technology| ssh_key_technology.name == name } + Technologies.find { |tech| tech.name.to_s == name.to_s } end - private_class_method :technology - def self.allowed_sizes(name) - technology(name).allowed_sizes - end - - def self.allowed_type?(type) - technology_names.include?(type.to_s) + def self.supported_sizes(name) + technology(name)&.supported_sizes end attr_reader :key_text, :key @@ -50,18 +39,7 @@ module Gitlab def type return unless valid? - case key - when OpenSSL::PKey::EC - :ecdsa - when OpenSSL::PKey::RSA - :rsa - when OpenSSL::PKey::DSA - :dsa - when Net::SSH::Authentication::ED25519::PubKey - :ed25519 - else - raise "Unsupported key type: #{key.class}" - end + technology.name end def bits @@ -80,5 +58,17 @@ module Gitlab raise "Unsupported key type: #{type}" end end + + private + + def technology + @technology ||= + begin + tech = Technologies.find { |tech| key.is_a?(tech.key_class) } + raise "Unsupported key type: #{key.class}" unless tech + + tech + end + end end end 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 |
