Attachment 907294 Details for Bug 1103639 – proposed fix

[patch] proposed fix

0001-Check-for-invalid-id-of-resource-group-property-cons.patch (text/plain), 11.67 KB, created by Tomas Jelinek on 2014-06-10 14:34:50 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomas Jelinek

Created: 2014-06-10 14:34:50 UTC

Size: 11.67 KB

patch

obsolete

>From a82882bce5f58336837630c768304af677c689c7 Mon Sep 17 00:00:00 2001
>From: Tomas Jelinek <tojeline@redhat.com>
>Date: Mon, 2 Jun 2014 16:12:06 +0200
>Subject: [PATCH] Check for invalid id of resource, group, property, constraint
> and rule
>
>---
> pcs/constraint.py            |  3 ++
> pcs/prop.py                  |  3 ++
> pcs/resource.py              | 10 +++---
> pcs/test/Makefile            |  1 +
> pcs/test/test_constraints.py | 12 +++++++
> pcs/test/test_properties.py  |  4 +++
> pcs/test/test_resource.py    |  8 +++++
> pcs/test/test_utils.py       | 82 ++++++++++++++++++++++++++++++++++++++++++++
> pcs/utils.py                 | 26 ++++++++++++++
> 9 files changed, 145 insertions(+), 4 deletions(-)
> create mode 100644 pcs/test/test_utils.py
>
>diff --git a/pcs/constraint.py b/pcs/constraint.py
>index e019245..5311d4b 100644
>--- a/pcs/constraint.py
>+++ b/pcs/constraint.py
>@@ -755,6 +755,9 @@ def location_add(argv,rm=False):
>         node = ""
>         score = ""
>     else:
>+        id_valid, id_error = utils.validate_xml_id(constraint_id, 'constraint id')
>+        if not id_valid:
>+            utils.err(id_error)
>         resource_name = argv.pop(0)
>         node = argv.pop(0)
>         score = argv.pop(0)
>diff --git a/pcs/prop.py b/pcs/prop.py
>index 2574478..631ff5e 100644
>--- a/pcs/prop.py
>+++ b/pcs/prop.py
>@@ -32,6 +32,9 @@ def set_property(argv):
>         if "--node" in utils.pcs_options:
>             utils.set_node_attribute(args[0], args[1], utils.pcs_options["--node"])
>         elif ("--force" in utils.pcs_options) or utils.is_valid_property(args[0]):
>+            id_valid, id_error = utils.validate_xml_id(args[0], 'property name')
>+            if not id_valid:
>+                utils.err(id_error)
>             utils.set_cib_property(args[0],args[1])
>         else:
>             utils.err("unknown cluster property: '%s', (use --force to override)" % args[0])
>diff --git a/pcs/resource.py b/pcs/resource.py
>index 8f76b55..8f86736 100644
>--- a/pcs/resource.py
>+++ b/pcs/resource.py
>@@ -307,8 +307,9 @@ def format_desc(indent, desc):
> # Create a resource using cibadmin
> # ra_class, ra_type & ra_provider must all contain valid info
> def resource_create(ra_id, ra_type, ra_values, op_values, meta_values=[], clone_opts=[]):
>-    if len(ra_id) == 0:
>-        utils.err ("resource id cannot be empty")
>+    ra_id_valid, ra_id_error = utils.validate_xml_id(ra_id, 'resource name')
>+    if not ra_id_valid:
>+        utils.err(ra_id_error)
> 
>     dom = utils.get_cib_dom()
> 
>@@ -1432,8 +1433,9 @@ def resource_group_add(group_name, resource_ids, passed_dom = None):
>     resources_element = top_element.getElementsByTagName("resources")[0]
>     group_found = False
> 
>-    if group_name == "":
>-        utils.err("group name cannot be empty")
>+    name_valid, name_error = utils.validate_xml_id(group_name, 'group name')
>+    if not name_valid:
>+        utils.err(name_error)
> 
>     for resource in top_element.getElementsByTagName("primitive"):
>         if resource.getAttribute("id") == group_name:
>diff --git a/pcs/test/Makefile b/pcs/test/Makefile
>index 36e560a..c737e24 100644
>--- a/pcs/test/Makefile
>+++ b/pcs/test/Makefile
>@@ -8,6 +8,7 @@ ifneq ($(python_version_major_minor),2.6)
> endif
> 
> test:
>+	python test_utils.py ${pyunit_flags}
> 	python test_cluster.py ${pyunit_flags}
> 	python test_resource.py ${pyunit_flags}
> 	python test_constraints.py ${pyunit_flags}
>diff --git a/pcs/test/test_constraints.py b/pcs/test/test_constraints.py
>index df2e115..2062eb8 100644
>--- a/pcs/test/test_constraints.py
>+++ b/pcs/test/test_constraints.py
>@@ -483,6 +483,14 @@ class ConstraintTest(unittest.TestCase):
>         ac (o,"Error: 'non-existant-resource' is not a resource\n")
>         assert r == 1
> 
>+        output, returnVal = pcs(temp_cib, "constraint rule add location-D1-rh7-1-INFINITY '#uname' eq rh7-2")
>+        ac(output, "Error: Unable to find constraint: location-D1-rh7-1-INFINITY\n")
>+        assert returnVal == 1
>+
>+        output, returnVal = pcs(temp_cib, "constraint rule add location-D2-rh7-2-INFINITY id=123 #uname eq rh7-2")
>+        ac(output, "Error: invalid rule id '123', '1' is not a valid first character for a rule id\n")
>+        assert returnVal == 1
>+
>     def testLocationBadRules(self):
>         o,r = pcs("resource create stateful0 Dummy --master")
>         ac(o,"")
>@@ -524,6 +532,10 @@ class ConstraintTest(unittest.TestCase):
>         ac(o,"Error: invalid score 'bar', use integer or INFINITY or -INFINITY\n")
>         assert r == 1
> 
>+        output, returnVal = pcs(temp_cib, "constraint location add loc:dummy D1 rh7-1 100")
>+        assert returnVal == 1
>+        ac(output, "Error: invalid constraint id 'loc:dummy', ':' is not a valid character for a constraint id\n")
>+
>     def testMasterSlaveConstraint(self):
>         os.system("CIB_file="+temp_cib+" cibadmin -R --scope nodes --xml-text '<nodes><node id=\"1\" uname=\"rh7-1\"/><node id=\"2\" uname=\"rh7-2\"/></nodes>'")
> 
>diff --git a/pcs/test/test_properties.py b/pcs/test/test_properties.py
>index 1db6ff7..d3f62a9 100644
>--- a/pcs/test/test_properties.py
>+++ b/pcs/test/test_properties.py
>@@ -111,6 +111,10 @@ class PropertyTest(unittest.TestCase):
>         assert r==1
>         ac(o,"Error: unknown cluster property: 'xxxx', (use --force to override)\n")
> 
>+        output, returnVal = pcs("property set 1234=5678 --force")
>+        ac(output, "Error: invalid property name '1234', '1' is not a valid first character for a property name\n")
>+        assert returnVal == 1
>+
>         o,r = pcs("property unset zzzzz")
>         assert r==1
>         ac(o,"Error: can't remove property: 'zzzzz' that doesn't exist\n")
>diff --git a/pcs/test/test_resource.py b/pcs/test/test_resource.py
>index 81228e7..3078d7e 100644
>--- a/pcs/test/test_resource.py
>+++ b/pcs/test/test_resource.py
>@@ -169,6 +169,10 @@ class ResourceTest(unittest.TestCase):
>         assert returnVal == 0
>         assert output == " Resource: bad_resource2 (class=ocf provider=heartbeat type=idontexist2)\n  Attributes: test4=bad3 \n  Operations: monitor interval=60s (bad_resource2-monitor-interval-60s)\n",[output]
> 
>+        output, returnVal = pcs(temp_cib, "resource create dum:my Dummy")
>+        assert returnVal == 1
>+        ac(output, "Error: invalid resource name 'dum:my', ':' is not a valid character for a resource name\n")
>+
>     def testDeleteResources(self):
> # Verify deleting resources works
>         line = "resource create --no-default-ops ClusterIP ocf:heartbeat:IPaddr2 ip=192.168.0.99 cidr_netmask=32 op monitor interval=30s"
>@@ -629,6 +633,10 @@ class ResourceTest(unittest.TestCase):
>         assert returnVal == 0
>         ac(output, '')
> 
>+        output, returnVal = pcs(temp_cib, "resource group add group:dummy dummy1")
>+        assert returnVal == 1
>+        ac(output, "Error: invalid group name 'group:dummy', ':' is not a valid character for a group name\n")
>+
>     def testGroupOrder(self):
>         output, returnVal = pcs(temp_cib, "resource create --no-default-ops A Dummy")
>         output, returnVal = pcs(temp_cib, "resource create --no-default-ops B Dummy")
>diff --git a/pcs/test/test_utils.py b/pcs/test/test_utils.py
>new file mode 100644
>index 0000000..49d4acc
>--- /dev/null
>+++ b/pcs/test/test_utils.py
>@@ -0,0 +1,82 @@
>+import os
>+import sys
>+import unittest
>+parentdir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
>+sys.path.insert(0, parentdir)
>+import utils
>+
>+class UtilsTest(unittest.TestCase):
>+    def testValidateXmlId(self):
>+        self.assertEquals((True, ""), utils.validate_xml_id("dummy"))
>+        self.assertEquals((True, ""), utils.validate_xml_id("DUMMY"))
>+        self.assertEquals((True, ""), utils.validate_xml_id("dUmMy"))
>+        self.assertEquals((True, ""), utils.validate_xml_id("dummy0"))
>+        self.assertEquals((True, ""), utils.validate_xml_id("dum0my"))
>+        self.assertEquals((True, ""), utils.validate_xml_id("dummy-"))
>+        self.assertEquals((True, ""), utils.validate_xml_id("dum-my"))
>+        self.assertEquals((True, ""), utils.validate_xml_id("dummy."))
>+        self.assertEquals((True, ""), utils.validate_xml_id("dum.my"))
>+        self.assertEquals((True, ""), utils.validate_xml_id("_dummy"))
>+        self.assertEquals((True, ""), utils.validate_xml_id("dummy_"))
>+        self.assertEquals((True, ""), utils.validate_xml_id("dum_my"))
>+
>+        self.assertEquals(
>+            (False, "test id cannot be empty"),
>+            utils.validate_xml_id("", "test id")
>+        )
>+
>+        msg = "invalid test id '%s', '%s' is not a valid first character for a test id"
>+        self.assertEquals(
>+            (False, msg % ("0", "0")),
>+            utils.validate_xml_id("0", "test id")
>+        )
>+        self.assertEquals(
>+            (False, msg % ("-", "-")),
>+            utils.validate_xml_id("-", "test id")
>+        )
>+        self.assertEquals(
>+            (False, msg % (".", ".")),
>+            utils.validate_xml_id(".", "test id")
>+        )
>+        self.assertEquals(
>+            (False, msg % (":", ":")),
>+            utils.validate_xml_id(":", "test id")
>+        )
>+        self.assertEquals(
>+            (False, msg % ("0dummy", "0")),
>+            utils.validate_xml_id("0dummy", "test id")
>+        )
>+        self.assertEquals(
>+            (False, msg % ("-dummy", "-")),
>+            utils.validate_xml_id("-dummy", "test id")
>+        )
>+        self.assertEquals(
>+            (False, msg % (".dummy", ".")),
>+            utils.validate_xml_id(".dummy", "test id")
>+        )
>+        self.assertEquals(
>+            (False, msg % (":dummy", ":")),
>+            utils.validate_xml_id(":dummy", "test id")
>+        )
>+
>+        msg = "invalid test id '%s', '%s' is not a valid character for a test id"
>+        self.assertEquals(
>+            (False, msg % ("dum:my", ":")),
>+            utils.validate_xml_id("dum:my", "test id")
>+        )
>+        self.assertEquals(
>+            (False, msg % ("dummy:", ":")),
>+            utils.validate_xml_id("dummy:", "test id")
>+        )
>+        self.assertEquals(
>+            (False, msg % ("dum?my", "?")),
>+            utils.validate_xml_id("dum?my", "test id")
>+        )
>+        self.assertEquals(
>+            (False, msg % ("dummy?", "?")),
>+            utils.validate_xml_id("dummy?", "test id")
>+        )
>+
>+
>+if __name__ == "__main__":
>+    unittest.main()
>diff --git a/pcs/utils.py b/pcs/utils.py
>index c507962..c829d44 100644
>--- a/pcs/utils.py
>+++ b/pcs/utils.py
>@@ -973,6 +973,9 @@ def rule_add(elem, argv):
>                 subexpression = ET.SubElement(expression,"date_spec")
>             continue
>         if arg[0] == "id":
>+            id_valid, id_error = validate_xml_id(arg[1], 'rule id')
>+            if not id_valid:
>+                err(id_error)
>             rule.set(arg[0], arg[1])
>         elif arg[0] == "score":
>             if is_score_or_opt(arg[1]):
>@@ -1378,6 +1381,29 @@ def is_score(var):
>         var.isdigit() or (len(var) > 1 and var[0] == "-" and var[1:].isdigit())
>     )
> 
>+def validate_xml_id(var, description="id"):
>+    # see NCName definition
>+    # http://www.w3.org/TR/REC-xml-names/#NT-NCName
>+    # http://www.w3.org/TR/REC-xml/#NT-Name
>+    if len(var) < 1:
>+        return False, "%s cannot be empty" % description
>+    first_char_re = re.compile("[a-zA-Z_]")
>+    if not first_char_re.match(var[0]):
>+        return (
>+            False,
>+            "invalid %s '%s', '%s' is not a valid first character for a %s"
>+                % (description, var, var[0], description)
>+        )
>+    char_re = re.compile("[a-zA-Z0-9_.-]")
>+    for char in var[1:]:
>+        if not char_re.match(char):
>+            return (
>+                False,
>+                "invalid %s '%s', '%s' is not a valid character for a %s"
>+                    % (description, var, char, description)
>+            )
>+    return True, ""
>+
> def is_systemctl():
>     if os.path.exists('/usr/bin/systemctl'):
>         return True
>-- 
>1.9.1
>

Actions: View | Diff

Attachments on bug 1103639: 901477 | 907294