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