Add Palo Alto Firewalls support #113

XioNoX · 2017-01-18T08:48:35Z

This PR includes the content of the PR #52
Plus the following modifications:

Generic address book and address sets
PAN application (> layer 4) with pan-application:: xxx yyy
Raises errors if policies/services names are too long
uses service_name and protocol in service name instead of port list
Used autopep8 (atom beautify) on paloaltofw.py

If this gets accepted, we also need to make sure the original author of PR #52 gets the credits as he did the largest part of the work.

Plus the following modifications: * Generic applications (known as services in PAN) and applications sets (layer 4) * PAN application (> layer 4) * Raises errors if policies/services names are too long * uses service_name and protocol in service name instead of port list * Used autopep8 (atom beautify) on paloaltofw.py

googlebot · 2017-01-18T08:48:37Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

ankenyr · 2017-01-18T19:38:32Z

aclgen.py

@@ -54,6 +54,7 @@
 from lib import speedway
 from lib import srxlo
 from lib import windows_advfirewall
+from lib import paloaltofw


Please move this to be in alphabetical order.

ankenyr · 2017-01-18T19:46:27Z

lib/aclgenerator.py

@@ -225,6 +225,7 @@ class ACLGenerator(object):
                         'owner',
                         'qos',
                         'routing_instance',
+                         'pan-application'


Forgot a comma?
Also is this strictly necessary? I don't believe it is.

Indeed, I wasn't sure of its purpose when I originally added it.

ankenyr · 2017-01-18T19:48:23Z

lib/policy.py

@@ -1600,6 +1617,7 @@ def __ne__(self, other):
    'protocol': 'PROTOCOL',
    'protocol-except': 'PROTOCOL_EXCEPT',
    'qos': 'QOS',
+    'pan-application':'PAN_APPLICATION',


Can we put a space after the :?

ankenyr · 2017-01-18T19:49:55Z

lib/policy.py

@@ -1612,7 +1630,7 @@ def __ne__(self, other):
    'timeout': 'TIMEOUT',
    'traffic-type': 'TRAFFIC_TYPE',
    'verbatim': 'VERBATIM',
-    'vpn': 'VPN',
+#    'vpn': 'VPN',


Was this meant to be commented out? I don't see why this should be.

Indeed no. This is a workaround specific to our setup, as vpn is a zone name and not a special keyword.
Cf. #62
I removed the comment sign.

ankenyr · 2017-01-18T19:51:02Z

lib/policy.py

@@ -2080,6 +2099,11 @@ def p_qos_spec(p):
  """ qos_spec : QOS ':' ':' STRING """
  p[0] = VarType(VarType.QOS, p[4])

+def p_pan_application_spec(p):


two spaces should be separating this def, same thing for under it.

ankenyr · 2017-01-18T19:52:20Z

lib/policy_simple.py

@@ -249,6 +249,8 @@ class ProtocolExcept(Field):
 class Qos(Field):
  """A rate-limit-icmp field."""

+class PANApplication(Field):


Same here, please use two spaces between these classes.

ankenyr · 2017-01-18T19:54:13Z

lib/paloaltofw.py

+import aclgenerator
+import nacaddr
+
+import re


This looks to be unused.

Indeed, removed.

XioNoX · 2017-01-18T23:13:33Z

I signed it! Corporation Mozilla.

ankenyr · 2017-01-20T03:03:11Z

lib/paloaltofw.py

