[PATCHES v4] Fix GPO unapply log

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCHES v4] Fix GPO unapply log

Samba - samba-technical mailing list
And this time I'll actually attach the patches. Sorry about that.

--
David Mulder
SUSE Labs Software Engineer - Samba
[hidden email]
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)


fix_unapply.mbox (23K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Fix GPO unapply log

Samba - samba-technical mailing list
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.
>


patch.txt (38K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Fix GPO unapply log

Samba - samba-technical mailing list
GPO tests pass for me. Merged with the machine code (from the other set
of patches), those tests pass also. So Garming's changes are good with me.

On 12/14/2017 03:03 PM, Garming Sam wrote:

> 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.
>>

--
David Mulder
SUSE Labs Software Engineer - Samba
[hidden email]
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Fix GPO unapply log

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Dec 15, 2017 at 11:03:31AM +1300, Garming Sam via samba-technical wrote:
> 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?

LGTM. RB+ and pushed ! Thanks Garming.

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

> From de2d7fd4c39d2e154737b7e10466ad9bea116359 Mon Sep 17 00:00:00 2001
> From: Garming Sam <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 <[hidden email]>
> Date: Wed, 22 Nov 2017 10:58:55 +1300
> Subject: [PATCH 2/7] libgpo: typo credentaials -> credentials
>
> Signed-off-by: Garming Sam <[hidden email]>
> ---
>  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 <[hidden email]>
> Date: Wed, 22 Nov 2017 11:00:35 +1300
> Subject: [PATCH 3/7] libgpo: Tidy up some if statements
>
> Signed-off-by: Garming Sam <[hidden email]>
> ---
>  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 <[hidden email]>
> Date: Wed, 22 Nov 2017 11:00:56 +1300
> Subject: [PATCH 4/7] libgpo: Remedy some longer lines
>
> Signed-off-by: Garming Sam <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
> Reviewed-by: Garming Sam <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
> Reviewed-by: Garming Sam <[hidden email]>
> ---
>  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 <[hidden email]>
> Date: Wed, 6 Dec 2017 10:16:11 -0700
> Subject: [PATCH 7/7] gpo: Test that unapply works
>
> Signed-off-by: David Mulder <[hidden email]>
> Reviewed-by: Garming Sam <[hidden email]>
> ---
>  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
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Fix GPO unapply log

Samba - samba-technical mailing list
Hmm, went to merge, but I don't see the push at https://git.samba.org/samba

On 12/15/2017 12:00 PM, Jeremy Allison wrote:

> On Fri, Dec 15, 2017 at 11:03:31AM +1300, Garming Sam via samba-technical wrote:
>> 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?
> LGTM. RB+ and pushed ! Thanks Garming.
>
>> Garming
>>
>> On 14/12/17 08:00, David Mulder wrote:
>>> And this time I'll actually attach the patches. Sorry about that.
>>>
>> From de2d7fd4c39d2e154737b7e10466ad9bea116359 Mon Sep 17 00:00:00 2001
>> From: Garming Sam <[hidden email]>
>> 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 <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> Date: Wed, 22 Nov 2017 10:58:55 +1300
>> Subject: [PATCH 2/7] libgpo: typo credentaials -> credentials
>>
>> Signed-off-by: Garming Sam <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> Date: Wed, 22 Nov 2017 11:00:35 +1300
>> Subject: [PATCH 3/7] libgpo: Tidy up some if statements
>>
>> Signed-off-by: Garming Sam <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> Date: Wed, 22 Nov 2017 11:00:56 +1300
>> Subject: [PATCH 4/7] libgpo: Remedy some longer lines
>>
>> Signed-off-by: Garming Sam <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> 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 <[hidden email]>
>> Reviewed-by: Garming Sam <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> 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 <[hidden email]>
>> Reviewed-by: Garming Sam <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> Date: Wed, 6 Dec 2017 10:16:11 -0700
>> Subject: [PATCH 7/7] gpo: Test that unapply works
>>
>> Signed-off-by: David Mulder <[hidden email]>
>> Reviewed-by: Garming Sam <[hidden email]>
>> ---
>>  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
>>
>

--
David Mulder
SUSE Labs Software Engineer - Samba
[hidden email]
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Fix GPO unapply log

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Jeremy, maybe you meant to reply to the thread about Garming's schema
patches? I see those were just merged, but not the GPO patches.


On 12/15/2017 12:00 PM, Jeremy Allison wrote:

> On Fri, Dec 15, 2017 at 11:03:31AM +1300, Garming Sam via samba-technical wrote:
>> 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?
> LGTM. RB+ and pushed ! Thanks Garming.
>
>> Garming
>>
>> On 14/12/17 08:00, David Mulder wrote:
>>> And this time I'll actually attach the patches. Sorry about that.
>>>
>> From de2d7fd4c39d2e154737b7e10466ad9bea116359 Mon Sep 17 00:00:00 2001
>> From: Garming Sam <[hidden email]>
>> 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 <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> Date: Wed, 22 Nov 2017 10:58:55 +1300
>> Subject: [PATCH 2/7] libgpo: typo credentaials -> credentials
>>
>> Signed-off-by: Garming Sam <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> Date: Wed, 22 Nov 2017 11:00:35 +1300
>> Subject: [PATCH 3/7] libgpo: Tidy up some if statements
>>
>> Signed-off-by: Garming Sam <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> Date: Wed, 22 Nov 2017 11:00:56 +1300
>> Subject: [PATCH 4/7] libgpo: Remedy some longer lines
>>
>> Signed-off-by: Garming Sam <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> 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 <[hidden email]>
>> Reviewed-by: Garming Sam <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> 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 <[hidden email]>
>> Reviewed-by: Garming Sam <[hidden email]>
>> ---
>>  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 <[hidden email]>
>> Date: Wed, 6 Dec 2017 10:16:11 -0700
>> Subject: [PATCH 7/7] gpo: Test that unapply works
>>
>> Signed-off-by: David Mulder <[hidden email]>
>> Reviewed-by: Garming Sam <[hidden email]>
>> ---
>>  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
>>
>

--
David Mulder
SUSE Labs Software Engineer - Samba
[hidden email]
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Fix GPO unapply log

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Dec 15, 2017 at 12:29:35PM -0700, David Mulder via samba-technical wrote:
> Hmm, went to merge, but I don't see the push at https://git.samba.org/samba

It's in the autobuild queue, not done yet.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Fix GPO unapply log

Samba - samba-technical mailing list
Ah, I see.

On 12/15/2017 12:37 PM, Jeremy Allison wrote:
> On Fri, Dec 15, 2017 at 12:29:35PM -0700, David Mulder via samba-technical wrote:
>> Hmm, went to merge, but I don't see the push at https://git.samba.org/samba
> It's in the autobuild queue, not done yet.
>

--
David Mulder
SUSE Labs Software Engineer - Samba
[hidden email]
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)