[PATCH] samba-tool: validate password early in `domain provision`

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
Hi all

Review appreciated.


Thanks,
- Jamie McClymont

0001-samba-tool-validate-password.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
On Mon, 2017-12-04 at 10:57 +1300, Jamie McClymont via samba-technical
wrote:
> Hi all
>
> Review appreciated.

G'Day Jamie,

This looks good.  Could you look into a test for this, so we can ensure
it keeps working?  Both testing the python binding for
check_password_quality() and please test the integration in samba-tool
even better (there is blackbox testing for samba-tool that can scan for
output strings).

Thanks,

Andrew Bartlett


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





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
Hi Andrew

New patch contains the initial change, plus the functionality in
non-interactive mode you requested irl, and unit+blackbox tests.

Thanks,
- Jamie

On 04/12/17 11:26, Andrew Bartlett via samba-technical wrote:
> This looks good.  Could you look into a test for this, so we can ensure
> it keeps working?  Both testing the python binding for
> check_password_quality() and please test the integration in samba-tool
> even better (there is blackbox testing for samba-tool that can scan for
> output strings).


samba-tool-password-validation.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
Hi again

New patch contains squashed changes plus code-style fixes, as requested irl.

Thanks,
- Jamie

On 04/12/17 14:59, Jamie McClymont via samba-technical wrote:
> New patch contains the initial change, plus the functionality in
> non-interactive mode you requested irl, and unit+blackbox tests.

samba-tool-password-validation-squashed-with-style-fixes.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
Ugh, found some spaces in a C file - sorry about that, I'll get my
editor set up properly now!

Thanks,
- Jamie


On 04/12/17 15:46, Jamie McClymont via samba-technical wrote:
> New patch contains squashed changes plus code-style fixes, as requested irl.

samba-tool-password-validation-squashed-with-complete-style-fixes.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
On Monday, 4 December 2017 05:12:19 CET Jamie McClymont via samba-technical
wrote:
> Ugh, found some spaces in a C file - sorry about that, I'll get my
> editor set up properly now!

Do you use vim?

https://github.com/cryptomilk/git-modeline.vim

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Okay, hopefully the final version...

Attached version also fixes an ldap test which was using an invalid
password, allowing the patch to pass all tests.

Thanks
- Jamie

On 04/12/17 15:46, Jamie McClymont via samba-technical wrote:
> New patch contains squashed changes plus code-style fixes, as requested irl.

samba-tool-password-validation-final-with-ldap-test-fix.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
On Wed, 2017-12-06 at 10:31 +1300, Jamie McClymont via samba-technical
wrote:
> Okay, hopefully the final version...
>
> Attached version also fixes an ldap test which was using an invalid
> password, allowing the patch to pass all tests.

Thanks Jamie.  In Samba Team practice, the order of fixes and tests
matters.

That is, make test should nominally pass on each commit.  We don't
force the 4-hour test to run on each, but you should make it pass as
far as is reasonable.

So, mark the openldap thing as knownfail when you put the new
restriction in, then fix it removing the knownfail.

Likewise, in normal cases (where the bug is in the code, not a test),
write the test, set the knownfail, then write the fix removing the
knownfail.

Thanks!

Andrew Bartlett

> Thanks
> - Jamie
>
> On 04/12/17 15:46, Jamie McClymont via samba-technical wrote:
> > New patch contains squashed changes plus code-style fixes, as requested irl.
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
On 06/12/17 11:02, Andrew Bartlett via samba-technical wrote:
> So, mark the openldap thing as knownfail when you put the new
> restriction in, then fix it removing the knownfail.
Got it, fixed in the attached.

Thanks!

samba-tool-password-validation-always-passing-tests.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
On Wed, 2017-12-06 at 13:20 +1300, Jamie McClymont via samba-technical
wrote:
> On 06/12/17 11:02, Andrew Bartlett via samba-technical wrote:
> > So, mark the openldap thing as knownfail when you put the new
> > restriction in, then fix it removing the knownfail.
>
> Got it, fixed in the attached.

This looks good.

Reviewed-by: Andrew Bartlett <[hidden email]>

Can I get a second team reviewer please?

Thanks,

Andrew Bartlett

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





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
You should create a problem specific knownfail file in
selftest/knownfail.d/ named for the problem - such as
selftest/knownfail.d/validate-password-early
Then you don't have to modify the main selftest/knownfail file.
This feature was added to Samba self test a while ago.

See:
https://lists.samba.org/archive/samba-technical/2017-September/122956.html
See:
https://git.samba.org/?p=garming/samba-autobuild/.git;a=blobdiff;f=selftest/knownfail.d/ldap-linked-attributes;fp=selftest/knownfail.d/ldap-linked-attributes;h=0000000000000000000000000000000000000000;hp=90230a2868c5018beb89f6ca983dbdffd9586c90;hb=e999677a42945e4ab48a5936dfe2224e1786530c;hpb=efe953b9d27bec8b997729ac57785b9c005a7aee

On 12/5/2017 7:20 PM, Jamie McClymont via samba-technical wrote:
> On 06/12/17 11:02, Andrew Bartlett via samba-technical wrote:
>> So, mark the openldap thing as knownfail when you put the new
>> restriction in, then fix it removing the knownfail.
> Got it, fixed in the attached.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
On 06/12/17 13:40, jim via samba-technical wrote:
> You should create a problem specific knownfail file in
> selftest/knownfail.d/ named for the problem
Thanks, fixed.

-Jamie

samba-tool-password-validation-always-passing-tests-knownfaild.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
Thanks Jamie,

Just one more thing caught my eye:

On Wed, 2017-12-06 at 13:56 +1300, Jamie McClymont via samba-technical
wrote:
>   def test_searchone(self):

That just needs to be renamed.

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





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
On 06/12/17 14:03, Andrew Bartlett via samba-technical wrote:
> Thanks Jamie,
>
> Just one more thing caught my eye:
>
> On Wed, 2017-12-06 at 13:56 +1300, Jamie McClymont via samba-technical
> wrote:
>>   def test_searchone(self):
>
> That just needs to be renamed.
Whoops, thanks!

samba-tool-password-validation-testname.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] samba-tool: validate password early in `domain provision`

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
With the rename

Reviewed-by: Gary Lockyer <[hidden email]>

On 06/12/17 14:03, Andrew Bartlett via samba-technical wrote:

> Thanks Jamie,
>
> Just one more thing caught my eye:
>
> On Wed, 2017-12-06 at 13:56 +1300, Jamie McClymont via samba-technical
> wrote:
>>   def test_searchone(self):
>
> That just needs to be renamed.
>
> Andrew Bartlett
>


signature.asc (484 bytes) Download Attachment