[PATCHES] simple gpo patches fixes

[David Mulder via samba-technical]

Here I've separated the fixes I need in for my sambaXP presentation.
There is NO new functionality here, just some minor fixes and
reorganization of sources. The reorganization makes if possible to
inherit correctly and implement new gpos. This includes a revision of my
gpt version patch, which now keeps the functionality *unchanged* for the
kdc, and will only apply to new gpos implemented for client machines (of
which there are currently no gpos).

 python/samba/gp_sec_ext.py            | 136 ++++++++++++++++++++++++++++
 python/samba/gpclass.py               | 291
+++++++++++++++++++++++++++--------------------------------
 source4/scripting/bin/samba_gpoupdate |  79 +---------------
 3 files changed, 269 insertions(+), 237 deletions(-)

[David Mulder via samba-technical]

I've attached a new set of patches that use lpcfg_cache_path() to ensure
the cache directory exists with the correct permissions.

Hi David,
Yes, that seems fine. The rest of the code internally seems to do much
the same with 0755 for any subdirectories. Neither xx_path nor the
lpcfg_common_path do the right thing in either case and it doesn't
seem to be worth changing that behaviour. I was mostly just concerned
about the top level cache path.
Cheers,
Garming

[Garming Sam via samba-technical]

Can you put braces around this? Looks like the whole file is full of
this (and other coding style violations), I'll perhaps try and fix the
rest of them later myself.

+ if (!PyArg_ParseTuple(args, "s", &name))
+ return NULL;


Otherwise,

Reviewed-by: Garming Sam <***@catalyst.net.nz>

Can someone else please review this as well?


Cheers.

Post by David Mulder via samba-technical
I've attached a new set of patches that use lpcfg_cache_path() to ensure
the cache directory exists with the correct permissions.

Hi David,
Yes, that seems fine. The rest of the code internally seems to do much
the same with 0755 for any subdirectories. Neither xx_path nor the
lpcfg_common_path do the right thing in either case and it doesn't
seem to be worth changing that behaviour. I was mostly just concerned
about the top level cache path.
Cheers,
Garming

[David Mulder via samba-technical]

Oh, shoot. I know better than to do that (I was lecturing someone else
about this recently).
I just copy and pasted from another function in that file without
thinking. I'll definitely get that fixed.

Post by Garming Sam via samba-technical
Can you put braces around this? Looks like the whole file is full of
this (and other coding style violations), I'll perhaps try and fix the
rest of them later myself.
+ if (!PyArg_ParseTuple(args, "s", &name))
+ return NULL;
Otherwise,
Can someone else please review this as well?
Cheers.

Post by David Mulder via samba-technical
I've attached a new set of patches that use lpcfg_cache_path() to ensure
the cache directory exists with the correct permissions.

Hi David,
Yes, that seems fine. The rest of the code internally seems to do much
the same with 0755 for any subdirectories. Neither xx_path nor the
lpcfg_common_path do the right thing in either case and it doesn't
seem to be worth changing that behaviour. I was mostly just concerned
about the top level cache path.
Cheers,
Garming

[David Mulder via samba-technical]

Fixed.

 python/samba/gp_sec_ext.py            | 136 +++++++++++++++++++++++++++
 python/samba/gpclass.py               | 293
+++++++++++++++++++++++++++--------------------------------
 source4/param/pyparam.c               |  19 ++++
 source4/scripting/bin/samba_gpoupdate |  79 +---------------
 4 files changed, 290 insertions(+), 237 deletions(-)

Post by David Mulder via samba-technical
Oh, shoot. I know better than to do that (I was lecturing someone else
about this recently).
I just copy and pasted from another function in that file without
thinking. I'll definitely get that fixed.

Post by Garming Sam via samba-technical
Can you put braces around this? Looks like the whole file is full of
this (and other coding style violations), I'll perhaps try and fix the
rest of them later myself.
+ if (!PyArg_ParseTuple(args, "s", &name))
+ return NULL;
Otherwise,
Can someone else please review this as well?
Cheers.

Post by David Mulder via samba-technical
I've attached a new set of patches that use lpcfg_cache_path() to ensure
the cache directory exists with the correct permissions.

Hi David,
Yes, that seems fine. The rest of the code internally seems to do much
the same with 0755 for any subdirectories. Neither xx_path nor the
lpcfg_common_path do the right thing in either case and it doesn't
seem to be worth changing that behaviour. I was mostly just concerned
about the top level cache path.
Cheers,
Garming

[Douglas Bagnall via samba-technical]

From 89cef25f9343fad2d11b161bd89281f9633e1d08 Mon Sep 17 00:00:00 2001
Date: Mon, 12 Mar 2018 09:44:38 -0600
Subject: [PATCH 1/2] gpo: Separate logic from implementation in gpclass
This patch does not change functionality, it only
rearranges sources to make it possible to add new
gpo extensions. The previous logic has the
gp_sec_ext tightly coupled with the parent gp_ext
class. This patch decouples that relationship.
Group Policy extensions can now be added as a
single file which will import parent functionality
from the samba.gpclass module. The gp_sec_ext has
been modified this way.

In general it is better to structure these kinds of changes as a few
well defined steps. That makes it easier to see that each step is
leaving things logically as they were. By my count, you do six things
in this commit:

1. rename inf_to to file_to
2. shift an __init__ definition up the class hierarchy.
3. shift some classes into the new module
4. shift some functions from the script into the gpclass module
5. change formatting in one or two places.
6. something to do with a subclassable read method.

It would be better to do these in six commits.

---
python/samba/gp_sec_ext.py | 136 +++++++++++++++++
python/samba/gpclass.py | 277 ++++++++++++++--------------------
source4/scripting/bin/samba_gpoupdate | 79 +---------
3 files changed, 255 insertions(+), 237 deletions(-)
create mode 100644 python/samba/gp_sec_ext.py
diff --git a/python/samba/gp_sec_ext.py b/python/samba/gp_sec_ext.py
new file mode 100644
index 00000000000..b50f1fb3f3e
--- /dev/null
+++ b/python/samba/gp_sec_ext.py
@@ -0,0 +1,136 @@
+import os.path

This should start with a a copyright/GPL blurb, along the lines of
gpclass.py and all the others.

