[PATCHES v4] Fix GPO unapply log

Garming Sam garming at catalyst.net.nz
Thu Dec 14 22:03:31 UTC 2017


Hi,

I've marked my reviews and also added some tidy-up patches for some of
the earlier patches.

Can someone else please review and push?

Cheers,

Garming

On 14/12/17 08:00, David Mulder wrote:
> And this time I'll actually attach the patches. Sorry about that.
>

-------------- next part --------------
From de2d7fd4c39d2e154737b7e10466ad9bea116359 Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Wed, 22 Nov 2017 10:57:18 +1300
Subject: [PATCH 1/7] libgpo: Always check for ldap_server argument

Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
 libgpo/pygpo.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libgpo/pygpo.c b/libgpo/pygpo.c
index d7bb173..fba0e34 100644
--- a/libgpo/pygpo.c
+++ b/libgpo/pygpo.c
@@ -192,9 +192,11 @@ static int py_ads_init(ADS *self, PyObject *args, PyObject *kwds)
 	} else {
 		realm = lp_realm();
 		workgroup = lp_workgroup();
-		if (!ldap_server) return -1;
 	}
 
+	if (ldap_server == NULL) {
+		return -1;
+	}
 	if ( !(self->ads_ptr = ads_init(realm, workgroup, ldap_server)) )
 		return -1;
 
-- 
1.9.1


From 02fcbacdc683229eefc8669cf4d0a3d1bb926a6e Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Wed, 22 Nov 2017 10:58:55 +1300
Subject: [PATCH 2/7] libgpo: typo credentaials -> credentials

Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
 libgpo/pygpo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libgpo/pygpo.c b/libgpo/pygpo.c
index fba0e34..46063b4 100644
--- a/libgpo/pygpo.c
+++ b/libgpo/pygpo.c
@@ -166,7 +166,7 @@ static int py_ads_init(ADS *self, PyObject *args, PyObject *kwds)
 		if (!py_check_dcerpc_type(py_creds, "samba.credentials",
 					  "Credentials")) {
 			PyErr_Format(PyExc_TypeError,
-				     "Expected samba.credentaials "
+				     "Expected samba.credentials "
 				     "for credentials argument");
 			return -1;
 		}
-- 
1.9.1


From 448cfcc890b3caf717e5d60ca25ed4693df7fca1 Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Wed, 22 Nov 2017 11:00:35 +1300
Subject: [PATCH 3/7] libgpo: Tidy up some if statements

Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
 libgpo/pygpo.c | 48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/libgpo/pygpo.c b/libgpo/pygpo.c
