[PATCHES] rename samba_gpoupdate to gpupdate

David Mulder dmulder at suse.com
Fri Jul 6 16:45:53 UTC 2018


Douglas, I've touched up all the issues you mentioned, and also the ones
I mentioned to you in my response.
One thing, I checked on needing check_safe_path() in gp_sec_ext.py, and
I was mistaken. It actually calls check_safe_path() on that path in the
parse function, but the sysvol path split was unnecessary (since it's
handled by check_safe_path()), so I removed that.

I've pushed everything to the same branch at

https://gitlab.com/samba-team/devel/samba/tree/david-mulder-gpo



On 07/06/2018 07:57 AM, David Mulder via samba-technical wrote:
>
> 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