From 811eb29d37c1739e40f34085d0dafe71d60b3112 Mon Sep 17 00:00:00 2001 From: Garfield Freeman Date: Thu, 6 Jan 2022 14:49:52 -0800 Subject: [PATCH 1/2] fix: Fixes `refresh()` for attrib style params This fixes "attrib" style params, which are (currently) only present in the various rule objects in policies.py. Doing `rule.apply()` when the uuid is set and the name is unchanged does not cause PAN-OS to complain, so this fix does not cause any regressions / change in standard behavior. Doing `rule.apply() when the uuid is set and the name IS changed causes PAN-OS to silently create another rule with a new uuid. This of course means that the uuid of the object that was applied differs between the user's object hierarchy and what's on live. In light of this, we might want to investigate how to handle changing the unique identifier from the name to the uuid of the rule, but some research is needed on if this is actually possible from an XPATH perspective as well as how to gracefully handle backwards compatibility (before the uuid field existed). Fixes #392 --- panos/base.py | 56 +++++++++--- tests/test_params.py | 210 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 252 insertions(+), 14 deletions(-) create mode 100644 tests/test_params.py diff --git a/panos/base.py b/panos/base.py index 1cc2784f..aea62497 100644 --- a/panos/base.py +++ b/panos/base.py @@ -686,6 +686,8 @@ def update(self, variable): ) device.set_config_changed() path, value, var_path = self._get_param_specific_info(variable) + if var_path.vartype == "attrib": + raise NotImplementedError("Cannot update 'attrib' style params") xpath = "{0}/{1}".format(self.xpath(), path) if value is None: @@ -2475,6 +2477,11 @@ def element(self, with_children=True, comparable=False): self.xml_merge(ans, itertools.chain(*iterchain)) # Now that the whole element is built, mixin an attrib vartypes. + # + # We do this here instead of in xml_merge() because attributes are considered + # part of the identity in that function, and I'm not sure we want to manage + # a list of what attributes are considered part of an element's identity and + # what should be mixed in. for p in paths: if p.vartype != "attrib": continue @@ -2484,24 +2491,31 @@ def element(self, with_children=True, comparable=False): if attrib_value is None or p.exclude: continue e = ans - find_path = [ - ".", - ] for ap in attrib_path: if not ap: continue + finder = None + tag = None + attribs = {} if ap.startswith("entry "): junk, var_to_use = ap.split() sol_value = panos.string_or_list(settings[var_to_use])[0] - find_path.append("entry[@name='{0}']".format(sol_val)) + finder = "entry[@name='{0}']".format(sol_value) + tag = "entry" + attribs["name"] = sol_value elif ap == "entry[@name='localhost.localdomain']": - find_path.append(ap) + finder = ap + tag = "entry" + attribs["name"] = "localhost.localdomain" else: - find_path.append(ap.format(**settings)) - if len(find_path) > 1: - e = e.find("/".join(find_path)) - if e is not None: - e.attrib[attrib_name] = attrib_value + finder = ap.format(**settings) + tag = finder + e2 = e.find("./{0}".format(finder)) + if e2 is None: + e = ET.SubElement(e, tag, attribs) + else: + e = e2 + e.attrib[attrib_name] = attrib_value return ans @@ -2842,6 +2856,8 @@ def _set_inner_xml_tag_text(self, elm, value, comparable=False): elif self.vartype == "none": # There is no variable, so don't try to populate it pass + elif self.vartype == "attrib": + raise ValueError("attrib not yet supported for classic objects") else: elm.text = str(value) @@ -2945,10 +2961,13 @@ def element(self, elm, settings, comparable=False): return None e = elm + attr = None # Build the element tokens = self.path.split("/") if self.vartype == "exist": del tokens[-1] + elif self.vartype == "attrib": + attr = tokens.pop() for token in tokens: if not token: continue @@ -2965,7 +2984,7 @@ def element(self, elm, settings, comparable=False): e.append(child) e = child - self._set_inner_xml_tag_text(e, value, comparable) + self._set_inner_xml_tag_text(e, value, comparable, attr) return elm @@ -3013,10 +3032,13 @@ def parse_xml(self, xml, settings, possibilities): # thus not needed return None + attr = None e = xml tokens = self.path.split("/") if self.vartype == "exist": del tokens[-1] + elif self.vartype == "attrib": + attr = tokens.pop() for p in tokens: # Skip this path part if there is no path part if not p: @@ -3063,15 +3085,16 @@ def parse_xml(self, xml, settings, possibilities): e = ans # Pull the value, properly formatted, from this last element - self.parse_value_from_xml_last_tag(e, settings) + self.parse_value_from_xml_last_tag(e, settings, attr) - def _set_inner_xml_tag_text(self, elm, value, comparable=False): + def _set_inner_xml_tag_text(self, elm, value, comparable=False, attr=None): """Sets the final elm's .text as appropriate given the vartype. Args: elm (xml.etree.ElementTree.Element): The element whose .text to set. value (various): The value to put in the .text, conforming to the vartype of this parameter. comparable (bool): Make updates for element string comparisons. For encrypted fields, if the text should be set to a password hash (True) or left as a basestring (False). For entry and member vartypes, sort the entries (True) or leave them as-is (False). + attr (str): For `vartype="attrib"`, the attribute name. """ # Format the element text appropriately @@ -3102,10 +3125,12 @@ def _set_inner_xml_tag_text(self, elm, value, comparable=False): elm.text = str(int(value)) elif self.vartype == "encrypted" and comparable: elm.text = self._sha1_hash(str(value)) + elif self.vartype == "attrib": + elm.attrib[attr] = value else: elm.text = str(value) - def parse_value_from_xml_last_tag(self, elm, settings): + def parse_value_from_xml_last_tag(self, elm, settings, attr): """Actually do the parsing for this parameter. The value parsed is saved into the ``settings`` dict. @@ -3115,6 +3140,7 @@ def parse_value_from_xml_last_tag(self, elm, settings): document passed in to ``parse_xml()`` that contains the actual value to parse out for this parameter. settings (dict): The dict where the parsed value will be saved. + attr (str): For `vartype="attrib"`, the attribute name. Raises: ValueError: If a param is in an incorrect format. @@ -3143,6 +3169,8 @@ def parse_value_from_xml_last_tag(self, elm, settings): pass elif self.vartype == "int": settings[self.param] = int(elm.text) + elif self.vartype == "attrib": + settings[self.param] = elm.attrib.get(attr, None) else: settings[self.param] = elm.text diff --git a/tests/test_params.py b/tests/test_params.py new file mode 100644 index 00000000..b78d0335 --- /dev/null +++ b/tests/test_params.py @@ -0,0 +1,210 @@ +import pytest +import xml.etree.ElementTree as ET + +from panos.base import ENTRY, Root +from panos.base import VersionedPanObject, VersionedParamPath +from panos.firewall import Firewall + + +class FakeObject(VersionedPanObject): + SUFFIX = ENTRY + ROOT = Root.VSYS + + def _setup(self): + self._xpaths.add_profile(value="/fake") + + params = [] + + params.append(VersionedParamPath("uuid", vartype="attrib", path="uuid",),) + params.append(VersionedParamPath("size", vartype="int", path="size",),) + params.append(VersionedParamPath("listing", vartype="member", path="listing",),) + params.append(VersionedParamPath("pb1", vartype="exist", path="pb1",),) + params.append(VersionedParamPath("pb2", vartype="exist", path="pb2",),) + params.append(VersionedParamPath("live", vartype="yesno", path="live",),) + params.append( + VersionedParamPath("disabled", vartype="yesno", path="disabled",), + ) + params.append( + VersionedParamPath("uuid2", vartype="attrib", path="level-2/uuid",), + ) + params.append(VersionedParamPath("age", vartype="int", path="level-2/age",),) + params.append( + VersionedParamPath( + "interfaces", vartype="member", path="level-2/interface", + ), + ) + + self._params = tuple(params) + + +def _verify_render(o, expected): + ans = o.element_str().decode("utf-8") + + assert ans == expected + + +def _refreshed_object(): + fw = Firewall("127.0.0.1", "admin", "admin", "secret") + fw._version_info = (9999, 0, 0) + + o = FakeObject() + fw.add(o) + + o = o.refreshall_from_xml(_refresh_xml())[0] + + return o + + +def _refresh_xml(): + return ET.fromstring( + """ + + + 5 + + first + second + + + yes + + 12 + + third + fourth + + + +""" + ) + + +# int at base level +def test_render_int(): + _verify_render( + FakeObject("test", size=5), '5', + ) + + +def test_parse_int(): + o = _refreshed_object() + + assert o.size == 5 + + +# member list at base level +def test_render_member(): + _verify_render( + FakeObject("test", listing=["one", "two"]), + 'onetwo', + ) + + +def test_parse_member(): + o = _refreshed_object() + + assert o.listing == ["first", "second"] + + +# exist at base level +def test_render_exist(): + _verify_render( + FakeObject("test", pb1=True), '', + ) + + +def test_parse_exists(): + o = _refreshed_object() + + assert o.pb1 + assert not o.pb2 + + +# yesno at base level +def test_render_yesno(): + _verify_render( + FakeObject("test", disabled=True), + 'yes', + ) + + +def test_parse_yesno(): + o = _refreshed_object() + + assert o.disabled + + +# attrib +def test_render_attrib(): + _verify_render( + FakeObject("test", uuid="123-456"), '', + ) + + +def test_parse_attrib(): + o = _refreshed_object() + + assert o.uuid == "123-456" + + +# int at depth 1 +def test_render_d1_int(): + _verify_render( + FakeObject("test", age=12), + '12', + ) + + +def test_parse_d1_int(): + o = _refreshed_object() + + assert o.age == 12 + + +# member list at depth 1 +def test_render_d1_member(): + _verify_render( + FakeObject("test", interfaces=["third", "fourth"]), + "".join( + [ + '', + "thirdfourth", + "", + ] + ), + ) + + +def test_parse_d1_member(): + o = _refreshed_object() + + assert o.interfaces == ["third", "fourth"] + + +# uuid at depth 1 +def test_render_d1_attrib_standalone(): + _verify_render( + FakeObject("test", uuid2="456-789"), + '', + ) + + +def test_render_d1_attrib_mixed(): + _verify_render( + FakeObject("test", uuid2="456-789", age=12), + '12', + ) + + +def test_parse_d1_attrib(): + o = _refreshed_object() + + assert o.uuid2 == "456-789" + + +# should raise an exception +def test_update_attrib_raises_not_implemented_exception(): + o = _refreshed_object() + + with pytest.raises(NotImplementedError): + o.update("uuid") From 0e0019ed014a8d6716141bcb6eed09dc377606c1 Mon Sep 17 00:00:00 2001 From: Garfield Freeman Date: Fri, 7 Jan 2022 07:49:18 -0800 Subject: [PATCH 2/2] rerun the build --- tests/test_params.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_params.py b/tests/test_params.py index b78d0335..4073ae2e 100644 --- a/tests/test_params.py +++ b/tests/test_params.py @@ -7,6 +7,8 @@ class FakeObject(VersionedPanObject): + """Fake object for testing.""" + SUFFIX = ENTRY ROOT = Root.VSYS