+from gpclass import file_to, gp_inf_ext
+
+ return '%d' % (int(self.val)/60)
+
+ return '%d' % (int(self.val)*24)
+
+ old_val = self.gp_db.gpostore.get(self.attribute)
+ self.logger.info('%s was changed from %s to %s' % (self.attribute,
+ old_val, val))
+ self.gp_db.gpostore.store(self.attribute, val)
+ self.gp_db.store(str(self), self.attribute, old_val)
+ self.gp_db.gpostore.delete(self.attribute)
+ self.gp_db.delete(str(self), self.attribute)
+
+ return { 'kdc:user_ticket_lifetime': (self.set_kdc_tdb, self.explicit),
+ 'kdc:service_ticket_lifetime': (self.set_kdc_tdb,
+ self.mins_to_hours),
+ 'kdc:renewal_lifetime': (self.set_kdc_tdb,
+ self.days_to_hours),
+ }
+
+ return 'Kerberos Policy'
+
+ '''This class takes the .inf file parameter (essentially a GPO file mapped
+ to a GUID), hashmaps it to the Samba parameter, which then uses an ldb
+ object to update the parameter to Samba4. Not registry oriented whatsoever.
+ '''
+
+ old_val = self.ldb.get_minPwdAge()
+ self.logger.info('KDC Minimum Password age was changed from %s to %s' \
+ % (old_val, val))
+ self.gp_db.store(str(self), self.attribute, old_val)
+ self.ldb.set_minPwdAge(val)
+
+ old_val = self.ldb.get_maxPwdAge()
+ self.logger.info('KDC Maximum Password age was changed from %s to %s' \
+ % (old_val, val))
+ self.gp_db.store(str(self), self.attribute, old_val)
+ self.ldb.set_maxPwdAge(val)
+
+ old_val = self.ldb.get_minPwdLength()
+ self.logger.info(
+ 'KDC Minimum Password length was changed from %s to %s' \
+ % (old_val, val))
+ self.gp_db.store(str(self), self.attribute, old_val)
+ self.ldb.set_minPwdLength(val)
+
+ old_val = self.ldb.get_pwdProperties()
+ self.logger.info('KDC Password Properties were changed from %s to %s' \
+ % (old_val, val))
+ self.gp_db.store(str(self), self.attribute, old_val)
+ self.ldb.set_pwdProperties(val)
+
+ seconds = 60
+ minutes = 60
+ hours = 24
+ sam_add = 10000000
+ val = (self.val)
+ val = int(val)
+ return str(-(val * seconds * minutes * hours * sam_add))
+
+ '''ldap value : samba setter'''
+ return { "minPwdAge" : (self.ch_minPwdAge, self.days2rel_nttime),
+ "maxPwdAge" : (self.ch_maxPwdAge, self.days2rel_nttime),
+ # Could be none, but I like the method assignment in
+ # update_samba
+ "minPwdLength" : (self.ch_minPwdLength, self.explicit),
+ "pwdProperties" : (self.ch_pwdProperties, self.explicit),
+
+ }
+
+ return 'System Access'
+
+ 1) Identifies the GPO if it has a certain kind of filepath,
+ 2) Finally parses it.
+ '''
+
+ count = 0
+
+ return "Security GPO extension"
+
+ return os.path.join(rootpath,
+ "MACHINE/Microsoft/Windows NT/SecEdit/GptTmpl.inf")
+
+ return os.path.join(rootpath, "Machine/Registry.pol")
+
+ return os.path.join(rootpath, "User/Registry.pol")
+
+ return {"System Access": {"MinimumPasswordAge": ("minPwdAge",
+ inf_to_ldb),
+ "MaximumPasswordAge": ("maxPwdAge",
+ inf_to_ldb),
+ "MinimumPasswordLength": ("minPwdLength",
+ inf_to_ldb),
+ "PasswordComplexity": ("pwdProperties",
+ inf_to_ldb),
+ },
+ "Kerberos Policy": {"MaxTicketAge": (
+ "kdc:user_ticket_lifetime",
+ inf_to_kdc_tdb
+ ),
+ "MaxServiceAge": (
+ "kdc:service_ticket_lifetime",
+ inf_to_kdc_tdb
+ ),
+ "MaxRenewAge": (
+ "kdc:renewal_lifetime",
+ inf_to_kdc_tdb
+ ),
+ }
+ }
+
diff --git a/python/samba/gpclass.py b/python/samba/gpclass.py
index 33c9001cb6d..6888a18999a 100644
--- a/python/samba/gpclass.py
+++ b/python/samba/gpclass.py
@@ -25,6 +25,11 @@ from StringIO import StringIO
from abc import ABCMeta, abstractmethod
import xml.etree.ElementTree as etree
import re
+from samba.net import Net
+from samba.dcerpc import nbt
+from samba import smb
+import samba.gpo as gpo
+import chardet
from enum import Enum
__metaclass__ = ABCMeta
+ self.logger = logger
+
@abstractmethod
pass
pass
@abstractmethod
pass
+ self.ldb = ldb
+ self.gp_db = gp_db
+ self.lp = lp
+
+ # Fixing the bug where only some Linux Boxes capitalize MACHINE
+ blist = afile.split('/')
+ idx = afile.lower().split('/').index('machine')
+ for case in [
+ blist[idx].upper(),
+ blist[idx].capitalize(),
+ blist[idx].lower()
+ bfile = '/'.join(blist[:idx]) + '/' + case + '/' + \
+ '/'.join(blist[idx+1:])
+ return self.read(conn.loadfile(bfile.replace('/', '\\')))
+ continue
+ return self.read(conn.loadfile(afile.replace('/', '\\')))
+ self.logger.error(str(e))
+ return None
+
@abstractmethod
pass
__metaclass__ = ABCMeta
return self.val
- (upd_sam, value) = self.mapper().get(self.attribute)
+ (upd_sam, value) = self.mapper()[self.attribute]
upd_sam(value())
@abstractmethod
pass
- return '%d' % (int(self.val)/60)
-
- return '%d' % (int(self.val)*24)
-
- old_val = self.gp_db.gpostore.get(self.attribute)
- self.logger.info('%s was changed from %s to %s' % (self.attribute,
- old_val, val))
- self.gp_db.gpostore.store(self.attribute, val)
- self.gp_db.store(str(self), self.attribute, old_val)
- self.gp_db.gpostore.delete(self.attribute)
- self.gp_db.delete(str(self), self.attribute)
-
- return { 'kdc:user_ticket_lifetime': (self.set_kdc_tdb, self.explicit),
- 'kdc:service_ticket_lifetime': (self.set_kdc_tdb,
- self.mins_to_hours),
- 'kdc:renewal_lifetime': (self.set_kdc_tdb,
- self.days_to_hours),
- }
-
- return 'Kerberos Policy'
-
- '''This class takes the .inf file parameter (essentially a GPO file mapped
- to a GUID), hashmaps it to the Samba parameter, which then uses an ldb
- object to update the parameter to Samba4. Not registry oriented whatsoever.
- '''
-
- old_val = self.ldb.get_minPwdAge()
- self.logger.info('KDC Minimum Password age was changed from %s to %s' \
- % (old_val, val))
- self.gp_db.store(str(self), self.attribute, old_val)
- self.ldb.set_minPwdAge(val)
-
- old_val = self.ldb.get_maxPwdAge()
- self.logger.info('KDC Maximum Password age was changed from %s to %s' \
- % (old_val, val))
- self.gp_db.store(str(self), self.attribute, old_val)
- self.ldb.set_maxPwdAge(val)
-
- old_val = self.ldb.get_minPwdLength()
- self.logger.info(
- 'KDC Minimum Password length was changed from %s to %s' \
- % (old_val, val))
- self.gp_db.store(str(self), self.attribute, old_val)
- self.ldb.set_minPwdLength(val)
-
- old_val = self.ldb.get_pwdProperties()
- self.logger.info('KDC Password Properties were changed from %s to %s' \
- % (old_val, val))
- self.gp_db.store(str(self), self.attribute, old_val)
- self.ldb.set_pwdProperties(val)
-
- seconds = 60
- minutes = 60
- hours = 24
- sam_add = 10000000
- val = (self.val)
- val = int(val)
- return str(-(val * seconds * minutes * hours * sam_add))
-
- '''ldap value : samba setter'''
- return { "minPwdAge" : (self.ch_minPwdAge, self.days2rel_nttime),
- "maxPwdAge" : (self.ch_maxPwdAge, self.days2rel_nttime),
- # Could be none, but I like the method assignment in
- # update_samba
- "minPwdLength" : (self.ch_minPwdLength, self.explicit),
- "pwdProperties" : (self.ch_pwdProperties, self.explicit),
-
- }
-
- return 'System Access'
-
-
- 1) Identifies the GPO if it has a certain kind of filepath,
- 2) Finally parses it.
- '''
-
- count = 0
-
- self.logger = logger
-
- return "Security GPO extension"
-
- return os.path.join(rootpath,
- "MACHINE/Microsoft/Windows NT/SecEdit/GptTmpl.inf")
-
- return os.path.join(rootpath, "Machine/Registry.pol")
-
- return os.path.join(rootpath, "User/Registry.pol")
+ pass
- return {"System Access": {"MinimumPasswordAge": ("minPwdAge",
- inf_to_ldb),
- "MaximumPasswordAge": ("maxPwdAge",
- inf_to_ldb),
- "MinimumPasswordLength": ("minPwdLength",
- inf_to_ldb),
- "PasswordComplexity": ("pwdProperties",
- inf_to_ldb),
- },
- "Kerberos Policy": {"MaxTicketAge": (
- "kdc:user_ticket_lifetime",
- inf_to_kdc_tdb
- ),
- "MaxServiceAge": (
- "kdc:service_ticket_lifetime",
- inf_to_kdc_tdb
- ),
- "MaxRenewAge": (
- "kdc:renewal_lifetime",
- inf_to_kdc_tdb
- ),
- }
- }
-
+ pass
+
ret = False
inftable = self.apply_map()
- policy = conn.loadfile(path.replace('/', '\\'))
current_section = None
# So here we would declare a boolean,
self.gp_db.commit()
return ret
- self.ldb = ldb
- self.gp_db = gp_db
- self.lp = lp
+ pass
- # Fixing the bug where only some Linux Boxes capitalize MACHINE
+''' Fetch the hostname of a writable DC '''
+ net = Net(creds=creds, lp=lp)
+ cldap_ret = net.finddc(domain=lp.get('realm'), flags=(nbt.NBT_SERVER_LDAP |
+ nbt.NBT_SERVER_DS))
+ return cldap_ret.pdc_dns_name
+
+''' Fetch a list of GUIDs for applicable GPOs '''
+ gpos = []
+ ads = gpo.ADS_STRUCT(dc_hostname, lp, creds)
+ gpos = ads.get_gpo_list(creds.get_username())
+ return gpos
+
+ gp_db = store.get_gplog(creds.get_username())
+ dc_hostname = get_dc_hostname(creds, lp)
+ conn = smb.SMB(dc_hostname, 'sysvol', lp=lp, creds=creds)
+ logger.error('Error connecting to \'%s\' using SMB' % dc_hostname)
+ raise
+ gpos = get_gpo_list(dc_hostname, creds, lp)
+
+ guid = gpo_obj.name
+ continue
+ path = os.path.join(lp.get('realm').lower(), 'Policies', guid)
+ local_path = os.path.join(lp.get("path", "sysvol"), path)
+ version = int(gpo.gpo_get_sysvol_gpt_version(local_path)[1])
+ logger.info('GPO %s has changed' % guid)
+ gp_db.state(GPOSTATE.APPLY)
+ gp_db.state(GPOSTATE.ENFORCE)
+ gp_db.set_guid(guid)
+ store.start()
- blist = afile.split('/')
- idx = afile.lower().split('/').index('machine')
- for case in [blist[idx].upper(), blist[idx].capitalize(),
- bfile = '/'.join(blist[:idx]) + '/' + case + '/' + \
- '/'.join(blist[idx+1:])
- return self.read_inf(bfile, conn)
- continue
- return self.read_inf(afile, conn)
- return None
+ ext.parse(ext.list(path), test_ldb, conn, gp_db, lp)
+ logger.error('Failed to parse gpo %s for extension %s' % \
+ (guid, str(ext)))
+ logger.error('Message was: ' + str(e))
+ store.cancel()
+ continue
+ store.store(guid, '%i' % version)
+ store.commit()
+
+ item = gp_db.apply_log_pop()
+ yield item
+ break
+
+ gp_db = store.get_gplog(creds.get_username())
+ gp_db.state(GPOSTATE.UNAPPLY)
+ gp_db.set_guid(gpo_guid)
+ unapply_attributes = gp_db.list(gp_extensions)
+ attr_obj = attr[-1](logger, test_ldb, gp_db, lp, attr[0], attr[1])
+ attr_obj.mapper()[attr[0]][0](attr[1]) # Set the old value
+ gp_db.delete(str(attr_obj), attr[0])
+ gp_db.commit()
diff --git a/source4/scripting/bin/samba_gpoupdate b/source4/scripting/bin/samba_gpoupdate
index 26e0984413e..89b3ed77616 100755
--- a/source4/scripting/bin/samba_gpoupdate
+++ b/source4/scripting/bin/samba_gpoupdate
from samba.samdb import SamDB
SamDB = None
-from samba.gpclass import *
-from samba.net import Net
-from samba.dcerpc import nbt
-from samba import smb
-import samba.gpo as gpo
+from samba.gpclass import apply_gp, unapply_gp, GPOStorage
+from samba.gp_sec_ext import gp_sec_ext
import logging
-import chardet
-
-''' Fetch the hostname of a writable DC '''
- net = Net(creds=creds, lp=lp)
- cldap_ret = net.finddc(domain=lp.get('realm'), flags=(nbt.NBT_SERVER_LDAP |
- nbt.NBT_SERVER_DS))
- return cldap_ret.pdc_dns_name
-
-''' Fetch a list of GUIDs for applicable GPOs '''
- gpos = []
- ads = gpo.ADS_STRUCT(dc_hostname, lp, creds)
- gpos = ads.get_gpo_list(creds.get_username())
- return gpos
-
- gp_db = store.get_gplog(creds.get_username())
- dc_hostname = get_dc_hostname(creds, lp)
- conn = smb.SMB(dc_hostname, 'sysvol', lp=lp, creds=creds)
- logger.error('Error connecting to \'%s\' using SMB' % dc_hostname)
- raise
- gpos = get_gpo_list(dc_hostname, creds, lp)
-
- guid = gpo_obj.name
- continue
- path = os.path.join(lp.get('realm').lower(), 'Policies', guid)
- local_path = os.path.join(lp.get("path", "sysvol"), path)
- version = int(gpo.gpo_get_sysvol_gpt_version(local_path)[1])
- logger.info('GPO %s has changed' % guid)
- gp_db.state(GPOSTATE.APPLY)
- gp_db.state(GPOSTATE.ENFORCE)
- gp_db.set_guid(guid)
- store.start()
- ext.parse(ext.list(path), test_ldb, conn, gp_db, lp)
- logger.error('Failed to parse gpo %s for extension %s' % \
- (guid, str(ext)))
- logger.error('Message was: ' + str(e))
- store.cancel()
- continue
- store.store(guid, '%i' % version)
- store.commit()
-
- item = gp_db.apply_log_pop()
- yield item
- break
-
- gp_db = store.get_gplog(creds.get_username())
- gp_db.state(GPOSTATE.UNAPPLY)
- gp_db.set_guid(gpo_guid)
- unapply_attributes = gp_db.list(gp_extensions)
- attr_obj = attr[-1](logger, test_ldb, gp_db, lp, attr[0], attr[1])
- attr_obj.mapper()[attr[0]][0](attr[1]) # Set the old value
- gp_db.delete(str(attr_obj), attr[0])
- gp_db.commit()
parser = optparse.OptionParser('samba_gpoupdate [options]')
--
2.13.6
From 47e58fcf5b97dcb7c1004420af11231b1220cc99 Mon Sep 17 00:00:00 2001
Date: Mon, 8 Jan 2018 07:17:29 -0700
Subject: [PATCH 2/2] gpo: Read GPO versions locally, not from sysvol
This patch does not change current functionality
for the kdc. Non-kdc clients cannot read directly
from the sysvol, so we need to store the GPT.INI
file locally to read each gpo version.
---
python/samba/gpclass.py | 20 ++++++++++++++++++--
source4/param/pyparam.c | 19 +++++++++++++++++++
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/python/samba/gpclass.py b/python/samba/gpclass.py
index 6888a18999a..2678069090a 100644
--- a/python/samba/gpclass.py
+++ b/python/samba/gpclass.py
gpos = ads.get_gpo_list(creds.get_username())
return gpos
+ # gpo.gpo_get_sysvol_gpt_version() reads the GPT.INI from a local file.
+ # If we don't have a sysvol path locally (if we're not a kdc), then
+ # store the file locally here so it can be read on a client.
+ sysvol = lp.get("path", "sysvol")
+ local_path = os.path.join(sysvol, path, 'GPT.INI')
+ gpt_path = lp.cache_path(os.path.join('gpt', path))
+ local_path = os.path.join(gpt_path, 'GPT.INI')
+ os.makedirs(os.path.dirname(local_path), 0o700)
+ data = conn.loadfile(os.path.join(path, 'GPT.INI').replace('/', '\\'))
+ encoding = chardet.detect(data)
+ open(local_path, 'w').write(data.decode(encoding['encoding']))
+ return int(gpo.gpo_get_sysvol_gpt_version(os.path.dirname(local_path))[1])
+
gp_db = store.get_gplog(creds.get_username())
dc_hostname = get_dc_hostname(creds, lp)
continue
path = os.path.join(lp.get('realm').lower(), 'Policies', guid)
- local_path = os.path.join(lp.get("path", "sysvol"), path)
- version = int(gpo.gpo_get_sysvol_gpt_version(local_path)[1])
+ version = gpo_version(lp, conn, path)
logger.info('GPO %s has changed' % guid)
gp_db.state(GPOSTATE.APPLY)
diff --git a/source4/param/pyparam.c b/source4/param/pyparam.c
index f16c2c0b227..1d99ada09da 100644
--- a/source4/param/pyparam.c
+++ b/source4/param/pyparam.c
@@ -358,6 +358,22 @@ static PyObject *py_samdb_url(PyObject *self, PyObject *unused)
return PyStr_FromFormat("tdb://%s/sam.ldb", lpcfg_private_dir(lp_ctx));
}
+static PyObject *py_cache_path(PyObject *self, PyObject *args)
+{
+ struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
+ char *name, *path;
+ PyObject *ret;
+
+ if (!PyArg_ParseTuple(args, "s", &name)) {
+ return NULL;
+ }
+
+ path = lpcfg_cache_path(NULL, lp_ctx, name);
+ ret = PyStr_FromString(path);

There are many reasons lpcfg_cache_path() can return NULL, which
PyString_FromString() can't cope with, so you need to check.

Also, in Python 3, is a string what you want? It means the path has to
be utf-8 -- is that something we know? I don't know what we normally
do here.

cheers,
Douglas

+ talloc_free(path);
+
+ return ret;
+}
static PyMethodDef py_lp_ctx_methods[] = {
{ "load", py_lp_ctx_load, METH_VARARGS,
@@ -394,6 +410,9 @@ static PyMethodDef py_lp_ctx_methods[] = {
{ "samdb_url", py_samdb_url, METH_NOARGS,
"S.samdb_url() -> string\n"
"Returns the current URL for sam.ldb." },
+ { "cache_path", py_cache_path, METH_VARARGS,
+ "S.cache_path(name) -> string\n"
+ "Returns a path in the Samba cache directory." },
{ NULL }
};
--
2.13.6

[David Mulder via samba-technical]

Thanks for the review!

Post by Douglas Bagnall via samba-technical

From 89cef25f9343fad2d11b161bd89281f9633e1d08 Mon Sep 17 00:00:00 2001
Date: Mon, 12 Mar 2018 09:44:38 -0600
Subject: [PATCH 1/2] gpo: Separate logic from implementation in gpclass
This patch does not change functionality, it only
rearranges sources to make it possible to add new
gpo extensions. The previous logic has the
gp_sec_ext tightly coupled with the parent gp_ext
class. This patch decouples that relationship.
Group Policy extensions can now be added as a
single file which will import parent functionality
from the samba.gpclass module. The gp_sec_ext has
been modified this way.

In general it is better to structure these kinds of changes as a few
well defined steps. That makes it easier to see that each step is
leaving things logically as they were. By my count, you do six things
1. rename inf_to to file_to
2. shift an __init__ definition up the class hierarchy.
3. shift some classes into the new module
4. shift some functions from the script into the gpclass module
5. change formatting in one or two places.
6. something to do with a subclassable read method.
It would be better to do these in six commits.

I've broken down that patch into sizable chunks. I've pushed them to the
git repo: https://github.com/samba-team/samba/pull/154

Post by Douglas Bagnall via samba-technical

---
python/samba/gp_sec_ext.py | 136 +++++++++++++++++
python/samba/gpclass.py | 277 ++++++++++++++--------------------
source4/scripting/bin/samba_gpoupdate | 79 +---------
3 files changed, 255 insertions(+), 237 deletions(-)
create mode 100644 python/samba/gp_sec_ext.py
diff --git a/python/samba/gp_sec_ext.py b/python/samba/gp_sec_ext.py
new file mode 100644
index 00000000000..b50f1fb3f3e
--- /dev/null
+++ b/python/samba/gp_sec_ext.py
@@ -0,0 +1,136 @@
+import os.path

This should start with a a copyright/GPL blurb, along the lines of
gpclass.py and all the others.

Added the copyright notice.

Post by Douglas Bagnall via samba-technical

+from gpclass import file_to, gp_inf_ext
+
+ return '%d' % (int(self.val)/60)
+
+ return '%d' % (int(self.val)*24)
+
+ old_val = self.gp_db.gpostore.get(self.attribute)
+ self.logger.info('%s was changed from %s to %s' % (self.attribute,
+ old_val, val))
+ self.gp_db.gpostore.store(self.attribute, val)
+ self.gp_db.store(str(self), self.attribute, old_val)
+ self.gp_db.gpostore.delete(self.attribute)
+ self.gp_db.delete(str(self), self.attribute)
+
+ return { 'kdc:user_ticket_lifetime': (self.set_kdc_tdb, self.explicit),
+ 'kdc:service_ticket_lifetime': (self.set_kdc_tdb,
+ self.mins_to_hours),
+ 'kdc:renewal_lifetime': (self.set_kdc_tdb,
+ self.days_to_hours),
+ }
+
+ return 'Kerberos Policy'
+
+ '''This class takes the .inf file parameter (essentially a GPO file mapped
+ to a GUID), hashmaps it to the Samba parameter, which then uses an ldb
+ object to update the parameter to Samba4. Not registry oriented whatsoever.
+ '''
+
+ old_val = self.ldb.get_minPwdAge()
+ self.logger.info('KDC Minimum Password age was changed from %s to %s' \
+ % (old_val, val))
+ self.gp_db.store(str(self), self.attribute, old_val)
+ self.ldb.set_minPwdAge(val)
+
+ old_val = self.ldb.get_maxPwdAge()
+ self.logger.info('KDC Maximum Password age was changed from %s to %s' \
+ % (old_val, val))
+ self.gp_db.store(str(self), self.attribute, old_val)
+ self.ldb.set_maxPwdAge(val)
+
+ old_val = self.ldb.get_minPwdLength()
+ self.logger.info(
+ 'KDC Minimum Password length was changed from %s to %s' \
+ % (old_val, val))
+ self.gp_db.store(str(self), self.attribute, old_val)
+ self.ldb.set_minPwdLength(val)
+
+ old_val = self.ldb.get_pwdProperties()
+ self.logger.info('KDC Password Properties were changed from %s to %s' \
+ % (old_val, val))
+ self.gp_db.store(str(self), self.attribute, old_val)
+ self.ldb.set_pwdProperties(val)
+
+ seconds = 60
+ minutes = 60
+ hours = 24
+ sam_add = 10000000
+ val = (self.val)
+ val = int(val)
+ return str(-(val * seconds * minutes * hours * sam_add))
+
+ '''ldap value : samba setter'''
+ return { "minPwdAge" : (self.ch_minPwdAge, self.days2rel_nttime),
+ "maxPwdAge" : (self.ch_maxPwdAge, self.days2rel_nttime),
+ # Could be none, but I like the method assignment in
+ # update_samba
+ "minPwdLength" : (self.ch_minPwdLength, self.explicit),
+ "pwdProperties" : (self.ch_pwdProperties, self.explicit),
+
+ }
+
+ return 'System Access'
+
+ 1) Identifies the GPO if it has a certain kind of filepath,
+ 2) Finally parses it.
+ '''
+
+ count = 0
+
+ return "Security GPO extension"
+
+ return os.path.join(rootpath,
+ "MACHINE/Microsoft/Windows NT/SecEdit/GptTmpl.inf")
+
+ return os.path.join(rootpath, "Machine/Registry.pol")
+
+ return os.path.join(rootpath, "User/Registry.pol")
+
+ return {"System Access": {"MinimumPasswordAge": ("minPwdAge",
+ inf_to_ldb),
+ "MaximumPasswordAge": ("maxPwdAge",
+ inf_to_ldb),
+ "MinimumPasswordLength": ("minPwdLength",
+ inf_to_ldb),
+ "PasswordComplexity": ("pwdProperties",
+ inf_to_ldb),
+ },
+ "Kerberos Policy": {"MaxTicketAge": (
+ "kdc:user_ticket_lifetime",
+ inf_to_kdc_tdb
+ ),
+ "MaxServiceAge": (
+ "kdc:service_ticket_lifetime",
+ inf_to_kdc_tdb
+ ),
+ "MaxRenewAge": (
+ "kdc:renewal_lifetime",
+ inf_to_kdc_tdb
+ ),
+ }
+ }
+
diff --git a/python/samba/gpclass.py b/python/samba/gpclass.py
index 33c9001cb6d..6888a18999a 100644
--- a/python/samba/gpclass.py
+++ b/python/samba/gpclass.py
@@ -25,6 +25,11 @@ from StringIO import StringIO
from abc import ABCMeta, abstractmethod
import xml.etree.ElementTree as etree
import re
+from samba.net import Net
+from samba.dcerpc import nbt
+from samba import smb
+import samba.gpo as gpo
+import chardet
from enum import Enum
__metaclass__ = ABCMeta
+ self.logger = logger
+
@abstractmethod
pass
pass
@abstractmethod
pass
+ self.ldb = ldb
+ self.gp_db = gp_db
+ self.lp = lp
+
+ # Fixing the bug where only some Linux Boxes capitalize MACHINE
+ blist = afile.split('/')
+ idx = afile.lower().split('/').index('machine')
+ for case in [
+ blist[idx].upper(),
+ blist[idx].capitalize(),
+ blist[idx].lower()
+ bfile = '/'.join(blist[:idx]) + '/' + case + '/' + \
+ '/'.join(blist[idx+1:])
+ return self.read(conn.loadfile(bfile.replace('/', '\\')))
+ continue
+ return self.read(conn.loadfile(afile.replace('/', '\\')))
+ self.logger.error(str(e))
+ return None
+
@abstractmethod
pass
__metaclass__ = ABCMeta
return self.val
- (upd_sam, value) = self.mapper().get(self.attribute)
+ (upd_sam, value) = self.mapper()[self.attribute]
upd_sam(value())
@abstractmethod
pass
- return '%d' % (int(self.val)/60)
-
- return '%d' % (int(self.val)*24)
-
- old_val = self.gp_db.gpostore.get(self.attribute)
- self.logger.info('%s was changed from %s to %s' % (self.attribute,
- old_val, val))
- self.gp_db.gpostore.store(self.attribute, val)
- self.gp_db.store(str(self), self.attribute, old_val)
- self.gp_db.gpostore.delete(self.attribute)
- self.gp_db.delete(str(self), self.attribute)
-
- return { 'kdc:user_ticket_lifetime': (self.set_kdc_tdb, self.explicit),
- 'kdc:service_ticket_lifetime': (self.set_kdc_tdb,
- self.mins_to_hours),
- 'kdc:renewal_lifetime': (self.set_kdc_tdb,
- self.days_to_hours),
- }
-
- return 'Kerberos Policy'
-
- '''This class takes the .inf file parameter (essentially a GPO file mapped
- to a GUID), hashmaps it to the Samba parameter, which then uses an ldb
- object to update the parameter to Samba4. Not registry oriented whatsoever.
- '''
-
- old_val = self.ldb.get_minPwdAge()
- self.logger.info('KDC Minimum Password age was changed from %s to %s' \
- % (old_val, val))
- self.gp_db.store(str(self), self.attribute, old_val)
- self.ldb.set_minPwdAge(val)
-
- old_val = self.ldb.get_maxPwdAge()
- self.logger.info('KDC Maximum Password age was changed from %s to %s' \
- % (old_val, val))
- self.gp_db.store(str(self), self.attribute, old_val)
- self.ldb.set_maxPwdAge(val)
-
- old_val = self.ldb.get_minPwdLength()
- self.logger.info(
- 'KDC Minimum Password length was changed from %s to %s' \
- % (old_val, val))
- self.gp_db.store(str(self), self.attribute, old_val)
- self.ldb.set_minPwdLength(val)
-
- old_val = self.ldb.get_pwdProperties()
- self.logger.info('KDC Password Properties were changed from %s to %s' \
- % (old_val, val))
- self.gp_db.store(str(self), self.attribute, old_val)
- self.ldb.set_pwdProperties(val)
-
- seconds = 60
- minutes = 60
- hours = 24
- sam_add = 10000000
- val = (self.val)
- val = int(val)
- return str(-(val * seconds * minutes * hours * sam_add))
-
- '''ldap value : samba setter'''
- return { "minPwdAge" : (self.ch_minPwdAge, self.days2rel_nttime),
- "maxPwdAge" : (self.ch_maxPwdAge, self.days2rel_nttime),
- # Could be none, but I like the method assignment in
- # update_samba
- "minPwdLength" : (self.ch_minPwdLength, self.explicit),
- "pwdProperties" : (self.ch_pwdProperties, self.explicit),
-
- }
-
- return 'System Access'
-
-
- 1) Identifies the GPO if it has a certain kind of filepath,
- 2) Finally parses it.
- '''
-
- count = 0
-
- self.logger = logger
-
- return "Security GPO extension"
-
- return os.path.join(rootpath,
- "MACHINE/Microsoft/Windows NT/SecEdit/GptTmpl.inf")
-
- return os.path.join(rootpath, "Machine/Registry.pol")
-
- return os.path.join(rootpath, "User/Registry.pol")
+ pass
- return {"System Access": {"MinimumPasswordAge": ("minPwdAge",
- inf_to_ldb),
- "MaximumPasswordAge": ("maxPwdAge",
- inf_to_ldb),
- "MinimumPasswordLength": ("minPwdLength",
- inf_to_ldb),
- "PasswordComplexity": ("pwdProperties",
- inf_to_ldb),
- },
- "Kerberos Policy": {"MaxTicketAge": (
- "kdc:user_ticket_lifetime",
- inf_to_kdc_tdb
- ),
- "MaxServiceAge": (
- "kdc:service_ticket_lifetime",
- inf_to_kdc_tdb
- ),
- "MaxRenewAge": (
- "kdc:renewal_lifetime",
- inf_to_kdc_tdb
- ),
- }
- }
-
+ pass
+
ret = False
inftable = self.apply_map()
- policy = conn.loadfile(path.replace('/', '\\'))
current_section = None
# So here we would declare a boolean,
self.gp_db.commit()
return ret
- self.ldb = ldb
- self.gp_db = gp_db
- self.lp = lp
+ pass
- # Fixing the bug where only some Linux Boxes capitalize MACHINE
+''' Fetch the hostname of a writable DC '''
+ net = Net(creds=creds, lp=lp)
+ cldap_ret = net.finddc(domain=lp.get('realm'), flags=(nbt.NBT_SERVER_LDAP |
+ nbt.NBT_SERVER_DS))
+ return cldap_ret.pdc_dns_name
+
+''' Fetch a list of GUIDs for applicable GPOs '''
+ gpos = []
+ ads = gpo.ADS_STRUCT(dc_hostname, lp, creds)
+ gpos = ads.get_gpo_list(creds.get_username())
+ return gpos
+
+ gp_db = store.get_gplog(creds.get_username())
+ dc_hostname = get_dc_hostname(creds, lp)
+ conn = smb.SMB(dc_hostname, 'sysvol', lp=lp, creds=creds)
+ logger.error('Error connecting to \'%s\' using SMB' % dc_hostname)
+ raise
+ gpos = get_gpo_list(dc_hostname, creds, lp)
+
+ guid = gpo_obj.name
+ continue
+ path = os.path.join(lp.get('realm').lower(), 'Policies', guid)
+ local_path = os.path.join(lp.get("path", "sysvol"), path)
+ version = int(gpo.gpo_get_sysvol_gpt_version(local_path)[1])
+ logger.info('GPO %s has changed' % guid)
+ gp_db.state(GPOSTATE.APPLY)
+ gp_db.state(GPOSTATE.ENFORCE)
+ gp_db.set_guid(guid)
+ store.start()
- blist = afile.split('/')
- idx = afile.lower().split('/').index('machine')
- for case in [blist[idx].upper(), blist[idx].capitalize(),
- bfile = '/'.join(blist[:idx]) + '/' + case + '/' + \
- '/'.join(blist[idx+1:])
- return self.read_inf(bfile, conn)
- continue
- return self.read_inf(afile, conn)
- return None
+ ext.parse(ext.list(path), test_ldb, conn, gp_db, lp)
+ logger.error('Failed to parse gpo %s for extension %s' % \
+ (guid, str(ext)))
+ logger.error('Message was: ' + str(e))
+ store.cancel()
+ continue
+ store.store(guid, '%i' % version)
+ store.commit()
+
+ item = gp_db.apply_log_pop()
+ yield item
+ break
+
+ gp_db = store.get_gplog(creds.get_username())
+ gp_db.state(GPOSTATE.UNAPPLY)
+ gp_db.set_guid(gpo_guid)
+ unapply_attributes = gp_db.list(gp_extensions)
+ attr_obj = attr[-1](logger, test_ldb, gp_db, lp, attr[0], attr[1])
+ attr_obj.mapper()[attr[0]][0](attr[1]) # Set the old value
+ gp_db.delete(str(attr_obj), attr[0])
+ gp_db.commit()
diff --git a/source4/scripting/bin/samba_gpoupdate b/source4/scripting/bin/samba_gpoupdate
index 26e0984413e..89b3ed77616 100755
--- a/source4/scripting/bin/samba_gpoupdate
+++ b/source4/scripting/bin/samba_gpoupdate
from samba.samdb import SamDB
SamDB = None
-from samba.gpclass import *
-from samba.net import Net
-from samba.dcerpc import nbt
-from samba import smb
-import samba.gpo as gpo
+from samba.gpclass import apply_gp, unapply_gp, GPOStorage
+from samba.gp_sec_ext import gp_sec_ext
import logging
-import chardet
-
-''' Fetch the hostname of a writable DC '''
- net = Net(creds=creds, lp=lp)
- cldap_ret = net.finddc(domain=lp.get('realm'), flags=(nbt.NBT_SERVER_LDAP |
- nbt.NBT_SERVER_DS))
- return cldap_ret.pdc_dns_name
-
-''' Fetch a list of GUIDs for applicable GPOs '''
- gpos = []
- ads = gpo.ADS_STRUCT(dc_hostname, lp, creds)
- gpos = ads.get_gpo_list(creds.get_username())
- return gpos
-
- gp_db = store.get_gplog(creds.get_username())
- dc_hostname = get_dc_hostname(creds, lp)
- conn = smb.SMB(dc_hostname, 'sysvol', lp=lp, creds=creds)
- logger.error('Error connecting to \'%s\' using SMB' % dc_hostname)
- raise
- gpos = get_gpo_list(dc_hostname, creds, lp)
-
- guid = gpo_obj.name
- continue
- path = os.path.join(lp.get('realm').lower(), 'Policies', guid)
- local_path = os.path.join(lp.get("path", "sysvol"), path)
- version = int(gpo.gpo_get_sysvol_gpt_version(local_path)[1])
- logger.info('GPO %s has changed' % guid)
- gp_db.state(GPOSTATE.APPLY)
- gp_db.state(GPOSTATE.ENFORCE)
- gp_db.set_guid(guid)
- store.start()
- ext.parse(ext.list(path), test_ldb, conn, gp_db, lp)
- logger.error('Failed to parse gpo %s for extension %s' % \
- (guid, str(ext)))
- logger.error('Message was: ' + str(e))
- store.cancel()
- continue
- store.store(guid, '%i' % version)
- store.commit()
-
- item = gp_db.apply_log_pop()
- yield item
- break
-
- gp_db = store.get_gplog(creds.get_username())
- gp_db.state(GPOSTATE.UNAPPLY)
- gp_db.set_guid(gpo_guid)
- unapply_attributes = gp_db.list(gp_extensions)
- attr_obj = attr[-1](logger, test_ldb, gp_db, lp, attr[0], attr[1])
- attr_obj.mapper()[attr[0]][0](attr[1]) # Set the old value
- gp_db.delete(str(attr_obj), attr[0])
- gp_db.commit()
parser = optparse.OptionParser('samba_gpoupdate [options]')
--
2.13.6
From 47e58fcf5b97dcb7c1004420af11231b1220cc99 Mon Sep 17 00:00:00 2001
Date: Mon, 8 Jan 2018 07:17:29 -0700
Subject: [PATCH 2/2] gpo: Read GPO versions locally, not from sysvol
This patch does not change current functionality
for the kdc. Non-kdc clients cannot read directly
from the sysvol, so we need to store the GPT.INI
file locally to read each gpo version.
---
python/samba/gpclass.py | 20 ++++++++++++++++++--
source4/param/pyparam.c | 19 +++++++++++++++++++
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/python/samba/gpclass.py b/python/samba/gpclass.py
index 6888a18999a..2678069090a 100644
--- a/python/samba/gpclass.py
+++ b/python/samba/gpclass.py
gpos = ads.get_gpo_list(creds.get_username())
return gpos
+ # gpo.gpo_get_sysvol_gpt_version() reads the GPT.INI from a local file.
+ # If we don't have a sysvol path locally (if we're not a kdc), then
+ # store the file locally here so it can be read on a client.
+ sysvol = lp.get("path", "sysvol")
+ local_path = os.path.join(sysvol, path, 'GPT.INI')
+ gpt_path = lp.cache_path(os.path.join('gpt', path))
+ local_path = os.path.join(gpt_path, 'GPT.INI')
+ os.makedirs(os.path.dirname(local_path), 0o700)
+ data = conn.loadfile(os.path.join(path, 'GPT.INI').replace('/', '\\'))
+ encoding = chardet.detect(data)
+ open(local_path, 'w').write(data.decode(encoding['encoding']))
+ return int(gpo.gpo_get_sysvol_gpt_version(os.path.dirname(local_path))[1])
+
gp_db = store.get_gplog(creds.get_username())
dc_hostname = get_dc_hostname(creds, lp)
continue
path = os.path.join(lp.get('realm').lower(), 'Policies', guid)
- local_path = os.path.join(lp.get("path", "sysvol"), path)
- version = int(gpo.gpo_get_sysvol_gpt_version(local_path)[1])
+ version = gpo_version(lp, conn, path)
logger.info('GPO %s has changed' % guid)
gp_db.state(GPOSTATE.APPLY)
diff --git a/source4/param/pyparam.c b/source4/param/pyparam.c
index f16c2c0b227..1d99ada09da 100644
--- a/source4/param/pyparam.c
+++ b/source4/param/pyparam.c
@@ -358,6 +358,22 @@ static PyObject *py_samdb_url(PyObject *self, PyObject *unused)
return PyStr_FromFormat("tdb://%s/sam.ldb", lpcfg_private_dir(lp_ctx));
}
+static PyObject *py_cache_path(PyObject *self, PyObject *args)
+{
+ struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
+ char *name, *path;
+ PyObject *ret;
+
+ if (!PyArg_ParseTuple(args, "s", &name)) {
+ return NULL;
+ }
+
+ path = lpcfg_cache_path(NULL, lp_ctx, name);
+ ret = PyStr_FromString(path);

There are many reasons lpcfg_cache_path() can return NULL, which
PyString_FromString() can't cope with, so you need to check.

Added a check for this.

Post by Douglas Bagnall via samba-technical
Also, in Python 3, is a string what you want? It means the path has to
be utf-8 -- is that something we know? I don't know what we normally
do here.

None of this code works in python3 yet, afaik. The libgpo python code
was only just barely ported to python3.

Post by Douglas Bagnall via samba-technical
cheers,
Douglas

+ talloc_free(path);
+
+ return ret;
+}
static PyMethodDef py_lp_ctx_methods[] = {
{ "load", py_lp_ctx_load, METH_VARARGS,
@@ -394,6 +410,9 @@ static PyMethodDef py_lp_ctx_methods[] = {
{ "samdb_url", py_samdb_url, METH_NOARGS,
"S.samdb_url() -> string\n"
"Returns the current URL for sam.ldb." },
+ { "cache_path", py_cache_path, METH_VARARGS,
+ "S.cache_path(name) -> string\n"
+ "Returns a path in the Samba cache directory." },
{ NULL }
};
--
2.13.6

[Douglas Bagnall via samba-technical]

Post by David Mulder via samba-technical
I've broken down that patch into sizable chunks. I've pushed them to the
git repo: https://github.com/samba-team/samba/pull/154

Good. You can add my reviewed-by to all of them except the last one
where I've got a question or two (these questions may be stupid: it
would be good if the other reviewer knows *something* about GPO).

Post by David Mulder via samba-technical
+ # gpo.gpo_get_sysvol_gpt_version() reads the GPT.INI from a local file.
+ # If we don't have a sysvol path locally (if we're not a kdc), then
+ # store the file locally here so it can be read on a client.
+ sysvol = lp.get("path", "sysvol")
+ local_path = os.path.join(sysvol, path, 'GPT.INI')

Is there ever going to be a chance that 'path' here could contain "../"
or start with "/"? That would lead to the wrong result.

The existing use looks OK, but I feel a bit nervous about having a
function around that opens files for writing without checking the
path.

Post by David Mulder via samba-technical
+ gpt_path = lp.cache_path(os.path.join('gpt', path))
+ local_path = os.path.join(gpt_path, 'GPT.INI')
+ os.makedirs(os.path.dirname(local_path), 0o700)
+ data = conn.loadfile(os.path.join(path, 'GPT.INI').replace('/', '\\'))
+ encoding = chardet.detect(data)

^^^^^^^
BTW, AFAICT, chardet is not a standard Python module, and is not listed
as a dependency anywhere in Samba -- it just happens to be on every
machine we've tried. This problem wasn't introduced with this
patchset, but it should be fixed.

Post by David Mulder via samba-technical
+ open(local_path, 'w').write(data.decode(encoding['encoding']))
+ return int(gpo.gpo_get_sysvol_gpt_version(os.path.dirname(local_path))[1])
+
--- a/source4/param/pyparam.c
+++ b/source4/param/pyparam.c
+static PyObject *py_cache_path(PyObject *self, PyObject *args)
+{
+ struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
+ char *name, *path;
+ PyObject *ret;
+
+ if (!PyArg_ParseTuple(args, "s", &name)) {
+ return NULL;
+ }
+
+ path = lpcfg_cache_path(NULL, lp_ctx, name);
+ if (!path) {

It would be good to have something like

PyErr_Format(PyExc_RuntimeError,
"Unable to access cache %s",
name);

in here before the return NULL, so that python has an explanation
(using RuntimeError is not great, but it is what we do). The
PyArg_ParseTuple() case is different because it sets an exception
itself.

Post by David Mulder via samba-technical
+ return NULL;
+ }
+ ret = PyStr_FromString(path);
+ talloc_free(path);
+
+ return ret;
+}

Post by Douglas Bagnall via samba-technical
Also, in Python 3, is a string what you want? It means the path has to
be utf-8 -- is that something we know? I don't know what we normally
do here.

None of this code works in python3 yet, afaik. The libgpo python code
was only just barely ported to python3.

OK.

cheers,
Douglas

[David Mulder via samba-technical]

Post by Douglas Bagnall via samba-technical

Post by David Mulder via samba-technical
I've broken down that patch into sizable chunks. I've pushed them to the
git repo: https://github.com/samba-team/samba/pull/154

Good. You can add my reviewed-by to all of them except the last one
where I've got a question or two (these questions may be stupid: it
would be good if the other reviewer knows *something* about GPO).

