[PATCHES] rename samba_gpoupdate to gpupdate

David Mulder dmulder at suse.com
Fri Jul 6 13:57:17 UTC 2018



On 07/05/2018 10:58 PM, Douglas Bagnall wrote:
> These comments relate to
> https://gitlab.com/samba-team/samba/merge_requests/16
> which I have modified slightly and pushed back to
> https://gitlab.com/samba-team/devel/samba/tree/david-mulder-gpo
>
> David, first I will explain the small changes I have made to the patch
> series, then I'll go over the remaining things I have problems with or
> don't understand. So this first bit is the difference between two
> branches, not the actual revised patches which you can see at the url
> above.
>
>> diff --git a/python/samba/gpclass.py b/python/samba/gpclass.py
>> index c53492e2e4c..b55826ef241 100644
>> --- a/python/samba/gpclass.py
>> +++ b/python/samba/gpclass.py
>> @@ -17,6 +17,7 @@
>>  
>>  import sys
>>  import os
>> +import errno
>>  import tdb
>>  sys.path.insert(0, "bin/python")
>>  from samba import NTSTATUSError
>> @@ -32,7 +33,6 @@ import samba.gpo as gpo
>>  from samba.param import LoadParm
>>  from uuid import UUID
>>  from tempfile import NamedTemporaryFile
>> -from shutil import move as mv
>>  
>>  try:
>>      from enum import Enum
>> @@ -365,8 +365,9 @@ def cache_gpo_dir(conn, cache, sub_dir):
>>      loc_sub_dir = sub_dir.upper()
>>      try:
>>          os.makedirs(os.path.join(cache, loc_sub_dir), mode=0o755)
>> -    except OSError:
>> -        pass # File Exists
>> +    except OSError as e:
>> +        if e.errno == errno.EEXIST:
>> +            pass # File Exists
> Here it is easy and better to ignore only the particular error you
> expect (and not e.g. paper over "no space left on device").
Looks good.
>
>>      for fdata in conn.list(sub_dir):
>>          if fdata['attrib'] & FILE_ATTRIBUTE_DIRECTORY:
>>              cache_gpo_dir(conn, cache, os.path.join(sub_dir, fdata['name']))
>> @@ -477,7 +478,7 @@ def register_gp_extension(guid, name, path,
>>      try:
>>          uuid = guid[1:-1]
>>          UUID(uuid, version=4)
>> -    except:
>> +    except ValueError:
>>          return False
> And similarly here (and in general) it is better to catch only the
> exception you expect. BTW I think uuid.UUID() copes with curly-bracket
> guids, so you don't really need the [1:-1]. Also I think
> samba.misc.GUID would do it.
>
>>      lp = LoadParm()
>> @@ -495,12 +496,9 @@ def register_gp_extension(guid, name, path,
>>      parser.set(guid, 'NoMachinePolicy', 0 if machine else 1)
>>      parser.set(guid, 'NoUserPolicy', 0 if user else 1)
>>  
>> -    with NamedTemporaryFile(delete=False) as f:
>> -        if os.path.exists(ext_conf):
>> -            os.remove(ext_conf)
>> +    with NamedTemporaryFile(delete=False, dir=os.path.dirname(ext_conf)) as f:
>>          parser.write(f)
>> -        f.close()
>> -        mv(f.name, ext_conf)
>> +    os.rename(f.name, ext_conf)
> The way you had this was not atomic. The trick is to make the
> replacement on the same file system (which the same directory
> hopefully ensures) then rename it over the old one without deletion.
I used shutil.move because it can copy across file systems, though
simply using the same directory makes more sense.
You have your os.rename nested to the *wrong depth* though. That rename
should be in the scope of the with statement, otherwise you use 'f'
after it's been destroyed.
Also, this fixed needs to be added to the unregister_gp_extension()
function as well (in the next commit).
>
>>  
>>      return True
>>  
>> diff --git a/python/samba/tests/gpo.py b/python/samba/tests/gpo.py
>> index d66f5761fba..77a2323ad75 100644
>> --- a/python/samba/tests/gpo.py
>> +++ b/python/samba/tests/gpo.py
>> @@ -99,6 +99,9 @@ class GPOTests(tests.TestCase):
>>          path = '/usr/local/samba/var/locks/sysvol/../../../../../../root/'
>>          self.assertRaises(OSError, check_safe_path, path)
>>  
>> +        self.assertEqual(check_safe_path('/etc/passwd'), 'etc/passwd')
>> +        self.assertEqual(check_safe_path('\\\\etc/\\passwd'), 'etc/passwd')
>> +
> These tests ensure leading slashes are removed. That is necessary
> because leading slashes restart a path in os.path.join(), which these
> paths get fed into. e.g:
Good catch.
>
>>>> import os
>>>> os.path.join('/path/to/cache/dir', '/etc/passwd')
> '/etc/passwd'
>
>
> Now onto some issues that I haven't fixed:
>  
>> From 7859a8be1a2d9ff04bb1c79e4126a29115152359 Mon Sep 17 00:00:00 2001
>> From: David Mulder <dmulder at suse.com>
>> Date: Wed, 16 May 2018 10:37:09 -0600
>> Subject: [PATCH 05/26] gpo: Offline policy application via cache
>>
>> Read policy files from the cache, rather than
>> the sysvol (unless on a dc, then read from the
>> local sysvol). This enables offline policy
>> apply.
>>
> This comment is now wrong, because you always read from the local
> files?
>
>
>> From 4c0788f94d30391c67db81a5a4261faba4fe918d Mon Sep 17 00:00:00 2001
>> From: David Mulder <dmulder at suse.com>
>> Date: Wed, 13 Jun 2018 14:45:09 -0600
>> Subject: [PATCH 07/26] gpo: add register_gp_extension for registering gp
>>  extensions
>>
>> Signed-off-by: David Mulder <dmulder at suse.com>
>> ---
>>  python/samba/gpclass.py | 37 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/python/samba/gpclass.py b/python/samba/gpclass.py
>> index bc673a0bb23..0105f270cc2 100644
>> --- a/python/samba/gpclass.py
>> +++ b/python/samba/gpclass.py
>> @@ -30,6 +30,9 @@ from samba.net import Net
>>  from samba.dcerpc import nbt
>>  from samba import smb
>>  import samba.gpo as gpo
>> +from samba.param import LoadParm
>> +from uuid import UUID
>> +from tempfile import NamedTemporaryFile
>>  
>>  try:
>>      from enum import Enum
>> @@ -503,3 +506,37 @@ def unapply_gp(lp, creds, test_ldb, logger, store, gp_extensions):
>>              gp_db.delete(str(attr_obj), attr[0])
>>          gp_db.commit()
>>  
>> +def register_gp_extension(guid, name, path,
>> +                          smb_conf=None, machine=True, user=True):
>> +    # Check that the module exists
>> +    if not os.path.exists(path):
>> +        return False
>> +    # Check for valid guid
>> +    if not guid[0] == '{' or not guid[-1] == '}':
>> +        return False
>> +    try:
>> +        uuid = guid[1:-1]
>> +        UUID(uuid, version=4)
>> +    except ValueError:
>> +        return False
> Since you're doing this again and again...
>
>> +
>> +    lp = LoadParm()
>> +    if smb_conf is not None:
>> +        lp.load(smb_conf)
>> +    else:
>> +        lp.load_default()
>> +    ext_conf = lp.state_path('gpext.conf')
>> +    parser = ConfigParser()
>> +    parser.read(ext_conf)
> ...and this, it would be nice to have helper functions.
>
> Something like this:
>
> def parse_gpext_conf(smb_conf):
>     lp = LoadParm()
>     if smb_conf is not None:
>         lp.load(smb_conf)
>     else:
>         lp.load_default()
>     ext_conf = lp.state_path('gpext.conf')
>     parser = ConfigParser()
>     parser.read(ext_conf)
>     return parser
>
> and check_guid() and atomic_write_conf()
>
> which would be easily unittestable.
All good points.
>
>> From d784ee0afc7a779c3217deac77741cb7afd909ab Mon Sep 17 00:00:00 2001
>> From: David Mulder <dmulder at suse.com>
>> Date: Fri, 4 May 2018 14:09:30 -0600
>> Subject: [PATCH 13/26] gpo: gp_sec_ext should check whether to apply
>>
>> Whether an extension should apply should be
>> determined by the extension, not by the
>> calling script.
>>
>> Signed-off-by: David Mulder <dmulder at suse.com>
>> ---
>>  python/samba/gp_sec_ext.py           | 20 ++++++++++++++++++++
>>  python/samba/gpclass.py              | 14 ++++++--------
>>  source4/scripting/bin/samba-gpupdate | 20 +++-----------------
>>  3 files changed, 29 insertions(+), 25 deletions(-)
>>
>> diff --git a/python/samba/gp_sec_ext.py b/python/samba/gp_sec_ext.py
>> index bbd385f73c6..9e1764b7fb9 100644
>> --- a/python/samba/gp_sec_ext.py
>> +++ b/python/samba/gp_sec_ext.py
>> @@ -17,8 +17,17 @@
>>  
>>  import os.path
>>  from gpclass import gp_ext_setter, gp_inf_ext
>> +from samba.auth import system_session
>> +try:
>> +    from samba.samdb import SamDB
>> +except:
>> +    SamDB = None
> I don't understand under what circumstances you wouldn't have Samdb.
> Can you explain that?
We package samba in such a way that the DC code is distinct from the
client code. On a client without the DC package installed, that import
would cause a crash.
>
>>  class inf_to_kdc_tdb(gp_ext_setter):
>> +    def __init__(self, logger, gp_db, lp, attribute, val, ldb):
>> +        super(inf_to_kdc_tdb, self).__init__(logger, gp_db, lp, attribute, val)
>> +        self.ldb = ldb
>> +
>>      def mins_to_hours(self):
>>          return '%d' % (int(self.val)/60)
>>  
>> @@ -53,6 +62,10 @@ class inf_to_ldb(gp_ext_setter):
>>      object to update the parameter to Samba4. Not registry oriented whatsoever.
>>      '''
>>  
>> +    def __init__(self, logger, gp_db, lp, attribute, val, ldb):
>> +        super(inf_to_ldb, self).__init__(logger, gp_db, lp, attribute, val)
>> +        self.ldb = ldb
>> +
>>      def ch_minPwdAge(self, val):
>>          old_val = self.ldb.get_minPwdAge()
>>          self.logger.info('KDC Minimum Password age was changed from %s to %s' \
>> @@ -127,6 +140,13 @@ class gp_sec_ext(gp_inf_ext):
>>          return os.path.join(rootpath, "User/Registry.pol")
>>  
>>      def apply_map(self):
>> +        if SamDB:
>> +            self.ldb = SamDB(self.lp.samdb_url(),
>> +                             session_info=system_session(),
>> +                             credentials=self.creds,
>> +                             lp=self.lp)
>> +        else:
>> +            return {}
> and then from here I'm all a bit lost.
Luke originally wrote the gp_ext_setter class to take the ldb arg
always, which really doesn't make sense on anything but a samba DC. Here
we make those two child classes which inherit from gp_ext_setter take
that argument.
>
>> From 889011b57588d80781e2d330743b1da1bc5c13fb Mon Sep 17 00:00:00 2001
>> From: David Mulder <dmulder at suse.com>
>> Date: Mon, 7 May 2018 10:18:00 -0600
>> Subject: [PATCH 16/26] gpupdate: Don't fail with dc installed
>>
>> Don't fail if the dc is installed, but
>> we're joined as a client.
>>
>> Signed-off-by: David Mulder <dmulder at suse.com>
>> ---
>>  python/samba/gp_sec_ext.py | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/python/samba/gp_sec_ext.py b/python/samba/gp_sec_ext.py
>> index 4f2092ba403..58f6fb66ecd 100644
>> --- a/python/samba/gp_sec_ext.py
>> +++ b/python/samba/gp_sec_ext.py
>> @@ -141,10 +141,13 @@ class gp_sec_ext(gp_inf_ext):
> When it comes to this I just feel things are wrong:
>
>>      def apply_map(self):
>>          if SamDB:
>> -            self.ldb = SamDB(self.lp.samdb_url(),
>> -                             session_info=system_session(),
>> -                             credentials=self.creds,
>> -                             lp=self.lp)
>> +            try:
>> +                self.ldb = SamDB(self.lp.samdb_url(),
>> +                                 session_info=system_session(),
>> +                                 credentials=self.creds,
>> +                                 lp=self.lp)
>> +            except:
>> +                return {}
>>          else:
>>              return {}
>>          return {"System Access": {"MinimumPasswordAge": ("minPwdAge",
> There are little things I can point to, like the excepts should be
> more specific, and there is no need for the if/try nesting -- if SamDB
> is None you'll get an exception anyway -- and the whole thing is
> repeated enough times to be its own function, but I also need some
> convincing about the actual goal here.
This has just been pulled from the samba-gpupdate script. Before this,
we always initialized a samdb, and passed it down to each client side
extension, but this doesn't make sense if we have multiple extensions,
and only one of them is using this samdb.
The idea is, if we don't have SamDB, or if we fail to open the db (we're
joined as a client machine, not as a DC), then fail and don't apply the
extension.
I agree with the points you make though, this could be simplified.
>
>
>> From 9824bb9904856a071fa985ff3374cb82af438808 Mon Sep 17 00:00:00 2001
>> From: David Mulder <dmulder at suse.com>
>> Date: Wed, 9 May 2018 13:16:38 -0600
>> Subject: [PATCH 21/26] gpo: Implement process_group_policy() gp_ext func
>>
>> MS spec describes the policy callback as a
>> function called ProcessGroupPolicy which accepts
>> a pDeletedGPOList and a pChangedGPOList param.
>> The Group Policy Client Side Extension then
>> iterates over the deleted, then the changed gpo
>> lists and applies/unapplies policy. We should do
>> this also.
>>
>> Signed-off-by: David Mulder <dmulder at suse.com>
>> ---
>>  python/samba/gp_sec_ext.py | 35 ++++++++++++++++++-------------
>>  python/samba/gpclass.py    | 43 ++++++++++++++++++++------------------
>> @@ -168,3 +154,24 @@ class gp_sec_ext(gp_inf_ext):
>>                                     }
>>                 }
>>  
>> +    def process_group_policy(self, deleted_gpo_list, changed_gpo_list):
>> +        if SamDB:
>> +            try:
>> +                ldb = SamDB(self.lp.samdb_url(),
>> +                            session_info=system_session(),
>> +                            credentials=self.creds,
>> +                            lp=self.lp)
>> +            except:
>> +                return
>> +        else:
>> +            return
>> +        inf_file = 'MACHINE/Microsoft/Windows NT/SecEdit/GptTmpl.inf'
>> +        for gpo in deleted_gpo_list:
>> +            pass
>                ^^^^
> This loop does nothing, and the next bit looks like it could use
This loop gets implemented in a follow up patch. It's just stubbed out
right now because the backend isn't implemented yet (also in follow up
code).
> check_safe_path(), but I might be wrong:
>
>> +        for gpo in changed_gpo_list:
>> +            if gpo.file_sys_path:
>> +                self.gp_db.set_guid(gpo.name)
>> +                path = gpo.file_sys_path.split('\\sysvol\\')[-1]
>> +                self.parse(os.path.join(path, inf_file))
>> +
check_safe_path() gets added here in follow up code.
> I think I will stop there for now, mainly because it is late on Friday
> afternoon.
>
> Douglas
>

Douglas, I'm hoping to get the patches from merge requests 8, 10, and 15
into 4.9. Merge request 16 can wait. You and Andrew have already
reviewed 8 and 10, so these are likely ready to merge (and are not
dependent on 15 or 16 to merge).

-- 
David Mulder
SUSE Labs Software Engineer - Samba
dmulder at suse.com
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)





More information about the samba-technical mailing list