Skip to content

Commit

Permalink
When we process the same name in multiple filters in the same policy,…
Browse files Browse the repository at this point in the history 
… the

current code fails as it sets the shortened name in the first filter
processing, but sees the same name in subsequent filters as conflicting with
the previous declaration.

In the first term processing, we define values in self.address_book as deduped
and shortened values to the length of _TABLE_NAME_MAX_LENGTH. On further term
processing, we repeat the same process, but this time there is a preceding,
shortened value set in self.address_book that causes the code to raise
DuplicateShortenedTableName.

Instead, we will keep a record of short names and their long names equivalent,
and we will only fail if there is a conflict in which a short name refers to
two different long name objects.

We also remove the need to have an extra deduplication logic for
self.address_book since self.address_book will never contain duplicate names in
a regular run when processing a term, and on further term processing, it will
just either add or define to itself, so duplicates should not happen naturally.

In addition, add some new test cases to cover edge cases and to prove that this
codechange fixes this bug (test case testTableSameLongNameDiffFilter).
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=126325475
  • Loading branch information
cmas authored and Carlos Mas committed Jun 30, 2016
1 parent 942e6ea commit 213626a
Show file tree
Hide file tree
Showing 2 changed files with 182 additions and 34 deletions.
62 changes: 37 additions & 25 deletions lib/packetfilter.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,12 +374,7 @@ def _SetDefaultAction(self):
class PacketFilter(aclgenerator.ACLGenerator):
"""Generates filters and terms from provided policy object."""

# Table names can be at most 31 characters long.
# Names longer than this length will be truncated.
# A simple logic is implemented in order to look for
# possible duplicates after truncation.
_TABLE_NAME_MAX_LENGTH = 31

_DEF_MAX_LENGTH = 31
_PLATFORM = 'packetfilter'
_DEFAULT_PROTOCOL = 'all'
SUFFIX = '.pf'
Expand All @@ -394,6 +389,7 @@ class PacketFilter(aclgenerator.ACLGenerator):
def _TranslatePolicy(self, pol, exp_info):
self.pf_policies = []
self.address_book = {}
self.def_short_to_long = {}
current_date = datetime.datetime.utcnow().date()
exp_info_date = current_date + datetime.timedelta(weeks=exp_info)

Expand Down Expand Up @@ -449,16 +445,44 @@ def _TranslatePolicy(self, pol, exp_info):
raise DuplicateTermError(
'You have a duplicate term: %s' % term.name)
term_names.add(term.name)

for source_addr in term.source_address:
if source_addr.parent_token not in self.address_book:
self.address_book[source_addr.parent_token] = set([source_addr])
src_token = source_addr.parent_token[:self._DEF_MAX_LENGTH]

if (src_token in self.def_short_to_long and
self.def_short_to_long[src_token] != source_addr.parent_token):
raise DuplicateShortenedTableName(
'There is a shortened name conflict between names %s and %s '
'(different named objects would conflict when shortened to %s)'
% (self.def_short_to_long[src_token],
source_addr.parent_token,
src_token))
else:
self.def_short_to_long[src_token] = source_addr.parent_token

if src_token not in self.address_book:
self.address_book[src_token] = set([source_addr])
else:
self.address_book[source_addr.parent_token].add(source_addr)
self.address_book[src_token].add(source_addr)

for dest_addr in term.destination_address:
if dest_addr.parent_token not in self.address_book:
self.address_book[dest_addr.parent_token] = set([dest_addr])
dst_token = dest_addr.parent_token[:self._DEF_MAX_LENGTH]

if (dst_token in self.def_short_to_long and
self.def_short_to_long[dst_token] != dest_addr.parent_token):
raise DuplicateShortenedTableName(
'There is a shortened name conflict between names %s and %s '
'(different named objects would conflict when shortened to %s)'
%(self.def_short_to_long[dst_token],
dest_addr.parent_token,
dst_token))
else:
self.def_short_to_long[dst_token] = dest_addr.parent_token

if dst_token not in self.address_book:
self.address_book[dst_token] = set([dest_addr])
else:
self.address_book[dest_addr.parent_token].add(dest_addr)
self.address_book[dst_token].add(dest_addr)

if not term:
continue
Expand All @@ -474,19 +498,7 @@ def _TranslatePolicy(self, pol, exp_info):

new_terms.append(self._TERM(term, filter_name, all_protocols_stateful,
filter_type, direction))
shortened_deduped_list = {}

for key in self.address_book:
if len(key) > self._TABLE_NAME_MAX_LENGTH:
name = key[:self._TABLE_NAME_MAX_LENGTH]
else:
name = key
if name in shortened_deduped_list.keys():
raise DuplicateShortenedTableName(
'The shortened name %s has a collision.', name)
else:
shortened_deduped_list[name] = self.address_book[key]
self.address_book = shortened_deduped_list