Post by David Mulder via samba-technical
+ # gpo.gpo_get_sysvol_gpt_version() reads the GPT.INI from a local file.
+ # If we don't have a sysvol path locally (if we're not a kdc), then
+ # store the file locally here so it can be read on a client.
+ sysvol = lp.get("path", "sysvol")
+ local_path = os.path.join(sysvol, path, 'GPT.INI')

Is there ever going to be a chance that 'path' here could contain "../"
or start with "/"? That would lead to the wrong result.
The existing use looks OK, but I feel a bit nervous about having a
function around that opens files for writing without checking the
path.

No. This 'path' variable is defined in apply_gp(). It is always set to
'realm_name/Policies/{GPO-GUID}'.

Post by Douglas Bagnall via samba-technical

Post by David Mulder via samba-technical
+ gpt_path = lp.cache_path(os.path.join('gpt', path))
+ local_path = os.path.join(gpt_path, 'GPT.INI')
+ os.makedirs(os.path.dirname(local_path), 0o700)
+ data = conn.loadfile(os.path.join(path, 'GPT.INI').replace('/', '\\'))
+ encoding = chardet.detect(data)

^^^^^^^
BTW, AFAICT, chardet is not a standard Python module, and is not listed
as a dependency anywhere in Samba -- it just happens to be on every
machine we've tried. This problem wasn't introduced with this
patchset, but it should be fixed.

