s4:samldb LDB module - make sure to not add identical "servicePrincipalName"s more...

The service principal names need to be case-insensitively unique, otherwise we
end up in a LDB ERR_ATTRIBUTE_OR_VALUE_EXISTS error.
This issue has been discovered on the technical mailing list (thread:
cannot rename windows xp machine in samba4) when trying to rename a AD
client workstation.

diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c
index 168e55b2f75118a2d1cc50fc50490fe533be4bd4..d17d809d77035951851f953b302b1fde1301df42 100644 (file)
--- a/source4/dsdb/samdb/ldb_modules/samldb.c
+++ b/source4/dsdb/samdb/ldb_modules/samldb.c
@@ -1816,7 +1816,7 @@ static int samldb_service_principal_names_change(struct samldb_ctx *ac)
        struct ldb_result *res;
        const char *dns_hostname = NULL, *old_dns_hostname = NULL,
                   *sam_accountname = NULL, *old_sam_accountname = NULL;
-       unsigned int i;
+       unsigned int i, j;
        int ret;
 
        el = dsdb_get_single_valued_attr(ac->msg, "dNSHostName",
@@ -1957,15 +1957,28 @@ static int samldb_service_principal_names_change(struct samldb_ctx *ac)
        }
 
        if (res->msgs[0]->num_elements == 1) {
-               /* Yes, we do have "servicePrincipalName"s. First we update them
+               /*
+                * Yes, we do have "servicePrincipalName"s. First we update them
                 * locally, that means we do always substitute the current
                 * "dNSHostName" with the new one and/or "sAMAccountName"
-                * without "$" with the new one and then we append this to the
-                * modification request (Windows behaviour). */
+                * without "$" with the new one and then we append the
+                * modified "servicePrincipalName"s as a message element
+                * replace to the modification request (Windows behaviour). We
+                * need also to make sure that the values remain case-
+                * insensitively unique.
+                */
+
+               ret = ldb_msg_add_empty(ac->msg, "servicePrincipalName",
+                                       LDB_FLAG_MOD_REPLACE, &el);
+               if (ret != LDB_SUCCESS) {
+                       return ret;
+               }
 
                for (i = 0; i < res->msgs[0]->elements[0].num_values; i++) {
                        char *old_str, *new_str, *pos;
                        const char *tok;
+                       struct ldb_val *vals;
+                       bool found = false;
 
                        old_str = (char *)
                                res->msgs[0]->elements[0].values[i].data;
@@ -1993,16 +2006,32 @@ static int samldb_service_principal_names_change(struct samldb_ctx *ac)
                                }
                        }
 
-                       ret = ldb_msg_add_string(ac->msg,
-                                                "servicePrincipalName",
-                                                new_str);
-                       if (ret != LDB_SUCCESS) {
-                               return ret;
+                       /* Uniqueness check */
+                       for (j = 0; (!found) && (j < el->num_values); j++) {
+                               if (strcasecmp((char *)el->values[j].data,
+                                              new_str) == 0) {
+                                       found = true;
+                               }
+                       }
+                       if (found) {
+                               continue;
                        }
-               }
 
-               el = ldb_msg_find_element(ac->msg, "servicePrincipalName");
-               el->flags = LDB_FLAG_MOD_REPLACE;
+                       /*
+                        * append the new "servicePrincipalName" - code derived
+                        * from ldb_msg_add_value()
+                        */
+                       vals = talloc_realloc(ac->msg, el->values,
+                                             struct ldb_val,
+                                             el->num_values + 1);
+                       if (vals == NULL) {
+                               return ldb_module_oom(ac->module);
+                       }
+                       el->values = vals;
+                       el->values[el->num_values].data = (uint8_t *) new_str;
+                       el->values[el->num_values].length = strlen(new_str);
+                       ++(el->num_values);
+               }
        }
 
        talloc_free(res);

