Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adds icmp type and code to Cisco object-group output #300

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adds icmp type and code to Cisco object-group output #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions capirca/lib/cisco.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,15 @@ def __str__(self):
else self.PROTO_MAP.get(proto)
for proto in self.term.protocol]

# icmp lookups
icmp_types = ['']
if self.term.icmp_type:
icmp_types = self.NormalizeIcmpTypes(self.term.icmp_type,
self.term.protocol, self.af)
icmp_codes = ['']
if self.term.icmp_code:
icmp_codes = self.term.icmp_code

# addresses
source_address = self.term.source_address
if not self.term.source_address:
Expand All @@ -859,14 +868,24 @@ def __str__(self):
for sport in source_port:
for dport in destination_port:
for proto in protocol:
ret_str.append(
self._TermletToStr(_ACTION_TABLE.get(str(
self.term.action[0])), proto, saddr, sport, daddr, dport))
# cisconx uses icmp for both ipv4 and ipv6
if self.platform == 'cisconx':
if self.af == 6:
proto = 'icmp' if proto == 'icmpv6' else proto
for icmp_type in icmp_types:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than looping here, making an explicit decision that this is for "icmp" handling only would make this correct, and readable.

In the case of icmp/icmpv6, this would handle it with the way you have, with self._TermletToStr(_ACTION_TABLE.get(str(
self.term.action[0])), proto, saddr, sport, daddr, dport, icmp_type, icmp_code)

Else, it would handle it the way it originally was.
self._TermletToStr(_ACTION_TABLE.get(str(
self.term.action[0])), proto, saddr, sport, daddr, dport))

for icmp_code in icmp_codes:
ret_str.append(
self._TermletToStr(_ACTION_TABLE.get(str(
self.term.action[0])), proto, saddr, sport, daddr, dport, icmp_type, icmp_code))

return '\n'.join(ret_str)

def _TermletToStr(self, action, proto, saddr, sport, daddr, dport):
def _TermletToStr(self, action, proto, saddr, sport, daddr, dport, icmp_type, icmp_code):
"""Output a portion of a cisco term/filter only, based on the 5-tuple."""
# str(icmp_type) is needed to ensure 0 maps to '0' instead of FALSE
icmp_type = str(icmp_type)
icmp_code = str(icmp_code)

# Empty addr/port destinations should emit 'any'
if saddr and saddr != 'any':
saddr = 'net-group %s' % saddr
Expand All @@ -882,6 +901,11 @@ def _TermletToStr(self, action, proto, saddr, sport, daddr, dport):
else:
dport = ''

if icmp_type:
if icmp_code:
return (' %s %s %s %s %s %s' % (action, proto, saddr, daddr, icmp_type, icmp_code)).rstrip()
return (' %s %s %s %s %s' % (action, proto, saddr, daddr, icmp_type)).rstrip()

return (' %s %s %s%s %s%s' % (
action, proto, saddr, sport, daddr, dport)).rstrip()

Expand Down
32 changes: 32 additions & 0 deletions tests/lib/cisco_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,23 @@
action:: accept
}
"""
GOOD_TERM_24 = """
term good_term_24 {
protocol:: icmp
source-address:: SOME_HOST
icmp-type:: echo-reply
action:: accept
}
"""
GOOD_TERM_25 = """
term good_term_25 {
protocol:: icmp
source-address:: SOME_HOST
icmp-type:: unreachable
icmp-code:: 13
action:: accept
}
"""
LONG_COMMENT_TERM = """
term long-comment-term {
comment:: "%s "
Expand Down Expand Up @@ -656,6 +673,21 @@ def testObjectGroup(self):
self.naming.GetNetAddr.assert_has_calls([mock.call('SOME_HOST'),
mock.call('SOME_HOST')])
self.naming.GetServiceByProto.assert_called_once_with('HTTP', 'tcp')

def testObjectGroupIcmp(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also please add a testcase for icmpv6 here, thanks!

ip_grp = ['object-group network ipv4 SOME_HOST']
ip_grp.append(' 10.0.0.0/8')
ip_grp.append('exit')

self.naming.GetNetAddr.return_value = [
nacaddr.IP('10.0.0.0/8', token='SOME_HOST')]

pol = policy.ParsePolicy(
GOOD_OBJGRP_HEADER + GOOD_TERM_24 + GOOD_TERM_25, self.naming)
acl = cisco.Cisco(pol, EXP_INFO)

self.assertIn(' permit icmp net-group SOME_HOST any 0', str(acl))
self.assertIn(' permit icmp net-group SOME_HOST any 3 13', str(acl))

def testInet6(self):
self.naming.GetNetAddr.return_value = [nacaddr.IP('10.0.0.0/8'),
Expand Down