Thanks. I'll look for an alternative.

Post by Douglas Bagnall via samba-technical

Post by David Mulder via samba-technical
+ open(local_path, 'w').write(data.decode(encoding['encoding']))
+ return int(gpo.gpo_get_sysvol_gpt_version(os.path.dirname(local_path))[1])
+
--- a/source4/param/pyparam.c
+++ b/source4/param/pyparam.c
+static PyObject *py_cache_path(PyObject *self, PyObject *args)
+{
+ struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
+ char *name, *path;
+ PyObject *ret;
+
+ if (!PyArg_ParseTuple(args, "s", &name)) {
+ return NULL;
+ }
+
+ path = lpcfg_cache_path(NULL, lp_ctx, name);
+ if (!path) {

It would be good to have something like
PyErr_Format(PyExc_RuntimeError,
"Unable to access cache %s",
name);
in here before the return NULL, so that python has an explanation
(using RuntimeError is not great, but it is what we do). The
PyArg_ParseTuple() case is different because it sets an exception
itself.

Ok, added.

Post by Douglas Bagnall via samba-technical

Post by David Mulder via samba-technical
+ return NULL;
+ }
+ ret = PyStr_FromString(path);
+ talloc_free(path);
+
+ return ret;
+}

Post by Douglas Bagnall via samba-technical
Also, in Python 3, is a string what you want? It means the path has to
be utf-8 -- is that something we know? I don't know what we normally
do here.