self.pf_policies.append((header, filter_name, filter_type, new_terms))

def __str__(self):
Expand Down
154 changes: 145 additions & 9 deletions tests/lib/packetfilter_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,16 +243,34 @@
}
"""

LONG_NAME_TERM = """
term multiple-name {
LONG_NAME_TERM_DNS_TCP = """
term multiple-name-dns-tcp {
protocol:: tcp
destination-address:: PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME
destination-port:: SMTP
destination-port:: DNS
action:: accept
}
"""

LONG_NAME_TERM_DNS_UDP = """
term multiple-name-dns-udp {
protocol:: udp
destination-address:: PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME
destination-port:: DNS
action:: accept
}
"""

DUPLICATE_LONG_NAME_TERM = """
NON_SHORTENED_LONG_NAME_TERM_DNS_UDP = """
term multiple-name-dns-udp {
protocol:: udp
destination-address:: PROD_NETWORK_EXTREAMLY_LONG_VER
destination-port:: DNS
action:: accept
}
"""

DUPLICATE_DIFFERENT_LONG_NAME_TERM = """
term multiple-name {
protocol:: tcp
destination-address:: PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME
Expand Down Expand Up @@ -656,24 +674,24 @@ def testTableNameShortened(self):
prod_network = nacaddr.IP('10.0.0.0/8')
prod_network.parent_token = 'PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME'
self.naming.GetNetAddr.return_value = [prod_network]
self.naming.GetServiceByProto.return_value = ['25']
self.naming.GetServiceByProto.return_value = ['53']

acl = packetfilter.PacketFilter(policy.ParsePolicy(
GOOD_HEADER_DIRECTIONAL + LONG_NAME_TERM, self.naming), EXP_INFO)
GOOD_HEADER_DIRECTIONAL + LONG_NAME_TERM_DNS_TCP, self.naming), EXP_INFO)
result = str(acl)
self.failUnless(
'table <PROD_NETWORK_EXTREAMLY_LONG_VER> {10.0.0.0/8}' in result,
'did not find shortened name in header.')
self.failUnless(
'pass out quick proto { tcp } from { any } to '
'{ <PROD_NETWORK_EXTREAMLY_LONG_VER> } '
'port { 25 } flags S/SA keep state'
'port { 53 } flags S/SA keep state'
in result,
'did not find actual term for multiple-name')

self.naming.GetNetAddr.assert_called_once_with(
'PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME')
self.naming.GetServiceByProto.assert_called_once_with('SMTP', 'tcp')
self.naming.GetServiceByProto.assert_called_once_with('DNS', 'tcp')

def testTableDuplicateShortNameError(self):
prod_network = nacaddr.IP('10.0.0.0/8')
Expand All @@ -689,13 +707,131 @@ def testTableDuplicateShortNameError(self):
packetfilter.PacketFilter.__init__,
packetfilter.PacketFilter.__new__(packetfilter.PacketFilter),
policy.ParsePolicy(
GOOD_HEADER_DIRECTIONAL + DUPLICATE_LONG_NAME_TERM, self.naming),
GOOD_HEADER_DIRECTIONAL + DUPLICATE_DIFFERENT_LONG_NAME_TERM,
self.naming),
EXP_INFO)
self.naming.GetNetAddr.assert_has_calls([
mock.call('PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME'),
mock.call('PROD_NETWORK_EXTREAMLY_LONG_VERY_GOOD_NAME')])
self.naming.GetServiceByProto.assert_called_once_with('SMTP', 'tcp')

def testTableSameLongNameSameFilter(self):
prod_network = nacaddr.IP('10.0.0.0/8')
prod_network.parent_token = 'PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME'
self.naming.GetNetAddr.return_value = [prod_network]
self.naming.GetServiceByProto.return_value = ['53']

acl = packetfilter.PacketFilter(policy.ParsePolicy(
GOOD_HEADER_DIRECTIONAL + LONG_NAME_TERM_DNS_TCP + LONG_NAME_TERM_DNS_UDP,
self.naming), EXP_INFO)
result = str(acl)
self.failUnless(
'table <PROD_NETWORK_EXTREAMLY_LONG_VER> {10.0.0.0/8}' in result,
'did not find shortened name in header.')
self.failUnless(
'pass out quick proto { tcp } from { any } to '
'{ <PROD_NETWORK_EXTREAMLY_LONG_VER> } '
'port { 53 } flags S/SA keep state'
in result,
'did not find actual TCP term for multiple-name')
self.failUnless(
'pass out quick proto { udp } from { any } to '
'{ <PROD_NETWORK_EXTREAMLY_LONG_VER> } '
'port { 53 } keep state'
in result,
'did not find actual UDP for multiple-name')

self.naming.GetNetAddr.assert_has_calls([
mock.call('PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME'),
mock.call('PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME')])
self.naming.GetServiceByProto.assert_has_calls([
mock.call('DNS', 'tcp'),
mock.call('DNS', 'udp')])

def testTableSameLongNameDiffFilter(self):
prod_network = nacaddr.IP('10.0.0.0/8')
prod_network.parent_token = 'PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME'
self.naming.GetNetAddr.return_value = [prod_network]
self.naming.GetServiceByProto.return_value = ['53']

acl = packetfilter.PacketFilter(policy.ParsePolicy(
GOOD_HEADER_DIRECTIONAL + LONG_NAME_TERM_DNS_TCP +
GOOD_HEADER_DIRECTIONAL + LONG_NAME_TERM_DNS_UDP,
self.naming), EXP_INFO)
result = str(acl)
self.failUnless(
'table <PROD_NETWORK_EXTREAMLY_LONG_VER> {10.0.0.0/8}' in result,
'did not find shortened name in header.')
self.failUnless(
'pass out quick proto { tcp } from { any } to '
'{ <PROD_NETWORK_EXTREAMLY_LONG_VER> } '
'port { 53 } flags S/SA keep state'
in result,
'did not find actual TCP term for multiple-name')
self.failUnless(
'pass out quick proto { udp } from { any } to '
'{ <PROD_NETWORK_EXTREAMLY_LONG_VER> } '
'port { 53 } keep state'
in result,
'did not find actual UDP for multiple-name')

self.naming.GetNetAddr.assert_has_calls([
mock.call('PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME'),
mock.call('PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME')])
self.naming.GetServiceByProto.assert_has_calls([
mock.call('DNS', 'tcp'),
mock.call('DNS', 'udp')])

def testTableDiffObjectsShortenedAndNonShortened(self):
prod_network = nacaddr.IP('10.0.0.0/8')
prod_network.parent_token = 'PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME'
prod_network_two = nacaddr.IP('172.0.0.1/8')
prod_network_two.parent_token = 'PROD_NETWORK_EXTREAMLY_LONG_VER'
self.naming.GetNetAddr.side_effect = [
[prod_network], [prod_network_two]]
self.naming.GetServiceByProto.return_value = ['53']

self.assertRaises(
packetfilter.DuplicateShortenedTableName,
packetfilter.PacketFilter.__init__,
packetfilter.PacketFilter.__new__(packetfilter.PacketFilter),
policy.ParsePolicy(
GOOD_HEADER_DIRECTIONAL + LONG_NAME_TERM_DNS_TCP +
NON_SHORTENED_LONG_NAME_TERM_DNS_UDP,
self.naming),
EXP_INFO)
self.naming.GetNetAddr.assert_has_calls([
mock.call('PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME'),
mock.call('PROD_NETWORK_EXTREAMLY_LONG_VER')])
self.naming.GetServiceByProto.assert_has_calls([
mock.call('DNS', 'tcp'),
mock.call('DNS', 'udp')])

def testTableDuplicateShortNameErrorDiffFilter(self):
prod_network = nacaddr.IP('10.0.0.0/8')
prod_network.parent_token = 'PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME'
prod_network_two = nacaddr.IP('172.0.0.1/8')
prod_network_two.parent_token = 'PROD_NETWORK_EXTREAMLY_LONG_VER'
self.naming.GetNetAddr.side_effect = [
[prod_network], [prod_network_two]]
self.naming.GetServiceByProto.return_value = ['53']

self.assertRaises(
packetfilter.DuplicateShortenedTableName,
packetfilter.PacketFilter.__init__,
packetfilter.PacketFilter.__new__(packetfilter.PacketFilter),
policy.ParsePolicy(
GOOD_HEADER_DIRECTIONAL + LONG_NAME_TERM_DNS_TCP +
GOOD_HEADER_DIRECTIONAL + NON_SHORTENED_LONG_NAME_TERM_DNS_UDP,
self.naming),
EXP_INFO)
self.naming.GetNetAddr.assert_has_calls([
mock.call('PROD_NETWORK_EXTREAMLY_LONG_VERY_NO_GOOD_NAME'),
mock.call('PROD_NETWORK_EXTREAMLY_LONG_VER')])
self.naming.GetServiceByProto.assert_has_calls([
mock.call('DNS', 'tcp'),
mock.call('DNS', 'udp')])

def testTermNameConflict(self):
self.assertRaises(
packetfilter.DuplicateTermError,
Expand Down

0 comments on commit 213626a

Please sign in to comment.