index 46063b4..e94f0f0 100644
--- a/libgpo/pygpo.c
+++ b/libgpo/pygpo.c
@@ -159,8 +159,9 @@ static int py_ads_init(ADS *self, PyObject *args, PyObject *kwds)
 		"credentials", NULL};
 	if (!PyArg_ParseTupleAndKeywords(args, kwds, "sO|O",
 					 discard_const_p(char *, kwlist),
-					 &ldap_server, &lp_obj, &py_creds))
+					 &ldap_server, &lp_obj, &py_creds)) {
 		return -1;
+	}
 
 	if (py_creds) {
 		if (!py_check_dcerpc_type(py_creds, "samba.credentials",
@@ -197,8 +198,11 @@ static int py_ads_init(ADS *self, PyObject *args, PyObject *kwds)
 	if (ldap_server == NULL) {
 		return -1;
 	}
-	if ( !(self->ads_ptr = ads_init(realm, workgroup, ldap_server)) )
+
+	self->ads_ptr = ads_init(realm, workgroup, ldap_server);
+	if (self->ads_ptr == NULL) {
 		return -1;
+	}
 
 	return 0;
 }
@@ -223,23 +227,28 @@ static PyObject* py_ads_connect(ADS *self)
 			Py_RETURN_FALSE;
 		}
 	} else {
-		char *passwd;
-
-		if (asprintf(&(self->ads_ptr->auth.user_name), "%s$",
-			     lp_netbios_name()) == -1) {
-			PyErr_SetString(PyExc_SystemError, "Failed to asprintf");
+		char *passwd = NULL;
+		int ret = asprintf(&(self->ads_ptr->auth.user_name), "%s$",
+				   lp_netbios_name());
+		if (ret == -1) {
+			PyErr_SetString(PyExc_SystemError,
+					"Failed to asprintf");
 			TALLOC_FREE(frame);
 			Py_RETURN_FALSE;
-		} else
+		} else {
 			self->ads_ptr->auth.flags |= ADS_AUTH_USER_CREDS;
+		}
+
 		if (!secrets_init()) {
-			PyErr_SetString(PyExc_SystemError, "secrets_init() failed");
+			PyErr_SetString(PyExc_SystemError,
+					"secrets_init() failed");
 			TALLOC_FREE(frame);
 			Py_RETURN_FALSE;
 		}
-		if (!(passwd =
-		      secrets_fetch_machine_password(self->ads_ptr->server.workgroup,
-						     NULL, NULL))) {
+
+		passwd = secrets_fetch_machine_password(self->ads_ptr->server.workgroup,
+							NULL, NULL);
+		if (passwd == NULL) {
 			PyErr_SetString(PyExc_SystemError,
 					"Failed to fetch the machine account password");
 			TALLOC_FREE(frame);
@@ -346,7 +355,7 @@ static ADS_STATUS find_samaccount(ADS_STRUCT *ads, TALLOC_CTX *mem_ctx,
 
 	if (dn_ret) {
 		*dn_ret = talloc_strdup(mem_ctx, dn);
-		if (!*dn_ret) {
+		if (*dn_ret == NULL) {
 			status = ADS_ERROR(LDAP_NO_MEMORY);
 			goto out;
 		}
@@ -473,18 +482,25 @@ void initgpo(void)
 	PyObject *m;
 
 	debug_setup_talloc_log();
+
 	/* Instantiate the types */
 	m = Py_InitModule3("gpo", py_gpo_methods, "libgpo python bindings");
-	if (m == NULL) return;
+	if (m == NULL) {
+		return;
+	}
+
 	PyModule_AddObject(m, "version",
 			   PyString_FromString(SAMBA_VERSION_STRING));
 
-	if (PyType_Ready(&ads_ADSType) < 0)
+	if (PyType_Ready(&ads_ADSType) < 0) {
 		return;
+	}
+
 	PyModule_AddObject(m, "ADS_STRUCT", (PyObject *)&ads_ADSType);
 
-	if (pytalloc_BaseObject_PyType_Ready(&GPOType) < 0)
+	if (pytalloc_BaseObject_PyType_Ready(&GPOType) < 0) {
 		return;
+	}
 
 	Py_INCREF((PyObject *)(void *)&GPOType);
 	PyModule_AddObject(m, "GROUP_POLICY_OBJECT",
-- 
1.9.1


From 0510b319fe8ac26c8eedc17806b0ca4d97f068e9 Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Wed, 22 Nov 2017 11:00:56 +1300
Subject: [PATCH 4/7] libgpo: Remedy some longer lines

Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
 libgpo/pygpo.c | 64 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/libgpo/pygpo.c b/libgpo/pygpo.c
index e94f0f0..7a02a0d 100644
--- a/libgpo/pygpo.c
+++ b/libgpo/pygpo.c
@@ -54,11 +54,14 @@ static PyGetSetDef GPO_setters[] = {
 		NULL},
 	{discard_const_p(char, "file_sys_path"), (getter)GPO_get_file_sys_path,
 		NULL, NULL, NULL},
-	{discard_const_p(char, "display_name"), (getter)GPO_get_display_name, NULL,
-		NULL, NULL},
-	{discard_const_p(char, "name"), (getter)GPO_get_name, NULL, NULL, NULL},
-	{discard_const_p(char, "link"), (getter)GPO_get_link, NULL, NULL, NULL},
-	{discard_const_p(char, "user_extensions"), (getter)GPO_get_user_extensions,
+	{discard_const_p(char, "display_name"), (getter)GPO_get_display_name,
+		NULL, NULL, NULL},
+	{discard_const_p(char, "name"), (getter)GPO_get_name, NULL, NULL,
+		NULL},
+	{discard_const_p(char, "link"), (getter)GPO_get_link, NULL, NULL,
+		NULL},
+	{discard_const_p(char, "user_extensions"),
+		(getter)GPO_get_user_extensions,
 		NULL, NULL, NULL},
 	{discard_const_p(char, "machine_extensions"),
 		(getter)GPO_get_machine_extensions, NULL, NULL, NULL},
@@ -81,7 +84,8 @@ static PyObject *py_gpo_get_unix_path(PyObject *self, PyObject *args,
 					 discard_const_p(char *, kwlist),
 					 &cache_dir)) {
 		PyErr_SetString(PyExc_SystemError,
-				"Failed to parse arguments to gpo_get_unix_path()");
+				"Failed to parse arguments to "
+				"gpo_get_unix_path()");
 		goto out;
 	}
 
@@ -113,7 +117,8 @@ out:
 }
 
 static PyMethodDef GPO_methods[] = {
-	{"get_unix_path", (PyCFunction)py_gpo_get_unix_path, METH_KEYWORDS, NULL },
+	{"get_unix_path", (PyCFunction)py_gpo_get_unix_path, METH_KEYWORDS,
+		NULL },
 	{NULL}
 };
 
@@ -155,8 +160,9 @@ static int py_ads_init(ADS *self, PyObject *args, PyObject *kwds)
 	PyObject *lp_obj = NULL;
 	struct loadparm_context *lp_ctx = NULL;
 
-	static const char *kwlist[] = {"ldap_server", "loadparm_context",
-		"credentials", NULL};
+	static const char *kwlist[] = {
+		"ldap_server", "loadparm_context", "credentials", NULL
+	};
 	if (!PyArg_ParseTupleAndKeywords(args, kwds, "sO|O",
 					 discard_const_p(char *, kwlist),
 					 &ldap_server, &lp_obj, &py_creds)) {
@@ -222,7 +228,8 @@ static PyObject* py_ads_connect(ADS *self)
 
 		status = ads_connect_user_creds(self->ads_ptr);
 		if (!ADS_ERR_OK(status)) {
-			PyErr_SetString(PyExc_SystemError, "ads_connect() failed");
+			PyErr_SetString(PyExc_SystemError,
+					"ads_connect() failed");
 			TALLOC_FREE(frame);
 			Py_RETURN_FALSE;
 		}
@@ -250,12 +257,14 @@ static PyObject* py_ads_connect(ADS *self)
 							NULL, NULL);
 		if (passwd == NULL) {
 			PyErr_SetString(PyExc_SystemError,
-					"Failed to fetch the machine account password");
+					"Failed to fetch the machine account "
+					"password");
 			TALLOC_FREE(frame);
 			Py_RETURN_FALSE;
 		}
 		self->ads_ptr->auth.password = smb_xstrdup(passwd);
-		self->ads_ptr->auth.realm = smb_xstrdup(self->ads_ptr->server.realm);
+		self->ads_ptr->auth.realm =
+			smb_xstrdup(self->ads_ptr->server.realm);
 		if (!strupper_m(self->ads_ptr->auth.realm)) {
 			PyErr_SetString(PyExc_SystemError, "Failed to strdup");
 			TALLOC_FREE(frame);
@@ -265,7 +274,8 @@ static PyObject* py_ads_connect(ADS *self)
 
 		status = ads_connect(self->ads_ptr);
 		if (!ADS_ERR_OK(status)) {
-			PyErr_SetString(PyExc_SystemError, "ads_connect() failed");
+			PyErr_SetString(PyExc_SystemError,
+					"ads_connect() failed");
 			TALLOC_FREE(frame);
 			SAFE_FREE(passwd);
 			Py_RETURN_FALSE;
@@ -320,14 +330,15 @@ static ADS_STATUS find_samaccount(ADS_STRUCT *ads, TALLOC_CTX *mem_ctx,
 	char *dn = NULL;
 	uint32_t uac = 0;
 
-	filter = talloc_asprintf(mem_ctx, "(sAMAccountName=%s)", samaccountname);
+	filter = talloc_asprintf(mem_ctx, "(sAMAccountName=%s)",
+				 samaccountname);
 	if (filter == NULL) {
 		status = ADS_ERROR_NT(NT_STATUS_NO_MEMORY);
 		goto out;
 	}
 
-	status = ads_do_search_all(ads, ads->config.bind_path, LDAP_SCOPE_SUBTREE,
-				   filter, attrs, &res);
+	status = ads_do_search_all(ads, ads->config.bind_path,
+				   LDAP_SCOPE_SUBTREE, filter, attrs, &res);
 
 	if (!ADS_ERR_OK(status)) {
 		goto out;
@@ -387,22 +398,27 @@ static PyObject *py_ads_get_gpo_list(ADS *self, PyObject *args, PyObject *kwds)
 					 discard_const_p(char *, kwlist),
 					 &samaccountname)) {
 		PyErr_SetString(PyExc_SystemError,
-				"Failed to parse arguments to py_ads_get_gpo_list()");
+				"Failed to parse arguments to "
+				"py_ads_get_gpo_list()");
 		goto out;
 	}
 
 	frame = talloc_stackframe();
 
-	status = find_samaccount(self->ads_ptr, frame, samaccountname, &uac, &dn);
+	status = find_samaccount(self->ads_ptr, frame,
+				 samaccountname, &uac, &dn);
 	if (!ADS_ERR_OK(status)) {
 		TALLOC_FREE(frame);
-		PyErr_SetString(PyExc_SystemError, "Failed to find samAccountName");
+		PyErr_SetString(PyExc_SystemError,
+				"Failed to find samAccountName");
 		goto out;
 	}
 
-	if (uac & UF_WORKSTATION_TRUST_ACCOUNT || uac & UF_SERVER_TRUST_ACCOUNT) {
+	if (uac & UF_WORKSTATION_TRUST_ACCOUNT ||
+	    uac & UF_SERVER_TRUST_ACCOUNT) {
 		flags |= GPO_LIST_FLAG_MACHINE;
-		status = gp_get_machine_token(self->ads_ptr, frame, dn, &token);
+		status = gp_get_machine_token(self->ads_ptr, frame, dn,
+					      &token);
 	} else {
 		status = ads_get_sid_token(self->ads_ptr, frame, dn, &token);
 	}
@@ -455,7 +471,8 @@ out:
 static PyMethodDef ADS_methods[] = {
 	{ "connect", (PyCFunction)py_ads_connect, METH_NOARGS,
 		"Connect to the LDAP server" },
-	{ "get_gpo_list", (PyCFunction)py_ads_get_gpo_list, METH_KEYWORDS, NULL },
+	{ "get_gpo_list", (PyCFunction)py_ads_get_gpo_list, METH_KEYWORDS,
+		NULL },
 	{ NULL }
 };
 
@@ -471,7 +488,8 @@ static PyTypeObject ads_ADSType = {
 };
 
 static PyMethodDef py_gpo_methods[] = {
-	{"gpo_get_sysvol_gpt_version", (PyCFunction) py_gpo_get_sysvol_gpt_version,
+	{"gpo_get_sysvol_gpt_version",
+		(PyCFunction)py_gpo_get_sysvol_gpt_version,
 		METH_VARARGS, NULL},
 	{NULL}
 };
-- 
1.9.1


From 52d380f8a09debf639573096ccadfdeb2a30adbd Mon Sep 17 00:00:00 2001
From: David Mulder <dmulder at suse.com>
Date: Mon, 20 Nov 2017 06:41:19 -0700
Subject: [PATCH 5/7] gpo: Fix the empty apply log

The apply log wasn't being saved, apparently the pointers to elements
of the tree were getting lost.

Signed-off-by: David Mulder <dmulder at suse.com>
Reviewed-by: Garming Sam <garming at catalyst.net.nz>
---
 python/samba/gpclass.py | 65 ++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/python/samba/gpclass.py b/python/samba/gpclass.py
index 5a0ca9f..780ef55 100644
--- a/python/samba/gpclass.py
+++ b/python/samba/gpclass.py
@@ -95,10 +95,11 @@ class gp_log:
             self.gpdb = etree.fromstring(db_log)
         else:
             self.gpdb = etree.Element('gp')
-        self.user = self.gpdb.find('user[@name="%s"]' % user)
-        if self.user is None:
-            self.user = etree.SubElement(self.gpdb, 'user')
-            self.user.attrib['name'] = user
+        self.user = user
+        user_obj = self.gpdb.find('user[@name="%s"]' % user)
+        if user_obj is None:
+            user_obj = etree.SubElement(self.gpdb, 'user')
+            user_obj.attrib['name'] = user
 
     def state(self, value):
         ''' Policy application state
@@ -113,7 +114,8 @@ class gp_log:
         '''
         # If we're enforcing, but we've unapplied, apply instead
         if value == GPOSTATE.ENFORCE:
-            apply_log = self.user.find('applylog')
+            user_obj = self.gpdb.find('user[@name="%s"]' % self.user)
+            apply_log = user_obj.find('applylog')
             if apply_log is None or len(apply_log) == 0:
                 self._state = GPOSTATE.APPLY
             else:
@@ -126,14 +128,16 @@ class gp_log:
         param guid          - guid value of the GPO from which we're applying
                               policy
         '''
-        self.guid = self.user.find('guid[@value="%s"]' % guid)
-        if self.guid is None:
-            self.guid = etree.SubElement(self.user, 'guid')
-            self.guid.attrib['value'] = guid
+        self.guid = guid
+        user_obj = self.gpdb.find('user[@name="%s"]' % self.user)
+        obj = user_obj.find('guid[@value="%s"]' % guid)
+        if obj is None:
+            obj = etree.SubElement(user_obj, 'guid')
+            obj.attrib['value'] = guid
         if self._state == GPOSTATE.APPLY:
-            apply_log = self.user.find('applylog')
+            apply_log = user_obj.find('applylog')
             if apply_log is None:
-                apply_log = etree.SubElement(self.user, 'applylog')
+                apply_log = etree.SubElement(user_obj, 'applylog')
             item = etree.SubElement(apply_log, 'guid')
             item.attrib['count'] = '%d' % (len(apply_log)-1)
             item.attrib['value'] = guid
@@ -145,14 +149,15 @@ class gp_log:
         Removes the GPO guid last added to the list, which is the most recently
         applied GPO.
         '''
-        apply_log = self.user.find('applylog')
+        user_obj = self.gpdb.find('user[@name="%s"]' % self.user)
+        apply_log = user_obj.find('applylog')
         if apply_log is not None:
             ret = apply_log.find('guid[@count="%d"]' % (len(apply_log)-1))
             if ret is not None:
                 apply_log.remove(ret)
                 return ret.attrib['value']
-            if len(apply_log) == 0 and apply_log in self.user:
-                self.user.remove(apply_log)
+            if len(apply_log) == 0 and apply_log in user_obj:
+                user_obj.remove(apply_log)
         return None
 
     def store(self, gp_ext_name, attribute, old_val):
@@ -164,10 +169,12 @@ class gp_log:
         '''
         if self._state == GPOSTATE.UNAPPLY or self._state == GPOSTATE.ENFORCE:
             return None
-        assert self.guid is not None, "gpo guid was not set"
-        ext = self.guid.find('gp_ext[@name="%s"]' % gp_ext_name)
+        user_obj = self.gpdb.find('user[@name="%s"]' % self.user)
+        guid_obj = user_obj.find('guid[@value="%s"]' % self.guid)
+        assert guid_obj is not None, "gpo guid was not set"
+        ext = guid_obj.find('gp_ext[@name="%s"]' % gp_ext_name)
         if ext is None:
-            ext = etree.SubElement(self.guid, 'gp_ext')
+            ext = etree.SubElement(guid_obj, 'gp_ext')
             ext.attrib['name'] = gp_ext_name
         attr = ext.find('attribute[@name="%s"]' % attribute)
         if attr is None:
@@ -182,8 +189,10 @@ class gp_log:
         return              - The value of the attribute prior to policy
                               application
         '''
-        assert self.guid is not None, "gpo guid was not set"
-        ext = self.guid.find('gp_ext[@name="%s"]' % gp_ext_name)
+        user_obj = self.gpdb.find('user[@name="%s"]' % self.user)
+        guid_obj = user_obj.find('guid[@value="%s"]' % self.guid)
+        assert guid_obj is not None, "gpo guid was not set"
+        ext = guid_obj.find('gp_ext[@name="%s"]' % gp_ext_name)
         if ext is not None:
             attr = ext.find('attribute[@name="%s"]' % attribute)
             if attr is not None:
@@ -198,12 +207,14 @@ class gp_log:
         return              - list of (attr, value, apply_func) tuples for
                               unapplying policy
         '''
-        assert self.guid is not None, "gpo guid was not set"
+        user_obj = self.gpdb.find('user[@name="%s"]' % self.user)
+        guid_obj = user_obj.find('guid[@value="%s"]' % self.guid)
+        assert guid_obj is not None, "gpo guid was not set"
         ret = []
         data_maps = {}
         for gp_ext in gp_extensions:
             data_maps.update(gp_ext.apply_map())
-        exts = self.guid.findall('gp_ext')
+        exts = guid_obj.findall('gp_ext')
         if exts is not None:
             for ext in exts:
                 ext_map = {val[0]: val[1] for (key, val) in \
@@ -220,21 +231,19 @@ class gp_log:
                               attribute
         param attribute     - attribute to remove
         '''
-        assert self.guid is not None, "gpo guid was not set"
-        ext = self.guid.find('gp_ext[@name="%s"]' % gp_ext_name)
+        user_obj = self.gpdb.find('user[@name="%s"]' % self.user)
+        guid_obj = user_obj.find('guid[@value="%s"]' % self.guid)
+        assert guid_obj is not None, "gpo guid was not set"
+        ext = guid_obj.find('gp_ext[@name="%s"]' % gp_ext_name)
         if ext is not None:
             attr = ext.find('attribute[@name="%s"]' % attribute)
             if attr is not None:
                 ext.remove(attr)
                 if len(ext) == 0:
-                    self.guid.remove(ext)
+                    guid_obj.remove(ext)
 
     def commit(self):
         ''' Write gp_log changes to disk '''
-        if len(self.guid) == 0 and self.guid in self.user:
-            self.user.remove(self.guid)
-        if len(self.user) == 0 and self.user in self.gpdb:
-            self.gpdb.remove(self.user)
         self.gpostore.store(self.username, etree.tostring(self.gpdb, 'utf-8'))
 
 class GPOStorage:
-- 
1.9.1


From c430d1207ca93fa29dcb1ca766a902bdfdd51322 Mon Sep 17 00:00:00 2001
From: David Mulder <dmulder at suse.com>
Date: Fri, 1 Dec 2017 11:18:55 -0700
Subject: [PATCH 6/7] gpo: Only commit the earliest change to the log

Otherwise we overwrite the original value,
leaving the setting tattooed on unapplied

Signed-off-by: David Mulder <dmulder at suse.com>
Reviewed-by: Garming Sam <garming at catalyst.net.nz>
---
 python/samba/gpclass.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/samba/gpclass.py b/python/samba/gpclass.py
index 780ef55..00330eb 100644
--- a/python/samba/gpclass.py
+++ b/python/samba/gpclass.py
@@ -180,7 +180,7 @@ class gp_log:
         if attr is None:
             attr = etree.SubElement(ext, 'attribute')
             attr.attrib['name'] = attribute
-        attr.text = old_val
+            attr.text = old_val
 
     def retrieve(self, gp_ext_name, attribute):
         ''' Retrieve a stored attribute from the gp_log
-- 
1.9.1


From aa012766749d89af6dd3b29ac8c9bb0fbb6579d7 Mon Sep 17 00:00:00 2001
From: David Mulder <dmulder at suse.com>
Date: Wed, 6 Dec 2017 10:16:11 -0700
Subject: [PATCH 7/7] gpo: Test that unapply works

Signed-off-by: David Mulder <dmulder at suse.com>
Reviewed-by: Garming Sam <garming at catalyst.net.nz>
---
 source4/scripting/bin/samba_gpoupdate |   4 +-
 source4/torture/gpo/apply.c           | 124 ++++++++++++++++++++++++++--------
 2 files changed, 99 insertions(+), 29 deletions(-)

diff --git a/source4/scripting/bin/samba_gpoupdate b/source4/scripting/bin/samba_gpoupdate
index 4585297..f593e47 100755
--- a/source4/scripting/bin/samba_gpoupdate
+++ b/source4/scripting/bin/samba_gpoupdate
@@ -36,7 +36,7 @@ from samba import smb
 import logging
 
 ''' Fetch the hostname of a writable DC '''
-def get_dc_hostname():
+def get_dc_hostname(creds, lp):
     net = Net(creds=creds, lp=lp)
     cldap_ret = net.finddc(domain=lp.get('realm'), flags=(nbt.NBT_SERVER_LDAP |
         nbt.NBT_SERVER_DS))
@@ -52,7 +52,7 @@ def get_gpo_list(dc_hostname, creds, lp):
 
 def apply_gp(lp, creds, test_ldb, logger, store, gp_extensions):
     gp_db = store.get_gplog(creds.get_username())
-    dc_hostname = get_dc_hostname()
+    dc_hostname = get_dc_hostname(creds, lp)
     try:
         conn =  smb.SMB(dc_hostname, 'sysvol', lp=lp, creds=creds)
     except:
diff --git a/source4/torture/gpo/apply.c b/source4/torture/gpo/apply.c
index 1d16834..88da0b1 100644
--- a/source4/torture/gpo/apply.c
+++ b/source4/torture/gpo/apply.c
@@ -40,13 +40,13 @@ struct torture_suite *gpo_apply_suite(TALLOC_CTX *ctx)
 	return suite;
 }
 
-static int exec_wait(const char **cmd)
+static int exec_wait(char **cmd)
 {
 	int ret;
 	pid_t pid = fork();
 	switch (pid) {
 		case 0:
-			execv(cmd[0], discard_const_p(char *, &(cmd[1])));
+			execv(cmd[0], &(cmd[1]));
 			ret = -1;
 			break;
 		case -1:
@@ -60,7 +60,7 @@ static int exec_wait(const char **cmd)
 	return ret;
 }
 
-static int unix2nttime(char *sval)
+static int unix2nttime(const char *sval)
 {
 	return (strtoll(sval, NULL, 10) * -1 / 60 / 60 / 24 / 10000000);
 }
@@ -80,6 +80,7 @@ PasswordComplexity = %d\n\
 
 bool torture_gpo_system_access_policies(struct torture_context *tctx)
 {
+	TALLOC_CTX *ctx = talloc_new(tctx);
 	int ret, vers = 0, i;
 	const char *sysvol_path = NULL, *gpo_dir = NULL;
 	const char *gpo_file = NULL, *gpt_file = NULL;
@@ -92,23 +93,25 @@ bool torture_gpo_system_access_policies(struct torture_context *tctx)
 		"pwdProperties",
 		NULL
 	};
-	const struct ldb_val *val;
 	FILE *fp = NULL;
 	const char **gpo_update_cmd;
+	char **gpo_unapply_cmd;
 	int minpwdcases[] = { 0, 1, 998 };
 	int maxpwdcases[] = { 0, 1, 999 };
 	int pwdlencases[] = { 0, 1, 14 };
 	int pwdpropcases[] = { 0, 1, 1 };
 	struct ldb_message *old_message = NULL;
+	const char **itr;
+	int gpo_update_len = 0;
 
 	sysvol_path = lpcfg_path(lpcfg_service(tctx->lp_ctx, "sysvol"),
 				 lpcfg_default_service(tctx->lp_ctx), tctx);
 	torture_assert(tctx, sysvol_path, "Failed to fetch the sysvol path");
 
 	/* Ensure the sysvol path exists */
-	gpo_dir = talloc_asprintf(tctx, "%s/%s", sysvol_path, GPODIR);
+	gpo_dir = talloc_asprintf(ctx, "%s/%s", sysvol_path, GPODIR);
 	mkdir_p(gpo_dir, S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);
-	gpo_file = talloc_asprintf(tctx, "%s/%s", gpo_dir, GPOFILE);
+	gpo_file = talloc_asprintf(ctx, "%s/%s", gpo_dir, GPOFILE);
 
 	/* Get the gpo update command */
 	gpo_update_cmd = lpcfg_gpo_update_command(tctx->lp_ctx);
@@ -116,11 +119,11 @@ bool torture_gpo_system_access_policies(struct torture_context *tctx)
 		       "Failed to fetch the gpo update command");
 
 	/* Open and read the samba db and store the initial password settings */
-	samdb = samdb_connect(tctx, tctx->ev, tctx->lp_ctx,
+	samdb = samdb_connect(ctx, tctx->ev, tctx->lp_ctx,
 			      system_session(tctx->lp_ctx), 0);
 	torture_assert(tctx, samdb, "Failed to connect to the samdb");
 
-	ret = ldb_search(samdb, tctx, &result, ldb_get_default_basedn(samdb),
+	ret = ldb_search(samdb, ctx, &result, ldb_get_default_basedn(samdb),
 			 LDB_SCOPE_BASE, attrs, NULL);
 	torture_assert(tctx, ret == LDB_SUCCESS && result->count == 1,
 		       "Searching the samdb failed");
@@ -130,14 +133,14 @@ bool torture_gpo_system_access_policies(struct torture_context *tctx)
 	for (i = 0; i < 3; i++) {
 		/* Write out the sysvol */
 		if ( (fp = fopen(gpo_file, "w")) ) {
-			fputs(talloc_asprintf(tctx, GPTTMPL, minpwdcases[i],
+			fputs(talloc_asprintf(ctx, GPTTMPL, minpwdcases[i],
 					      maxpwdcases[i], pwdlencases[i],
 					      pwdpropcases[i]), fp);
 			fclose(fp);
 		}
 
 		/* Update the version in the GPT.INI */
-		gpt_file = talloc_asprintf(tctx, "%s/%s", sysvol_path, GPTINI);
+		gpt_file = talloc_asprintf(ctx, "%s/%s", sysvol_path, GPTINI);
 		if ( (fp = fopen(gpt_file, "r")) ) {
 			char line[256];
 			while (fgets(line, 256, fp)) {
@@ -149,49 +152,116 @@ bool torture_gpo_system_access_policies(struct torture_context *tctx)
 			fclose(fp);
 		}
 		if ( (fp = fopen(gpt_file, "w")) ) {
-			char *data = talloc_asprintf(tctx, "[General]\nVersion=%d\n",
+			char *data = talloc_asprintf(ctx,
+						     "[General]\nVersion=%d\n",
 						     ++vers);
 			fputs(data, fp);
 			fclose(fp);
 		}
 
 		/* Run the gpo update command */
-		ret = exec_wait(gpo_update_cmd);
+		ret = exec_wait(discard_const_p(char *, gpo_update_cmd));
 		torture_assert(tctx, ret == 0,
 			       "Failed to execute the gpo update command");
 
-		ret = ldb_search(samdb, tctx, &result, ldb_get_default_basedn(samdb),
+		ret = ldb_search(samdb, ctx, &result,
+				 ldb_get_default_basedn(samdb),
 				 LDB_SCOPE_BASE, attrs, NULL);
 		torture_assert(tctx, ret == LDB_SUCCESS && result->count == 1,
 			       "Searching the samdb failed");
 
 		/* minPwdAge */
-		val = ldb_msg_find_ldb_val(result->msgs[0], attrs[0]);
-		torture_assert(tctx, unix2nttime((char*)val->data) == minpwdcases[i],
+		torture_assert_int_equal(tctx, unix2nttime(
+						ldb_msg_find_attr_as_string(
+							result->msgs[0],
+							attrs[0],
+							"")), minpwdcases[i],
 			       "The minPwdAge was not applied");
 
 		/* maxPwdAge */
-		val = ldb_msg_find_ldb_val(result->msgs[0], attrs[1]);
-		torture_assert(tctx, unix2nttime((char*)val->data) == maxpwdcases[i],
+		torture_assert_int_equal(tctx, unix2nttime(
+						ldb_msg_find_attr_as_string(
+							result->msgs[0],
+							attrs[1],
+							"")), maxpwdcases[i],
 			       "The maxPwdAge was not applied");
 
 		/* minPwdLength */
-		val = ldb_msg_find_ldb_val(result->msgs[0], attrs[2]);
-		torture_assert(tctx, atoi((char*)val->data) == pwdlencases[i],
-			       "The minPwdLength was not applied");
+		torture_assert_int_equal(tctx, ldb_msg_find_attr_as_int(
+							result->msgs[0],
+							attrs[2],
+							-1),
+					       pwdlencases[i],
+				"The minPwdLength was not applied");
 
 		/* pwdProperties */
-		val = ldb_msg_find_ldb_val(result->msgs[0], attrs[3]);
-		torture_assert(tctx, atoi((char*)val->data) == pwdpropcases[i],
+		torture_assert_int_equal(tctx, ldb_msg_find_attr_as_int(
+							result->msgs[0],
+							attrs[3],
+							-1),
+					       pwdpropcases[i],
 			       "The pwdProperties were not applied");
 	}
 
-	for (i = 0; i < old_message->num_elements; i++) {
-		old_message->elements[i].flags = LDB_FLAG_MOD_REPLACE;
+	/* Unapply the settings and verify they are removed */
+	for (itr = gpo_update_cmd; *itr != NULL; itr++) {
+		gpo_update_len++;
 	}
+	gpo_unapply_cmd = talloc_array(ctx, char*, gpo_update_len+2);
+	for (i = 0; i < gpo_update_len; i++) {
+		gpo_unapply_cmd[i] = talloc_strdup(gpo_unapply_cmd,
+						   gpo_update_cmd[i]);
+	}
+	gpo_unapply_cmd[i] = talloc_asprintf(gpo_unapply_cmd, "--unapply");
+	gpo_unapply_cmd[i+1] = NULL;
+	ret = exec_wait(gpo_unapply_cmd);
+	torture_assert(tctx, ret == 0,
+		       "Failed to execute the gpo unapply command");
+	ret = ldb_search(samdb, ctx, &result, ldb_get_default_basedn(samdb),
+			 LDB_SCOPE_BASE, attrs, NULL);
+	torture_assert(tctx, ret == LDB_SUCCESS && result->count == 1,
+		       "Searching the samdb failed");
+	/* minPwdAge */
+	torture_assert_int_equal(tctx, unix2nttime(ldb_msg_find_attr_as_string(
+						result->msgs[0],
+						attrs[0],
+						"")),
+		       unix2nttime(ldb_msg_find_attr_as_string(old_message,
+							       attrs[0],
+							       "")
+				  ),
+		       "The minPwdAge was not unapplied");
+	/* maxPwdAge */
+	torture_assert_int_equal(tctx, unix2nttime(ldb_msg_find_attr_as_string(
+						result->msgs[0],
+						attrs[1],
+						"")),
+		       unix2nttime(ldb_msg_find_attr_as_string(old_message,
+							       attrs[1],
+							       "")
+				  ),
+		       "The maxPwdAge was not unapplied");
+	/* minPwdLength */
+	torture_assert_int_equal(tctx, ldb_msg_find_attr_as_int(
+						result->msgs[0],
+						attrs[2],
+						-1),
+				       ldb_msg_find_attr_as_int(
+						old_message,
+						attrs[2],
+						-2),
+			"The minPwdLength was not unapplied");
+	/* pwdProperties */
+	torture_assert_int_equal(tctx, ldb_msg_find_attr_as_int(
+						result->msgs[0],
+						attrs[3],
+						-1),
+					ldb_msg_find_attr_as_int(
+						old_message,
+						attrs[3],
+						-2),
+			"The pwdProperties were not unapplied");
 
-	ret = ldb_modify(samdb, old_message);
-	torture_assert(tctx, ret == 0, "Failed to reset password settings.");
-
+	talloc_free(ctx);
 	return true;
 }
-- 
1.9.1



More information about the samba-technical mailing list