None of this code works in python3 yet, afaik. The libgpo python code
was only just barely ported to python3.

OK.
cheers,
Douglas

[David Mulder via samba-technical]

Douglas could you take a look at the current set of patches on:

https://github.com/samba-team/samba/pull/154

These patches delegate all the work for downloading the cache to the
check_refresh_gpo_list() function in libgpo (which was written
previously for this very purpose). So, there are no longer file
permission issues, etc, being handled in the python code (I believe
Garming was concerned about this).

Post by Douglas Bagnall via samba-technical

Post by David Mulder via samba-technical
I've broken down that patch into sizable chunks. I've pushed them to the
git repo: https://github.com/samba-team/samba/pull/154

Good. You can add my reviewed-by to all of them except the last one
where I've got a question or two (these questions may be stupid: it
would be good if the other reviewer knows *something* about GPO).

Post by David Mulder via samba-technical
+ # gpo.gpo_get_sysvol_gpt_version() reads the GPT.INI from a local file.
+ # If we don't have a sysvol path locally (if we're not a kdc), then
+ # store the file locally here so it can be read on a client.
+ sysvol = lp.get("path", "sysvol")
+ local_path = os.path.join(sysvol, path, 'GPT.INI')

Is there ever going to be a chance that 'path' here could contain "../"
or start with "/"? That would lead to the wrong result.
The existing use looks OK, but I feel a bit nervous about having a
function around that opens files for writing without checking the
path.

