From 9f0a38d6c0727d29da2718eefa130aee4b94f01c Mon Sep 17 00:00:00 2001 From: Vishal Tak Date: Mon, 30 Oct 2023 16:36:31 +0530 Subject: [PATCH] Add egress ip options in remote development agent configs Add options to configure the egress ip allow list for the remote development module of the gitlab agent for kubernetes. Add custom model validators for ip address cidr, array of ip address cidr and network policy egress for remote development. Changelog: added EE: true --- app/validators/ip_cidr_array_validator.rb | 25 +++++++++ app/validators/ip_cidr_validator.rb | 45 ++++++++++++++++ ...2104_add_network_policy_egress_to_agent.rb | 22 ++++++++ db/schema_migrations/20231107062104 | 1 + db/structure.sql | 1 + .../remote_development_agent_config.rb | 5 ++ ...t_agent_configs_network_policy_egress.json | 21 ++++++++ .../network_policy_egress_validator.rb | 54 +++++++++++++++++++ .../remote_development_agent_config_spec.rb | 27 ++++++++++ .../network_policy_egress_validator_spec.rb | 46 ++++++++++++++++ locale/gitlab.pot | 24 +++++++++ .../ip_cidr_array_validator_spec.rb | 45 ++++++++++++++++ spec/validators/ip_cidr_validator_spec.rb | 45 ++++++++++++++++ 13 files changed, 361 insertions(+) create mode 100644 app/validators/ip_cidr_array_validator.rb create mode 100644 app/validators/ip_cidr_validator.rb create mode 100644 db/migrate/20231107062104_add_network_policy_egress_to_agent.rb create mode 100644 db/schema_migrations/20231107062104 create mode 100644 ee/app/validators/json_schemas/remote_development_agent_configs_network_policy_egress.json create mode 100644 ee/app/validators/remote_development/network_policy_egress_validator.rb create mode 100644 ee/spec/validators/remote_development/network_policy_egress_validator_spec.rb create mode 100644 spec/validators/ip_cidr_array_validator_spec.rb create mode 100644 spec/validators/ip_cidr_validator_spec.rb diff --git a/app/validators/ip_cidr_array_validator.rb b/app/validators/ip_cidr_array_validator.rb new file mode 100644 index 00000000000000..fff1368508fa14 --- /dev/null +++ b/app/validators/ip_cidr_array_validator.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +# IpCidrArrayValidator +# +# Validates that an array of IP are a valid IPv4 or IPv6 CIDR address. +# +# Example: +# +# class Group < ActiveRecord::Base +# validates :ip_array, presence: true, ip_cidr_array: true +# end + +class IpCidrArrayValidator < ActiveModel::EachValidator # rubocop:disable Gitlab/NamespacedClass -- This is a globally shareable validator, but it's unclear what namespace it should belong in + def validate_each(record, attribute, value) + unless value.is_a?(Array) + record.errors.add(attribute, _("must be an array of CIDR values")) + return + end + + value.each do |cidr| + single_validator = IpCidrValidator.new(attributes: attribute) + single_validator.validate_each(record, attribute, cidr) + end + end +end diff --git a/app/validators/ip_cidr_validator.rb b/app/validators/ip_cidr_validator.rb new file mode 100644 index 00000000000000..b1760a99d6d481 --- /dev/null +++ b/app/validators/ip_cidr_validator.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +# IpCidrValidator +# +# Validates that an IP is a valid IPv4 or IPv6 CIDR address. +# +# Example: +# +# class Group < ActiveRecord::Base +# validates :ip, presence: true, ip_cidr: true +# end + +class IpCidrValidator < ActiveModel::EachValidator # rubocop:disable Gitlab/NamespacedClass -- This is a globally shareable validator, but it's unclear what namespace it should belong in + def validate_each(record, attribute, value) + # NOTE: We want this to be usable for nullable fields, so we don't validate presence. + # Use a separate `presence` validation for the field if needed. + return true if value.blank? + + # rubocop:disable Layout/LineLength -- The error message is bigger than the line limit + unless valid_cidr_format?(value) + record.errors.add( + attribute, + format(_( + "IP '%{value}' is not a valid CIDR: IP should be followed by a slash followed by an integer subnet mask (for example: '192.168.1.0/24')"), + value: value + ) + ) + return + end + # rubocop:enable Layout/LineLength + + IPAddress.parse(value) + rescue ArgumentError => e + record.errors.add( + attribute, + format(_("IP '%{value}' is not a valid CIDR: %{message}"), value: value, message: e.message) + ) + end + + private + + def valid_cidr_format?(cidr) + cidr.count('/') == 1 && cidr.split('/').last =~ /^\d+$/ + end +end diff --git a/db/migrate/20231107062104_add_network_policy_egress_to_agent.rb b/db/migrate/20231107062104_add_network_policy_egress_to_agent.rb new file mode 100644 index 00000000000000..c7f9da1831ca41 --- /dev/null +++ b/db/migrate/20231107062104_add_network_policy_egress_to_agent.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class AddNetworkPolicyEgressToAgent < Gitlab::Database::Migration[2.2] + milestone '16.6' + + NETWORK_POLICY_EGRESS_DEFAULT = [{ + allow: "0.0.0.0/0", + except: [ + - "10.0.0.0/8", + - "172.16.0.0/12", + - "192.168.0.0/16" + ] + }] + + def change + add_column :remote_development_agent_configs, + :network_policy_egress, + :jsonb, + null: false, + default: NETWORK_POLICY_EGRESS_DEFAULT + end +end diff --git a/db/schema_migrations/20231107062104 b/db/schema_migrations/20231107062104 new file mode 100644 index 00000000000000..b58cae8fc668bd --- /dev/null +++ b/db/schema_migrations/20231107062104 @@ -0,0 +1 @@ +d31386b36b5db29deb9041febc116915f94fa7c551f1d91d5f474671dccdc709 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 2de7aa4a6ab569..7254f10f2ded5e 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -22464,6 +22464,7 @@ CREATE TABLE remote_development_agent_configs ( dns_zone text NOT NULL, network_policy_enabled boolean DEFAULT true NOT NULL, gitlab_workspaces_proxy_namespace text DEFAULT 'gitlab-workspaces'::text NOT NULL, + network_policy_egress jsonb DEFAULT '[{"allow": "0.0.0.0/0", "except": ["10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"]}]'::jsonb NOT NULL, CONSTRAINT check_72947a4495 CHECK ((char_length(gitlab_workspaces_proxy_namespace) <= 63)), CONSTRAINT check_9f5cd54d1c CHECK ((char_length(dns_zone) <= 256)) ); diff --git a/ee/app/models/remote_development/remote_development_agent_config.rb b/ee/app/models/remote_development/remote_development_agent_config.rb index bf9b6f510fff45..ebe4d4cc505fcc 100644 --- a/ee/app/models/remote_development/remote_development_agent_config.rb +++ b/ee/app/models/remote_development/remote_development_agent_config.rb @@ -18,6 +18,11 @@ class RemoteDevelopmentAgentConfig < ApplicationRecord # by throwing an `ArgumentError` on `#save`, instead of `#save!`. # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129708#note_1538946504 for more context. validates :enabled, inclusion: { in: [true], message: 'is currently immutable, and must be set to true' } + + validates :network_policy_egress, + json_schema: { filename: 'remote_development_agent_configs_network_policy_egress' } + validates :network_policy_egress, 'remote_development/network_policy_egress': true + # noinspection RubyResolve - likely due to https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31540 before_validation :prevent_dns_zone_update, if: ->(record) { record.persisted? && record.dns_zone_changed? } diff --git a/ee/app/validators/json_schemas/remote_development_agent_configs_network_policy_egress.json b/ee/app/validators/json_schemas/remote_development_agent_configs_network_policy_egress.json new file mode 100644 index 00000000000000..be2ed83402c12e --- /dev/null +++ b/ee/app/validators/json_schemas/remote_development_agent_configs_network_policy_egress.json @@ -0,0 +1,21 @@ +{ + "description": "Network Policy Egress rules for Remote Development Agent Configs", + "type": "array", + "items": { + "type": "object", + "required": [ + "allow" + ], + "properties": { + "allow": { + "type": "string" + }, + "except": { + "type": "array", + "items": { + "type": "string" + } + } + } + } +} diff --git a/ee/app/validators/remote_development/network_policy_egress_validator.rb b/ee/app/validators/remote_development/network_policy_egress_validator.rb new file mode 100644 index 00000000000000..736e09d6959218 --- /dev/null +++ b/ee/app/validators/remote_development/network_policy_egress_validator.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module RemoteDevelopment + class NetworkPolicyEgressValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + unless value.is_a?(Array) + record.errors.add(attribute, _("must be an array")) + return + end + + value.each do |egress_rule| + unless egress_rule.is_a?(Hash) + record.errors.add(attribute, _("must be an array of hash")) + break + end + + # noinspection RubyMismatchedArgumentType - RubyMine is resolving egress_rule as an array instead of a hash + allow = egress_rule.deep_symbolize_keys.fetch(:allow, nil) + # noinspection RubyMismatchedArgumentType - RubyMine is resolving egress_rule as an array instead of a hash + except = egress_rule.deep_symbolize_keys.fetch(:except, []) + + if allow.nil? + record.errors.add( + attribute, + _("must be an array of hash containing 'allow' attribute of type string") + ) + break + end + + unless allow.is_a?(String) + record.errors.add( + attribute, + format(_("'allow: %{allow}' must be a string"), allow: allow) + ) + break + end + + allow_validator = IpCidrValidator.new(attributes: attribute) + allow_validator.validate_each(record, attribute, allow) + + unless except.is_a?(Array) + record.errors.add( + attribute, + format(_("'except: %{except}' must be an array of string"), except: except) + ) + break + end + + except_validator = IpCidrArrayValidator.new(attributes: attribute) + except_validator.validate_each(record, attribute, except) + end + end + end +end diff --git a/ee/spec/models/remote_development/remote_development_agent_config_spec.rb b/ee/spec/models/remote_development/remote_development_agent_config_spec.rb index dee953aaba9a68..5a5817fb51e260 100644 --- a/ee/spec/models/remote_development/remote_development_agent_config_spec.rb +++ b/ee/spec/models/remote_development/remote_development_agent_config_spec.rb @@ -2,9 +2,22 @@ require 'spec_helper' +# noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542 RSpec.describe RemoteDevelopment::RemoteDevelopmentAgentConfig, feature_category: :remote_development do # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31543 let_it_be_with_reload(:agent) { create(:ee_cluster_agent, :with_remote_development_agent_config) } + let(:default_network_policy_egress) do + [ + { + allow: "0.0.0.0/0", + except: [ + - "10.0.0.0/8", + - "172.16.0.0/12", + - "192.168.0.0/16" + ] + }.deep_stringify_keys + ] + end subject { agent.remote_development_agent_config } @@ -45,5 +58,19 @@ ) end end + + it 'when network_policy_egress is not specified explicitly' do + expect(subject).to be_valid + expect(subject.network_policy_egress).to eq(default_network_policy_egress) + end + + it 'when network_policy_egress is nil' do + subject.network_policy_egress = nil + expect(subject).not_to be_valid + expect(subject.errors[:network_policy_egress]).to include( + 'must be a valid json schema', + 'must be an array' + ) + end end end diff --git a/ee/spec/validators/remote_development/network_policy_egress_validator_spec.rb b/ee/spec/validators/remote_development/network_policy_egress_validator_spec.rb new file mode 100644 index 00000000000000..9c1a957b1e8717 --- /dev/null +++ b/ee/spec/validators/remote_development/network_policy_egress_validator_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe RemoteDevelopment::NetworkPolicyEgressValidator, feature_category: :remote_development do + let(:model) do + Class.new do + include ActiveModel::Model + include ActiveModel::Validations + + attr_accessor :egress + alias_method :egress_before_type_cast, :egress + + validates :egress, 'remote_development/network_policy_egress': true + end.new + end + + using RSpec::Parameterized::TableSyntax + + # noinspection RubyMismatchedArgumentType - RubyMine is resolving `#|` from Array, instead of Rspec::Parameterized + where(:egress, :validity, :errors) do + # rubocop:disable Layout/LineLength -- The RSpec table syntax often requires long lines for errors + nil | false | { egress: ['must be an array'] } + 'not-an-array' | false | { egress: ['must be an array'] } + [nil] | false | { egress: ['must be an array of hash'] } + [{ allow: nil }] | false | { egress: ["must be an array of hash containing 'allow' attribute of type string"] } + [{ except: [] }] | false | { egress: ["must be an array of hash containing 'allow' attribute of type string"] } + [{ allow: 1 }] | false | { egress: ["'allow: 1' must be a string"] } + [{ allow: '10.0.0.0/32', except: nil }] | false | { egress: ["'except: ' must be an array of string"] } + [{ allow: '10.0.0.0/40', except: [] }] | false | { egress: ["IP '10.0.0.0/40' is not a valid CIDR: Prefix must be in range 0..32, got: 40"] } + [{ allow: '10.0.0.0/32', except: ['10.0.0.0/40'] }] | false | { egress: ["IP '10.0.0.0/40' is not a valid CIDR: Prefix must be in range 0..32, got: 40"] } + [] | true | {} + [{ allow: '10.0.0.0/32', except: [] }] | true | {} + # rubocop:enable Layout/LineLength + end + + with_them do + before do + model.egress = egress + model.validate + end + + it { expect(model.valid?).to eq(validity) } + it { expect(model.errors.messages).to eq(errors) } + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 2185dc9b9410df..14cb51d34c6a3b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1375,6 +1375,12 @@ msgstr "" msgid "'%{value}' days of inactivity must be greater than or equal to 90" msgstr "" +msgid "'allow: %{allow}' must be a string" +msgstr "" + +msgid "'except: %{except}' must be an array of string" +msgstr "" + msgid "'projects' is not yet supported" msgstr "" @@ -24188,6 +24194,12 @@ msgstr "" msgid "INFO: Your SSH key is expiring soon. Please generate a new key." msgstr "" +msgid "IP '%{value}' is not a valid CIDR: %{message}" +msgstr "" + +msgid "IP '%{value}' is not a valid CIDR: IP should be followed by a slash followed by an integer subnet mask (for example: '192.168.1.0/24')" +msgstr "" + msgid "IP Address" msgstr "" @@ -57529,6 +57541,18 @@ msgstr "" msgid "must be after start" msgstr "" +msgid "must be an array" +msgstr "" + +msgid "must be an array of CIDR values" +msgstr "" + +msgid "must be an array of hash" +msgstr "" + +msgid "must be an array of hash containing 'allow' attribute of type string" +msgstr "" + msgid "must be an email you have verified" msgstr "" diff --git a/spec/validators/ip_cidr_array_validator_spec.rb b/spec/validators/ip_cidr_array_validator_spec.rb new file mode 100644 index 00000000000000..4ea46d714c2921 --- /dev/null +++ b/spec/validators/ip_cidr_array_validator_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IpCidrArrayValidator, feature_category: :shared do + let(:model) do + Class.new do + include ActiveModel::Model + include ActiveModel::Validations + + attr_accessor :cidr_array + alias_method :cidr_array_before_type_cast, :cidr_array + + validates :cidr_array, ip_cidr_array: true + end.new + end + + using RSpec::Parameterized::TableSyntax + + # noinspection RubyMismatchedArgumentType - RubyMine is resolving `#|` from Array, instead of Rspec::Parameterized + where(:cidr_array, :validity, :errors) do + # rubocop:disable Layout/LineLength -- The RSpec table syntax often requires long lines for errors + nil | false | { cidr_array: ["must be an array of CIDR values"] } + '' | false | { cidr_array: ["must be an array of CIDR values"] } + ['172.0.0.1/256'] | false | { cidr_array: ["IP '172.0.0.1/256' is not a valid CIDR: Invalid netmask 256"] } + %w[172.0.0.1/24 invalid-CIDR] | false | { cidr_array: ["IP 'invalid-CIDR' is not a valid CIDR: IP should be followed by a slash followed by an integer subnet mask (for example: '192.168.1.0/24')"] } + %w[172.0.0.1/256 invalid-CIDR] | false | { cidr_array: ["IP '172.0.0.1/256' is not a valid CIDR: Invalid netmask 256", "IP 'invalid-CIDR' is not a valid CIDR: IP should be followed by a slash followed by an integer subnet mask (for example: '192.168.1.0/24')"] } + ['172.0.0.1/24', nil] | true | {} + %w[172.0.0.1/24 2001:db8::8:800:200c:417a/128] | true | {} + [] | true | {} + [nil] | true | {} + [''] | true | {} + # rubocop:enable Layout/LineLength + end + + with_them do + before do + model.cidr_array = cidr_array + model.validate + end + + it { expect(model.valid?).to eq(validity) } + it { expect(model.errors.messages).to eq(errors) } + end +end diff --git a/spec/validators/ip_cidr_validator_spec.rb b/spec/validators/ip_cidr_validator_spec.rb new file mode 100644 index 00000000000000..213991d9f4f217 --- /dev/null +++ b/spec/validators/ip_cidr_validator_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IpCidrValidator, feature_category: :shared do + let(:model) do + Class.new do + include ActiveModel::Model + include ActiveModel::Validations + + attr_accessor :cidr + alias_method :cidr_before_type_cast, :cidr + + validates :cidr, ip_cidr: true + end.new + end + + using RSpec::Parameterized::TableSyntax + + where(:cidr, :validity, :errors) do + # rubocop:disable Layout/LineLength -- The RSpec table syntax often requires long lines for errors' + 'invalid-CIDR' | false | { cidr: ["IP 'invalid-CIDR' is not a valid CIDR: IP should be followed by a slash followed by an integer subnet mask (for example: '192.168.1.0/24')"] } + '172.0.0.1|256' | false | { cidr: ["IP '172.0.0.1|256' is not a valid CIDR: IP should be followed by a slash followed by an integer subnet mask (for example: '192.168.1.0/24')"] } + '172.0.0.1' | false | { cidr: ["IP '172.0.0.1' is not a valid CIDR: IP should be followed by a slash followed by an integer subnet mask (for example: '192.168.1.0/24')"] } + '172.0.0.1/2/12' | false | { cidr: ["IP '172.0.0.1/2/12' is not a valid CIDR: IP should be followed by a slash followed by an integer subnet mask (for example: '192.168.1.0/24')"] } + '172.0.0.1/256' | false | { cidr: ["IP '172.0.0.1/256' is not a valid CIDR: Invalid netmask 256"] } + '2001:db8::8:800:200c:417a/129' | false | { cidr: ["IP '2001:db8::8:800:200c:417a/129' is not a valid CIDR: Prefix must be in range 0..128, got: 129"] } + '2001:db8::8:800:200c:417a' | false | { cidr: ["IP '2001:db8::8:800:200c:417a' is not a valid CIDR: IP should be followed by a slash followed by an integer subnet mask (for example: '192.168.1.0/24')"] } + '2001:db8::8:800:200c:417a/128' | true | {} + '172.0.0.1/32' | true | {} + '' | true | {} + nil | true | {} + # rubocop:enable Layout/LineLength + end + + with_them do + before do + model.cidr = cidr + model.validate + end + + it { expect(model.valid?).to eq(validity) } + it { expect(model.errors.messages).to eq(errors) } + end +end -- GitLab