From 4158efc7dc4d5c422227216c1a4b79508174357f 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. This also aligns validation with downstream enforcement. netaddr accepted zero-padded IPv4 octets (e.g. 010.0.0.53, or ::ffff:010.0.0.1) that Diego rejects at enforcement time via Go's net.ParseIP, so such rules passed CC but never applied. IPAddr rejects them, surfacing the error to the operator at creation instead of failing silently later. The v3 message validator already rejected leading zeros; this brings the v2 path into line. Zero-padded IPv6 hextets remain valid (RFC 4291), unchanged. Range endpoints must be plain addresses, so a prefix is rejected there. Add a rule_validator spec covering the v2 validate_destination path (IPv4 leading-zero rejection, IPv6 padding, IPv4-mapped addresses), which previously had no dedicated coverage, and fix pre-existing test fixtures that relied on netaddr accepting malformed addresses (a trailing-dot range endpoint and a zero-padded octet). --- Gemfile | 1 - Gemfile.lock | 2 - .../security_group_rule_validator.rb | 25 ++-- app/models/runtime/security_group.rb | 2 - lib/cloud_controller/rule_validator.rb | 38 +++--- .../cloud_controller/rule_validator_spec.rb | 119 ++++++++++++++++++ .../security_group_rule_validator_spec.rb | 2 +- .../models/runtime/security_group_spec.rb | 6 +- 8 files changed, 161 insertions(+), 34 deletions(-) create mode 100644 spec/unit/lib/cloud_controller/rule_validator_spec.rb diff --git a/Gemfile b/Gemfile index 1043c9171f9..063d97840e3 100644 --- a/Gemfile +++ b/Gemfile @@ -12,7 +12,6 @@ gem 'httpclient' gem 'json-diff' gem 'json-schema' gem 'mime-types', '~> 3.7' -gem 'netaddr', '>= 2.0.4' gem 'newrelic_rpm' gem 'nokogiri', '>=1.10.5' gem 'oj' diff --git a/Gemfile.lock b/Gemfile.lock index 483e7f5446a..2b55b80efcf 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -158,7 +158,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) @@ -459,7 +458,6 @@ DEPENDENCIES mime-types (~> 3.7) mock_redis 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..11773e887d0 100644 --- a/app/messages/validators/security_group_rule_validator.rb +++ b/app/messages/validators/security_group_rule_validator.rb @@ -149,34 +149,39 @@ def validate_destination(destination, protocol, allowed_ip_version, record, inde address_list = destination.split('-') zeros_error_message = 'destination octets cannot contain leading zeros' - add_rule_error(zeros_error_message, record, index) unless CloudController::RuleValidator.no_leading_zeros(address_list) + no_leading_zeros = CloudController::RuleValidator.no_leading_zeros(address_list) + add_rule_error(zeros_error_message, record, index) unless no_leading_zeros 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) \ + # The leading-zeros error already explains this parse failure; don't pile on the generic one. + add_rule_error(error_message, record, index) if parsed_ip.nil? && no_leading_zeros + 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..47bb452cd9a 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,36 @@ 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(val) + raise IPAddr::InvalidAddressError unless ip.public_send(family) + + ip + 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..a43c0b56d23 --- /dev/null +++ b/spec/unit/lib/cloud_controller/rule_validator_spec.rb @@ -0,0 +1,119 @@ +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 + + # Zero-padded IPv4 octets are ambiguous (decimal vs. octal) and are rejected downstream by Diego and vxlan-policy-agent. + context 'with leading zeros in an IPv4 address' do + context 'in the first octet' do + let(:destination) { '010.0.0.53' } + + it { is_expected.to be false } + end + + context 'in every octet' do + let(:destination) { '03.005.010.02' } + + it { is_expected.to be false } + end + + context 'in a CIDR' do + let(:destination) { '010.000.000.000/24' } + + it { is_expected.to be false } + end + + context 'in a range endpoint' do + let(:destination) { '010.0.0.1-10.0.0.9' } + + it { is_expected.to be false } + end + end + + context 'with a valid IPv6 address' do + let(:destination) { '2001:db8::1' } + + it { is_expected.to be true } + end + + # Zero-padded IPv6 hextets are valid (RFC 4291); exhaustive coverage lives in the v3 spec (#4367). + context 'with a zero-padded IPv6 segment' do + let(:destination) { '2001:0db8::1' } + + it { is_expected.to be true } + end + + # A zero-padded embedded octet carries the same decimal/octal ambiguity as a bare IPv4 address. + context 'with an IPv4-mapped IPv6 address' do + context 'with a plain embedded octet' do + let(:destination) { '::ffff:10.0.0.1' } + + it { is_expected.to be true } + end + + context 'with a zero-padded embedded octet' do + let(:destination) { '::ffff:010.0.0.1' } + + it { is_expected.to be false } + end + end + + context 'with a valid IPv6 range' do + let(:destination) { '2001:db8::1-2001:db8::ff' } + + 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 diff --git a/spec/unit/messages/validators/security_group_rule_validator_spec.rb b/spec/unit/messages/validators/security_group_rule_validator_spec.rb index 9ca99b78edd..308f5699d40 100644 --- a/spec/unit/messages/validators/security_group_rule_validator_spec.rb +++ b/spec/unit/messages/validators/security_group_rule_validator_spec.rb @@ -1487,7 +1487,7 @@ def self.name [ { protocol: 'icmpv6', - destination: '1.0.0.000-1.0.0.200', + destination: '1.0.0.1-1.0.0.200', type: -1, code: 255 } diff --git a/spec/unit/models/runtime/security_group_spec.rb b/spec/unit/models/runtime/security_group_spec.rb index b1ffaa9884d..0f62b246587 100644 --- a/spec/unit/models/runtime/security_group_spec.rb +++ b/spec/unit/models/runtime/security_group_spec.rb @@ -115,7 +115,7 @@ def build_all_rule(attrs={}) end context 'when it is a valid range' do - let(:rule) { build_transport_rule('protocol' => protocol, 'destination' => '1.1.1.1.-2.2.2.2') } + let(:rule) { build_transport_rule('protocol' => protocol, 'destination' => '1.1.1.1-2.2.2.2') } it 'is valid' do expect(subject).to be_valid @@ -281,7 +281,7 @@ def build_all_rule(attrs={}) end context 'when it is a valid range' do - let(:rule) { build_transport_rule('protocol' => protocol, 'destination' => '1.1.1.1.-2.2.2.2') } + let(:rule) { build_transport_rule('protocol' => protocol, 'destination' => '1.1.1.1-2.2.2.2') } it 'is valid' do expect(subject).to be_valid @@ -758,7 +758,7 @@ def build_all_rule(attrs={}) end context 'when it is a valid range' do - let(:rule) { build_all_rule('destination' => '1.1.1.1.-2.2.2.2') } + let(:rule) { build_all_rule('destination' => '1.1.1.1-2.2.2.2') } it 'is valid' do expect(subject).to be_valid