From 44010c08b9a5b38e4afbf25b4b163354926c0020 Mon Sep 17 00:00:00 2001 From: johha Date: Fri, 31 Jul 2026 12:14:58 +0200 Subject: [PATCH] Replace netaddr gem with stdlib IPAddr The netaddr gem is effectively unmaintained (no releases despite open requests). Security group rule validation only needs parsing, family detection, and range-endpoint ordering, all of which IPAddr covers. Behaviour is preserved: - leading-zero IPv4 octets are normalised before parsing (IPAddr rejects them, netaddr accepted them); - range endpoints must be plain addresses, so a prefix is rejected there. Add a rule_validator spec covering the v2 validate_destination path, which previously had no dedicated coverage. --- Gemfile | 1 - Gemfile.lock | 2 - .../security_group_rule_validator.rb | 19 ++-- app/models/runtime/security_group.rb | 2 - lib/cloud_controller/rule_validator.rb | 48 +++++++---- .../cloud_controller/rule_validator_spec.rb | 86 +++++++++++++++++++ 6 files changed, 130 insertions(+), 28 deletions(-) create mode 100644 spec/unit/lib/cloud_controller/rule_validator_spec.rb diff --git a/Gemfile b/Gemfile index 03cc493bd84..5d396027e22 100644 --- a/Gemfile +++ b/Gemfile @@ -13,7 +13,6 @@ gem 'json-diff' gem 'json-schema' gem 'mime-types', '~> 3.7' gem 'multipart-parser' -gem 'netaddr', '>= 2.0.4' gem 'newrelic_rpm' gem 'nokogiri', '>=1.10.5' gem 'oj' diff --git a/Gemfile.lock b/Gemfile.lock index f447789d438..ad9f90e862b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -159,7 +159,6 @@ GEM mutex_m (0.3.0) mysql2 (0.5.7) bigdecimal - netaddr (2.0.6) newrelic_rpm (10.6.0) logger nio4r (2.7.5) @@ -461,7 +460,6 @@ DEPENDENCIES mock_redis multipart-parser mysql2 (~> 0.5.7) - netaddr (>= 2.0.4) newrelic_rpm nokogiri (>= 1.10.5) oj diff --git a/app/messages/validators/security_group_rule_validator.rb b/app/messages/validators/security_group_rule_validator.rb index 127aa7dbda5..3827df84579 100644 --- a/app/messages/validators/security_group_rule_validator.rb +++ b/app/messages/validators/security_group_rule_validator.rb @@ -153,30 +153,33 @@ def validate_destination(destination, protocol, allowed_ip_version, record, inde if address_list.length == 1 parsed_ip = CloudController::RuleValidator.parse_ip(address_list.first) add_rule_error(error_message, record, index) unless parsed_ip - add_rule_error("for protocol \"#{protocol}\" you cannot use IPv#{parsed_ip.version} addresses", record, index) \ + add_rule_error("for protocol \"#{protocol}\" you cannot use IPv#{ip_version(parsed_ip)} addresses", record, index) \ unless valid_ip_version?(allowed_ip_version, parsed_ip) elsif address_list.length == 2 ips = CloudController::RuleValidator.parse_ip(address_list) return add_rule_error('destination IP address range is invalid', record, index) unless ips - sorted_ips = if ips.first.is_a?(NetAddr::IPv4) - NetAddr.sort_IPv4(ips) - else - NetAddr.sort_IPv6(ips) - end + sorted_ips = ips.sort reversed_range_error = 'beginning of IP address range is numerically greater than the end of its range (range endpoints are inverted)' add_rule_error(reversed_range_error, record, index) unless ips.first == sorted_ips.first - add_rule_error("for protocol \"#{protocol}\" you cannot use IPv#{ips.first.version} addresses", record, index) \ + add_rule_error("for protocol \"#{protocol}\" you cannot use IPv#{ip_version(ips.first)} addresses", record, index) \ unless valid_ip_version?(allowed_ip_version, sorted_ips.first) else add_rule_error(error_message, record, index) end end + def ip_version(parsed_ip) + return 4 if parsed_ip.ipv4? + return 6 if parsed_ip.ipv6? + + raise ArgumentError.new("unsupported IP family: #{parsed_ip}") + end + def valid_ip_version?(allowed_ip_version, parsed_ip) - parsed_ip.nil? || allowed_ip_version.nil? || parsed_ip.version == allowed_ip_version + parsed_ip.nil? || allowed_ip_version.nil? || ip_version(parsed_ip) == allowed_ip_version end def add_rule_error(message, record, index) diff --git a/app/models/runtime/security_group.rb b/app/models/runtime/security_group.rb index d12000dc6b9..5eaae1a8bfa 100644 --- a/app/models/runtime/security_group.rb +++ b/app/models/runtime/security_group.rb @@ -1,5 +1,3 @@ -require 'netaddr' - module VCAP::CloudController class SecurityGroup < Sequel::Model SECURITY_GROUP_NAME_REGEX = /\A[[:alnum:][:punct:][:print:]]+\Z/ diff --git a/lib/cloud_controller/rule_validator.rb b/lib/cloud_controller/rule_validator.rb index 817b5922cb2..d5c48376bd3 100644 --- a/lib/cloud_controller/rule_validator.rb +++ b/lib/cloud_controller/rule_validator.rb @@ -1,3 +1,5 @@ +require 'ipaddr' + module CloudController class RuleValidator class_attribute :required_fields, :optional_fields @@ -53,11 +55,7 @@ def self.validate_destination(destination) ips = parse_ip(address_list) return false if ips.nil? - sorted_ips = if ips.first.is_a?(NetAddr::IPv4) - NetAddr.sort_IPv4(ips) - else - NetAddr.sort_IPv6(ips) - end + sorted_ips = ips.sort return true if ips.first == sorted_ips.first end @@ -122,26 +120,46 @@ def self.no_leading_zeros(destination) private_class_method def self.parse_ipv4(val) if val.is_a?(Array) - val.map do |ip| - NetAddr::IPv4.parse(ip) - end + val.map { |ip| parse_address(ip, :ipv4?) } else - NetAddr::IPv4Net.parse(val) + parse_cidr(val, :ipv4?) end - rescue NetAddr::ValidationError + rescue IPAddr::Error nil end private_class_method def self.parse_ipv6(val) if val.is_a?(Array) - val.map do |ip| - NetAddr::IPv6.parse(ip) - end + val.map { |ip| parse_address(ip, :ipv6?) } else - NetAddr::IPv6Net.parse(val) + parse_cidr(val, :ipv6?) end - rescue NetAddr::ValidationError + rescue IPAddr::Error nil end + + # A range endpoint must be a plain address, so a prefix is not allowed here. + private_class_method def self.parse_address(val, family) + raise IPAddr::InvalidAddressError if val.include?('/') + + parse_cidr(val, family) + end + + private_class_method def self.parse_cidr(val, family) + ip = IPAddr.new(drop_leading_zeros(val)) + raise IPAddr::InvalidAddressError unless ip.public_send(family) + + ip + end + + # IPAddr rejects zero-padded IPv4 octets (e.g. '010.0.0.53'); normalise them to plain decimals. + private_class_method def self.drop_leading_zeros(val) + return val unless val.include?('.') + + addr, prefix = val.split('/', 2) + # For every octet, delete its leading zeros when a digit follows (e.g. '03.005.10.02' -> '3.5.10.2', but a lone '0' stays); non-numeric octets are left for IPAddr to reject. + addr = addr.split('.').map { |octet| octet.sub(/\A0+(?=\d)/, '') }.join('.') + prefix ? "#{addr}/#{prefix}" : addr + end end end diff --git a/spec/unit/lib/cloud_controller/rule_validator_spec.rb b/spec/unit/lib/cloud_controller/rule_validator_spec.rb new file mode 100644 index 00000000000..1fc67637e97 --- /dev/null +++ b/spec/unit/lib/cloud_controller/rule_validator_spec.rb @@ -0,0 +1,86 @@ +require 'spec_helper' + +module CloudController + RSpec.describe RuleValidator do + describe '.validate_destination' do + subject { described_class.validate_destination(destination) } + + before do + TestConfig.override( + enable_ipv6: true, + security_groups: { enable_comma_delimited_destinations: false } + ) + end + + context 'with a single valid IPv4 address' do + let(:destination) { '192.168.10.2' } + + it { is_expected.to be true } + end + + context 'with a valid CIDR' do + let(:destination) { '10.0.0.0/8' } + + it { is_expected.to be true } + end + + context 'with a valid ascending range' do + let(:destination) { '192.168.10.2-192.168.15.254' } + + it { is_expected.to be true } + end + + context 'with an inverted range' do + let(:destination) { '200.0.0.0-150.0.0.0' } + + it { is_expected.to be false } + end + + context 'with a range whose second endpoint is a CIDR' do + let(:destination) { '1.1.1.1-2.2.2.2/30' } + + it { is_expected.to be false } + end + + context 'with a malformed address' do + let(:destination) { '999.999.999.999' } + + it { is_expected.to be false } + end + + # This path deliberately does not run the no_leading_zeros check (unlike the v3 message + # validator), so zero-padded octets are accepted and normalised, matching prior behaviour. + context 'with leading zeros' do + context 'in the first octet' do + let(:destination) { '010.0.0.53' } + + it { is_expected.to be true } + end + + context 'in every octet' do + let(:destination) { '03.005.010.02' } + + it { is_expected.to be true } + end + + context 'in a CIDR' do + let(:destination) { '010.000.000.000/24' } + + it { is_expected.to be true } + end + end + + context 'with a valid IPv6 address' do + let(:destination) { '2001:db8::1' } + + it { is_expected.to be true } + end + + context 'with an inverted IPv6 range' do + let(:destination) { '2001:db8::ff-2001:db8::1' } + + it { is_expected.to be false } + end + end + end +end