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

[Andrew Bartlett]

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. 

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