gp: PAM Access should implicitly deny ALL w/ allow

If an allow entry is specified, the PAM Access
CSE should implicitly deny ALL (everyone other
than the explicit allow entries).

Signed-off-by: David Mulder <dmulder@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>

diff --git a/python/samba/gp/vgp_access_ext.py b/python/samba/gp/vgp_access_ext.py
index efd91ef93fb5298686341a71bfc5ad1c8b4ddc10..474d5e278ee302079d77375e295616a8f424335a 100644 (file)
--- a/python/samba/gp/vgp_access_ext.py
+++ b/python/samba/gp/vgp_access_ext.py
@@ -19,6 +19,7 @@ from samba.gp.gpclass import gp_xml_ext
 from hashlib import blake2b
 from tempfile import NamedTemporaryFile
 from samba.common import get_bytes
+from samba.gp.util.logging import log
 
 intro = '''
 ### autogenerated by samba
@@ -31,11 +32,34 @@ intro = '''
 
 '''
 
+# The deny all file is implicit any time an allow entry is used
+DENY_BOUND = 9000000000
+DENY_FILE = '_gp_DENY_ALL.conf'
+
+# Each policy MUST create it's own DENY_ALL file if an allow entry exists,
+# otherwise policies will conflict and one could remove a DENY_ALL when another
+# one still requires it.
+def deny_file(access):
+    deny_filename = os.path.join(access,
+                        '%d%s' % (select_next_deny(access), DENY_FILE))
+    with NamedTemporaryFile(delete=False, dir=access) as f:
+        with open(f.name, 'w') as w:
+            w.write(intro)
+            w.write('-:ALL:ALL')
+        os.chmod(f.name, 0o644)
+        os.rename(f.name, deny_filename)
+    return deny_filename
+
+def select_next_deny(directory):
+    configs = [re.match(r'(\d+)', f) for f in os.listdir(directory) if DENY_FILE in f]
+    return max([int(m.group(1)) for m in configs if m]+[DENY_BOUND])+1
+
 # Access files in /etc/security/access.d are read in the order of the system
 # locale. Here we number the conf files to ensure they are read in the correct
-# order.
+# order. Ignore the deny file, since allow entries should always come before
+# the implicit deny ALL.
 def select_next_conf(directory):
-    configs = [re.match(r'(\d+)', f) for f in os.listdir(directory)]
+    configs = [re.match(r'(\d+)', f) for f in os.listdir(directory) if DENY_FILE not in f]
     return max([int(m.group(1)) for m in configs if m]+[0])+1
 
 class vgp_access_ext(gp_xml_ext):
@@ -47,9 +71,10 @@ class vgp_access_ext(gp_xml_ext):
         for guid, settings in deleted_gpo_list:
             self.gp_db.set_guid(guid)
             if str(self) in settings:
-                for attribute, access_file in settings[str(self)].items():
-                    if os.path.exists(access_file):
-                        os.unlink(access_file)
+                for attribute, policy_files in settings[str(self)].items():
+                    for access_file in policy_files.split(':'):
+                        if os.path.exists(access_file):
+                            os.unlink(access_file)
                     self.gp_db.delete(str(self), attribute)
             self.gp_db.commit()
 
@@ -63,14 +88,20 @@ class vgp_access_ext(gp_xml_ext):
                 path = os.path.join(gpo.file_sys_path, deny)
                 deny_conf = self.parse(path)
                 entries = []
+                policy_files = []
                 if allow_conf:
                     policy = allow_conf.find('policysetting')
                     data = policy.find('data')
-                    for listelement in data.findall('listelement'):
+                    allow_listelements = data.findall('listelement')
+                    for listelement in allow_listelements:
                         adobject = listelement.find('adobject')
                         name = adobject.find('name').text
                         domain = adobject.find('domain').text
                         entries.append('+:%s\\%s:ALL' % (domain, name))
+                    if len(allow_listelements) > 0:
+                        log.info('Adding an implicit deny ALL because an allow'
+                                 ' entry is present')
+                        policy_files.append(deny_file(access))
                 if deny_conf:
                     policy = deny_conf.find('policysetting')
                     data = policy.find('data')
@@ -79,10 +110,14 @@ class vgp_access_ext(gp_xml_ext):
                         name = adobject.find('name').text
                         domain = adobject.find('domain').text
                         entries.append('-:%s\\%s:ALL' % (domain, name))
+                        if len(allow_listelements) > 0:
+                            log.warn("Deny entry '%s' is meaningless with "
+                                     "allow present" % entries[-1])
                 if len(entries) == 0:
                     continue
                 conf_id = select_next_conf(access)
                 access_file = os.path.join(access, '%010d_gp.conf' % conf_id)
+                policy_files.append(access_file)
                 access_contents = '\n'.join(entries)
                 attribute = blake2b(get_bytes(access_contents)).hexdigest()
                 old_val = self.gp_db.retrieve(str(self), attribute)
@@ -96,7 +131,7 @@ class vgp_access_ext(gp_xml_ext):
                         w.write(access_contents)
                     os.chmod(f.name, 0o644)
                     os.rename(f.name, access_file)
-                self.gp_db.store(str(self), attribute, access_file)
+                self.gp_db.store(str(self), attribute, ':'.join(policy_files))
                 self.gp_db.commit()
 
     def rsop(self, gpo):
@@ -113,13 +148,16 @@ class vgp_access_ext(gp_xml_ext):
             if allow_conf:
                 policy = allow_conf.find('policysetting')
                 data = policy.find('data')
-                for listelement in data.findall('listelement'):
+                allow_listelements = data.findall('listelement')
+                for listelement in allow_listelements:
                     adobject = listelement.find('adobject')
                     name = adobject.find('name').text
                     domain = adobject.find('domain').text
                     if str(self) not in output.keys():
                         output[str(self)] = []
                     output[str(self)].append('+:%s\\%s:ALL' % (name, domain))
+                if len(allow_listelements) > 0:
+                    output[str(self)].append('-:ALL:ALL')
             if deny_conf:
                 policy = deny_conf.find('policysetting')
                 data = policy.find('data')

diff --git a/python/samba/netcmd/gpo.py b/python/samba/netcmd/gpo.py
index 2bcee59bd932b1e38468ec2d8136ecfd34849335..9cd08273aaadf38aa0a35a2c0a36e6c7af98eff1 100644 (file)
--- a/python/samba/netcmd/gpo.py
+++ b/python/samba/netcmd/gpo.py
@@ -3815,7 +3815,8 @@ class cmd_add_access(Command):
     """Adds a VGP Host Access Group Policy to the sysvol
 
 This command adds a host access setting to the sysvol for applying to winbind
-clients.
+clients. Any time an allow entry is detected by the client, an implicit deny
+ALL will be assumed.
 
 Example:
 samba-tool gpo manage access add {31B2F340-016D-11D2-945F-00C04FB984F9} allow goodguy example.com