diff --git a/app/messages/validators/security_group_rule_validator.rb b/app/messages/validators/security_group_rule_validator.rb index ecaaebfe4ac..127aa7dbda5 100644 --- a/app/messages/validators/security_group_rule_validator.rb +++ b/app/messages/validators/security_group_rule_validator.rb @@ -26,29 +26,28 @@ def validate(record) end validate_allowed_keys(rule, record, index) - - add_rule_error("protocol must be 'tcp', 'udp', 'icmp', or 'all'", record, index) unless valid_protocol(rule[:protocol]) + add_rule_error("protocol must be 'tcp', 'udp', 'icmp', 'icmpv6' or 'all'", record, index) unless valid_protocol(rule[:protocol]) if valid_destination_type(rule[:destination], record, index) destinations = rule[:destination].split(',', -1) add_rule_error("maximum destinations per rule exceeded - must be under #{MAX_DESTINATIONS_PER_RULE}", record, index) unless destinations.length <= MAX_DESTINATIONS_PER_RULE destinations.each do |d| - validate_destination(d, record, index) + validate_destination(d, rule[:protocol], get_allowed_ip_version(rule), record, index) end end validate_description(rule, record, index) validate_log(rule, record, index) + validate_protocol(rule, record, index) + end + end - case rule[:protocol] - when 'tcp', 'udp' - validate_tcp_udp_protocol(rule, record, index) - when 'icmp' - validate_icmp_protocol(rule, record, index) - when 'all' - add_rule_error('ports are not allowed for protocols of type all', record, index) if rule[:ports] - end + def get_allowed_ip_version(rule) + if rule[:protocol] == 'icmp' + 4 + elsif rule[:protocol] == 'icmpv6' + 6 end end @@ -57,7 +56,7 @@ def boolean?(value) end def valid_protocol(protocol) - protocol.is_a?(String) && %w[tcp udp icmp all].include?(protocol) + protocol.is_a?(String) && %w[tcp udp icmp icmpv6 all].include?(protocol) end def validate_allowed_keys(rule, record, index) @@ -73,6 +72,20 @@ def validate_log(rule, record, index) add_rule_error('log must be a boolean', record, index) if rule[:log] && !boolean?(rule[:log]) end + def validate_protocol(rule, record, index) + case rule[:protocol] + when 'tcp', 'udp' + validate_tcp_udp_protocol(rule, record, index) + when 'icmp' + validate_icmp_protocol(rule, record, index) + when 'icmpv6' + add_rule_error('icmpv6 cannot be used if enable_ipv6 is false', record, index) unless CloudController::RuleValidator.ipv6_enabled? + validate_icmp_protocol(rule, record, index) + when 'all' + add_rule_error('ports are not allowed for protocols of type all', record, index) if rule[:ports] + end + end + def validate_tcp_udp_protocol(rule, record, index) add_rule_error('ports are required for protocols of type TCP and UDP', record, index) unless rule[:ports] @@ -128,7 +141,7 @@ def valid_destination_type(destination, record, index) true end - def validate_destination(destination, record, index) + def validate_destination(destination, protocol, allowed_ip_version, record, index) error_message = 'destination must be a valid CIDR, IP address, or IP address range' error_message = 'destination must contain valid CIDR(s), IP address(es), or IP address range(s)' if CloudController::RuleValidator.comma_delimited_destinations_enabled? add_rule_error('empty destination specified in comma-delimited list', record, index) if destination.empty? @@ -137,12 +150,14 @@ def validate_destination(destination, record, index) 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) - if address_list.length == 1 - add_rule_error(error_message, record, index) unless CloudController::RuleValidator.parse_ip(address_list.first) - + 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) \ + 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) @@ -153,12 +168,17 @@ def validate_destination(destination, record, index) 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) \ + unless valid_ip_version?(allowed_ip_version, sorted_ips.first) else add_rule_error(error_message, record, index) end end + def valid_ip_version?(allowed_ip_version, parsed_ip) + parsed_ip.nil? || allowed_ip_version.nil? || parsed_ip.version == allowed_ip_version + end + def add_rule_error(message, record, index) record.errors.add("Rules[#{index}]:", message) end diff --git a/app/models/runtime/security_group.rb b/app/models/runtime/security_group.rb index b9aec77934c..e473f9d8eaa 100644 --- a/app/models/runtime/security_group.rb +++ b/app/models/runtime/security_group.rb @@ -71,7 +71,7 @@ def validate_rules validation_errors = case protocol when 'tcp', 'udp' CloudController::TransportRuleValidator.validate(stringified_rule) - when 'icmp' + when 'icmp', 'icmpv6' CloudController::ICMPRuleValidator.validate(stringified_rule) when 'all' CloudController::RuleValidator.validate(stringified_rule) diff --git a/docs/v3/source/includes/api_resources/_security_groups.erb b/docs/v3/source/includes/api_resources/_security_groups.erb index 73cf1919626..9c649172948 100644 --- a/docs/v3/source/includes/api_resources/_security_groups.erb +++ b/docs/v3/source/includes/api_resources/_security_groups.erb @@ -21,6 +21,13 @@ "code": 0, "description": "Allow ping requests to private services" }, + { + "protocol": "icmpv6", + "destination": "::/0", + "type": -1, + "code": -1, + "description": "Allow all ICMPv6 traffic" + }, { "protocol": "tcp", "destination": "1.1.1.1,2.2.2.2/24,10.0.0.0-10.0.0.255", diff --git a/docs/v3/source/includes/resources/security_groups/_object.md.erb b/docs/v3/source/includes/resources/security_groups/_object.md.erb index 9f13f64d93a..38026e936cd 100644 --- a/docs/v3/source/includes/resources/security_groups/_object.md.erb +++ b/docs/v3/source/includes/resources/security_groups/_object.md.erb @@ -25,8 +25,8 @@ Name | Type | Description | Name | Type | Description | Required | Default | ---- | ---- | ----------- | -------- | ------- -| **protocol** | _string_ | Protocol type Valid values are `tcp`, `udp`, `icmp`, or `all` | yes | N/A | -| **destination** | _string_ | The destination where the rule applies. Must be a singular Valid CIDR, IP address, or IP address range unless `cc.security_groups.enable_comma_delimited_destinations` is enabled. Then, the destination can be a comma-delimited string of CIDRs, IP addresses, or IP address ranges. Octets within destinations cannot contain leading zeros; eg. `10.0.0.0/24` is valid, but `010.00.000.0/24` is *not*. | yes | N/A | +| **protocol** | _string_ | Protocol type Valid values are `tcp`, `udp`, `icmp`, `icmpv6` or `all` | yes | N/A | +| **destination** | _string_ | The destination where the rule applies. Must be a singular valid CIDR, IP address, or IP address range unless `cc.security_groups.enable_comma_delimited_destinations` is enabled. Then, the destination can be a comma-delimited string of CIDRs, IP addresses, or IP address ranges. Octets within destinations cannot contain leading zeros; eg. `10.0.0.0/24` is valid, but `010.00.000.0/24` is *not*. For `icmp`, only IPv4 addresses are allowed and for `icmpv6` only IPv6 addresses. | yes | N/A | | **ports** | _string_ | Ports that the rule applies to; can be a single port (`9000`), a comma-separated list (`9000,9001`), or a range (`9000-9200`) | no | `null` | | **type** | _integer_ |[Type](https://www.iana.org/assignments/icmp-parameters/icmp-parameters.xhtml#icmp-parameters-types) required for ICMP protocol; valid values are between -1 and 255 (inclusive), where -1 allows all | no | `null` | | **code** | _integer_ |[Code](https://www.iana.org/assignments/icmp-parameters/icmp-parameters.xhtml#icmp-parameters-codes) required for ICMP protocol; valid values are between -1 and 255 (inclusive), where -1 allows all | no | `null` | diff --git a/lib/cloud_controller/rule_validator.rb b/lib/cloud_controller/rule_validator.rb index 92ea4fa3f71..817b5922cb2 100644 --- a/lib/cloud_controller/rule_validator.rb +++ b/lib/cloud_controller/rule_validator.rb @@ -77,6 +77,10 @@ def self.parse_ip(val) ipv4 || ipv6 end + def self.ipv6_enabled? + config.get(:enable_ipv6) + end + def self.comma_delimited_destinations_enabled? config.get(:security_groups, :enable_comma_delimited_destinations) 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 66e6cd4f134..40f092d5495 100644 --- a/spec/unit/messages/validators/security_group_rule_validator_spec.rb +++ b/spec/unit/messages/validators/security_group_rule_validator_spec.rb @@ -48,9 +48,9 @@ def self.name it 'returns indexed errors corresponding to each invalid rule' do expect(subject).not_to be_valid - expect(subject.errors.full_messages).to include "Rules[0]: protocol must be 'tcp', 'udp', 'icmp', or 'all'" + expect(subject.errors.full_messages).to include "Rules[0]: protocol must be 'tcp', 'udp', 'icmp', 'icmpv6' or 'all'" expect(subject.errors.full_messages).to include 'Rules[0]: destination must be a valid CIDR, IP address, or IP address range' - expect(subject.errors.full_messages).to include "Rules[1]: protocol must be 'tcp', 'udp', 'icmp', or 'all'" + expect(subject.errors.full_messages).to include "Rules[1]: protocol must be 'tcp', 'udp', 'icmp', 'icmpv6' or 'all'" expect(subject.errors.full_messages).to include 'Rules[1]: destination must be a valid CIDR, IP address, or IP address range' end end @@ -1064,7 +1064,7 @@ def self.name it 'adds an error' do expect(subject).not_to be_valid - expect(subject.errors.full_messages).to include "Rules[0]: protocol must be 'tcp', 'udp', 'icmp', or 'all'" + expect(subject.errors.full_messages).to include "Rules[0]: protocol must be 'tcp', 'udp', 'icmp', 'icmpv6' or 'all'" end end @@ -1073,7 +1073,7 @@ def self.name it 'is not valid' do expect(subject).not_to be_valid - expect(subject.errors.full_messages).to include "Rules[0]: protocol must be 'tcp', 'udp', 'icmp', or 'all'" + expect(subject.errors.full_messages).to include "Rules[0]: protocol must be 'tcp', 'udp', 'icmp', 'icmpv6' or 'all'" end end @@ -1082,7 +1082,7 @@ def self.name it 'adds an error' do expect(subject).not_to be_valid - expect(subject.errors.full_messages).to include "Rules[0]: protocol must be 'tcp', 'udp', 'icmp', or 'all'" + expect(subject.errors.full_messages).to include "Rules[0]: protocol must be 'tcp', 'udp', 'icmp', 'icmpv6' or 'all'" end end @@ -1251,6 +1251,23 @@ def self.name end end + context 'icmp protocol contains an IPv4 destination range' do + let(:rules) do + [ + { + protocol: 'icmp', + destination: '1.0.0.1-1.0.0.200', + type: -1, + code: 255 + } + ] + end + + it 'is valid' do + expect(subject).to be_valid + end + end + context 'the icmp rules are not provided when the protocol is icmp' do let(:rules) do [ @@ -1303,6 +1320,224 @@ def self.name expect(subject.errors.full_messages).to include 'Rules[0]: code must be an integer between -1 and 255 (inclusive)' end end + + context 'ipv6 is disabled' do + before do + TestConfig.config[:enable_ipv6] = false + end + + context 'icmpv6 protocol in a rule' do + let(:rules) do + [ + { + protocol: 'icmpv6', + destination: '2001:db8::/32', + type: -1, + code: 255 + } + ] + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages).to include 'Rules[0]: icmpv6 cannot be used if enable_ipv6 is false' + end + end + end + + context 'ipv6 is enabled' do + before do + TestConfig.config[:enable_ipv6] = true + end + + context 'icmp protocol contains an IPv6 destination' do + let(:rules) do + [ + { + protocol: 'icmp', + destination: '2001:db8::/32', + type: -1, + code: 255 + } + ] + end + + it 'is invalid' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages).to include 'Rules[0]: for protocol "icmp" you cannot use IPv6 addresses' + end + end + + context 'icmp protocol contains an IPv6 destination range' do + let(:rules) do + [ + { + protocol: 'icmp', + destination: '2001:0db8::1-2001:0db8::ff', + type: -1, + code: 255 + } + ] + end + + it 'is invalid' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages).to include 'Rules[0]: for protocol "icmp" you cannot use IPv6 addresses' + end + end + + context 'icmp protocol contains a comma-delimited list of IPv6 destinations' do + before do + TestConfig.config[:security_groups][:enable_comma_delimited_destinations] = true + end + + let(:rules) do + [ + { + protocol: 'icmp', + destination: '2001:db8::/32,2001:db8:85a3::/64', + type: -1, + code: 255 + } + ] + end + + it 'is invalid' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages).to include 'Rules[0]: for protocol "icmp" you cannot use IPv6 addresses' + end + end + + context 'icmpv6 protocol contains an IPv6 destination' do + let(:rules) do + [ + { + protocol: 'icmpv6', + destination: '2001:db8::/32', + type: -1, + code: 255 + } + ] + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'icmpv6 protocol contains an IPv6 destination range' do + let(:rules) do + [ + { + protocol: 'icmpv6', + destination: '2001:0db8::1-2001:0db8::ff', + type: -1, + code: 255 + } + ] + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'icmpv6 protocol contains a comma-delimited list of IPv6 destinations' do + before do + TestConfig.config[:security_groups][:enable_comma_delimited_destinations] = true + end + + let(:rules) do + [ + { + protocol: 'icmpv6', + destination: '2001:db8::/32,2001:db8:85a3::/64', + type: -1, + code: 255 + } + ] + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'icmpv6 protocol contains an IPv4 destination' do + let(:rules) do + [ + { + protocol: 'icmpv6', + destination: '10.0.0.0/8', + type: -1, + code: 255 + } + ] + end + + it 'is invalid' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages).to include 'Rules[0]: for protocol "icmpv6" you cannot use IPv4 addresses' + end + end + + context 'icmpv6 protocol contains an IPv4 destination range' do + let(:rules) do + [ + { + protocol: 'icmpv6', + destination: '1.0.0.000-1.0.0.200', + type: -1, + code: 255 + } + ] + end + + it 'is invalid' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages).to include 'Rules[0]: for protocol "icmpv6" you cannot use IPv4 addresses' + end + end + + context 'icmpv6 protocol contains a comma-delimited list of IPv4/IPv6 destinations' do + before do + TestConfig.config[:security_groups][:enable_comma_delimited_destinations] = true + end + + let(:rules) do + [ + { + protocol: 'icmpv6', + destination: '10.0.0.0/8,2001:db8::/32', + type: -1, + code: 255 + } + ] + end + + it 'is invalid' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages).to include 'Rules[0]: for protocol "icmpv6" you cannot use IPv4 addresses' + end + end + + context 'the icmp rules are not provided when the protocol is icmpv6' do + let(:rules) do + [ + { + protocol: 'icmpv6', + destination: '2001:db8::/32' + } + ] + end + + it 'is invalid' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages).to include 'Rules[0]: type is required for protocols of type ICMP' + expect(subject.errors.full_messages).to include 'Rules[0]: code is required for protocols of type ICMP' + end + end + end end end end diff --git a/spec/unit/models/runtime/security_group_spec.rb b/spec/unit/models/runtime/security_group_spec.rb index 5899a56c0e0..d4089e1c3f6 100644 --- a/spec/unit/models/runtime/security_group_spec.rb +++ b/spec/unit/models/runtime/security_group_spec.rb @@ -19,6 +19,15 @@ def build_icmp_rule(attrs={}) }.merge(attrs) end + def build_icmpv6_rule(attrs={}) + { + 'protocol' => 'icmpv6', + 'type' => 0, + 'code' => 0, + 'destination' => '::/0' + }.merge(attrs) + end + def build_all_rule(attrs={}) { 'protocol' => 'all', @@ -601,6 +610,18 @@ def build_all_rule(attrs={}) end end + context 'when it is a valid IPv6 CIDR' do + before do + TestConfig.config[:enable_ipv6] = true + end + + let(:rule) { build_icmpv6_rule('destination' => '2001:db8::/32') } + + it 'is valid' do + expect(subject).to be_valid + end + end + context 'when it is a valid range' do let(:rule) { build_icmp_rule('destination' => '1.1.1.1-2.2.2.2') } @@ -608,6 +629,18 @@ def build_all_rule(attrs={}) expect(subject).to be_valid end end + + context 'when it is a valid IPv6 range' do + before do + TestConfig.config[:enable_ipv6] = true + end + + let(:rule) { build_icmpv6_rule('destination' => '2001:0db8::1-2001:0db8::ff') } + + it 'is valid' do + expect(subject).to be_valid + end + end end context 'bad' do