[PATCH] Patch to implement AD password lockout in Samba's AD DC

[Andrew Bartlett]

On Tue, 2014-03-25 at 14:37 +1300, Andrew Bartlett wrote:
> On Mon, 2014-03-24 at 21:10 +0100, Stefan (metze) Metzmacher wrote:
> > Am 23.03.2014 01:06, schrieb Andrew Bartlett:
> > > On Sat, 2014-03-22 at 23:24 +1300, Andrew Bartlett wrote:
> > >> On Fri, 2014-03-21 at 17:16 +0100, Stefan (metze) Metzmacher wrote:
> > >>> Hi Andrew,
> > >>>
> > >>>>> I've now tested with Windows and Samba and have a patch series at:
> > >>>>>
> > >>>>> http://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/metze-master4-abartlet2
> > >>>>
> > >>>> Updated patches have been pushed!
> > >>>>
> > >>>> Hopefully we are getting closer.
> > >>>
> > >>> I merged this together with my branch
> > >>> and the result can be found at
> > >>>
> > >>> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-abartlet2
> > >>>
> > >>> Please have a look at
> > >>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=a4a8098f2dd374c1109742637bf180b15099d243
> > >>> and
> > >>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=c831e273cfe36c5dff68443391f015c58adf1df7
> > >>>
> > >>> which I reworked at bit. They require your sign-off to be refreshed.
> > >>
> > >> These look good, and much clearer.   Signed-off-by: Andrew Bartlett
> > >> <abartlet at samba.org>
> > >>
> > >>> I need to review and run the tests from
> > >>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=a5eb6e1028ae679a327835239c16ca2d896ec17c
> > >>> again in order to decide about
> > >>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=56dfed17d70048fefd1b7e10c81103109d6e44c8
> > >>
> > >> I think this is now covered.
> > > 
> > > Do be clearer, the in the password_lockout.py test, we use SAMR to
> > > unlock the account, and in that instance the test clearly shows that
> > > values are reset to 0.
> > > 
> > >>> and
> > >>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=5e41a86f5e0421116b15b2a3f9b937ed241a0e3a
> > >>
> > >> badPwdCount is not reset on a successful LDAP password change.  The
> > >> tests of samr code I've just done show that SAMR password changes also
> > >> don't change that.  I don't currently have good enough tests for what
> > >> happens to the lockoutTime, but to get this far it must not be relevant
> > >> (ie in the past), and for LDAP the test I've just added to
> > >> password_lockout.py shows this patch should be dropped, it doesn't
> > >> actually update lockoutTime.
> > >>
> > >>> I've started to look at the tests, but I'm not done yet...
> > > 
> > > BTW, with the patches I sent we pass our own 'make test TESTS=passwords'
> > > on my branch.  Naturally the patches need to be squashed into the
> > > matching commits, but I think we are finally getting very close.
> > 
> > I've updated the password_lockout.py and verified that the current
> > implementation
> > isn't correct.
> > 
> > https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-abartlet2
> > 
> > I think we need to have a trigger in an ldb module that sets badPwdCount = 0
> > is lockoutTime is set to 0.
> > 
> > This matches [MS-SAMR] 3.1.1.8.3 lockoutTime
> > http://msdn.microsoft.com/en-us/library/cc245684.aspx
> > 
> > The SAMR reset of ACB_AUTOLOCK has only impact if
> > the account is locked out. I think it only sets lockoutTime = 0.
> > 
> > I have the feeling that somehow
> > 3.1.1.8.10 userAccountControl
> > http://msdn.microsoft.com/en-us/library/cc245673.aspx
> > is also correct, but I don't know how exactly.
> > 
> > I just found
> > userAccountControl = dsdb.UF_NORMAL_ACCOUNT|dsdb.UF_LOCKOUT
> > doesn't reset, but
> > userAccountControl = dsdb.UF_NORMAL_ACCOUNT
> > does reset lockoutTime = 0 and badPwdCount = 0.
> > 
> > I haven't changed the tests yet, but I've tested it locally.
> 
> Please be careful with the test changes.
> 
> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=6b7ee49dfb4087fd39c2a2e22441e48da4d0ba60 removed the test that unlock for password changes happens after the timeout expires.

Trusting that this much is covered in the other half of the password
change code, I instead focused on how to test all 3 methods, and checked
it against Windows 2012.

See attached.  

I however don't see how we can actually implement your findings on SAMR
with any ease, and would rather diverge from Windows in that area.  I'm
sorry to say it, but now see why I was hesitant to add more tests - this
is a total rat-hole, and a time sink.  I know it is important to get
this right, but the finer details of 0 vs undefined is insane. 

Andrew Bartlett

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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: no-check-samr.patch
Type: text/x-patch
Size: 1245 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140325/13bd9353/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-selftest-Confirm-lockout-behaviour-with-each-method.patch
Type: text/x-patch
Size: 5215 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140325/13bd9353/attachment-0001.bin>