diff --git a/source4/dsdb/tests/python/sam.py b/source4/dsdb/tests/python/sam.py
index 8417b26cb7ec94ae25237dbbff1368850ead11ab..c5727cd080d0cebdbeea6876f757acccab2b6bdf 100755 (executable)
--- a/source4/dsdb/tests/python/sam.py
+++ b/source4/dsdb/tests/python/sam.py
@@ -2432,10 +2432,53 @@ class SamTests(samba.tests.TestCase):
         self.assertTrue(len(res) == 1)
         self.assertEquals(res[0]["dNSHostName"][0], "testname2.testdom")
         self.assertEquals(res[0]["sAMAccountName"][0], "testname2$")
-        self.assertTrue(res[0]["servicePrincipalName"][0] == "HOST/testname2" or
-                        res[0]["servicePrincipalName"][1] == "HOST/testname2")
-        self.assertTrue(res[0]["servicePrincipalName"][0] == "HOST/testname2.testdom" or
-                        res[0]["servicePrincipalName"][1] == "HOST/testname2.testdom")
+        self.assertTrue(len(res[0]["servicePrincipalName"]) == 2)
+        self.assertTrue("HOST/testname2" in res[0]["servicePrincipalName"])
+        self.assertTrue("HOST/testname2.testdom" in res[0]["servicePrincipalName"])
+
+        m = Message()
+        m.dn = Dn(ldb, "cn=ldaptestcomputer,cn=computers," + self.base_dn)
+        m["servicePrincipalName"] = MessageElement("HOST/testname2.testdom",
+                                                   FLAG_MOD_ADD, "servicePrincipalName")
+        try:
+            ldb.modify(m)
+            self.fail()
+        except LdbError, (num, _):
+            self.assertEquals(num, ERR_ATTRIBUTE_OR_VALUE_EXISTS)
+
+        m = Message()
+        m.dn = Dn(ldb, "cn=ldaptestcomputer,cn=computers," + self.base_dn)
+        m["servicePrincipalName"] = MessageElement("HOST/testname3",
+                                                   FLAG_MOD_ADD, "servicePrincipalName")
+        ldb.modify(m)
+
+        res = ldb.search("cn=ldaptestcomputer,cn=computers," + self.base_dn,
+                         scope=SCOPE_BASE, attrs=["dNSHostName", "sAMAccountName", "servicePrincipalName"])
+        self.assertTrue(len(res) == 1)
+        self.assertEquals(res[0]["dNSHostName"][0], "testname2.testdom")
+        self.assertEquals(res[0]["sAMAccountName"][0], "testname2$")
+        self.assertTrue(len(res[0]["servicePrincipalName"]) == 3)
+        self.assertTrue("HOST/testname2" in res[0]["servicePrincipalName"])
+        self.assertTrue("HOST/testname3" in res[0]["servicePrincipalName"])
+        self.assertTrue("HOST/testname2.testdom" in res[0]["servicePrincipalName"])
+
+        m = Message()
+        m.dn = Dn(ldb, "cn=ldaptestcomputer,cn=computers," + self.base_dn)
+        m["dNSHostName"] = MessageElement("testname3.testdom",
+                                          FLAG_MOD_REPLACE, "dNSHostName")
+        m["servicePrincipalName"] = MessageElement("HOST/testname3.testdom",
+                                                   FLAG_MOD_ADD, "servicePrincipalName")
+        ldb.modify(m)
+
+        res = ldb.search("cn=ldaptestcomputer,cn=computers," + self.base_dn,
+                         scope=SCOPE_BASE, attrs=["dNSHostName", "sAMAccountName", "servicePrincipalName"])
+        self.assertTrue(len(res) == 1)
+        self.assertEquals(res[0]["dNSHostName"][0], "testname3.testdom")
+        self.assertEquals(res[0]["sAMAccountName"][0], "testname2$")
+        self.assertTrue(len(res[0]["servicePrincipalName"]) == 3)
+        self.assertTrue("HOST/testname2" in res[0]["servicePrincipalName"])
+        self.assertTrue("HOST/testname3" in res[0]["servicePrincipalName"])
+        self.assertTrue("HOST/testname3.testdom" in res[0]["servicePrincipalName"])
 
         delete_force(self.ldb, "cn=ldaptestcomputer,cn=computers," + self.base_dn)