Post by David Mulder via samba-technical
+ gpt_path = lp.cache_path(os.path.join('gpt', path))
+ local_path = os.path.join(gpt_path, 'GPT.INI')
+ os.makedirs(os.path.dirname(local_path), 0o700)
+ data = conn.loadfile(os.path.join(path, 'GPT.INI').replace('/', '\\'))
+ encoding = chardet.detect(data)

^^^^^^^
BTW, AFAICT, chardet is not a standard Python module, and is not listed
as a dependency anywhere in Samba -- it just happens to be on every
machine we've tried. This problem wasn't introduced with this
patchset, but it should be fixed.

Post by David Mulder via samba-technical
+ open(local_path, 'w').write(data.decode(encoding['encoding']))
+ return int(gpo.gpo_get_sysvol_gpt_version(os.path.dirname(local_path))[1])
+
--- a/source4/param/pyparam.c
+++ b/source4/param/pyparam.c
+static PyObject *py_cache_path(PyObject *self, PyObject *args)
+{
+ struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
+ char *name, *path;
+ PyObject *ret;
+
+ if (!PyArg_ParseTuple(args, "s", &name)) {
+ return NULL;
+ }
+
+ path = lpcfg_cache_path(NULL, lp_ctx, name);
+ if (!path) {

It would be good to have something like
PyErr_Format(PyExc_RuntimeError,
"Unable to access cache %s",
name);
in here before the return NULL, so that python has an explanation
(using RuntimeError is not great, but it is what we do). The
PyArg_ParseTuple() case is different because it sets an exception
itself.

Post by David Mulder via samba-technical
+ return NULL;
+ }
+ ret = PyStr_FromString(path);
+ talloc_free(path);
+
+ return ret;
+}

Post by Douglas Bagnall via samba-technical
Also, in Python 3, is a string what you want? It means the path has to
be utf-8 -- is that something we know? I don't know what we normally
do here.

None of this code works in python3 yet, afaik. The libgpo python code
was only just barely ported to python3.

OK.
cheers,
Douglas

[Douglas Bagnall via samba-technical]

hi David,

Post by David Mulder via samba-technical
https://github.com/samba-team/samba/pull/154

I'm skipping patches 1-6, which are still OK.

Post by David Mulder via samba-technical
From 5fc6676a3e57181e58a981085d224172a8026e9d Mon Sep 17 00:00:00 2001
Date: Tue, 10 Apr 2018 15:07:34 -0600
Subject: [PATCH 07/11] param: Add python binding for lpcfg_cache_path

and this one looks good now too, except for one thing...

