Skip to content

Commit 4158efc

Browse files
committed
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).
1 parent 12e5d15 commit 4158efc

8 files changed

Lines changed: 161 additions & 34 deletions

File tree

Gemfile

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ gem 'httpclient'
1212
gem 'json-diff'
1313
gem 'json-schema'
1414
gem 'mime-types', '~> 3.7'
15-
gem 'netaddr', '>= 2.0.4'
1615
gem 'newrelic_rpm'
1716
gem 'nokogiri', '>=1.10.5'
1817
gem 'oj'

Gemfile.lock

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,6 @@ GEM
158158
mutex_m (0.3.0)
159159
mysql2 (0.5.7)
160160
bigdecimal
161-
netaddr (2.0.6)
162161
newrelic_rpm (10.6.0)
163162
logger
164163
nio4r (2.7.5)
@@ -459,7 +458,6 @@ DEPENDENCIES
459458
mime-types (~> 3.7)
460459
mock_redis
461460
mysql2 (~> 0.5.7)
462-
netaddr (>= 2.0.4)
463461
newrelic_rpm
464462
nokogiri (>= 1.10.5)
465463
oj

app/messages/validators/security_group_rule_validator.rb

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -149,34 +149,39 @@ def validate_destination(destination, protocol, allowed_ip_version, record, inde
149149
address_list = destination.split('-')
150150

151151
zeros_error_message = 'destination octets cannot contain leading zeros'
152-
add_rule_error(zeros_error_message, record, index) unless CloudController::RuleValidator.no_leading_zeros(address_list)
152+
no_leading_zeros = CloudController::RuleValidator.no_leading_zeros(address_list)
153+
add_rule_error(zeros_error_message, record, index) unless no_leading_zeros
153154
if address_list.length == 1
154155
parsed_ip = CloudController::RuleValidator.parse_ip(address_list.first)
155-
add_rule_error(error_message, record, index) unless parsed_ip
156-
add_rule_error("for protocol \"#{protocol}\" you cannot use IPv#{parsed_ip.version} addresses", record, index) \
156+
# The leading-zeros error already explains this parse failure; don't pile on the generic one.
157+
add_rule_error(error_message, record, index) if parsed_ip.nil? && no_leading_zeros
158+
add_rule_error("for protocol \"#{protocol}\" you cannot use IPv#{ip_version(parsed_ip)} addresses", record, index) \
157159
unless valid_ip_version?(allowed_ip_version, parsed_ip)
158160
elsif address_list.length == 2
159161
ips = CloudController::RuleValidator.parse_ip(address_list)
160162

161163
return add_rule_error('destination IP address range is invalid', record, index) unless ips
162164

163-
sorted_ips = if ips.first.is_a?(NetAddr::IPv4)
164-
NetAddr.sort_IPv4(ips)
165-
else
166-
NetAddr.sort_IPv6(ips)
167-
end
165+
sorted_ips = ips.sort
168166

169167
reversed_range_error = 'beginning of IP address range is numerically greater than the end of its range (range endpoints are inverted)'
170168
add_rule_error(reversed_range_error, record, index) unless ips.first == sorted_ips.first
171-
add_rule_error("for protocol \"#{protocol}\" you cannot use IPv#{ips.first.version} addresses", record, index) \
169+
add_rule_error("for protocol \"#{protocol}\" you cannot use IPv#{ip_version(ips.first)} addresses", record, index) \
172170
unless valid_ip_version?(allowed_ip_version, sorted_ips.first)
173171
else
174172
add_rule_error(error_message, record, index)
175173
end
176174
end
177175

176+
def ip_version(parsed_ip)
177+
return 4 if parsed_ip.ipv4?
178+
return 6 if parsed_ip.ipv6?
179+
180+
raise ArgumentError.new("unsupported IP family: #{parsed_ip}")
181+
end
182+
178183
def valid_ip_version?(allowed_ip_version, parsed_ip)
179-
parsed_ip.nil? || allowed_ip_version.nil? || parsed_ip.version == allowed_ip_version
184+
parsed_ip.nil? || allowed_ip_version.nil? || ip_version(parsed_ip) == allowed_ip_version
180185
end
181186

182187
def add_rule_error(message, record, index)

app/models/runtime/security_group.rb

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
require 'netaddr'
2-
31
module VCAP::CloudController
42
class SecurityGroup < Sequel::Model
53
SECURITY_GROUP_NAME_REGEX = /\A[[:alnum:][:punct:][:print:]]+\Z/

lib/cloud_controller/rule_validator.rb

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
require 'ipaddr'
2+
13
module CloudController
24
class RuleValidator
35
class_attribute :required_fields, :optional_fields
@@ -53,11 +55,7 @@ def self.validate_destination(destination)
5355
ips = parse_ip(address_list)
5456
return false if ips.nil?
5557

56-
sorted_ips = if ips.first.is_a?(NetAddr::IPv4)
57-
NetAddr.sort_IPv4(ips)
58-
else
59-
NetAddr.sort_IPv6(ips)
60-
end
58+
sorted_ips = ips.sort
6159

6260
return true if ips.first == sorted_ips.first
6361
end
@@ -122,26 +120,36 @@ def self.no_leading_zeros(destination)
122120

123121
private_class_method def self.parse_ipv4(val)
124122
if val.is_a?(Array)
125-
val.map do |ip|
126-
NetAddr::IPv4.parse(ip)
127-
end
123+
val.map { |ip| parse_address(ip, :ipv4?) }
128124
else
129-
NetAddr::IPv4Net.parse(val)
125+
parse_cidr(val, :ipv4?)
130126
end
131-
rescue NetAddr::ValidationError
127+
rescue IPAddr::Error
132128
nil
133129
end
134130

135131
private_class_method def self.parse_ipv6(val)
136132
if val.is_a?(Array)
137-
val.map do |ip|
138-
NetAddr::IPv6.parse(ip)
139-
end
133+
val.map { |ip| parse_address(ip, :ipv6?) }
140134
else
141-
NetAddr::IPv6Net.parse(val)
135+
parse_cidr(val, :ipv6?)
142136
end
143-
rescue NetAddr::ValidationError
137+
rescue IPAddr::Error
144138
nil
145139
end
140+
141+
# A range endpoint must be a plain address, so a prefix is not allowed here.
142+
private_class_method def self.parse_address(val, family)
143+
raise IPAddr::InvalidAddressError if val.include?('/')
144+
145+
parse_cidr(val, family)
146+
end
147+
148+
private_class_method def self.parse_cidr(val, family)
149+
ip = IPAddr.new(val)
150+
raise IPAddr::InvalidAddressError unless ip.public_send(family)
151+
152+
ip
153+
end
146154
end
147155
end
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
require 'spec_helper'
2+
3+
module CloudController
4+
RSpec.describe RuleValidator do
5+
describe '.validate_destination' do
6+
subject { described_class.validate_destination(destination) }
7+
8+
before do
9+
TestConfig.override(
10+
enable_ipv6: true,
11+
security_groups: { enable_comma_delimited_destinations: false }
12+
)
13+
end
14+
15+
context 'with a single valid IPv4 address' do
16+
let(:destination) { '192.168.10.2' }
17+
18+
it { is_expected.to be true }
19+
end
20+
21+
context 'with a valid CIDR' do
22+
let(:destination) { '10.0.0.0/8' }
23+
24+
it { is_expected.to be true }
25+
end
26+
27+
context 'with a valid ascending range' do
28+
let(:destination) { '192.168.10.2-192.168.15.254' }
29+
30+
it { is_expected.to be true }
31+
end
32+
33+
context 'with an inverted range' do
34+
let(:destination) { '200.0.0.0-150.0.0.0' }
35+
36+
it { is_expected.to be false }
37+
end
38+
39+
context 'with a range whose second endpoint is a CIDR' do
40+
let(:destination) { '1.1.1.1-2.2.2.2/30' }
41+
42+
it { is_expected.to be false }
43+
end
44+
45+
context 'with a malformed address' do
46+
let(:destination) { '999.999.999.999' }
47+
48+
it { is_expected.to be false }
49+
end
50+
51+
# Zero-padded IPv4 octets are ambiguous (decimal vs. octal) and are rejected downstream by Diego and vxlan-policy-agent.
52+
context 'with leading zeros in an IPv4 address' do
53+
context 'in the first octet' do
54+
let(:destination) { '010.0.0.53' }
55+
56+
it { is_expected.to be false }
57+
end
58+
59+
context 'in every octet' do
60+
let(:destination) { '03.005.010.02' }
61+
62+
it { is_expected.to be false }
63+
end
64+
65+
context 'in a CIDR' do
66+
let(:destination) { '010.000.000.000/24' }
67+
68+
it { is_expected.to be false }
69+
end
70+
71+
context 'in a range endpoint' do
72+
let(:destination) { '010.0.0.1-10.0.0.9' }
73+
74+
it { is_expected.to be false }
75+
end
76+
end
77+
78+
context 'with a valid IPv6 address' do
79+
let(:destination) { '2001:db8::1' }
80+
81+
it { is_expected.to be true }
82+
end
83+
84+
# Zero-padded IPv6 hextets are valid (RFC 4291); exhaustive coverage lives in the v3 spec (#4367).
85+
context 'with a zero-padded IPv6 segment' do
86+
let(:destination) { '2001:0db8::1' }
87+
88+
it { is_expected.to be true }
89+
end
90+
91+
# A zero-padded embedded octet carries the same decimal/octal ambiguity as a bare IPv4 address.
92+
context 'with an IPv4-mapped IPv6 address' do
93+
context 'with a plain embedded octet' do
94+
let(:destination) { '::ffff:10.0.0.1' }
95+
96+
it { is_expected.to be true }
97+
end
98+
99+
context 'with a zero-padded embedded octet' do
100+
let(:destination) { '::ffff:010.0.0.1' }
101+
102+
it { is_expected.to be false }
103+
end
104+
end
105+
106+
context 'with a valid IPv6 range' do
107+
let(:destination) { '2001:db8::1-2001:db8::ff' }
108+
109+
it { is_expected.to be true }
110+
end
111+
112+
context 'with an inverted IPv6 range' do
113+
let(:destination) { '2001:db8::ff-2001:db8::1' }
114+
115+
it { is_expected.to be false }
116+
end
117+
end
118+
end
119+
end

spec/unit/messages/validators/security_group_rule_validator_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1487,7 +1487,7 @@ def self.name
14871487
[
14881488
{
14891489
protocol: 'icmpv6',
1490-
destination: '1.0.0.000-1.0.0.200',
1490+
destination: '1.0.0.1-1.0.0.200',
14911491
type: -1,
14921492
code: 255
14931493
}

spec/unit/models/runtime/security_group_spec.rb

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ def build_all_rule(attrs={})
115115
end
116116

117117
context 'when it is a valid range' do
118-
let(:rule) { build_transport_rule('protocol' => protocol, 'destination' => '1.1.1.1.-2.2.2.2') }
118+
let(:rule) { build_transport_rule('protocol' => protocol, 'destination' => '1.1.1.1-2.2.2.2') }
119119

120120
it 'is valid' do
121121
expect(subject).to be_valid
@@ -281,7 +281,7 @@ def build_all_rule(attrs={})
281281
end
282282

283283
context 'when it is a valid range' do
284-
let(:rule) { build_transport_rule('protocol' => protocol, 'destination' => '1.1.1.1.-2.2.2.2') }
284+
let(:rule) { build_transport_rule('protocol' => protocol, 'destination' => '1.1.1.1-2.2.2.2') }
285285

286286
it 'is valid' do
287287
expect(subject).to be_valid
@@ -758,7 +758,7 @@ def build_all_rule(attrs={})
758758
end
759759

760760
context 'when it is a valid range' do
761-
let(:rule) { build_all_rule('destination' => '1.1.1.1.-2.2.2.2') }
761+
let(:rule) { build_all_rule('destination' => '1.1.1.1-2.2.2.2') }
762762

763763
it 'is valid' do
764764
expect(subject).to be_valid

0 commit comments

Comments
 (0)