Commit 00b4c91c authored by James Edwards-Jones's avatar James Edwards-Jones Committed by Douwe Maan

Improvements for IP Address Enforcement prototype

- policies fixed
- schema updated
- Groups::UpdateService to handle params
- form added
- validate subnet, top-level group, and group presence
- ability to combine multiple IpRestriction removed
- avoid saving previous IP address in IpAddressState
- avoid user namespaces
- specs added
- license check added
parent d2a692db
......@@ -17,6 +17,7 @@
%br
%span.descr.text-muted= share_with_group_lock_help_text(@group)
= render_if_exists 'groups/settings/ip_restriction', f: f, group: @group
= render 'groups/settings/lfs', f: f
= render 'groups/settings/project_creation_level', f: f, group: @group
= render 'groups/settings/two_factor_auth', f: f
......
# frozen_string_literal: true
class CreateIpRestriction < ActiveRecord::Migration[5.1]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
create_table :ip_restrictions do |t|
t.references :group, references: :namespace,
column: :group_id,
type: :integer,
null: false,
index: true
t.string :range, null: false
end
add_foreign_key(:ip_restrictions, :namespaces, column: :group_id, on_delete: :cascade) # rubocop: disable Migration/AddConcurrentForeignKey
end
end
......@@ -1590,6 +1590,12 @@ ActiveRecord::Schema.define(version: 20190611161641) do
t.index ["usage", "project_id"], name: "index_internal_ids_on_usage_and_project_id", unique: true, where: "(project_id IS NOT NULL)", using: :btree
end
create_table "ip_restrictions", force: :cascade do |t|
t.integer "group_id", null: false
t.string "range", null: false
t.index ["group_id"], name: "index_ip_restrictions_on_group_id", using: :btree
end
create_table "issue_assignees", id: false, force: :cascade do |t|
t.integer "user_id", null: false
t.integer "issue_id", null: false
......@@ -3687,6 +3693,7 @@ ActiveRecord::Schema.define(version: 20190611161641) do
add_foreign_key "insights", "projects", on_delete: :cascade
add_foreign_key "internal_ids", "namespaces", name: "fk_162941d509", on_delete: :cascade
add_foreign_key "internal_ids", "projects", on_delete: :cascade
add_foreign_key "ip_restrictions", "namespaces", column: "group_id", on_delete: :cascade
add_foreign_key "issue_assignees", "issues", name: "fk_b7d881734a", on_delete: :cascade
add_foreign_key "issue_assignees", "users", name: "fk_5e0c8d9154", on_delete: :cascade
add_foreign_key "issue_links", "issues", column: "source_id", name: "fk_c900194ff2", on_delete: :cascade
......
......@@ -5,6 +5,10 @@ module EE
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
prepended do
around_action :set_current_ip_address
end
def verify_namespace_plan_check_enabled
render_404 unless ::Gitlab::CurrentSettings.should_check_namespace_plan?
end
......@@ -17,5 +21,11 @@ module EE
super
end
end
private
def set_current_ip_address(&block)
::Gitlab::IpAddressState.with(request.ip, &block) # rubocop: disable CodeReuse/ActiveRecord
end
end
end
......@@ -5,6 +5,10 @@ module EE
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
prepended do
before_action :set_ip_restriction, only: [:edit]
end
override :render_show_html
def render_show_html
if redirect_show_path
......@@ -28,6 +32,7 @@ module EE
params_ee << { insight_attributes: [:id, :project_id, :_destroy] } if current_group&.insights_available?
params_ee << :file_template_project_id if current_group&.feature_available?(:custom_file_templates_for_namespace)
params_ee << :custom_project_templates_group_id if current_group&.group_project_template_available?
params_ee << { ip_restriction_attributes: [:id, :range] } if current_group&.feature_available?(:group_ip_restriction)
end
end
......@@ -53,5 +58,11 @@ module EE
def default_group_view
EE::User::DEFAULT_GROUP_VIEW
end
def set_ip_restriction
return if group.ip_restriction.present?
group.build_ip_restriction
end
end
end
......@@ -18,6 +18,8 @@ module EE
has_many :epics
has_one :saml_provider
has_one :ip_restriction
accepts_nested_attributes_for :ip_restriction, allow_destroy: true
has_one :insight, foreign_key: :namespace_id
accepts_nested_attributes_for :insight, allow_destroy: true
has_one :scim_oauth_access_token
......@@ -181,6 +183,12 @@ module EE
projects.detect { |project| !project.empty_repo? }
end
def root_ancestor_ip_restriction
return ip_restriction if parent_id.nil?
root_ancestor.ip_restriction
end
# Overrides a method defined in `::EE::Namespace`
override :checked_file_template_project
def checked_file_template_project(*args, &blk)
......
# frozen_string_literal: true
class IpRestriction < ApplicationRecord
INVALID_SUBNET_ERRORS = [IPAddr::AddressFamilyError,
IPAddr::InvalidAddressError].freeze
belongs_to :group
validates :group_id, presence: true
validates :range, presence: true
validate :valid_subnet
validate :allow_root_group_only
def allows_address?(address)
IPAddr.new(range).include?(address)
rescue *INVALID_SUBNET_ERRORS
false
end
private
def valid_subnet
IPAddr.new(range)
rescue *INVALID_SUBNET_ERRORS
errors.add(:range, _('is an invalid IP address range'))
end
def allow_root_group_only
if group&.parent_id
errors.add(:base, _('IP subnet restriction only allowed for top-level groups'))
end
end
end
......@@ -107,6 +107,7 @@ class License < ApplicationRecord
insights
web_ide_terminal
incident_management
group_ip_restriction
]
EEU_FEATURES.freeze
......
......@@ -29,6 +29,10 @@ module EE
sso_enforcement_prevents_access?
end
condition(:ip_enforcement_prevents_access) do
!::Gitlab::IpRestriction::Enforcer.new(subject).allows_current_ip?
end
condition(:dependency_proxy_available) do
@subject.feature_available?(:dependency_proxy)
end
......@@ -99,6 +103,10 @@ module EE
rule { needs_new_sso_session }.policy do
prevent :read_group
end
rule { ip_enforcement_prevents_access & ~owner }.policy do
prevent :read_group
end
end
override :lookup_access_level!
......
......@@ -230,11 +230,19 @@ module EE
::Gitlab::Auth::GroupSaml::SsoEnforcer.group_access_restricted?(subject.group)
end
condition(:ip_enforcement_prevents_access) do
!::Gitlab::IpRestriction::Enforcer.new(subject.group).allows_current_ip? if subject.group
end
condition(:owner_cannot_destroy_project) do
::Gitlab::CurrentSettings.current_application_settings
.default_project_deletion_protection
end
rule { ip_enforcement_prevents_access }.policy do
prevent :read_project
end
rule { web_ide_terminal_available & can?(:create_pipeline) & can?(:maintainer_access) }.enable :create_web_ide_terminal
# Design abilities could also be prevented in the issue policy.
......
......@@ -12,6 +12,8 @@ module EE
return false if group.errors.present?
end
handle_ip_restriction_deletion
remove_insight_if_insight_project_absent
super.tap { |success| log_audit_event if success }
......@@ -69,6 +71,26 @@ module EE
end
end
def handle_ip_restriction_deletion
return unless ip_restriction_editable?
return unless group.ip_restriction.present?
ip_restriction_params = params[:ip_restriction_attributes]
return unless ip_restriction_params
if ip_restriction_params[:range]&.blank?
ip_restriction_params[:_destroy] = 1
end
end
def ip_restriction_editable?
return false if group.parent_id.present?
true
end
def log_audit_event
EE::Audit::GroupChangesAuditor.new(current_user, group).execute
end
......
- return unless group.feature_available?(:group_ip_restriction)
- read_only = group.parent_id.present?
%h5= _('Restrict access by IP address')
= f.fields_for :ip_restriction do |ip_restriction_form|
.form-group
- if read_only
= ip_restriction_form.text_field :range, value: group.root_ancestor_ip_restriction&.range, class: 'form-control', disabled: true, placeholder: _('No value set by top-level parent group.')
.form-text.text-muted
= _('IP address restriction is not editable in subgroups. Value inherited from top-level parent group.')
- else
= ip_restriction_form.text_field :range, class: 'form-control', placeholder: _('Enter IP address range')
.form-text.text-muted
- read_more_link = link_to(_('Read more'), 'https://gitlab.com/gitlab-org/gitlab-ee/issues/1985')
= _('This group, including all subgroups, projects and git repositories, will only be reachable from the specified IP address range. Example: <code>192.168.0.0/24</code>. %{read_more_link}.').html_safe % { read_more_link: read_more_link }
---
title: IP address restriction for groups
merge_request: 12669
author:
type: added
# frozen_string_literal: true
module Gitlab
class IpAddressState
THREAD_KEY = :current_ip_address
class << self
def with(address)
self.current = address
yield
ensure
self.current = nil
end
def current
Thread.current[THREAD_KEY]
end
protected
def current=(value)
Thread.current[THREAD_KEY] = value
end
end
end
end
# frozen_string_literal: true
module Gitlab
module IpRestriction
class Enforcer
def initialize(group)
@group = group
end
def allows_current_ip?
return true unless group&.feature_available?(:group_ip_restriction)
current_ip_address = Gitlab::IpAddressState.current
return true unless current_ip_address
allows_address?(current_ip_address)
end
private
attr_reader :group
def allows_address?(address)
return true unless group.root_ancestor_ip_restriction
group.root_ancestor_ip_restriction.allows_address?(address)
end
end
end
end
# frozen_string_literal: true
FactoryBot.define do
factory :ip_restriction do
range '192.168.0.0/24'
group
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::IpRestriction::Enforcer do
describe '#allows_current_ip?' do
let(:group) { create(:group) }
let(:current_ip) { '192.168.0.2' }
subject { described_class.new(group).allows_current_ip? }
before do
allow(Gitlab::IpAddressState).to receive(:current).and_return(current_ip)
stub_licensed_features(group_ip_restriction: true)
end
context 'without restriction' do
it { is_expected.to be_truthy }
end
context 'with restriction' do
before do
create(:ip_restriction, group: group, range: range)
end
context 'address is within the range' do
let(:range) { '192.168.0.0/24' }
it { is_expected.to be_truthy }
end
context 'address is outside the range' do
let(:range) { '10.0.0.0/8' }
it { is_expected.to be_falsey }
end
end
context 'feature is disabled' do
before do
stub_licensed_features(group_ip_restriction: false)
end
it { is_expected.to be_truthy }
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::IpAddressState do
describe '.with' do
let(:address) { '1.1.1.1' }
it 'saves IP address' do
described_class.with(address) do
expect(Thread.current[described_class::THREAD_KEY]).to eq(address)
end
end
it 'clears IP address after execution' do
described_class.with(address) { }
expect(Thread.current[described_class::THREAD_KEY]).to eq(nil)
end
it 'clears IP address after execution even when exception occurred' do
expect do
described_class.with(address) { raise 'boom!' }
end.to raise_error(StandardError)
expect(Thread.current[described_class::THREAD_KEY]).to eq(nil)
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe IpRestriction do
describe 'relations' do
it { is_expected.to belong_to(:group) }
end
describe 'validations' do
it { is_expected.to validate_presence_of(:range) }
it { is_expected.to validate_presence_of(:group_id) }
describe '#valid_subnet' do
subject { described_class.new(group: create(:group), range: range) }
context 'valid subnet' do
let(:range) { '192.168.0.0/24' }
it 'succeeds' do
expect(subject.valid?).to be_truthy
end
end
context 'invalid subnet' do
let(:range) { 'boom!' }
it 'fails' do
expect(subject.valid?).to be_falsey
expect(subject.errors[:range]).to include('is an invalid IP address range')
end
end
end
describe '#allow_root_group_only' do
subject { described_class.new(group: group, range: '192.168.0.0/24' ) }
context 'top-level group' do
let(:group) { create(:group) }
it 'succeeds' do
expect(subject.valid?).to be_truthy
end
end
context 'subgroup' do
let(:group) { create(:group, :nested) }
it 'fails' do
expect(subject.valid?).to be_falsey
expect(subject.errors[:base]).to include('IP subnet restriction only allowed for top-level groups')
end
end
end
end
describe '#allows_address?' do
let(:range) { '192.168.0.0/24' }
let(:address) { '192.168.0.1' }
subject { described_class.new(range: range).allows_address?(address) }
context 'address is within the range' do
it { is_expected.to be_truthy }
end
context 'address is outside the range' do
let(:range) { '10.0.0.0/8' }
it { is_expected.to be_falsey }
end
context 'range is invalid' do
let(:range) { nil }
it { is_expected.to be_falsey }
end
context 'address is invalid' do
let(:address) { nil }
it { is_expected.to be_falsey }
end
end
end
......@@ -87,6 +87,46 @@ describe GroupPolicy do
end
end
context 'with ip restriction' do
let(:current_user) { developer }
let(:group) { create(:group, :public) }
before do
allow(Gitlab::IpAddressState).to receive(:current).and_return('192.168.0.2')
stub_licensed_features(group_ip_restriction: true)
end
context 'without restriction' do
it { is_expected.to be_allowed(:read_group) }
end
context 'with restriction' do
before do
create(:ip_restriction, group: group, range: range)
end
context 'address is within the range' do
let(:range) { '192.168.0.0/24' }
it { is_expected.to be_allowed(:read_group) }
end
context 'address is outside the range' do
let(:range) { '10.0.0.0/8' }
context 'as developer' do
it { is_expected.to be_disallowed(:read_group) }
end
context 'as owner' do
let(:current_user) { owner }
it { is_expected.to be_allowed(:read_group) }
end
end
end
end
context 'when LDAP sync is not enabled' do
context 'owner' do
let(:current_user) { owner }
......
......@@ -262,6 +262,45 @@ describe ProjectPolicy do
end
end
end
context 'with ip restriction' do
let(:current_user) { create(:admin) }
let(:group) { create(:group, :public) }
let(:project) { create(:project, group: group) }
before do
allow(Gitlab::IpAddressState).to receive(:current).and_return('192.168.0.2')
stub_licensed_features(group_ip_restriction: true)
end
context 'group without restriction' do
it { is_expected.to be_allowed(:read_project) }
end
context 'group with restriction' do
before do
create(:ip_restriction, group: group, range: range)
end
context 'address is within the range' do
let(:range) { '192.168.0.0/24' }
it { is_expected.to be_allowed(:read_project) }
end
context 'address is outside the range' do
let(:range) { '10.0.0.0/8' }
it { is_expected.to be_disallowed(:read_project) }
end
end
context 'without group' do
let(:project) { create(:project, :repository, namespace: current_user.namespace) }
it { is_expected.to be_allowed(:read_project) }
end
end
end
describe 'read_vulnerability_feedback' do
......
# frozen_string_literal: true
require 'spec_helper'
describe GroupsController, type: :request do
let(:user) { create(:user) }
let(:group) { create(:group) }
describe 'PUT update' do
before do
group.add_owner(user)
login_as(user)
stub_licensed_features(group_ip_restriction: true)
end
subject do
put(group_path(group), params: params)
end
context 'setting ip_restriction' do
let(:group) { create(:group) }
let(:params) { { group: { ip_restriction_attributes: { range: range } } } }
let(:range) { '192.168.0.0/24' }
before do
stub_licensed_features(group_ip_restriction: true)
allow_any_instance_of(Gitlab::IpRestriction::Enforcer).to(
receive(:allows_current_ip?).and_return(true))
end
context 'top-level group' do
context 'when ip_restriction does not exist' do
context 'valid param' do
it 'creates ip restriction' do
expect { subject }
.to(change { group.reload.ip_restriction&.range }
.from(nil).to('192.168.0.0/24'))
expect(response).to have_gitlab_http_status(302)
end
end
context 'invalid param' do
let(:range) { 'boom!' }
it 'adds error message' do
expect { subject }
.not_to(change { group.reload.ip_restriction }.from(nil))
expect(response).to have_gitlab_http_status(200)
expect(response.body).to include('Ip restriction range is an invalid IP address range')
end
end
end
context 'when ip_restriction already exists' do
let!(:ip_restriction) { IpRestriction.create!(group: group, range: '10.0.0.0/8') }
let(:params) { { group: { ip_restriction_attributes: { id: ip_restriction.id, range: range } } } }
context 'ip restriction param set' do
context 'valid param' do
it 'updates ip restriction' do
expect { subject }
.to(change { group.reload.ip_restriction.range }
.from('10.0.0.0/8').to('192.168.0.0/24'))
expect(response).to have_gitlab_http_status(302)
end
end
context 'invalid param' do
let(:range) { 'boom!' }
it 'adds error message' do
expect { subject }
.not_to(change { group.reload.ip_restriction.range }
.from('10.0.0.0/8'))
expect(response).to have_gitlab_http_status(200)
expect(response.body).to include('Ip restriction range is an invalid IP address range')
end
end
end
context 'empty ip restriction param' do
let(:range) { '' }
it 'deletes ip restriction' do
expect { subject }
.to(change { group.reload.ip_restriction }.to(nil))
expect(response).to have_gitlab_http_status(302)
end
end
end
end
context 'subgroup' do
let(:group) { create(:group, :nested) }
it 'does not create ip restriction' do
expect { subject }
.not_to change { group.reload.ip_restriction }.from(nil)
expect(response).to have_gitlab_http_status(200)
expect(response.body).to include('Ip restriction base IP subnet restriction only allowed for top-level groups')
end
end
context 'feature is disabled' do
before do
stub_licensed_features(group_ip_restriction: false)
end
it 'does not create ip restriction' do
expect { subject }
.not_to change { group.reload.ip_restriction }.from(nil)
expect(response).to have_gitlab_http_status(302)
end
end
end
end
end
......@@ -154,6 +154,32 @@ describe Groups::UpdateService, '#execute' do
end
end
context 'setting ip_restriction' do
let(:group) { create(:group) }
subject { update_group(group, user, params) }
before do
stub_licensed_features(group_ip_restriction: true)
end
context 'when ip_restriction already exists' do
let!(:ip_restriction) { IpRestriction.create!(group: group, range: '10.0.0.0/8') }
context 'empty ip restriction param' do
let(:params) { { ip_restriction_attributes: { id: ip_restriction.id, range: '' } } }
it 'deletes ip restriction' do
expect(group.ip_restriction.range).to eql('10.0.0.0/8')
subject
expect(group.reload.ip_restriction).to be_nil
end
end
end
end
context 'updating protected params' do
let(:attrs) { { shared_runners_minutes_limit: 1000, extra_shared_runners_minutes_limit: 100 } }
......
# frozen_string_literal: true
require 'spec_helper'
describe 'groups/edit.html.haml' do
set(:user) { create(:user) }
let(:group) { create(:group) }
before do
group.add_owner(user)
assign(:group, group)
allow(view).to receive(:current_user) { user }
end
context 'ip_restriction' do
before do
stub_licensed_features(group_ip_restriction: true)
end
context 'top-level group' do
before do
create(:ip_restriction, group: group, range: '192.168.0.0/24')
end
it 'renders ip_restriction setting' do
render
expect(rendered).to render_template('groups/settings/_ip_restriction')
expect(rendered).to(have_field('group_ip_restriction_attributes_range',
{ disabled: false,
with: '192.168.0.0/24' }))
end
end
context 'subgroup' do
let(:group) { create(:group, :nested) }
before do
create(:ip_restriction, group: group.parent, range: '192.168.0.0/24')
group.build_ip_restriction
end
it 'show read-only ip_restriction setting of root ancestor' do
render
expect(rendered).to render_template('groups/settings/_ip_restriction')
expect(rendered).to(have_field('group_ip_restriction_attributes_range',
{ disabled: true,
with: '192.168.0.0/24' }))
end
end
context 'feature is disabled' do
before do
stub_licensed_features(group_ip_restriction: false)
end
it 'does not show ip_restriction setting' do
render
expect(rendered).to render_template('groups/settings/_ip_restriction')
expect(rendered).not_to have_field('group_ip_restriction_attributes_range')
end
end
end
end
......@@ -4764,6 +4764,9 @@ msgstr ""
msgid "Enforce DNS rebinding attack protection"
msgstr ""
msgid "Enter IP address range"
msgstr ""
msgid "Enter at least three characters to search"
msgstr ""
......@@ -6880,6 +6883,12 @@ msgstr ""
msgid "IP Address"
msgstr ""
msgid "IP address restriction is not editable in subgroups. Value inherited from top-level parent group."
msgstr ""
msgid "IP subnet restriction only allowed for top-level groups"
msgstr ""
msgid "Identifier"
msgstr ""
......@@ -8880,6 +8889,9 @@ msgstr ""
msgid "No start date"
msgstr ""
msgid "No value set by top-level parent group."
msgstr ""
msgid "No, directly import the existing email addresses and usernames."
msgstr ""
......@@ -11297,6 +11309,9 @@ msgstr ""
msgid "Restart Terminal"
msgstr ""
msgid "Restrict access by IP address"
msgstr ""
msgid "Resume"
msgstr ""
......@@ -13544,6 +13559,9 @@ msgstr ""
msgid "This group does not provide any group Runners yet."
msgstr ""
msgid "This group, including all subgroups, projects and git repositories, will only be reachable from the specified IP address range. Example: <code>192.168.0.0/24</code>. %{read_more_link}."
msgstr ""
msgid "This is a \"Ghost User\", created to hold all issues authored by users that have since been deleted. This user cannot be removed."
msgstr ""
......@@ -16267,6 +16285,9 @@ msgstr[1] ""
msgid "invalid milestone state `%{state}`"
msgstr ""
msgid "is an invalid IP address range"
msgstr ""
msgid "is enabled."
msgstr ""
......
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