Is get_firewall_policies() implemented correctly?

[ktbyers]

I am seeing some test failures on get_firewall_policies in napalm-fortios.

napalm-base defines get_firewall_polices as:

        {
            'policy_name': [{
                'position': 1,
                'packet_hits': 200,
                'byte_hits': 83883,
                'id': '230',
                'enabled': True,
                'schedule': 'Always',
                'log': 'all',
                'l3_src': 'any',
                'l3_dst': 'any',
                'service': 'HTTP',
                'src_zone': 'port2',
                'dst_zone': 'port3',
                'action': 'Permit'
            }]
        }

This is from base.py file.

The unit tests for napalm-fortios (mocked data) have the following for 'show firewall policy'

config firewall policy
    edit 1
        set uuid 9d747de8-dad4-51e5-7a7e-d078aa77140a
        set srcintf "any"
        set dstintf "any"
        set srcaddr "all"
        set dstaddr "all"
        set action accept
        set schedule "always"
        set service "ALL"
    next
    edit 2
        set name "bla"
        set uuid de597b64-aca8-51e6-5d00-b7874e6b72b8
        set srcintf "port3"
        set dstintf "port2"
        set srcaddr "all"
        set dstaddr "all"
        set schedule "always"
        set service "ALL"
        set logtraffic all
    next
end

The napalm-fortios driver returns the following (on get_firewall_policy):

{u'1': [{u'action': u'permit',
         u'byte_hits': -1,
         u'dst_zone': u'any',
         u'enabled': False,
         u'id': u'1',
         u'l3_dst': u'all',
         u'l3_src': u'all',
         u'log': u'False',
         u'packet_hits': -1,
         u'position': 2,
         u'schedule': u'always',
         u'service': u'ALL',
         u'src_zone': u'any'}],
 u'2': [{u'action': u'reject',
         u'byte_hits': -1,
         u'dst_zone': u'port2',
         u'enabled': False,
         u'id': u'2',
         u'l3_dst': u'all',
         u'l3_src': u'all',
         u'log': u'all',
         u'packet_hits': -1,
         u'position': 1,
         u'schedule': u'always',
         u'service': u'ALL',
         u'src_zone': u'port3'}]}

The key names being '1' and '2' looks wrong to me (should map to the policy_name per the definition in base.py).

[ktbyers]

@ebeahan This is the same issue I messaged about in the Slack channel.

[ebeahan]

@ktbyers Thanks! I'm taking a look.

[ebeahan]

On FortiOS CLI firewall policies are created within the config firewall policy context by stating edit <integer> where the integer is a unique value that becomes the policy id for that firewall policy. With that said, the key names being set to 1, 2, 3, etc. is correct since it's the policy id casted as a string. Also the policy ID is independent of the policies position (e.g. I created policy id 5 but move it's position above policy id 1).

I do see an issue with the position key of the inner dict in the output. If I understand the intent of position correctly, the value should represent where the policy lies in firewall evaluation order (ordered 1 to n with 1 being the top of the firewall policy, n being the final rule). The current way position is set is non-deterministic since the raw command output returned from pyFG is initial parsed into an intermediate dict structure potentially losing the original ordering.

One possible fix would be initializing an OrderedDict() vs a dict() for that intermediate dict to ensure the ordering remains accuracte for later on when position is being determined:

https://github.com/napalm-automation/napalm-fortios/blob/develop/napalm_fortios/fortios.py#L272

[ktbyers]

So with FortiOS is there only one firewall policy?

For example, on an ASA, I could have multiple firewall policies that would map to ACL names and could be bound to different interfaces.

I was expecting the 'policy_name' to correspond more to this.

What is the use of policy id in fortios. Is it just a way to reference the entries in the one firewall policy (so that you can delete them, move them, change them)?

[dbarrosop]

