From b84ca08e351fc9238bef4e6b4bf74158d25d4f1d Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Mon, 28 Aug 2017 21:33:35 +0100 Subject: [PATCH] Address review comments --- app/helpers/form_helper.rb | 4 ++-- app/models/application_setting.rb | 3 +-- app/models/key.rb | 1 - .../profiles/keys/_key_details.html.haml | 2 +- ...-to-restrict-min-key-length-and-techno.yml | 2 +- ...imum_key_length_to_application_settings.rb | 7 ++++--- doc/security/README.md | 2 +- doc/security/ssh_keys_restrictions.md | 7 ++++--- lib/gitlab/git_access.rb | 2 +- lib/gitlab/ssh_public_key.rb | 15 ++++++-------- spec/lib/gitlab/git_access_spec.rb | 20 ++++++++----------- 11 files changed, 29 insertions(+), 36 deletions(-) diff --git a/app/helpers/form_helper.rb b/app/helpers/form_helper.rb index 2e05b8c6d65..eeb130d5240 100644 --- a/app/helpers/form_helper.rb +++ b/app/helpers/form_helper.rb @@ -1,10 +1,10 @@ module FormHelper - def form_errors(model, headline = 'The form contains the following') + def form_errors(model, type: 'form') return unless model.errors.any? pluralized = 'error'.pluralize(model.errors.count) - headline = headline + ' ' + pluralized + ':' + headline = "The #{type} contains the following #{pluralized}:" content_tag(:div, class: 'alert alert-danger', id: 'error_explanation') do content_tag(:h4, headline) << diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 0f9053262c2..fcf31694ab5 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -442,8 +442,7 @@ class ApplicationSetting < ActiveRecord::Base def key_restriction_for(type) attr_name = "#{type}_key_restriction" - # rubocop:disable GitlabSecurity/PublicSend - has_attribute?(attr_name) ? public_send(attr_name) : FORBIDDEN_KEY_VALUE + has_attribute?(attr_name) ? public_send(attr_name) : FORBIDDEN_KEY_VALUE # rubocop:disable GitlabSecurity/PublicSend end private diff --git a/app/models/key.rb b/app/models/key.rb index 2334603b58b..a6b4dcfec0d 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -1,7 +1,6 @@ require 'digest/md5' class Key < ActiveRecord::Base - include AfterCommitQueue include Gitlab::CurrentSettings include Sortable diff --git a/app/views/profiles/keys/_key_details.html.haml b/app/views/profiles/keys/_key_details.html.haml index cf70320e7a8..77521417f47 100644 --- a/app/views/profiles/keys/_key_details.html.haml +++ b/app/views/profiles/keys/_key_details.html.haml @@ -16,7 +16,7 @@ %strong= @key.last_used_at.try(:to_s, :medium) || 'N/A' .col-md-8 - = form_errors(@key, 'The key has the following') unless @key.valid? + = form_errors(@key, type: 'key') unless @key.valid? %p %span.light Fingerprint: %code.key-fingerprint= @key.fingerprint 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 index 2e0f509b17c..8ec78bbd41f 100644 --- 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 @@ -1,5 +1,5 @@ --- -title: Add settings for minimum key strength and allowed key type +title: Add settings for minimum SSH key strength and allowed key type merge_request: 13712 author: Cory Hinshaw type: added 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 2882e7f8b45..5b6079002c0 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,12 +7,13 @@ class AddMinimumKeyLengthToApplicationSettings < ActiveRecord::Migration disable_ddl_transaction! def up - # A key restriction has two possible states: + # A key restriction has these possible states: # # * -1 means "this key type is completely disabled" - # * >= 0 means "keys must have at least this many bits to be valid" + # * 0 means "all keys of this type are valid" + # * > 0 means "keys must have at least this many bits to be valid" # - # A value of 0 is equivalent to "there are no restrictions on keys of this type" + # The default is 0, for backward compatibility add_column_with_default :application_settings, :rsa_key_restriction, :integer, default: 0 add_column_with_default :application_settings, :dsa_key_restriction, :integer, default: 0 add_column_with_default :application_settings, :ecdsa_key_restriction, :integer, default: 0 diff --git a/doc/security/README.md b/doc/security/README.md index 1f54948d113..0fea6be8b55 100644 --- a/doc/security/README.md +++ b/doc/security/README.md @@ -1,7 +1,7 @@ # Security - [Password length limits](password_length_limits.md) -- [Restrict allowed SSH key technologies and minimum length](ssh_keys_restrictions.md) +- [Restrict SSH key technologies and minimum length](ssh_keys_restrictions.md) - [Rack attack](rack_attack.md) - [Webhooks and insecure internal web services](webhooks.md) - [Information exclusivity](information_exclusivity.md) diff --git a/doc/security/ssh_keys_restrictions.md b/doc/security/ssh_keys_restrictions.md index 32ca7dacab3..213fa5bfef5 100644 --- a/doc/security/ssh_keys_restrictions.md +++ b/doc/security/ssh_keys_restrictions.md @@ -2,12 +2,13 @@ `ssh-keygen` allows users to create RSA keys with as few as 768 bits, which falls well below recommendations from certain standards groups (such as the US -NIST). Some organizations deploying Gitlab will need to enforce minimum key +NIST). Some organizations deploying GitLab will need to enforce minimum key strength, either to satisfy internal security policy or for regulatory compliance. -Similarly, certain standards groups recommend using RSA or ECDSA over the older -DSA and administrators may need to limit the allowed SSH key algorithms. +Similarly, certain standards groups recommend using RSA, ECDSA, or ED25519 over +the older DSA, and administrators may need to limit the allowed SSH key +algorithms. GitLab allows you to restrict the allowed SSH key technology as well as specify the minimum key length for each technology. diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 9eab26d111e..62d1ecae676 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -34,8 +34,8 @@ module Gitlab end def check(cmd, changes) - check_valid_actor! check_protocol! + check_valid_actor! check_active_user! check_project_accessibility! check_project_moved! diff --git a/lib/gitlab/ssh_public_key.rb b/lib/gitlab/ssh_public_key.rb index a3f8730fb04..89ca1298120 100644 --- a/lib/gitlab/ssh_public_key.rb +++ b/lib/gitlab/ssh_public_key.rb @@ -13,6 +13,10 @@ module Gitlab Technologies.find { |tech| tech.name.to_s == name.to_s } end + def self.technology_for_key(key) + Technologies.find { |tech| key.is_a?(tech.key_class) } + end + def self.supported_sizes(name) technology(name)&.supported_sizes end @@ -37,9 +41,7 @@ module Gitlab end def type - return unless valid? - - technology.name + technology.name if valid? end def bits @@ -63,12 +65,7 @@ module Gitlab def technology @technology ||= - begin - tech = Technologies.find { |tech| key.is_a?(tech.key_class) } - raise "Unsupported key type: #{key.class}" unless tech - - tech - end + self.class.technology_for_key(key) || raise("Unsupported key type: #{key.class}") end end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 9e4174ecdca..458627ee4de 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -165,12 +165,10 @@ describe Gitlab::GitAccess do stub_application_setting(rsa_key_restriction: 4096) end - it 'does not allow keys which are too small' do - aggregate_failures do - expect(actor).not_to be_valid - expect { pull_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.') - expect { push_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.') - end + it 'does not allow keys which are too small', aggregate_failures: true do + expect(actor).not_to be_valid + expect { pull_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.') + expect { push_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.') end end @@ -179,12 +177,10 @@ describe Gitlab::GitAccess do stub_application_setting(rsa_key_restriction: ApplicationSetting::FORBIDDEN_KEY_VALUE) end - it 'does not allow keys which are too small' do - aggregate_failures do - expect(actor).not_to be_valid - expect { pull_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/) - expect { push_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/) - end + it 'does not allow keys which are too small', aggregate_failures: true do + expect(actor).not_to be_valid + expect { pull_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/) + expect { push_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/) end end end