Initial support for Cisco FWSM and Checkpoint Firewalls. #760

andymillernz · 2022-07-02T03:34:40Z

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

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

Bug fix where devtype is set yet device is down - would continue to retry connection - now sets self._retry to 0 when a connection error occures Adjusted the stdio buffer size when spawning a sub process - needed as FWSM configs can be huge and were overflowing the buffer TODO: fix uptime/boottime for FWSM and CP's - test with Cisco ASA

claudious96

Thanks for the contribution!

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

claudious96 · 2022-07-05T08:35:49Z

suzieq/poller/controller/manager/static.py

+            # 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?

claudious96 · 2022-07-05T08:53:51Z

suzieq/poller/worker/nodes/node.py

+        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 do the job:

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

claudious96 · 2022-07-05T09:03:31Z

suzieq/poller/worker/services/arpnd.py

+                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 do you have an opinion on this?

claudious96 · 2022-07-05T09:05:42Z

suzieq/poller/worker/services/arpnd.py

+                    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?

claudious96 · 2022-07-05T09:24:07Z

suzieq/poller/worker/services/interfaces.py

+                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.

claudious96 · 2022-07-05T09:27:34Z

suzieq/poller/worker/services/routes.py

+                    # 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.

claudious96 · 2022-07-05T09:43:28Z

suzieq/poller/worker/nodes/node.py

                    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.

andymillernz · 2022-07-05T10:47:24Z

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 <#760 (comment)>: > + # 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 <#760 (comment)>: > + 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 <#760 (comment)>: > + 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 <#760 (comment)>: > + 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 <#760 (comment)>: > + 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 <#760 (comment)>: > + 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 <#760 (comment)>: > @@ -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 <#760 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADCYBEAVSAXVXQLCEK3R35LVSQATPANCNFSM52OMF54A>. You are receiving this because you authored the thread.

* Flake8 mods * Removed outbound socket requests * Added Cisco ASA support * Fixed FWSM boottime calculations * Removed fix to missing devices timeout as this will be fixed elsewhere

claudious96 · 2022-07-19T10:31:17Z

suzieq/poller/worker/services/arpnd.py

+
+        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 in lookup:
+                    entry['ipAddress'] = lookup[ip_address]
+                else:
+                    # Should never reach here unless some strange issue with the device config
+                    logger.error(f"ERROR: FWSM has non IP address entry in APR table: {ip_address}")
+


Even if it's very unlikely we shouldn't log this for various reasons:

we might end up logging to much

the parser logs at a higher level and only if something breaks badly. We intend to keep following this strategy.

if it is a configuration error in the device, these poller's stdout doesn't seem to me the right place to point that out

Sorry for being pedantic on this piece of code, I'd rather get rid of the entries that aren't ipAddresses and don't match the lookup table. Here is a code suggestion:

Suggested change

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 in lookup:

entry['ipAddress'] = lookup[ip_address]

else:

# Should never reach here unless some strange issue with the device config

logger.error(f"ERROR: FWSM has non IP address entry in APR table: {ip_address}")

drop_indices = []

for i, entry in enumerate(processed_data):

ip_address = entry['ipAddress']

try:

# check we have a valid address

_ = ipaddress.ip_address(ip_address)

except ValueError:

if ip_address in lookup:

entry['ipAddress'] = lookup[ip_address]

else:

drop_indices.append(i)

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

claudious96 · 2022-07-19T10:32:01Z

suzieq/poller/worker/services/routes.py

+                    entry['prefix'] = lookup[prefix]
+                else:
+                    # Should never reach here unless some strange issue with the device config
+                    logger.error(f"ERROR: FWSM has non IP address entry in route table: {entry['prefix']}")


Same comment as the ARPND service

claudious96 · 2022-07-19T10:38:46Z

suzieq/poller/worker/services/routes.py

+            try:
+                # check we have a valid address
+                _ = ipaddress.ip_address(entry['prefix'])
+            except Exception:


Suggested change

except Exception:

except ValueError:

claudious96 · 2022-07-19T10:39:10Z

suzieq/poller/worker/services/arpnd.py

+            try:
+                # check we have a valid address
+                _ = ipaddress.ip_address(ip_address)
+            except Exception:


Suggested change

except Exception:

except ValueError:

claudious96 reviewed Jul 5, 2022

View reviewed changes

Updated FWSM and CP support as per disucssions with core developers.

f3afaae

* Flake8 mods * Removed outbound socket requests * Added Cisco ASA support * Fixed FWSM boottime calculations * Removed fix to missing devices timeout as this will be fixed elsewhere

claudious96 requested changes Jul 19, 2022

View reviewed changes

andymillernz and others added 8 commits August 2, 2022 15:39

Merge branch 'netenglabs:develop' into develop

abc3ded

Further changes as per core developers - passes pylint and flake8

33a116e

Merge branch 'netenglabs:develop' into develop

189acc8

Merge branch 'netenglabs:develop' into develop

77fc3f3

Merge branch 'netenglabs:develop' into develop

73b0369

Merge branch 'netenglabs:develop' into develop

5300dd6

Merge branch 'netenglabs:develop' into develop

3f14825

Merge remote-tracking branch 'upstream/develop' into develop

6c95dc5

andymillernz closed this Aug 26, 2024

		# Fix error - "Separator is not found, and chunk exceed the limit"
		limit = 1024 * 128, # 128 KB, default is 64KB

Initial support for Cisco FWSM and Checkpoint Firewalls. #760

Initial support for Cisco FWSM and Checkpoint Firewalls. #760

Uh oh!

Conversation

andymillernz commented Jul 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test inclusion requirements

Type of change

Changes to the Documentation

Double Check

Uh oh!

claudious96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claudious96 Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andymillernz commented Jul 5, 2022 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andymillernz commented Jul 2, 2022 •

edited

Loading

claudious96 Jul 5, 2022 •

edited

Loading