Post by David Mulder via samba-technical
--- a/source4/param/pyparam.c
+++ b/source4/param/pyparam.c
@@ -358,6 +358,27 @@ static PyObject *py_samdb_url(PyObject *self, PyObject *unused)
return PyStr_FromFormat("tdb://%s/sam.ldb", lpcfg_private_dir(lp_ctx));
}
+static PyObject *py_cache_path(PyObject *self, PyObject *args)
+{
+ struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
+ char *name, *path;
+ PyObject *ret;

These uninitialised pointers should really be initialised to NULL.
Sorry for not noticing before, and you wouldn't guess by looking
around pyparam.c, but our coding standard requires this.

In the interests of getting this in I have fixed this for you.

Post by David Mulder via samba-technical
+
+ if (!PyArg_ParseTuple(args, "s", &name)) {
+ return NULL;
+ }
+
+ path = lpcfg_cache_path(NULL, lp_ctx, name);
+ if (!path) {
+ PyErr_Format(PyExc_RuntimeError,
+ "Unable to access cache %s", name);
+ return NULL;
+ }
+ ret = PyStr_FromString(path);
+ talloc_free(path);
+
+ return ret;
+}
static PyMethodDef py_lp_ctx_methods[] = {
{ "load", py_lp_ctx_load, METH_VARARGS,
@@ -394,6 +415,9 @@ static PyMethodDef py_lp_ctx_methods[] = {
{ "samdb_url", py_samdb_url, METH_NOARGS,
"S.samdb_url() -> string\n"
"Returns the current URL for sam.ldb." },
+ { "cache_path", py_cache_path, METH_VARARGS,
+ "S.cache_path(name) -> string\n"
+ "Returns a path in the Samba cache directory." },
{ NULL }
};
From 7ed5e92f26c4343ebbe03492e0f959557197696c Mon Sep 17 00:00:00 2001
Date: Wed, 11 Apr 2018 12:40:18 -0600
Subject: [PATCH 08/11] libgpo: gpo_copy_file() shouldn't explicitly call smb1
Don't call cli_openx directly to open a file this
calls smb1 code explicitly, which fails if we did
a multi-protocol negotiate and negotiated smb2+.
Use the higher level cli_open() instead.
---
libgpo/gpo_filesync.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libgpo/gpo_filesync.c b/libgpo/gpo_filesync.c
index 6e3efdaf6c1..bf0bb5381fc 100644
--- a/libgpo/gpo_filesync.c
+++ b/libgpo/gpo_filesync.c
@@ -50,7 +50,7 @@ NTSTATUS gpo_copy_file(TALLOC_CTX *mem_ctx,
int read_size = io_bufsize;
off_t nread = 0;
- result = cli_openx(cli, nt_path, O_RDONLY, DENY_NONE, &fnum);
+ result = cli_open(cli, nt_path, O_RDONLY, DENY_NONE, &fnum);

This one seems very sensible, but frankly I have no idea about this
SMB stuff. Andrew says it is OK, and on this rare occasion I will
trust him.

Post by David Mulder via samba-technical
if (!NT_STATUS_IS_OK(result)) {
goto out;
}
From 27bebbd0e2f2061a11f9a707a0b679431e093585 Mon Sep 17 00:00:00 2001
Date: Tue, 10 Apr 2018 15:04:53 -0600
Subject: [PATCH 09/11] libgpo: Add python bindings for check_refresh_gpo_list
---
libgpo/pygpo.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/libgpo/pygpo.c b/libgpo/pygpo.c
index ac6e3237a82..e5f63e1f440 100644
--- a/libgpo/pygpo.c
+++ b/libgpo/pygpo.c
@@ -290,6 +290,46 @@ static PyObject* py_ads_connect(ADS *self)
/* Parameter mapping and functions for the GP_EXT struct */
void initgpo(void);
+static PyObject *py_check_refresh_gpo_list(PyObject * self,
+ PyObject * args)
+{
+ TALLOC_CTX *frame = talloc_stackframe();
+ ADS *ads;
+ const char *cache_dir;
+ struct GROUP_POLICY_OBJECT *gpo_ptr = NULL;
+ PyObject *gpo_list;
+ PyObject *gpo_obj;
+ NTSTATUS status;

More uninitialised pointers.

In this case it *really* matters for cache_dir, because if a value
isn't provided by python...

Post by David Mulder via samba-technical
+ PyObject *ret = NULL;
+
+ if (!PyArg_ParseTuple(args, "OO|s", &ads, &gpo_list, &cache_dir)) {

...it will be left uninitialised here (being after the "|").

Post by David Mulder via samba-technical
+ goto out;
+ }
+ gpo_obj = PyList_GetItem(gpo_list, 0);

You should check for NULL here and exit (PyList_GetItem sets the
IndexError for you).

Post by David Mulder via samba-technical
+ gpo_ptr = (struct GROUP_POLICY_OBJECT *)pytalloc_get_ptr(gpo_obj);
+
+ if (!cache_dir) {
+ cache_dir = cache_path(GPO_CACHE_DIR);
+ if (!cache_dir) {
+ PyErr_SetString(PyExc_MemoryError,
+ "Failed to determine gpo cache dir");

I'm not convinced that MemoryError is the right thing here. The
failure *could* be a talloc error, but it could also be an I/O error,
and I suspect the latter is more likely -- but as pygpo.c seems to
follow this pattern I guess we're stuck.

Post by David Mulder via samba-technical
+ goto out;
+ }
+ }
+
+ status = check_refresh_gpo_list(ads->ads_ptr, frame, cache_dir, 0,
+ gpo_ptr);
+ if (!NT_STATUS_IS_OK(status)) {
+ PyErr_SetNTSTATUS(status);
+ goto out;
+ }
+
+ ret = Py_True;
+ TALLOC_FREE(frame);
+ return ret;
+}
+
/* Global methods aka do not need a special pyobject type */
static PyObject *py_gpo_get_sysvol_gpt_version(PyObject * self,
PyObject * args)
@@ -497,6 +537,9 @@ static PyMethodDef py_gpo_methods[] = {
{"gpo_get_sysvol_gpt_version",
(PyCFunction)py_gpo_get_sysvol_gpt_version,
METH_VARARGS, NULL},
+ {"check_refresh_gpo_list",
+ (PyCFunction)py_check_refresh_gpo_list,
+ METH_VARARGS, NULL},
{NULL}
};
From 6ffe43e3286a5f938df86b3bbfafbb4c5d9a4efa Mon Sep 17 00:00:00 2001
Date: Wed, 11 Apr 2018 12:45:40 -0600
Subject: [PATCH 10/11] gpo: python chardet is not a dep of samba

Good.

Post by David Mulder via samba-technical
From dfdb5aedbfff632ec9c33763c0d8bda11a4bc654 Mon Sep 17 00:00:00 2001
Date: Mon, 8 Jan 2018 07:17:29 -0700
Subject: [PATCH 11/11] gpo: Read GPO versions locally, not from sysvol

And I think this one is OK too.

So I will get Andrew to look at these and hopefully we'll push them
all except the "refresh_gpo_list" one that needs work and the "read
gpos locally" one that depends on it.

cheers,
Douglas

[David Mulder via samba-technical]

I've fixed the couple of issues you mentioned and re-pushed. Thanks!

Post by Douglas Bagnall via samba-technical
hi David,

Post by David Mulder via samba-technical
https://github.com/samba-team/samba/pull/154

I'm skipping patches 1-6, which are still OK.

Post by David Mulder via samba-technical
From 5fc6676a3e57181e58a981085d224172a8026e9d Mon Sep 17 00:00:00 2001
Date: Tue, 10 Apr 2018 15:07:34 -0600
Subject: [PATCH 07/11] param: Add python binding for lpcfg_cache_path

and this one looks good now too, except for one thing...

Post by David Mulder via samba-technical
--- a/source4/param/pyparam.c
+++ b/source4/param/pyparam.c
@@ -358,6 +358,27 @@ static PyObject *py_samdb_url(PyObject *self, PyObject *unused)
return PyStr_FromFormat("tdb://%s/sam.ldb", lpcfg_private_dir(lp_ctx));
}
+static PyObject *py_cache_path(PyObject *self, PyObject *args)
+{
+ struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
+ char *name, *path;
+ PyObject *ret;

These uninitialised pointers should really be initialised to NULL.
Sorry for not noticing before, and you wouldn't guess by looking
around pyparam.c, but our coding standard requires this.
In the interests of getting this in I have fixed this for you.

Post by David Mulder via samba-technical
+
+ if (!PyArg_ParseTuple(args, "s", &name)) {
+ return NULL;
+ }
+
+ path = lpcfg_cache_path(NULL, lp_ctx, name);
+ if (!path) {
+ PyErr_Format(PyExc_RuntimeError,
+ "Unable to access cache %s", name);
+ return NULL;
+ }
+ ret = PyStr_FromString(path);
+ talloc_free(path);
+
+ return ret;
+}
static PyMethodDef py_lp_ctx_methods[] = {
{ "load", py_lp_ctx_load, METH_VARARGS,
@@ -394,6 +415,9 @@ static PyMethodDef py_lp_ctx_methods[] = {
{ "samdb_url", py_samdb_url, METH_NOARGS,
"S.samdb_url() -> string\n"
"Returns the current URL for sam.ldb." },
+ { "cache_path", py_cache_path, METH_VARARGS,
+ "S.cache_path(name) -> string\n"
+ "Returns a path in the Samba cache directory." },
{ NULL }
};
From 7ed5e92f26c4343ebbe03492e0f959557197696c Mon Sep 17 00:00:00 2001
Date: Wed, 11 Apr 2018 12:40:18 -0600
Subject: [PATCH 08/11] libgpo: gpo_copy_file() shouldn't explicitly call smb1
Don't call cli_openx directly to open a file this
calls smb1 code explicitly, which fails if we did
a multi-protocol negotiate and negotiated smb2+.
Use the higher level cli_open() instead.
---
libgpo/gpo_filesync.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libgpo/gpo_filesync.c b/libgpo/gpo_filesync.c
index 6e3efdaf6c1..bf0bb5381fc 100644
--- a/libgpo/gpo_filesync.c
+++ b/libgpo/gpo_filesync.c
@@ -50,7 +50,7 @@ NTSTATUS gpo_copy_file(TALLOC_CTX *mem_ctx,
int read_size = io_bufsize;
off_t nread = 0;
- result = cli_openx(cli, nt_path, O_RDONLY, DENY_NONE, &fnum);
+ result = cli_open(cli, nt_path, O_RDONLY, DENY_NONE, &fnum);

This one seems very sensible, but frankly I have no idea about this
SMB stuff. Andrew says it is OK, and on this rare occasion I will
trust him.

Post by David Mulder via samba-technical
if (!NT_STATUS_IS_OK(result)) {
goto out;
}
From 27bebbd0e2f2061a11f9a707a0b679431e093585 Mon Sep 17 00:00:00 2001
Date: Tue, 10 Apr 2018 15:04:53 -0600
Subject: [PATCH 09/11] libgpo: Add python bindings for check_refresh_gpo_list
---
libgpo/pygpo.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/libgpo/pygpo.c b/libgpo/pygpo.c
index ac6e3237a82..e5f63e1f440 100644
--- a/libgpo/pygpo.c
+++ b/libgpo/pygpo.c
@@ -290,6 +290,46 @@ static PyObject* py_ads_connect(ADS *self)
/* Parameter mapping and functions for the GP_EXT struct */
void initgpo(void);
+static PyObject *py_check_refresh_gpo_list(PyObject * self,
+ PyObject * args)
+{
+ TALLOC_CTX *frame = talloc_stackframe();
+ ADS *ads;
+ const char *cache_dir;
+ struct GROUP_POLICY_OBJECT *gpo_ptr = NULL;
+ PyObject *gpo_list;
+ PyObject *gpo_obj;
+ NTSTATUS status;

More uninitialised pointers.
In this case it *really* matters for cache_dir, because if a value
isn't provided by python...

Post by David Mulder via samba-technical
+ PyObject *ret = NULL;
+
+ if (!PyArg_ParseTuple(args, "OO|s", &ads, &gpo_list, &cache_dir)) {

...it will be left uninitialised here (being after the "|").

Post by David Mulder via samba-technical
+ goto out;
+ }
+ gpo_obj = PyList_GetItem(gpo_list, 0);

You should check for NULL here and exit (PyList_GetItem sets the
IndexError for you).

Post by David Mulder via samba-technical
+ gpo_ptr = (struct GROUP_POLICY_OBJECT *)pytalloc_get_ptr(gpo_obj);
+
+ if (!cache_dir) {
+ cache_dir = cache_path(GPO_CACHE_DIR);
+ if (!cache_dir) {
+ PyErr_SetString(PyExc_MemoryError,
+ "Failed to determine gpo cache dir");

I'm not convinced that MemoryError is the right thing here. The
failure *could* be a talloc error, but it could also be an I/O error,
and I suspect the latter is more likely -- but as pygpo.c seems to
follow this pattern I guess we're stuck.

Post by David Mulder via samba-technical
+ goto out;
+ }
+ }
+
+ status = check_refresh_gpo_list(ads->ads_ptr, frame, cache_dir, 0,
+ gpo_ptr);
+ if (!NT_STATUS_IS_OK(status)) {
+ PyErr_SetNTSTATUS(status);
+ goto out;
+ }
+
+ ret = Py_True;
+ TALLOC_FREE(frame);
+ return ret;
+}
+
/* Global methods aka do not need a special pyobject type */
static PyObject *py_gpo_get_sysvol_gpt_version(PyObject * self,
PyObject * args)
@@ -497,6 +537,9 @@ static PyMethodDef py_gpo_methods[] = {
{"gpo_get_sysvol_gpt_version",
(PyCFunction)py_gpo_get_sysvol_gpt_version,
METH_VARARGS, NULL},
+ {"check_refresh_gpo_list",
+ (PyCFunction)py_check_refresh_gpo_list,
+ METH_VARARGS, NULL},
{NULL}
};
From 6ffe43e3286a5f938df86b3bbfafbb4c5d9a4efa Mon Sep 17 00:00:00 2001
Date: Wed, 11 Apr 2018 12:45:40 -0600
Subject: [PATCH 10/11] gpo: python chardet is not a dep of samba

Good.

Post by David Mulder via samba-technical
From dfdb5aedbfff632ec9c33763c0d8bda11a4bc654 Mon Sep 17 00:00:00 2001
Date: Mon, 8 Jan 2018 07:17:29 -0700
Subject: [PATCH 11/11] gpo: Read GPO versions locally, not from sysvol

And I think this one is OK too.
So I will get Andrew to look at these and hopefully we'll push them
all except the "refresh_gpo_list" one that needs work and the "read
gpos locally" one that depends on it.
cheers,
Douglas

[Douglas Bagnall via samba-technical]

hi David,

Post by David Mulder via samba-technical
I've fixed the couple of issues you mentioned and re-pushed. Thanks!

Unfortunately there is more to fix, which I didn't notice before because

Post by David Mulder via samba-technical
+static PyObject *py_check_refresh_gpo_list(PyObject * self,
+ PyObject * args)
+{
+ TALLOC_CTX *frame = talloc_stackframe();
+ ADS *ads = NULL;
+ const char *cache_dir = NULL;
+ struct GROUP_POLICY_OBJECT *gpo_ptr = NULL;
+ PyObject *gpo_list = NULL;
+ PyObject *gpo_obj = NULL;
+ NTSTATUS status;
+ PyObject *ret = NULL;
+
+ if (!PyArg_ParseTuple(args, "OO|s", &ads, &gpo_list, &cache_dir)) {
+ goto out;
+ }
+ gpo_obj = PyList_GetItem(gpo_list, 0);
+ if (!gpo_obj) {
+ goto out;
+ }
+ gpo_ptr = (struct GROUP_POLICY_OBJECT *)pytalloc_get_ptr(gpo_obj);

This is wrong. If the object supplied to the python function is not a
GPO object, nothing good will come of this. You need to check the type
of gpo_obj, with something like this

ret = PyObject_TypeCheck(gpo_obj, &GPOType);
if (! ret) {
error
}

or use py_check_dcerpc_type().

Douglas

Post by David Mulder via samba-technical
+
+ if (!cache_dir) {
+ cache_dir = cache_path(GPO_CACHE_DIR);
+ if (!cache_dir) {
+ PyErr_SetString(PyExc_MemoryError,
+ "Failed to determine gpo cache dir");
+ goto out;
+ }
+ }
+
+ status = check_refresh_gpo_list(ads->ads_ptr, frame, cache_dir, 0,
+ gpo_ptr);
+ if (!NT_STATUS_IS_OK(status)) {
+ PyErr_SetNTSTATUS(status);
+ goto out;
+ }
+
+ ret = Py_True;
+ TALLOC_FREE(frame);
+ return ret;
+}
+
/* Global methods aka do not need a special pyobject type */
static PyObject *py_gpo_get_sysvol_gpt_versi

[David Mulder via samba-technical]

Post by Douglas Bagnall via samba-technical
hi David,

Post by David Mulder via samba-technical
I've fixed the couple of issues you mentioned and re-pushed. Thanks!

Unfortunately there is more to fix, which I didn't notice before because

Post by David Mulder via samba-technical
+static PyObject *py_check_refresh_gpo_list(PyObject * self,
+ PyObject * args)
+{
+ TALLOC_CTX *frame = talloc_stackframe();
+ ADS *ads = NULL;
+ const char *cache_dir = NULL;
+ struct GROUP_POLICY_OBJECT *gpo_ptr = NULL;
+ PyObject *gpo_list = NULL;
+ PyObject *gpo_obj = NULL;
+ NTSTATUS status;
+ PyObject *ret = NULL;
+
+ if (!PyArg_ParseTuple(args, "OO|s", &ads, &gpo_list, &cache_dir)) {
+ goto out;
+ }
+ gpo_obj = PyList_GetItem(gpo_list, 0);
+ if (!gpo_obj) {
+ goto out;
+ }
+ gpo_ptr = (struct GROUP_POLICY_OBJECT *)pytalloc_get_ptr(gpo_obj);

This is wrong. If the object supplied to the python function is not a
GPO object, nothing good will come of this. You need to check the type
of gpo_obj, with something like this
ret = PyObject_TypeCheck(gpo_obj, &GPOType);
if (! ret) {
error
}
or use py_check_dcerpc_type().

Good point. And by the way, the reason for extracting the gpo_ptr from
the front of the list is due to the way we transform the linked list
into a python list in py_ads_get_gpo_list().

Post by Douglas Bagnall via samba-technical
Douglas

Post by David Mulder via samba-technical
+
+ if (!cache_dir) {
+ cache_dir = cache_path(GPO_CACHE_DIR);
+ if (!cache_dir) {
+ PyErr_SetString(PyExc_MemoryError,
+ "Failed to determine gpo cache dir");
+ goto out;
+ }
+ }
+
+ status = check_refresh_gpo_list(ads->ads_ptr, frame, cache_dir, 0,
+ gpo_ptr);
+ if (!NT_STATUS_IS_OK(status)) {
+ PyErr_SetNTSTATUS(status);
+ goto out;
+ }
+
+ ret = Py_True;
+ TALLOC_FREE(frame);
+ return ret;
+}
+
/* Global methods aka do not need a special pyobject type */
static PyObject *py_gpo_get_sysvol_gpt_versi

[David Mulder via samba-technical]

I've added that type check, as well as couple other checks. I also
wasn't checking the ads type input or verifying the list and it's length
before popping from it. Latest sources are on the github pull request.

Post by Douglas Bagnall via samba-technical
hi David,

Post by David Mulder via samba-technical
I've fixed the couple of issues you mentioned and re-pushed. Thanks!

Unfortunately there is more to fix, which I didn't notice before because

Post by David Mulder via samba-technical
+static PyObject *py_check_refresh_gpo_list(PyObject * self,
+ PyObject * args)
+{
+ TALLOC_CTX *frame = talloc_stackframe();
+ ADS *ads = NULL;
+ const char *cache_dir = NULL;
+ struct GROUP_POLICY_OBJECT *gpo_ptr = NULL;
+ PyObject *gpo_list = NULL;
+ PyObject *gpo_obj = NULL;
+ NTSTATUS status;
+ PyObject *ret = NULL;
+
+ if (!PyArg_ParseTuple(args, "OO|s", &ads, &gpo_list, &cache_dir)) {
+ goto out;
+ }
+ gpo_obj = PyList_GetItem(gpo_list, 0);
+ if (!gpo_obj) {
+ goto out;
+ }
+ gpo_ptr = (struct GROUP_POLICY_OBJECT *)pytalloc_get_ptr(gpo_obj);

This is wrong. If the object supplied to the python function is not a
GPO object, nothing good will come of this. You need to check the type
of gpo_obj, with something like this
ret = PyObject_TypeCheck(gpo_obj, &GPOType);
if (! ret) {
error
}
or use py_check_dcerpc_type().
Douglas

Post by David Mulder via samba-technical
+
+ if (!cache_dir) {
+ cache_dir = cache_path(GPO_CACHE_DIR);
+ if (!cache_dir) {
+ PyErr_SetString(PyExc_MemoryError,
+ "Failed to determine gpo cache dir");
+ goto out;
+ }
+ }
+
+ status = check_refresh_gpo_list(ads->ads_ptr, frame, cache_dir, 0,
+ gpo_ptr);
+ if (!NT_STATUS_IS_OK(status)) {
+ PyErr_SetNTSTATUS(status);
+ goto out;
+ }
+
+ ret = Py_True;
+ TALLOC_FREE(frame);
+ return ret;
+}
+
/* Global methods aka do not need a special pyobject type */
static PyObject *py_gpo_get_sysvol_gpt_versi

[Douglas Bagnall via samba-technical]

Post by David Mulder via samba-technical
I've added that type check, as well as couple other checks. I also
wasn't checking the ads type input or verifying the list and it's length
before popping from it. Latest sources are on the github pull request.

Thanks. The ADS check is definitely necessary.

I thought the list was OK without the checks (in that you'd get a Python
exception from PyList_GetItem(), not a segfault), but I see that you're
accepting an empty list, so you do need more checks.

Two questions:

1. Is there a test for the empty list?

2. If you're only ever looking at the first item, why is it a list?

The first question is a "there should be" hint, but the second one is a
genuine question from a GPO ignoramus.

Douglas

[David Mulder via samba-technical]

Post by Douglas Bagnall via samba-technical

Post by David Mulder via samba-technical
I've added that type check, as well as couple other checks. I also
wasn't checking the ads type input or verifying the list and it's length
before popping from it. Latest sources are on the github pull request.

Thanks. The ADS check is definitely necessary.
I thought the list was OK without the checks (in that you'd get a Python
exception from PyList_GetItem(), not a segfault), but I see that you're
accepting an empty list, so you do need more checks.
1. Is there a test for the empty list?

Sure, if the list is len == 0, it returns success (since there is
nothing to refresh).
        gp_list_len = PyList_Size(gpo_list);
        if (gp_list_len == 0) {
                ret = Py_True;
                goto out;
        }

Post by Douglas Bagnall via samba-technical
2. If you're only ever looking at the first item, why is it a list?

Because under the hood GROUP_POLICY_OBJECT is a linked list, but
py_ads_get_gpo_list() iterates over that linked list and turns it into a
python list (each element in the python list still holds it's *next
pointer, despite now being contained within a python list). In the C
code we need the linked list back to operate on, so we grab the first
item from the python list, which is the head of our C linked list. The
order of a python list is persistent, so we don't need to worry about
loosing the head of our linked list. The only time we'd end up operating
on something other than the head of our linked list, is if someone, say,
passed in a splice of the gpo_list, such as
gpo.check_refresh_gpo_list(gpo_list[1:]), in which case we'd take the
second item in the linked list as the head of our list. This will still
work as intended (since any member of the linked list can be the head),
and if someone splices the list this way, then that's probably what they
intended to happen anyway (this explicitly says to refresh all the gpos
expect the first, which makes sense).

Post by Douglas Bagnall via samba-technical
The first question is a "there should be" hint, but the second one is a
genuine question from a GPO ignoramus.
Douglas

[Douglas Bagnall via samba-technical]

Post by David Mulder via samba-technical

Post by Douglas Bagnall via samba-technical

Post by David Mulder via samba-technical
I've added that type check, as well as couple other checks. I also
wasn't checking the ads type input or verifying the list and it's length
before popping from it. Latest sources are on the github pull request.

Thanks. The ADS check is definitely necessary.
I thought the list was OK without the checks (in that you'd get a Python
exception from PyList_GetItem(), not a segfault), but I see that you're
accepting an empty list, so you do need more checks.
1. Is there a test for the empty list?

Sure, if the list is len == 0, it returns success (since there is
nothing to refresh).
        gp_list_len = PyList_Size(gpo_list);
        if (gp_list_len == 0) {
                ret = Py_True;
                goto out;
        }

Yeah, I saw that. I meant a python test that calls this with an empty list.
If there was it would have failed until now, right?

Now I'm noticing there are no tests for gpo.check_refresh_gpo_list() at all,
which there should be.

Post by David Mulder via samba-technical

Post by Douglas Bagnall via samba-technical
2. If you're only ever looking at the first item, why is it a list?

Because under the hood GROUP_POLICY_OBJECT is a linked list, but
py_ads_get_gpo_list() iterates over that linked list and turns it into a
python list (each element in the python list still holds it's *next
pointer, despite now being contained within a python list). In the C
code we need the linked list back to operate on, so we grab the first
item from the python list, which is the head of our C linked list. The
order of a python list is persistent, so we don't need to worry about
loosing the head of our linked list. The only time we'd end up operating
on something other than the head of our linked list, is if someone, say,
passed in a splice of the gpo_list, such as
gpo.check_refresh_gpo_list(gpo_list[1:]), in which case we'd take the
second item in the linked list as the head of our list. This will still
work as intended (since any member of the linked list can be the head),
and if someone splices the list this way, then that's probably what they
intended to happen anyway (this explicitly says to refresh all the gpos
expect the first, which makes sense).

I see problems with that. While this works as intended:

gpo.check_refresh_gpo_list(gpo_list[1:])

if I am following, none of the following will work:

gpo.check_refresh_gpo_list(gpo_list[:2]) # truncate the list
gpo.check_refresh_gpo_list(gpo_list[:1] + gpo_list[3:]) # remove one
del gpo_list[2]; gpo.check_refresh_gpo_list(gpo_list) # remove one
gpo_list.sort(); gpo.check_refresh_gpo_list(gpo_list) # reorder
gpo.check_refresh_gpo_list(gpo_list1 + gpo_list 2) # join

It would be *slightly* better to use an immutable tuple rather than a list,
and better still to have an opaque object from python's point of view. The
real solution would be to iterate through and reconstruct the linked list
from the Python list.

cheers,
Douglas

[David Mulder via samba-technical]

Post by Douglas Bagnall via samba-technical

Post by David Mulder via samba-technical

Post by Douglas Bagnall via samba-technical

Post by David Mulder via samba-technical
I've added that type check, as well as couple other checks. I also
wasn't checking the ads type input or verifying the list and it's length
before popping from it. Latest sources are on the github pull request.

Thanks. The ADS check is definitely necessary.
I thought the list was OK without the checks (in that you'd get a Python
exception from PyList_GetItem(), not a segfault), but I see that you're
accepting an empty list, so you do need more checks.
1. Is there a test for the empty list?

Sure, if the list is len == 0, it returns success (since there is
nothing to refresh).
        gp_list_len = PyList_Size(gpo_list);
        if (gp_list_len == 0) {
                ret = Py_True;
                goto out;
        }

Yeah, I saw that. I meant a python test that calls this with an empty list.
If there was it would have failed until now, right?
Now I'm noticing there are no tests for gpo.check_refresh_gpo_list() at all,
which there should be.

Yes, there is no test. I just tried adding one, but
check_refresh_gpo_list() dies when trying to call cli_full_connection(),
which is odd. Dies in the test env, but works fine outside. I'll post
later when I get this working.

Post by Douglas Bagnall via samba-technical

Post by David Mulder via samba-technical

Post by Douglas Bagnall via samba-technical
2. If you're only ever looking at the first item, why is it a list?

Because under the hood GROUP_POLICY_OBJECT is a linked list, but
py_ads_get_gpo_list() iterates over that linked list and turns it into a
python list (each element in the python list still holds it's *next
pointer, despite now being contained within a python list). In the C
code we need the linked list back to operate on, so we grab the first
item from the python list, which is the head of our C linked list. The
order of a python list is persistent, so we don't need to worry about
loosing the head of our linked list. The only time we'd end up operating
on something other than the head of our linked list, is if someone, say,
passed in a splice of the gpo_list, such as
gpo.check_refresh_gpo_list(gpo_list[1:]), in which case we'd take the
second item in the linked list as the head of our list. This will still
work as intended (since any member of the linked list can be the head),
and if someone splices the list this way, then that's probably what they
intended to happen anyway (this explicitly says to refresh all the gpos
expect the first, which makes sense).

gpo.check_refresh_gpo_list(gpo_list[1:])
gpo.check_refresh_gpo_list(gpo_list[:2]) # truncate the list
gpo.check_refresh_gpo_list(gpo_list[:1] + gpo_list[3:]) # remove one
del gpo_list[2]; gpo.check_refresh_gpo_list(gpo_list) # remove one
gpo_list.sort(); gpo.check_refresh_gpo_list(gpo_list) # reorder
gpo.check_refresh_gpo_list(gpo_list1 + gpo_list 2) # join
It would be *slightly* better to use an immutable tuple rather than a list,
and better still to have an opaque object from python's point of view. The
real solution would be to iterate through and reconstruct the linked list
from the Python list.

Ok, I've fixed this now.

Post by Douglas Bagnall via samba-technical
cheers,
Douglas

[Douglas Bagnall via samba-technical]

Post by David Mulder via samba-technical

Post by Douglas Bagnall via samba-technical
Now I'm noticing there are no tests for gpo.check_refresh_gpo_list() at all,
which there should be.

Yes, there is no test. I just tried adding one, but
check_refresh_gpo_list() dies when trying to call cli_full_connection(),
which is odd. Dies in the test env, but works fine outside. I'll post
later when I get this working.

[...]

Post by David Mulder via samba-technical

Post by Douglas Bagnall via samba-technical
It would be *slightly* better to use an immutable tuple rather than a list,
and better still to have an opaque object from python's point of view. The
real solution would be to iterate through and reconstruct the linked list
from the Python list.

Ok, I've fixed this now.

Right, that looks good! So it just needs that test.

Douglas