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

Sun Mar 16 22:19:06 MDT 2014

On Mon, 2014-03-17 at 00:07 +1300, Andrew Bartlett wrote:
> On Tue, 2014-03-11 at 19:36 +0100, Stefan (metze) Metzmacher wrote:
> > Am 11.03.2014 05:03, schrieb Andrew Bartlett:
> > > On Tue, 2014-03-04 at 12:40 +0100, Stefan (metze) Metzmacher wrote:
> > >> Am 04.03.2014 02:09, schrieb Andrew Bartlett:
> > >>> On Mon, 2014-03-03 at 16:26 +1300, Andrew Bartlett wrote:
> > >>>> On Tue, 2014-02-25 at 15:16 +1300, Andrew Bartlett wrote:
> > >>>>> On Tue, 2014-02-25 at 09:34 +1300, Andrew Bartlett wrote:
> > >>>>>> On Thu, 2014-02-20 at 10:16 +1300, Andrew Bartlett wrote:
> > >>>>>>> On Wed, 2014-02-19 at 22:08 +0100, Stefan (metze) Metzmacher wrote:
> > >>>>>>>> Hi Andrew,
> > >>>>>>>>
> > >>>>>>>>>> http://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/s4-bwdPwdCount-01
> > >>>>>>>>>
> > >>>>>>>>> I've updated the branch at 
> > >>>>>>>>>
> > >>>>>>>>> git://git.samba.org/abartlet/samba.git s4-badPwdCount-02
> > >>>>>>>>>
> > >>>>>>>>> http://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/s4-bwdPwdCount-02
> > >>>>>>>>>
> > >>>>>>>>> I have also uploaded these to gerrit at
> > >>>>>>>>> https://gerrit.sernet.de/#/q/status:open+project:samba+branch:master
> > >>>>>>>>> +topic:abartlet/s4-badPwdCount-02,n,z
> > >>>>>>>>>
> > >>>>>>>>> With the tests now finished, these changes are now ready for master.
> > >>>>>>>>>
> > >>>>>>>>> I will separately co-ordinate with the Heimdal team and work out how we
> > >>>>>>>>> can detect the correct Heimdal version, and look at updating our
> > >>>>>>>>> internal Heimdal.  (The reality is that only Debian builds against a
> > >>>>>>>>> system Heimdal, and we already have another special patch to cope with
> > >>>>>>>>> using a modern heimdal). 
> > >>>>>>>>>
> > >>>>>>>>> Please review/push.
> > >>>>>>>>
> > >>>>>>>> I'll have a look at them in detail tomorrow, but my first impression is that
> > >>>>>>>> you should use more helper variables, avoid deep indentation levels (try
> > >>>>>>>> to use early returns and avoid } else { if the if section calls return)
> > >>>>>>>> and avoid functions calls in the variable declaration section.
> > >>>>>>>
> > >>>>>>> I would appreciate that in detail, as it is a large patch set and so
> > >>>>>>> hard to guess exactly what chunks you feel can be improved.
> > >>>>>>>
> > >>>>>>> The password_hash change was particularly challenging to do in good
> > >>>>>>> style, and practical suggestions for how to improve it would be most
> > >>>>>>> welcome. 
> > >>>>>>>
> > >>>>>>>> Were is the password grace period documented in the microsoft docs?
> > >>>>>>>
> > >>>>>>> http://support.microsoft.com/kb/906305
> > >>>>>>
> > >>>>>> Just a quick note to say I've seen the (very detailed and specific!)
> > >>>>>> reviews in gerrit, but just haven't had a chance to make the required
> > >>>>>> changes or to otherwise comment.
> > >>>>>
> > >>>>> I've addressed most (but certainly not all) of the comments.  In short,
> > >>>>> I disagree with sending an error when the badPwdCount is updated or not,
> > >>>>> but I've addressed most of the extra, other than adding more tests.
> > >>>>>
> > >>>>> Attached is the diff between the old and new patch series. 
> > >>>>
> > >>>> Any chance we can get at least some of these in to master soon?  I know
> > >>>> the re-re-re-view process in gerrit must have been frustrating, is there
> > >>>> anything I can do to assist?
> > >>>>
> > >>>> BTW, the 60 min grace thing is also tested in the samlogon test, which
> > >>>> already verifies that it only applies to 'network' and not 'interactive'
> > >>>> SamLogon calls.
> > >>>
> > >>> I have addressed as much of the issues raised as I'm able to at the
> > >>> moment, and I think it is time to get this series into master.
> > >>>
> > >>> Can I please have the series in my s4-badPwdCount-02 branch (and in
> > >>> gerrit) reviewed and pushed into master please.
> > >>
> > >> I'm looking at it again now and continue tomorrow.
> > > 
> > > I have again addressed as much of the issues that I can at the moment. 
> > > 
> > > To be honest, having worked on this area since October, I'm more than a
> > > little exhausted of this area, but I have made another attempt to
> > > address as many of the structural concerns as I can.  In particular,
> > > I've broken out helper functions in auth_sam.c and password_hash.c
> > > 
> > > I have not extended the tests nor done extra tests as suggested - I've
> > > found that testing this area is a bottomless pit (hence why they are
> > > already so extensive), and I'm just out of time and energy for this.
> > > Naturally additional tests are welcome if you or someone else has the
> > > time, but I would suggest first we should get our own 'source3' code to
> > > pass our own tests again, as a better application of the effort. 
> > > 
> > > The issues of formatting I've left alone, and I would ask that if you
> > > really want to specify the exact formatting that you please download the
> > > patch set with:
> > > 
> > > git fetch https://gerrit.samba.org/samba refs/changes/87/87/6 && git
> > > checkout FETCH_HEAD
> > > 
> > > And then re-submit them with the attached script.
> > > 
> > > I do wish we had a sensible way these minor issues could be dealt with
> > > directly in the web interface.
> > > 
> > > Finally, I do appreciate your time and patience on this, and know this
> > > must be just as frustrating for you as it is for me.  If you would
> > > prefer I worked with someone else to finish the review work please let
> > > me know.
> > 
> > I'll have a look at it tomorrow.
> 
> Just following up to the list that metze and I have made some good
> progress.  I've expanded our tests to cover some of the issues raised,
> and found/fixed the resulting failures in our code, and metze has sorted
> out the formatting to be to his satisfaction. 
> 
> The outstanding issue I'll address before I re-push patches tomorrow is
> that we fail to update the badPwdCount in the AD DC when we don't have a
> password history, when changing a password.  This is quite odd, but it's
> too late tonight to diagnose that further.  I'll work on this more
> tomorrow. 

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

Thanks!

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