Commit b84ca08e authored by Nick Thomas's avatar Nick Thomas

Address review comments

parent 68470602
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) <<
......
......@@ -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
......
require 'digest/md5'
class Key < ActiveRecord::Base
include AfterCommitQueue
include Gitlab::CurrentSettings
include Sortable
......
......@@ -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
......
---
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
......@@ -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
......
# 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)
......
......@@ -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.
......
......@@ -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!
......
......@@ -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
......@@ -165,29 +165,25 @@ 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
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
end
context 'key type is not allowed' do
before 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
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
end
it_behaves_like '#check with a key that is not valid' do
let(:actor) { build(:rsa_key_2048, user: user) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment