Initial support for Cisco FWSM and Checkpoint Firewalls.

[andymillernz]

Description

Initial support for Cisco FWSM and Checkpoint Gaia Firewalls..

The devtype must be set to either "fwsm" or "cpgaia" as the auto detection is not yet implemented. These are L3 devices so services implemented include route, arpnd, interfaces and device.

Bugs fixed:

• Set self._retry to 0 in node.py when a connection fails. If devtype was set and the device was down we wouldn't exit the retry loop.

TODO:

• Fix uptime/boottime for FWSM and CP's
• Implement devconfig
• Test with Cisco ASA and implement as seperate devtype if needed

Test inclusion requirements

New Textfsm templates have associated test code

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Changes to the Documentation

devtype=fwsm or devtype=cpgaia must be used

FWSM implementation assumes the ipaddress used is in a particular "context" - and the host name is derived from the actual host + context name. The code does not attempt to change between contexts so a seperate hosts entry is required for each context.

Double Check

• [X] I have read the comments and followed the CONTRIBUTING.md.
• [X] I have explained my PR according to the information in the comments or in a linked issue.
• [X] My PR source branch is created from the develop branch.
• [X] My PR targets the develop branch.
• [ ] All my commits have --signoff applied

[andymillernz]

Hope the comments below help..

On 5/07/2022, at 9:51 PM, Claudio Usai @.***> wrote:

@claudious96 commented on this pull request.

Thanks for the contribution!

I left some comments here and there, mostly asking the reason for some choices or pointing out possible issues.

In suzieq/poller/controller/manager/static.py https://github.com/netenglabs/suzieq/pull/760#discussion_r913535699:

•        # Fix error - "Separator is not found, and chunk exceed the limit"
  

•        limit = 1024 * 128,  # 128 KB, default is 64KB
  

Could you give us more details about this fix? In which situation the 64KB buffer was not enough?

When processing arp’s and routes for Cisco FWSM we need to fix aliased ip address - where they show as a text name in the arp/route list. I originally fixed this by doing reverse DNS’s lookups and some templated fixes based upon name formats. However this was too specific to my network environment so fixed it the “correct” way by getting a list of “names” from the FWSM and using these to fix any non ip entries. The volume of text returned with arps + names exceeded the buffer size and causes the “chunk exceed” error..

FWSM’s can easily have 10K lines of config - with many thousands of aliases..

In suzieq/poller/worker/nodes/node.py https://github.com/netenglabs/suzieq/pull/760#discussion_r913553299:

•    if not isinstance(output, list):
  

•        # In some errors, the output returned is not a list
  

•        self.bootupTimestamp = -1
  

•        return
  

Could you add a log here? Something like this should to the job:

I think this is actually from your original code ????

⬇️ Suggested change

•    if not isinstance(output, list):
  

•        # In some errors, the output returned is not a list
  

•        self.bootupTimestamp = -1
  

•        return
  

•    if not isinstance(output, list):
  

•        # In some errors, the output returned is not a list
  

•        self.bootupTimestamp = -1
  

•        self.logger.error(f'nodeinit: Error getting version for '
  

•                          f'{self.address}:{self.port}')
  

•        return
  

In suzieq/poller/worker/services/arpnd.py https://github.com/netenglabs/suzieq/pull/760#discussion_r913562989:

•    for i, entry in enumerate(processed_data):
  

•        if "alias" in entry:
  

•            lookup [entry['alias']] = entry['ipAddress']
  

•            drop_indices.append(i)
  

•    processed_data = np.delete(processed_data, drop_indices).tolist()
  

•    for entry in processed_data:
  

•        ip_address = entry['ipAddress']
  

•        try:
  

•            # check we have a valid address
  

•            _ = ipaddress.ip_address(ip_address)
  

•        except Exception:
  

•            if ip_address not in lookup:
  

•                # This should never be needed....
  

•                try:
  

•                    ip = socket.gethostbyname(ip_address)
  

I don't think we can rely on this piece of code. The poller may not have access to the entire network. If an entry doesn't have an ip address in the device's arp table or in the lookup dictionary, then we should either store it as is or discard the entry. @ddutt https://github.com/ddutt do you have an opinion on this?

You could be correct - AND as per my comment it should never be used (it was from my original approach) so could be removed.. The following could be used...

    try:
            # check we have a valid address
            _ = ipaddress.ip_address(ip_address)
        except Exception:
            if ip_address in lookup:
                entry['ipAddress'] = lookup[ip_address]

In suzieq/poller/worker/services/arpnd.py https://github.com/netenglabs/suzieq/pull/760#discussion_r913565064:

•            lookup [entry['alias']] = entry['ipAddress']
  

•            drop_indices.append(i)
  

•    processed_data = np.delete(processed_data, drop_indices).tolist()
  

•    for entry in processed_data:
  

•        ip_address = entry['ipAddress']
  

•        try:
  

•            # check we have a valid address
  

•            _ = ipaddress.ip_address(ip_address)
  

•        except Exception:
  

•            if ip_address not in lookup:
  

•                # This should never be needed....
  

•                try:
  

•                    ip = socket.gethostbyname(ip_address)
  

•                except Exception:
  

•                    ip = fakeaddress # fake it..
  

As said above, if we get here I think we should either store the entry as is or discard it. I don't see any point in storing it with fake address. Am I missing something

Left over from the original approach before I started using the “show name” results

In suzieq/poller/worker/services/interfaces.py https://github.com/netenglabs/suzieq/pull/760#discussion_r913582683:

•            with contextlib.suppress(Exception):
  

•                vlan = int (name[4:])
  

•                entry["vlan"] = vlan
  

In which case do you expect an exception here? I think it'd be better to use a try ... except if you want to handle a specific situation or let the original exception propagate and be catched at the upper level. The latter would make it easier to notice if the output is different than expected and issue a fix.

with contextlib.supress is equivalent to a try..except - just nicer to use :) In this case there isn’t a fall through option - it either works and we update the vlan entry or fall through

In suzieq/poller/worker/services/routes.py https://github.com/netenglabs/suzieq/pull/760#discussion_r913586145:

•            drop_indices.append(i)
  

•    processed_data = np.delete(processed_data, drop_indices).tolist()
  

•    # FWSM has routes with subnet(netmasks) instead of prefixes
  

•    for entry in processed_data:
  

•        try:
  

•            # check we have a valid address
  

•            _ = ipaddress.ip_address(entry['prefix'])
  

•        except Exception:
  

•            prefix = entry['prefix']
  

•            if prefix not in lookup:
  

•                # This should never happen however if there is a 'non-IP' entry without an alais
  

•                # we need to fix - will create a new IP address for each "host" we find
  

•                try:
  

•                    # lets see if the default DNS will give use the real Ip address
  

•                    ip = socket.gethostbyname(entry['prefix'])
  

Same as before, we should not rely on the poller to resolve domains.

Suggest same fix code as for arpnd..

In suzieq/poller/worker/nodes/node.py https://github.com/netenglabs/suzieq/pull/760#discussion_r913601361:

@@ -693,6 +706,7 @@ async def _init_ssh(self, init_dev_data=True, use_lock=True) -> None: else: self.logger.error('Unable to connect to ' f'{self.address}:{self.port}, {e}')

•                    self._retry = 0
  

I'm not sure we need it here. What if the device is temporarily unreachable, in that case we should keep trying.

If devtype is “set” in the config and a device doesn’t exist and we do a run-once the current code will never exit. Needed to force an end of the retry’s here and allow upstream code to handle from then on. I didn’t test with a long running environment and devices dropping in and out so perhaps I have broken something??

— Reply to this email directly, view it on GitHub https://github.com/netenglabs/suzieq/pull/760#pullrequestreview-1028259149, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADCYBEAVSAXVXQLCEK3R35LVSQATPANCNFSM52OMF54A. You are receiving this because you authored the thread.