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

[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.

Thanks,

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: gerrit-review
Type: application/x-shellscript
Size: 684 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140311/c165bd08/attachment.bin>