diff options
26 files changed, 703 insertions, 141 deletions
diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 8367c22d1ca..8cc7111033f 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -66,6 +66,8 @@ 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] = @@ -83,6 +85,7 @@ 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 3b76da238e0..75d090359d0 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -81,6 +81,20 @@ 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 + end + end + def repository_storages_options_for_select options = Gitlab.config.repositories.storages.map do |name, storage| ["#{name} - #{storage['path']}", name] @@ -141,6 +155,10 @@ 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, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 8e446ff6dd8..988ee4802b9 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -20,6 +20,7 @@ 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 @@ -146,6 +147,24 @@ 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') } + validates_each :restricted_visibility_levels do |record, attr, value| value&.each do |level| unless Gitlab::VisibilityLevel.options.value?(level) @@ -170,7 +189,16 @@ 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 before_save :ensure_health_check_access_token @@ -212,6 +240,7 @@ 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'], @@ -239,6 +268,10 @@ 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, plantuml_enabled: false, plantuml_url: nil, project_export_enabled: true, diff --git a/app/models/key.rb b/app/models/key.rb index 49bc26122fa..27c91679ec9 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -1,6 +1,8 @@ require 'digest/md5' class Key < ActiveRecord::Base + include AfterCommitQueue + include Gitlab::CurrentSettings include Sortable LAST_USED_AT_REFRESH_TIME = 1.day.to_i @@ -12,14 +14,18 @@ class Key < ActiveRecord::Base validates :title, presence: true, length: { maximum: 255 } + validates :key, presence: true, length: { maximum: 5000 }, format: { with: /\A(ssh|ecdsa)-.*\Z/ } + validates :fingerprint, uniqueness: true, presence: { message: 'cannot be generated' } + validate :key_meets_minimum_bit_length, :key_type_is_allowed + delegate :name, :email, to: :user, prefix: true after_commit :add_to_shell, on: :create @@ -80,6 +86,10 @@ class Key < ActiveRecord::Base SystemHooksService.new.execute_hooks_for(self, :destroy) end + def public_key + @public_key ||= Gitlab::SSHPublicKey.new(key) + end + private def generate_fingerprint @@ -87,7 +97,40 @@ class Key < ActiveRecord::Base return unless self.key.present? - self.fingerprint = Gitlab::KeyFingerprint.new(self.key).fingerprint + 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 + 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 ') + + errors.add(:key, "type is not allowed. Must be #{allowed_types}") + end 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 959af5c0d13..1cda98ffea8 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -42,18 +42,57 @@ = link_to "(?)", help_page_path("integration/bitbucket") and GitLab.com = link_to "(?)", help_page_path("integration/gitlab") + + .form-group + .col-sm-offset-2.col-sm-10 + .checkbox + = f.label :project_export_enabled do + = f.check_box :project_export_enabled + Project export enabled + .form-group %label.control-label.col-sm-2 Enabled Git access protocols .col-sm-10 = select(:application_setting, :enabled_git_access_protocol, [['Both SSH and HTTP(S)', nil], ['Only SSH', 'ssh'], ['Only HTTP(S)', 'http']], {}, class: 'form-control') %span.help-block#clone-protocol-help Allow only the selected protocols to be used for Git access. + .form-group - .col-sm-offset-2.col-sm-10 - .checkbox - = f.label :project_export_enabled do - = f.check_box :project_export_enabled - Project export enabled + = 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) %fieldset %legend Account and Limit Settings diff --git a/changelogs/unreleased/17849-allow-admin-to-restrict-min-key-length-and-techno.yml b/changelogs/unreleased/17849-allow-admin-to-restrict-min-key-length-and-techno.yml new file mode 100644 index 00000000000..2e0f509b17c --- /dev/null +++ b/changelogs/unreleased/17849-allow-admin-to-restrict-min-key-length-and-techno.yml @@ -0,0 +1,5 @@ +--- +title: Add settings for minimum key strength and allowed key type +merge_request: 13712 +author: Cory Hinshaw +type: added 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 new file mode 100644 index 00000000000..ce87d8a26b6 --- /dev/null +++ b/db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb @@ -0,0 +1,24 @@ +class AddMinimumKeyLengthToApplicationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + 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 + 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 + end +end diff --git a/db/schema.rb b/db/schema.rb index 0f4b0c0c3b3..49ae4b48627 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -129,6 +129,11 @@ 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 end create_table "audit_events", force: :cascade do |t| diff --git a/doc/api/settings.md b/doc/api/settings.md index 94a9f8265fb..a43e13e6217 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -48,7 +48,12 @@ Example response: "plantuml_enabled": false, "plantuml_url": null, "terminal_max_session_time": 0, - "polling_interval_multiplier": 1.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"] } ``` @@ -88,6 +93,11 @@ 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"]`. ```bash curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/application/settings?signup_enabled=false&default_project_visibility=internal @@ -125,6 +135,11 @@ Example response: "plantuml_enabled": false, "plantuml_url": null, "terminal_max_session_time": 0, - "polling_interval_multiplier": 1.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"] } ``` diff --git a/doc/security/README.md b/doc/security/README.md index 38706e48ec5..1f54948d113 100644 --- a/doc/security/README.md +++ b/doc/security/README.md @@ -1,6 +1,7 @@ # Security - [Password length limits](password_length_limits.md) +- [Restrict allowed 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) diff --git a/doc/security/img/ssh_keys_restrictions_settings.png b/doc/security/img/ssh_keys_restrictions_settings.png Binary files differnew file mode 100644 index 00000000000..b62bfc2f7e0 --- /dev/null +++ b/doc/security/img/ssh_keys_restrictions_settings.png diff --git a/doc/security/ssh_keys_restrictions.md b/doc/security/ssh_keys_restrictions.md new file mode 100644 index 00000000000..32ca7dacab3 --- /dev/null +++ b/doc/security/ssh_keys_restrictions.md @@ -0,0 +1,18 @@ +# Restrict allowed SSH key technologies and minimum length + +`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 +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. + +GitLab allows you to restrict the allowed SSH key technology as well as specify +the minimum key length for each technology. + +In the Admin area under **Settings** (`/admin/application_settings`), look for +the "Visibility and Access Controls" area: + + diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 803b48dd88a..8f766ba4f8d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -744,6 +744,7 @@ 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 667ba468ce6..6ace0e1e390 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -122,6 +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`).' + optional(*::ApplicationSettingsHelper.visible_attributes) at_least_one_of(*::ApplicationSettingsHelper.visible_attributes) end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 3e8b83c0f90..9eab26d111e 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -34,6 +34,7 @@ module Gitlab end def check(cmd, changes) + check_valid_actor! check_protocol! check_active_user! check_project_accessibility! @@ -70,6 +71,14 @@ module Gitlab private + def check_valid_actor! + return unless actor.is_a?(Key) + + unless actor.valid? + raise UnauthorizedError, "Your SSH key #{actor.errors[:key].first}." + end + end + def check_protocol! unless protocol_allowed? raise UnauthorizedError, "Git access over #{protocol.upcase} is not allowed" diff --git a/lib/gitlab/key_fingerprint.rb b/lib/gitlab/key_fingerprint.rb deleted file mode 100644 index d9a79f7c291..00000000000 --- a/lib/gitlab/key_fingerprint.rb +++ /dev/null @@ -1,48 +0,0 @@ -module Gitlab - class KeyFingerprint - attr_reader :key, :ssh_key - - # Unqualified MD5 fingerprint for compatibility - delegate :fingerprint, to: :ssh_key, allow_nil: true - - def initialize(key) - @key = key - - @ssh_key = - begin - Net::SSH::KeyFactory.load_data_public_key(key) - rescue Net::SSH::Exception, NotImplementedError - end - end - - def valid? - ssh_key.present? - end - - def type - return unless valid? - - parts = ssh_key.ssh_type.split('-') - parts.shift if parts[0] == 'ssh' - - parts[0].upcase - end - - def bits - return unless valid? - - case type - when 'RSA' - ssh_key.n.num_bits - when 'DSS', 'DSA' - ssh_key.p.num_bits - when 'ECDSA' - ssh_key.group.order.num_bits - when 'ED25519' - 256 - else - raise "Unsupported key type: #{type}" - end - end - end -end diff --git a/lib/gitlab/ssh_public_key.rb b/lib/gitlab/ssh_public_key.rb new file mode 100644 index 00000000000..2df31bcc246 --- /dev/null +++ b/lib/gitlab/ssh_public_key.rb @@ -0,0 +1,84 @@ +module Gitlab + class SSHPublicKey + TYPES = %w[rsa dsa ecdsa ed25519].freeze + + Technology = Struct.new(:name, :allowed_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]) + ].freeze + + def self.technology_names + Technologies.map(&:name) + end + + def self.technology(name) + Technologies.find { |ssh_key_technology| ssh_key_technology.name == name } + 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) + end + + attr_reader :key_text, :key + + # Unqualified MD5 fingerprint for compatibility + delegate :fingerprint, to: :key, allow_nil: true + + def initialize(key_text) + @key_text = key_text + + @key = + begin + Net::SSH::KeyFactory.load_data_public_key(key_text) + rescue StandardError, NotImplementedError + end + end + + def valid? + key.present? + end + + 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 + end + + def bits + return unless valid? + + case type + when :rsa + key.n.num_bits + when :dsa + key.p.num_bits + when :ecdsa + key.group.order.num_bits + when :ed25519 + 256 + else + raise "Unsupported key type: #{type}" + end + end + end +end diff --git a/spec/factories/keys.rb b/spec/factories/keys.rb index a13b6e3596e..3f7c794b14a 100644 --- a/spec/factories/keys.rb +++ b/spec/factories/keys.rb @@ -18,5 +18,54 @@ FactoryGirl.define do factory :write_access_key, class: 'DeployKey' do can_push true end + + factory :rsa_key_2048 do + key do + 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDFf6RYK3qu/RKF/3ndJmL5xgMLp3O9' \ + '6x8lTay+QGZ0+9FnnAXMdUqBq/ZU6d/gyMB4IaW3nHzM1w049++yAB6UPCzMB8Uo27K5' \ + '/jyZCtj7Vm9PFNjF/8am1kp46c/SeYicQgQaSBdzIW3UDEa1Ef68qroOlvpi9PYZ/tA7' \ + 'M0YP0K5PXX+E36zaIRnJVMPT3f2k+GnrxtjafZrwFdpOP/Fol5BQLBgcsyiU+LM1SuaC' \ + 'rzd8c9vyaTA1CxrkxaZh+buAi0PmdDtaDrHd42gqZkXCKavyvgM5o2CkQ5LJHCgzpXy0' \ + '5qNFzmThBSkb+XtoxbyagBiGbVZtSVow6Xa7qewz= dummy@gitlab.com' + end + + factory :rsa_deploy_key_2048, class: 'DeployKey' + end + + factory :dsa_key_2048 do + key do + 'ssh-dss AAAAB3NzaC1kc3MAAAEBAO/3/NPLA/zSFkMOCaTtGo+uos1flfQ5f038Uk+G' \ + 'Y9AeLGzX+Srhw59GdVXmOQLYBrOt5HdGwqYcmLnE2VurUGmhtfeO5H+3p5pGJbkS0Gxp' \ + 'YH1HRO9lWsncF3Hh1w4lYsDjkclDiSTdfTuN8F4Kb3DXNnVSCieeonp+B25F/CXagyTQ' \ + '/pvNmHFeYgGCVdnBtFdi+xfxaZ8NKdPrGggzokbKHElDZQ4Xo5EpdcyLajgM7nB2r2Rz' \ + 'OrmeaevKi5lV68ehRa9Yyrb7vxvwiwBwOgqR/mnN7Gnaq1jUdmJY+ct04Qwx37f5jvhv' \ + '5gA4U40SGMoiHM8RFIN7Ksz0jsyX73MAAAAVALRWOfjfzHpK7KLz4iqDvvTUAevJAAAB' \ + 'AEa9NZ+6y9iQ5erGsdfLTXFrhSefTG0NhghoO/5IFkSGfd8V7kzTvCHaFrcfpEA5kP8t' \ + 'poeOG0TASB6tgGOxm1Bq4Wncry5RORBPJlAVpDGRcvZ931ddH7IgltEInS6za2uH6F/1' \ + 'M1QfKePSLr6xJ1ZLYfP0Og5KTp1x6yMQvfwV0a+XdA+EPgaJWLWp/pWwKWa0oLUgjsIH' \ + 'MYzuOGh5c708uZrmkzqvgtW2NgXhcIroRgynT3IfI2lP2rqqb3uuuE/qH5UCUFO+Dc3H' \ + 'nAFNeQDT/M25AERdPYBAY5a+iPjIgO+jT7BfmfByT+AZTqZySrCyc7nNZL3YgGLK0l6A' \ + '1GgAAAEBAN9FpFOdIXE+YEZhKl1vPmbcn+b1y5zOl6N4x1B7Q8pD/pLMziWROIS8uLzb' \ + 'aZ0sMIWezHIkxuo1iROMeT+jtCubn7ragaN6AX7nMpxYUH9+mYZZs/fyElt6wCviVhTI' \ + 'zM+u7VdQsnZttOOlQfogHdL+SpeAft0DsfJjlcgQnsLlHQKv6aPqCPYUST2nE7RyW/Ex' \ + 'PrMxLtOWt0/j8RYHbwwqvyeZqBz3ESBgrS9c5tBdBfauwYUV/E7gPLOU3OZFw9ue7o+z' \ + 'wzoTZqW6Xouy5wtWvSLQSLT5XwOslmQz8QMBxD0AQyDfEFGsBCWzmbTgKv9uqrBjubsS' \ + 'Taja+Cf9kMo== dummy@gitlab.com' + end + end + + factory :ecdsa_key_256 do + key do + 'ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYA' \ + 'AABBBJZmkzTgY0fiCQ+DVReyH/fFwTFz0XoR3RUO0u+199H19KFw7mNPxRSMOVS7tEtO' \ + 'Nj3Q7FcZXfqthHvgAzDiHsc= dummy@gitlab.com' + end + end + + factory :ed25519_key_256 do + key do + 'ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIETnVTgzqC1gatgSlC4zH6aYt2CAQzgJOhDRvf59ohL6 dummy@gitlab.com' + end + end end end diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index dbb0ae9c86e..9698dead67a 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -79,6 +79,26 @@ feature 'Admin updates settings' do end end + scenario 'Change Keys settings' do + uncheck 'RSA' + uncheck 'DSA' + uncheck 'ED25519' + select '384', from: 'Minimum ECDSA key length' + 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' + + uncheck 'ECDSA' + click_on 'Save' + + expect(page).to have_content "Allowed key types can't be blank" + end + def check_all_events page.check('Active') page.check('Push') diff --git a/spec/features/profiles/keys_spec.rb b/spec/features/profiles/keys_spec.rb index 6541ea6bf57..a4ccf222b50 100644 --- a/spec/features/profiles/keys_spec.rb +++ b/spec/features/profiles/keys_spec.rb @@ -28,6 +28,22 @@ feature 'Profile > SSH Keys' do expect(page).to have_content("Title: #{attrs[:title]}") expect(page).to have_content(attrs[:key]) end + + context 'when only DSA and ECDSA keys are allowed' do + before do + stub_application_setting(allowed_key_types: %w[dsa ecdsa]) + end + + scenario 'shows a validation error' do + attrs = attributes_for(:key) + + fill_in('Key', with: attrs[:key]) + 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') + end + end end scenario 'User sees their keys' do diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 295a979da76..a67902c7209 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -155,6 +155,48 @@ describe Gitlab::GitAccess do end end + shared_examples '#check with a key that is not valid' do + before do + project.add_master(user) + end + + context 'key is too small' do + before do + stub_application_setting(minimum_rsa_bits: 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.') + end + end + end + + context 'key type is not allowed' do + before do + stub_application_setting(allowed_key_types: ['ecdsa']) + 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.') + end + end + end + end + + it_behaves_like '#check with a key that is not valid' do + let(:actor) { build(:rsa_key_2048, user: user) } + end + + it_behaves_like '#check with a key that is not valid' do + let(:actor) { build(:rsa_deploy_key_2048, user: user) } + end + describe '#check_project_moved!' do before do project.add_master(user) diff --git a/spec/lib/gitlab/key_fingerprint_spec.rb b/spec/lib/gitlab/key_fingerprint_spec.rb deleted file mode 100644 index d643dc5342d..00000000000 --- a/spec/lib/gitlab/key_fingerprint_spec.rb +++ /dev/null @@ -1,82 +0,0 @@ -require 'spec_helper' - -describe Gitlab::KeyFingerprint, lib: true do - KEYS = { - rsa: - 'example.com ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC5z65PwQ1GE6foJgwk' \ - '9rmQi/glaXbUeVa5uvQpnZ3Z5+forcI7aTngh3aZ/H2UDP2L70TGy7kKNyp0J3a8/OdG' \ - 'Z08y5yi3JlbjFARO1NyoFEjw2H1SJxeJ43L6zmvTlu+hlK1jSAlidl7enS0ufTlzEEj4' \ - 'iJcuTPKdVzKRgZuTRVm9woWNVKqIrdRC0rJiTinERnfSAp/vNYERMuaoN4oJt8p/NEek' \ - 'rmFoDsQOsyDW5RAnCnjWUU+jFBKDpfkJQ1U2n6BjJewC9dl6ODK639l3yN4WOLZEk4tN' \ - 'UysfbGeF3rmMeflaD6O1Jplpv3YhwVGFNKa7fMq6k3Z0tszTJPYh', - ecdsa: - 'example.com ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAI' \ - 'bmlzdHAyNTYAAABBBKTJy43NZzJSfNxpv/e2E6Zy3qoHoTQbmOsU5FEfpWfWa1MdTeXQ' \ - 'YvKOi+qz/1AaNx6BK421jGu74JCDJtiZWT8=', - ed25519: - '@revoked example.com ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAfuCHKVTjq' \ - 'uxvt6CM6tdG4SLp1Btn/nOeHHE5UOzRdf', - dss: - 'example.com ssh-dss AAAAB3NzaC1kc3MAAACBAP1/U4EddRIpUt9KnC7s5Of2EbdS' \ - 'PO9EAMMeP4C2USZpRV1AIlH7WT2NWPq/xfW6MPbLm1Vs14E7gB00b/JmYLdrmVClpJ+f' \ - '6AR7ECLCT7up1/63xhv4O1fnxqimFQ8E+4P208UewwI1VBNaFpEy9nXzrith1yrv8iID' \ - 'GZ3RSAHHAAAAFQCXYFCPFSMLzLKSuYKi64QL8Fgc9QAAAIEA9+GghdabPd7LvKtcNrhX' \ - 'uXmUr7v6OuqC+VdMCz0HgmdRWVeOutRZT+ZxBxCBgLRJFnEj6EwoFhO3zwkyjMim4TwW' \ - 'eotUfI0o4KOuHiuzpnWRbqN/C/ohNWLx+2J6ASQ7zKTxvqhRkImog9/hWuWfBpKLZl6A' \ - 'e1UlZAFMO/7PSSoAAACBAJcQ4JODqhuGbXIEpqxetm7PWbdbCcr3y/GzIZ066pRovpL6' \ - 'qm3qCVIym4cyChxWwb8qlyCIi+YRUUWm1z/wiBYT2Vf3S4FXBnyymCkKEaV/EY7+jd4X' \ - '1bXI58OD2u+bLCB/sInM4fGB8CZUIWT9nJH0Ve9jJUge2ms348/QOJ1+' - }.freeze - - MD5_FINGERPRINTS = { - rsa: '06:b2:8a:92:df:0e:11:2c:ca:7b:8f:a4:ba:6e:4b:fd', - ecdsa: '45:ff:5b:98:9a:b6:8a:41:13:c1:30:8b:09:5e:7b:4e', - ed25519: '2e:65:6a:c8:cf:bf:b2:8b:9a:bd:6d:9f:11:5c:12:16', - dss: '57:98:86:02:5f:9c:f4:9b:ad:5a:1e:51:92:0e:fd:2b' - }.freeze - - BIT_COUNTS = { - rsa: 2048, - ecdsa: 256, - ed25519: 256, - dss: 1024 - }.freeze - - describe '#type' do - KEYS.each do |type, key| - it "calculates the type of #{type} keys" do - calculated_type = described_class.new(key).type - - expect(calculated_type).to eq(type.to_s.upcase) - end - end - end - - describe '#fingerprint' do - KEYS.each do |type, key| - it "calculates the MD5 fingerprint for #{type} keys" do - fp = described_class.new(key).fingerprint - - expect(fp).to eq(MD5_FINGERPRINTS[type]) - end - end - end - - describe '#bits' do - KEYS.each do |type, key| - it "calculates the number of bits in #{type} keys" do - bits = described_class.new(key).bits - - expect(bits).to eq(BIT_COUNTS[type]) - end - end - end - - describe '#key' do - it 'carries the unmodified key data' do - key = described_class.new(KEYS[:rsa]).key - - expect(key).to eq(KEYS[:rsa]) - end - end -end diff --git a/spec/lib/gitlab/ssh_public_key_spec.rb b/spec/lib/gitlab/ssh_public_key_spec.rb new file mode 100644 index 00000000000..d3314552d31 --- /dev/null +++ b/spec/lib/gitlab/ssh_public_key_spec.rb @@ -0,0 +1,132 @@ +require 'spec_helper' + +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]) + end + end + + describe '.allowed_sizes(name)' do + where(:name, :sizes) do + [ + ['rsa', [1024, 2048, 3072, 4096]], + ['dsa', [1024, 2048, 3072]], + ['ecdsa', [256, 384, 521]], + ['ed25519', [256]] + ] + end + + subject { described_class.allowed_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) + end + end + + describe '#valid?' do + subject { public_key } + + context 'with a valid SSH key' do + it { is_expected.to be_valid } + end + + context 'with an invalid SSH key' do + let(:key) { 'this is not a key' } + + it { is_expected.not_to be_valid } + end + end + + describe '#type' do + subject { public_key.type } + + where(:factory, :type) do + [ + [:rsa_key_2048, :rsa], + [:dsa_key_2048, :dsa], + [:ecdsa_key_256, :ecdsa], + [:ed25519_key_256, :ed25519] + ] + end + + with_them do + let(:key) { attributes_for(factory)[:key] } + + it { is_expected.to eq(type) } + end + + context 'with an invalid SSH key' do + let(:key) { 'this is not a key' } + + it { is_expected.to be_nil } + end + end + + describe '#bits' do + subject { public_key.bits } + + where(:factory, :bits) do + [ + [:rsa_key_2048, 2048], + [:dsa_key_2048, 2048], + [:ecdsa_key_256, 256], + [:ed25519_key_256, 256] + ] + end + + with_them do + let(:key) { attributes_for(factory)[:key] } + + it { is_expected.to eq(bits) } + end + + context 'with an invalid SSH key' do + let(:key) { 'this is not a key' } + + it { is_expected.to be_nil } + end + end + + describe '#fingerprint' do + subject { public_key.fingerprint } + + where(:factory, :fingerprint) do + [ + [:rsa_key_2048, '2e:ca:dc:e0:37:29:ed:fc:f0:1d:bf:66:d4:cd:51:b1'], + [:dsa_key_2048, 'bc:c1:a4:be:7e:8c:84:56:b3:58:93:53:c6:80:78:8c'], + [:ecdsa_key_256, '67:a3:a9:7d:b8:e1:15:d4:80:40:21:34:bb:ed:97:38'], + [:ed25519_key_256, 'e6:eb:45:8a:3c:59:35:5f:e9:5b:80:12:be:7e:22:73'] + ] + end + + with_them do + let(:key) { attributes_for(factory)[:key] } + + it { is_expected.to eq(fingerprint) } + end + + context 'with an invalid SSH key' do + let(:key) { 'this is not a key' } + + it { is_expected.to be_nil } + end + end + + describe '#key_text' do + let(:key) { 'this is not a key' } + + it 'carries the unmodified key data' do + expect(public_key.key_text).to eq(key) + end + end +end diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 359753b600e..44d473db07d 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -72,6 +72,27 @@ 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) } + + 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) } + + 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) } + + 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) } + end + it_behaves_like 'an object with email-formated attributes', :admin_notification_email do subject { setting } end @@ -441,4 +462,16 @@ describe ApplicationSetting do end 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) + end + + it 'get value as array of symbols' do + setting.allowed_key_types = ['rsa'] + expect(setting.allowed_key_types).to eq(['rsa']) + end + end end diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index 3508391c721..83b11baa371 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -1,6 +1,13 @@ require 'spec_helper' describe Key, :mailer do + include Gitlab::CurrentSettings + + describe 'modules' do + subject { described_class } + it { is_expected.to include_module(Gitlab::CurrentSettings) } + end + describe "Associations" do it { is_expected.to belong_to(:user) } end @@ -11,8 +18,10 @@ describe Key, :mailer 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('ssh-foo').for(:key) } - it { is_expected.to allow_value('ecdsa-foo').for(:key) } + it { is_expected.to allow_value(attributes_for(:rsa_key_2048)[: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(:ecdsa_key_256)[:key]).for(:key) } + it { is_expected.to allow_value(attributes_for(:ed25519_key_256)[:key]).for(:key) } it { is_expected.not_to allow_value('foo-bar').for(:key) } end @@ -95,6 +104,78 @@ describe Key, :mailer do end end + context 'validate it meets minimum bit length' do + where(:factory, :minimum, :result) do + [ + [: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] + ] + end + + with_them do + subject(:key) { build(factory) } + + before do + stub_application_setting("minimum_#{key.public_key.type}_bits" => 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 737c028ad53..60e7c2d0da3 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -19,6 +19,11 @@ 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') end end @@ -44,7 +49,12 @@ describe API::Settings, 'Settings' do help_page_text: 'custom help text', help_page_hide_commercial_content: true, help_page_support_url: 'http://example.com/help', - project_export_enabled: false + project_export_enabled: false, + minimum_rsa_bits: 2048, + minimum_dsa_bits: 2048, + minimum_ecdsa_bits: 384, + minimum_ed25519_bits: 256, + allowed_key_types: ['rsa'] expect(response).to have_http_status(200) expect(json_response['default_projects_limit']).to eq(3) @@ -61,6 +71,11 @@ 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']) end end |