Yes, there is only one firewall policy. Rules apply always to inbound/outbound interfaces pairs (which can be wildcards) but they are configured under a single flat list (the UI let's you filter/organize by interfaces)

[ktbyers]

I still think this use of policy name doesn't make sense for FortiOS. The policy name for FortiOS should be something like default (since there is only one firewall policy that always applies).

The FortiOS policy id corresponds more to ACL sequence number (on Cisco ASA; Cisco IOS firewall)...and doesn't correspond to policy_name.

Here is what I think it should be for FortiOS (which would work reasonable well for Cisco ASA and Cisco IOS firewall).

        {
            'policy_name': [{
                'packet_hits': 200,
                'byte_hits': 83883,
                'id': '230',
                'enabled': True,
                'schedule': 'Always',
                'log': 'all',
                'l3_src': 'any',
                'l3_dst': 'any',
                'service': 'HTTP',
                'src_zone': 'port2',
                'dst_zone': 'port3',
                'action': 'Permit'
            }]
        }

position should be removed and should be indicated instead by position in the list. Also, there is only service specified here and we need to add src_port, dest_port (and possible remove service). There also needs to be a specification of protocol (i.e. UDP/TCP/ICMP).

Note, id field already contains the FortiOS policy ID.

We also should have someone that is familiar with Juniper SRX review to make sure we are covering the major NAPALM firewall platforms. Also probably need to consider zone based firewall on Cisco IOS and make sure it covers that reasonably well.

I am also open to just eliminating this and re-implementing later (since current implementation has a lot of problems) and since FortiOS is currently the only platform it is implemented on.

[ebeahan]

@ktbyers Agree with what you're proposing. I was interpreting policies as the individual firewall rules vs a collection of rules. As you pointed out, it's really indifferent in terms of FortiOS so whatever approach will apply best across the board can be adopted. Also prefer the idea of dumping position and allowing the index position with the list to serve the same purpose.

[dbarrosop]

I have no particular opinion about this. If you plan to modify the definition of the object, could you contact the people involved in the original PR to synch with them, please?

[ktbyers]

@dbarrosop Yes, that makes sense.

[ebeahan]

@awlx @DiogoAndre We had some discussion here on whether to modify get_firewall_policies that would also better support Cisco ASA and IOS if that's something that is implemented in the future. It looks like you were both involved in the original discussions below:

napalm-automation/napalm-base#136
#26

Any thoughts? Like @dbarrosop mentioned above we wanted to sync up with those involved in the original PRs to add the feature.

[awlx]

@ebeahan I am totally fine with your idea to modify the behaviour of get_firewall_polices. If you are modifying it could you also add a field for NAT Status and NAT Pool?

The service field is a group of protocols and ports for the Fortigate so I would like to still have it. As this makes it easier to search for in the config.

The implementation of get_firewall_policies was only driven by the need to read the rules of a Fortigate. So yes it lacks functions for other platforms and I am happy if someone wants to improve it :). I have some ASAs and a SRX here so if you need help testing I'll be glad to help.

[jmcgill298]

Hey guys, I have a couple of thoughts to add to this:

1. policy ID servers as the name of the policy, and sequence number determines the order in which the policy is used for enforcement. I think both are important, so maybe do something like

...
'id': 10,
'seq': 3
...

2. Would it be useful to grab the comments also? Or does that typically fall under too much clutter, not enough value?

[dbarrosop]

@jmcgill298

1. we already have position which should be equivalent to seq. Or do you mean something else?
2. Where would comment be? Policy or individual entry?

[jmcgill298]

@dbarrosop

1. you do currently have position, but @ktbyers suggested format does not, so just saying I think that should be carried over.
2. The comment is per individual entry. My previous job of managing firewalls used it primarily as a reference to a ticket request, but some people use it to also provide the reason why it was added. So I see that it could be useful to some, and not useful to others.

[awlx]

Anyone at RIPE74 maybe it's better to discuss somethings of this in person.

[dbarrosop]

I am but probably not the best person to talk with about this topic. Maybe @mirceaulinic.