ICMP type & code validation while adding Security Group rules

While adding new 'Security Group Rule' viz. 'Custom ICMP rule' when
wrong ICMP 'type' or 'code' was given the errors it would show were
'Not a valid port number'; which is misleading. The errors should
rather be 'Not a valid ICMP type' or 'Not a valid ICMP code'.

Also for validating ICMP 'type' and 'code' there wasn't any dedicated
functionality in 'oslo.utils/netutils' so the code for validating 'TCP
ports' was used. TCP ports are in range 0 to 65535 while ICMP 'type'
and 'code' are in range 0 to 255, so using 'TCP port validation' code
is incorrect.

When -1 is used in 'ICMP code' or 'ICMP type' that means any number
0 to 255 is valid.

Here newer dedicated functionality of 'oslo.utils/netutils' is used
for validating ICMP 'type' and ICMP 'code'.

Change-Id: I8e227a0021d418294fa7b7756d58e39f2100850a
Closes-Bug: #1511748
(cherry picked from commit edfde8b3f5)
This commit is contained in:
Suraj Deshmukh 2015-11-25 14:53:17 +05:30 committed by Yves-Gwenael Bourhis
parent 0461621253
commit 0b4b2976af
4 changed files with 64 additions and 13 deletions

View File

@ -206,6 +206,30 @@ class ValidatorsTests(test.TestCase):
validators.validate_port_range,
port)
def test_icmp_type_validator(self):
VALID_ICMP_TYPES = (1, 0, 255, -1)
INVALID_ICMP_TYPES = (256, None, -2)
for icmp_type in VALID_ICMP_TYPES:
self.assertIsNone(validators.validate_icmp_type_range(icmp_type))
for icmp_type in INVALID_ICMP_TYPES:
self.assertRaises(ValidationError,
validators.validate_icmp_type_range,
icmp_type)
def test_icmp_code_validator(self):
VALID_ICMP_CODES = (1, 0, 255, None, -1,)
INVALID_ICMP_CODES = (256, -2)
for icmp_code in VALID_ICMP_CODES:
self.assertIsNone(validators.validate_icmp_code_range(icmp_code))
for icmp_code in INVALID_ICMP_CODES:
self.assertRaises(ValidationError,
validators.validate_icmp_code_range,
icmp_code)
def test_ip_proto_validator(self):
VALID_PROTO = (0, 255)
INVALID_PROTO = (-1, 256)

View File

@ -28,6 +28,20 @@ def validate_port_range(port):
raise ValidationError(_("Not a valid port number"))
def validate_icmp_type_range(icmp_type):
if not netutils.is_valid_icmp_type(icmp_type):
if icmp_type == -1:
return
raise ValidationError(_("Not a valid ICMP type"))
def validate_icmp_code_range(icmp_code):
if not netutils.is_valid_icmp_code(icmp_code):
if icmp_code == -1:
return
raise ValidationError(_("Not a valid ICMP code"))
def validate_ip_protocol(ip_proto):
if ip_proto not in range(0, 256):
raise ValidationError(_("Not a valid IP protocol number"))

View File

@ -186,7 +186,8 @@ class AddRule(forms.SelfHandlingForm):
'data-switch-on': 'rule_menu',
'data-rule_menu-icmp': _('Type')}),
validators=[
utils_validators.validate_port_range])
utils_validators.
validate_icmp_type_range])
icmp_code = forms.IntegerField(label=_("Code"),
required=False,
@ -197,7 +198,8 @@ class AddRule(forms.SelfHandlingForm):
'data-switch-on': 'rule_menu',
'data-rule_menu-icmp': _('Code')}),
validators=[
utils_validators.validate_port_range])
utils_validators.
validate_icmp_code_range])
remote = forms.ChoiceField(label=_('Remote'),
choices=[('cidr', _('CIDR')),
@ -290,18 +292,16 @@ class AddRule(forms.SelfHandlingForm):
icmp_code = cleaned_data.get("icmp_code", None)
self._update_and_pop_error(cleaned_data, 'ip_protocol', rule_menu)
if icmp_type is None:
msg = _('The ICMP type is invalid.')
if icmp_type == -1 and icmp_code != -1:
msg = _('ICMP code is provided but ICMP type is missing.')
raise ValidationError(msg)
if icmp_code is None:
msg = _('The ICMP code is invalid.')
raise ValidationError(msg)
if icmp_type not in range(-1, 256):
if self.errors.get('icmp_type'):
msg = _('The ICMP type not in range (-1, 255)')
raise ValidationError(msg)
if icmp_code not in range(-1, 256):
if self.errors.get('icmp_code'):
msg = _('The ICMP code not in range (-1, 255)')
raise ValidationError(msg)
self._update_and_pop_error(cleaned_data, 'from_port', icmp_type)
self._update_and_pop_error(cleaned_data, 'to_port', icmp_code)
self._update_and_pop_error(cleaned_data, 'port', None)

View File

@ -483,8 +483,8 @@ class SecurityGroupsViewTests(test.TestCase):
sec_group_list = self.security_groups.list()
icmp_rule = self.security_group_rules.list()[1]
# Call POST 4 times
for i in range(4):
# Call POST 5 times
for i in range(5):
api.network.security_group_backend(
IsA(http.HttpRequest)).AndReturn(self.secgroup_backend)
api.network.security_group_list(
@ -526,7 +526,7 @@ class SecurityGroupsViewTests(test.TestCase):
'remote': 'cidr'}
res = self.client.post(self.edit_url, formData)
self.assertNoMessages()
self.assertContains(res, "The ICMP code is invalid")
self.assertContains(res, "The ICMP code not in range (-1, 255)")
formData = {'method': 'AddRule',
'id': sec_group.id,
@ -538,7 +538,20 @@ class SecurityGroupsViewTests(test.TestCase):
'remote': 'cidr'}
res = self.client.post(self.edit_url, formData)
self.assertNoMessages()
self.assertContains(res, "The ICMP type is invalid")
self.assertContains(res, "The ICMP type not in range (-1, 255)")
formData = {'method': 'AddRule',
'id': sec_group.id,
'port_or_range': 'port',
'icmp_type': -1,
'icmp_code': icmp_rule.to_port,
'rule_menu': icmp_rule.ip_protocol,
'cidr': icmp_rule.ip_range['cidr'],
'remote': 'cidr'}
res = self.client.post(self.edit_url, formData)
self.assertNoMessages()
self.assertContains(
res, "ICMP code is provided but ICMP type is missing.")
@test.create_stubs({api.network: ('security_group_rule_create',
'security_group_list',