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

Mon Mar 24 22:26:25 MDT 2014

On Tue, 2014-03-25 at 17:00 +1300, Andrew Bartlett wrote:
> 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. 

This patch fixes up the timestamps to increment more often. 

The rest of the test issues shown up I still feel are just one big
rathole, exposing more about tiny details of how AD is implemented than
useful changes we have to mirror. 

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: 0001-dsdb-Make-dsdb_update_bad_pwd_count-use-higher-resol.patch
Type: text/x-patch
Size: 1268 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140325/90e94a3b/attachment.bin>