+
+
+class PaloAltoFW(aclgenerator.ACLGenerator):
+    """PaloAltoFW rendering class.


This docstring should only be one line. Arguments should not be documented here and the description below is kind of redundant. It inherits from the aclgenerator class so it should be obvious it renders a syntax output.

Updated, I think it has been copied from the JuniperSRX docstring, which is very similar.

ankenyr · 2017-01-20T03:04:20Z

policies/pol/sample_paloalto.pol

@@ -0,0 +1,48 @@
+################


Can we see an example of pan_application being used inside of here?

Added, as well as the expected output in the test folder.
Palo alto comes with a wide array of existing "Applications", that's why I made it a simple variable that the user can define.

Can someone make custom applications or is there a set list of things? If it is a set list should this have checking to make sure someone is not applying incorrect services?

It's also possible to make custom applications.

ankenyr · 2017-01-20T03:35:32Z

lib/paloaltofw.py

+            term_dup_check = set()
+            new_terms = []
+            for term in terms:
+                term.name = self.FixTermLength(term.name)


Not a big deal but is there a max term length? If so we should probably override it because it is likely different from 62 limit in aclgenerator.

I'm not sure I understand.
The final rule length is limited to 31 chars. I added an exception for that on line 208 (PaloAltoFWTooLongName)

There is a variable named _TERM_MAX_LENGTH. If there is a max length FixTermLength should be called and _TERM_MAX_LENGTH should be set. This will raise an error if the term name is to long. If there is no limit then FixTermLength is irrelevant. Make sense?

Currently the final name (limited at 31 chars) also contains the source and destination zone name (eg. internal_2_external-term-name. So only checking the term length is not possible (and ends up being quite short).
In order to save rule length, I'm considering the idea of requiring the terms to be unique (similar to the srx lib for example), and then either:
1/ Trimming the sources zone names to 3 char (eg. int_2_ext-term-name), then the prefix is always a fixed length.
or
2/ Not applying any prefix, and letting the user manually set a prefix (eg, src/dst zones) in the term if the user wants it.
I think I like 2/ better as it lets the user have more freedom.
What do you think?

2 gives more freedom but there should be checks to make sure a invalid ACL is not rendered. Please make sure that is not possible via raising an error.

Added a check to raise "PaloAltoFWDuplicateTermError" in case a term is duplicated.

As well as use _TERM_MAX_LENGTH instead of custom check.

ankenyr · 2017-01-20T03:56:13Z

lib/paloaltofw.py

+        self.modify_options(terms)
+
+    def modify_options(self, PAFWterm):
+        # for PAFWterm in terms:


Should this be commented out?

I believe the loop has been moved to line 346 (for term in new_terms:)
I removed the line mentioned here.

ankenyr · 2017-01-20T03:58:58Z

lib/paloaltofw.py

+        self.service_map[(ports, protocol)] = {'name': final_service_name}
+
+
+class Rule():


I am confused why this exists, it seems to replace the function of Term. Why was Term not able to be used instead?

Not sure why the original Author went that way.

Can we explore why this was chosen and if this could be rolled up into a term? I don't understand why this is being used this way and I don't like having code integrated that no one understands. I will expect a docstring for Rule explaining what it is and why it is used instead of the term object.

I'm going to need help to figure that one out.

… fix <31 char

…name is duplicated

ankenyr · 2017-01-20T20:47:37Z

lib/paloaltofw.py

+      rules.append(self.INDENT * 9 + "<action>" + Term.ACTIONS.get(
+          str(options["action"])) + "</action>")
+
+      if fz == tz == "any":


This seems weird. There are loops above over the from zone and to zone which is a list. From my bit of testing I don't see that this would ever have more than one from-zone or to-zone per rule in rules. So why are these added as a list? The way the logic is setup fz could theoretically be set to an empty list if someone fed in a modified policy object that had the options set to ['from-zone', '', 'to-zone', ''] this would cause a problem.

@ankenyr
Not sure why it's originally set up that way, but it can be related to the fact that PAN allows rules to have several source/destination zones.
Right now line 149

self.options["from_zone"] = [from_zone]

sets it to an array containing only 1 element.
But we could for example imagine it (in the future) being a list from the .pol file header, line 292

self.from_zone = filter_options[1].split(',')

(or something similar).

About the possibility of fz or tz being empty, I believe we can add a check after line 150:

if not from_zone or not to_zone: raise PaloAltoFWOptionError("Source or destination zone is empty.")

2nd option would be to default both to "any" if they are not set instead of raising an error.
But I think it makes more sens to have the user explicitly define "any" if he wants "any".

XioNoX · 2017-01-23T09:16:13Z

CLA signed with my personal email as well.

googlebot · 2017-01-23T09:16:16Z

CLAs look good, thanks!

… duplicated + update .pol and filter expected

…application is set

XioNoX · 2017-01-28T09:16:49Z

Commit 48a5f71 default the "service" field to "application-default" if "service" is not set, but "application" is.
If neither "service" nor "application" is set, the default "any" is used.

This is to follow PaloAlto recommendations and improve security. If "any" is used, the "application" filter can in some cases not be respected.

Here is PaloAlto suport take on it:

When the first packet is coming into firewall, the firewall rules are evaluated with 6 tuples (Source/Destination IP address, Source/Destination Ports, Protocol, and Zone). The application is not necessarily known in this first packets, it can take several packets to determine what the underlying application is. During this evaluation period, packets may be allowed through if there is a rule which has "any" instead of "application-default" for service or an application using dynamic tcp/udp ports. In this case you have a rule with "any" service it means that all ports will be allowed till application is identified.
After the session established with 6 tuples the application will be identified and it will be ultimately blocked by rule set accordingly.

If you change it to "application-default", firewall will only allow standard ports for applications you configured in a rule when the session is set up. So in security aspect we recommend to use "application-default" for service.

Please refer to KB below for more detail.
https://live.paloaltonetworks.com/t5/Management-Articles/Why-are-Rules-Denying-Applications-Allowing-Some-Packets/ta-p/58098
https://live.paloaltonetworks.com/t5/Management-Articles/URL-Block-Page-Response-Page-Appears-When-Custom-URL-is-Used/ta-p/56107

ankenyr · 2017-02-01T22:14:54Z

This is looking good enough that I think you should start making tests for the PR.

XioNoX · 2017-02-02T02:31:14Z

Great! Tests added!

ankenyr · 2017-02-09T20:00:50Z

lib/policy.py

@@ -1026,6 +1040,7 @@ def AddObject(self, obj):
      # qos?
      elif obj.var_type is VarType.QOS:
        self.qos = obj.value
+


Extra space added, please remove.

ankenyr · 2017-02-09T20:04:32Z

lib/policy.py

@@ -1334,6 +1349,7 @@ class VarType(object):
  HOP_LIMIT = 47
  LOG_NAME = 48
  FLEXIBLE_MATCH_RANGE = 49
+  PAN_APPLICATION = 50


Can we change this to 54? I have some internal stuff I will be pushing out soon and it will conflict.

#113 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=147091309

XioNoX mentioned this pull request Jan 18, 2017

Included a Palo Alto Firewall target #52

Closed

ankenyr reviewed Jan 18, 2017

View reviewed changes

XioNoX added 5 commits January 19, 2017 07:36

Moved import to alphabetical order

d5066dd

Removed unneeded WARN_IF_UNSUPPORTED pan-application

146105c

address review comments

7c05730

Add spacing around new class

008540f

Remove unused re import

d789600

ankenyr reviewed Jan 20, 2017

View reviewed changes

XioNoX and others added 5 commits January 20, 2017 12:54

Start adding tests, fixes some lint errors

81ffbd7

Answer github comments; fix intentation error; shorten policy name to…

03c78bb

… fix <31 char

Update filters_expected to match reality; Raises an error if service …

288e84a

…name is duplicated

Linted the file.

2523c88

Changing imports for external version.

cc462a6

ankenyr reviewed Jan 20, 2017

View reviewed changes

Replace automatic rule name prefix with raising an error if a term is…

32ca242

… duplicated + update .pol and filter expected

XioNoX added 2 commits January 27, 2017 07:31

Use builtin _TERM_MAX_LENGTH

c48d17f

Default service to application-default when no service is set but an …

48a5f71

…application is set

ankenyr and others added 2 commits February 1, 2017 10:51

Update paloaltofw.py

1d3c013

Adding docstrings

a442eb9

Add tests + fix supported tokens

5ee126a

ankenyr reviewed Feb 9, 2017

View reviewed changes

XioNoX and others added 2 commits February 10, 2017 06:38

Renumber pan_application vartype to 54, remove extra space

a6871d5

Merge branch 'master' into master

84233a1

ankenyr merged commit d5ff1d5 into google:master Feb 9, 2017

morrowc pushed a commit that referenced this pull request Mar 15, 2017

PR113 Add Palo Alto Firewalls support

7be942b

#113 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=147091309



		class PaloAltoFW(aclgenerator.ACLGenerator):
		"""PaloAltoFW rendering class.

		self.service_map[(ports, protocol)] = {'name': final_service_name}


		class Rule():

Add Palo Alto Firewalls support #113

Add Palo Alto Firewalls support #113

Conversation

XioNoX commented Jan 18, 2017 • edited Loading

googlebot commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XioNoX commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XioNoX Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

XioNoX commented Jan 23, 2017

googlebot commented Jan 23, 2017

XioNoX commented Jan 28, 2017

ankenyr commented Feb 1, 2017

XioNoX commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XioNoX commented Jan 18, 2017 •

edited

Loading

XioNoX Jan 23, 2017 •

edited

Loading