PATCHES: Password sync as active directory domain controller

Andrew Bartlett abartlet at samba.org
Wed Jul 20 23:04:23 UTC 2016


On Thu, 2016-07-21 at 06:54 +1200, Andrew Bartlett wrote:
> 
> I'll continue my review today.
> 
> Thank you for your incredible patience!

I'm sorry to raise this now, but this routine needs rework:


+        def check_current_pid_conflict(pid):
+            if pid is None:
+                return False
+
+            mypid = os.getpid()
+            if pid == mypid:
+                return False
+
+            try:
+                os.kill(pid, 0)
+            except OSError as (num, msg):
+                if num != errno.ESRCH:
+                    raise
+                return False
+
+            p = Popen("grep -q 'syncpasswords' /proc/%d/cmdline" %
pid, shell=True)
+            res = p.wait()
+            if res != 0:
+                return False
+            return True

We can't have a grep of files in /proc in Samba.  Sorry.

If you want mutual exclusion between the scripts, can you use a
transaction lock over the ldb, or fcntl locks on another file?

Otherwise, the main thing I see is that autobuild doesn't run with --
with-gpgme.  

Finally, after the https://evil32.com/ episode, we should not accept 8-
char key ids.  Please update the docs to only suggest 16-char IDs, and
make the code refuse to accept < 16-char IDs.

Otherwise, I'm finally OK with all this.  There is still a lot of code
here, and some of it is pretty dense, but these are the only objections
I have so far.  I really appreciate the efforts you have gone to to
address my concerns.

I'm going to look over the KDC changes now, and hope to give a final
review on all this soon!

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT   
https://catalyst.net.nz/services/samba







More information about the samba-technical mailing list