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

[Stefan (metze) Metzmacher]

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.

metze