diff --git a/app/validators/ip_cidr_array_validator.rb b/app/validators/ip_cidr_array_validator.rb new file mode 100644 index 0000000000000000000000000000000000000000..fff1368508fa14c325a6b971966f2449593c479d --- /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 0000000000000000000000000000000000000000..b1760a99d6d481628fb8fdfb94cd8980866a056a --- /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 0000000000000000000000000000000000000000..c7f9da1831ca41fd70932eda385719ba5e3c6f52 --- /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 0000000000000000000000000000000000000000..b58cae8fc668bdd2adb58adf9e884dd4ff28f70e --- /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 2de7aa4a6ab5695214c8e05731f583bcd76e02d6..7254f10f2ded5ec0f4204c09fd5100c15eebc7db 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 bf9b6f510fff45827a6547e5872341b580dd9819..ebe4d4cc505fcccf65083950de1ef77b66ac2ef2 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 0000000000000000000000000000000000000000..be2ed83402c12e70c1b059457760587037316909 --- /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 0000000000000000000000000000000000000000..736e09d6959218cdaaf88e5caf0917315c26770c --- /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 dee953aaba9a6839f34b705709db6ddcdab2aa3a..5a5817fb51e26078e9a6793123c5c6a7321cda39 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 0000000000000000000000000000000000000000..9c1a957b1e8717c4cdf851e428757995add72b78 --- /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 2185dc9b9410df5586d937bef6106b5ea7887ac5..14cb51d34c6a3b35617f52f4a42f4bf2de64b4c5 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 0000000000000000000000000000000000000000..4ea46d714c2921bc1d90506d9939c4cde18e8fde --- /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 0000000000000000000000000000000000000000..213991d9f4f217e34f1f4ce80d9f3b55549e865c --- /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