[PATCH] Fix 'net ads changetrustpw'

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
Hi,

as always, untested code is broken code. So I broke 'net ads changetrustpw' in
Samba 4.6.

The attached patch fixes the issue and adds a tests which verifies that it is
working.


Please review and push if OK.


Thanks,


        Andreas

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

fix_net_ads_changetrustpw.patch.txt (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On Wed, 09 Aug 2017 18:45:10 +0200
Andreas Schneider via samba-technical <[hidden email]>
wrote:

> Hi,
>
> as always, untested code is broken code. So I broke 'net ads
> changetrustpw' in Samba 4.6.
>
> The attached patch fixes the issue and adds a tests which verifies
> that it is working.
>
>
> Please review and push if OK.
>
>
> Thanks,
>
>
> Andreas
>

OK, I asked Andrew this and got a very non committal answer, so this
seems to a good place to ask everyone, why does Samba use the ancient
backticks ?

i.e `expr $failed + 1` instead of $((failed + 1))

Rowland
 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On Wednesday, 9 August 2017 19:03:34 CEST Rowland Penny via samba-technical
wrote:

> On Wed, 09 Aug 2017 18:45:10 +0200
> Andreas Schneider via samba-technical <[hidden email]>
>
> wrote:
> > Hi,
> >
> > as always, untested code is broken code. So I broke 'net ads
> > changetrustpw' in Samba 4.6.
> >
> > The attached patch fixes the issue and adds a tests which verifies
> > that it is working.
> >
> >
> > Please review and push if OK.
> >
> >
> > Thanks,
> >
> > Andreas
>
> OK, I asked Andrew this and got a very non committal answer, so this
> seems to a good place to ask everyone, why does Samba use the ancient
> backticks ?
>
> i.e `expr $failed + 1` instead of $((failed + 1))

I normally use $() but as this is copy and paste it is using the old way. On
newer scripts you can see $() in my scripts at least.

If you want to can start changing it in all scripts :-)


        Andreas

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On Thu, 10 Aug 2017 09:30:26 +0200
Andreas Schneider <[hidden email]> wrote:

> On Wednesday, 9 August 2017 19:03:34 CEST Rowland Penny via
> samba-technical wrote:
> > On Wed, 09 Aug 2017 18:45:10 +0200
> > Andreas Schneider via samba-technical
> > <[hidden email]>
> >
> > wrote:
> > > Hi,
> > >
> > > as always, untested code is broken code. So I broke 'net ads
> > > changetrustpw' in Samba 4.6.
> > >
> > > The attached patch fixes the issue and adds a tests which verifies
> > > that it is working.
> > >
> > >
> > > Please review and push if OK.
> > >
> > >
> > > Thanks,
> > >
> > > Andreas
> >
> > OK, I asked Andrew this and got a very non committal answer, so this
> > seems to a good place to ask everyone, why does Samba use the
> > ancient backticks ?
> >
> > i.e `expr $failed + 1` instead of $((failed + 1))
>
> I normally use $() but as this is copy and paste it is using the old
> way. On newer scripts you can see $() in my scripts at least.
>
> If you want to can start changing it in all scripts :-)
>
>
> Andreas
>

Yes, but where do I stop ?
Have you ever run any of the test sh scripts through shellcheck ?

It also sort of backs up up what Andrew has been saying lately, people
will just cut and paste bad code.
It wouldn't be allowed in a python, so why in sh ?

Rowland

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On Thursday, 10 August 2017 10:20:41 CEST Rowland Penny via samba-technical
wrote:
> Yes, but where do I stop ?
> Have you ever run any of the test sh scripts through shellcheck ?
>
> It also sort of backs up up what Andrew has been saying lately, people
> will just cut and paste bad code.
> It wouldn't be allowed in a python, so why in sh ?

I think the shell scripts need a cleanup alltogether and we should have much
more common function which are shared. Someone needs to start. Maybe $() is a
start ...

The problem is time. We do not have the resources to rework all of that.
Normally I try to do it when I write new stuff.


        Andreas

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On Thu, 10 Aug 2017 14:49:04 +0200
Andreas Schneider <[hidden email]> wrote:

> On Thursday, 10 August 2017 10:20:41 CEST Rowland Penny via
> samba-technical wrote:
> > Yes, but where do I stop ?
> > Have you ever run any of the test sh scripts through shellcheck ?
> >
> > It also sort of backs up up what Andrew has been saying lately,
> > people will just cut and paste bad code.
> > It wouldn't be allowed in a python, so why in sh ?
>
> I think the shell scripts need a cleanup alltogether and we should
> have much more common function which are shared. Someone needs to
> start. Maybe $() is a start ...
>
> The problem is time. We do not have the resources to rework all of
> that. Normally I try to do it when I write new stuff.
>
>
> Andreas
>

Time is what I have a lot of ;-)

Okay, stand by for a deluge as I run every 'sh' script through
shellcheck.

Rowland

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Aug 9, 2017 at 9:45 AM, Andreas Schneider via samba-technical
<[hidden email]> wrote:

> Hi,
>
> as always, untested code is broken code. So I broke 'net ads changetrustpw' in
> Samba 4.6.
>
> The attached patch fixes the issue and adds a tests which verifies that it is
> working.
>
>
> Please review and push if OK.

I have a couple of questions. While it looks OK,

1. I can't see why you changed princ to NULL in the following:

          ret = krb5_set_password(context,
                                                  &creds,
                                                  discard_const_p(char, newpw),
                                                  - princ,
                                                  + NULL,
                                                  &result_code,
                                                  &result_code_string,
                                                  &result_string);

Perhaps it's because we just got a changepw ticket for the princ, so
it is established in the creds who we are changing the password for.

2. Can you fix the indentation in the call to krb5_set_password?

Other than that, I think the backticks are fine until someone changes
them all to bash style $(...) for all tests.

So, conditional RB+ by me.

--
Regards,
Richard Sharpe
(何以解憂?唯有杜康。--曹操)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On Friday, 11 August 2017 02:06:15 CEST Richard Sharpe wrote:

> On Wed, Aug 9, 2017 at 9:45 AM, Andreas Schneider via samba-technical
>
> <[hidden email]> wrote:
> > Hi,
> >
> > as always, untested code is broken code. So I broke 'net ads
> > changetrustpw' in Samba 4.6.
> >
> > The attached patch fixes the issue and adds a tests which verifies that it
> > is working.
> >
> >
> > Please review and push if OK.
>
> I have a couple of questions. While it looks OK,
>
> 1. I can't see why you changed princ to NULL in the following:
>
>           ret = krb5_set_password(context,
>                                                   &creds,
>                                                   discard_const_p(char,
> newpw), - princ,
>                                                   + NULL,
>                                                   &result_code,
>                                                   &result_code_string,
>                                                   &result_string);
>
> Perhaps it's because we just got a changepw ticket for the princ, so
> it is established in the creds who we are changing the password for.

If we 'set' a password the client principal and the target principal are
different. That isn't the case, we change our own password so we need to pass
NULL.

>
> 2. Can you fix the indentation in the call to krb5_set_password?

What indentation? It looks correct to me. You know that the patch tool adds a
+ at the beginning so the indentation with tabs can look strange in patch
files. Please apply the patch and check the source code :-)

> Other than that, I think the backticks are fine until someone changes
> them all to bash style $(...) for all tests.
>
> So, conditional RB+ by me.


Thanks,


        Andreas

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On Thu, Aug 10, 2017 at 10:47 PM, Andreas Schneider <[hidden email]> wrote:

> On Friday, 11 August 2017 02:06:15 CEST Richard Sharpe wrote:
>> On Wed, Aug 9, 2017 at 9:45 AM, Andreas Schneider via samba-technical
>>
>> <[hidden email]> wrote:
>> > Hi,
>> >
>> > as always, untested code is broken code. So I broke 'net ads
>> > changetrustpw' in Samba 4.6.
>> >
>> > The attached patch fixes the issue and adds a tests which verifies that it
>> > is working.
>> >
>> >
>> > Please review and push if OK.
>>
>> I have a couple of questions. While it looks OK,
>>
>> 1. I can't see why you changed princ to NULL in the following:
>>
>>           ret = krb5_set_password(context,
>>                                                   &creds,
>>                                                   discard_const_p(char,
>> newpw), - princ,
>>                                                   + NULL,
>>                                                   &result_code,
>>                                                   &result_code_string,
>>                                                   &result_string);
>>
>> Perhaps it's because we just got a changepw ticket for the princ, so
>> it is established in the creds who we are changing the password for.
>
> If we 'set' a password the client principal and the target principal are
> different. That isn't the case, we change our own password so we need to pass
> NULL.
>
>>
>> 2. Can you fix the indentation in the call to krb5_set_password?
>
> What indentation? It looks correct to me. You know that the patch tool adds a
> + at the beginning so the indentation with tabs can look strange in patch
> files. Please apply the patch and check the source code :-)

OK, I can see what is happening. The indentation is wrong for the
whole file, at least in current master (pulled last night) as shown by
the attached screen shot.

In that case: RB+

--
Regards,
Richard Sharpe
(何以解憂?唯有杜康。--曹操)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On Fri, Aug 11, 2017 at 7:30 AM, Richard Sharpe
<[hidden email]> wrote:

> On Thu, Aug 10, 2017 at 10:47 PM, Andreas Schneider <[hidden email]> wrote:
>> On Friday, 11 August 2017 02:06:15 CEST Richard Sharpe wrote:
>>> On Wed, Aug 9, 2017 at 9:45 AM, Andreas Schneider via samba-technical
>>>
>>> <[hidden email]> wrote:
>>> > Hi,
>>> >
>>> > as always, untested code is broken code. So I broke 'net ads
>>> > changetrustpw' in Samba 4.6.
>>> >
>>> > The attached patch fixes the issue and adds a tests which verifies that it
>>> > is working.
>>> >
>>> >
>>> > Please review and push if OK.
>>>
>>> I have a couple of questions. While it looks OK,
>>>
>>> 1. I can't see why you changed princ to NULL in the following:
>>>
>>>           ret = krb5_set_password(context,
>>>                                                   &creds,
>>>                                                   discard_const_p(char,
>>> newpw), - princ,
>>>                                                   + NULL,
>>>                                                   &result_code,
>>>                                                   &result_code_string,
>>>                                                   &result_string);
>>>
>>> Perhaps it's because we just got a changepw ticket for the princ, so
>>> it is established in the creds who we are changing the password for.
>>
>> If we 'set' a password the client principal and the target principal are
>> different. That isn't the case, we change our own password so we need to pass
>> NULL.
>>
>>>
>>> 2. Can you fix the indentation in the call to krb5_set_password?
>>
>> What indentation? It looks correct to me. You know that the patch tool adds a
>> + at the beginning so the indentation with tabs can look strange in patch
>> files. Please apply the patch and check the source code :-)
>
> OK, I can see what is happening. The indentation is wrong for the
> whole file, at least in current master (pulled last night) as shown by
> the attached screen shot.
>
> In that case: RB+
With attachment this time:

--
Regards,
Richard Sharpe
(何以解憂?唯有杜康。--曹操)

krb5_setpw.c (211K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On pe, 11 elo 2017, Richard Sharpe via samba-technical wrote:

> On Fri, Aug 11, 2017 at 7:30 AM, Richard Sharpe
> <[hidden email]> wrote:
> > On Thu, Aug 10, 2017 at 10:47 PM, Andreas Schneider <[hidden email]> wrote:
> >> On Friday, 11 August 2017 02:06:15 CEST Richard Sharpe wrote:
> >>> On Wed, Aug 9, 2017 at 9:45 AM, Andreas Schneider via samba-technical
> >>>
> >>> <[hidden email]> wrote:
> >>> > Hi,
> >>> >
> >>> > as always, untested code is broken code. So I broke 'net ads
> >>> > changetrustpw' in Samba 4.6.
> >>> >
> >>> > The attached patch fixes the issue and adds a tests which verifies that it
> >>> > is working.
> >>> >
> >>> >
> >>> > Please review and push if OK.
> >>>
> >>> I have a couple of questions. While it looks OK,
> >>>
> >>> 1. I can't see why you changed princ to NULL in the following:
> >>>
> >>>           ret = krb5_set_password(context,
> >>>                                                   &creds,
> >>>                                                   discard_const_p(char,
> >>> newpw), - princ,
> >>>                                                   + NULL,
> >>>                                                   &result_code,
> >>>                                                   &result_code_string,
> >>>                                                   &result_string);
> >>>
> >>> Perhaps it's because we just got a changepw ticket for the princ, so
> >>> it is established in the creds who we are changing the password for.
> >>
> >> If we 'set' a password the client principal and the target principal are
> >> different. That isn't the case, we change our own password so we need to pass
> >> NULL.
> >>
> >>>
> >>> 2. Can you fix the indentation in the call to krb5_set_password?
> >>
> >> What indentation? It looks correct to me. You know that the patch tool adds a
> >> + at the beginning so the indentation with tabs can look strange in patch
> >> files. Please apply the patch and check the source code :-)
> >
> > OK, I can see what is happening. The indentation is wrong for the
> > whole file, at least in current master (pulled last night) as shown by
> > the attached screen shot.
> >
> > In that case: RB+
>
> With attachment this time:
You have attached a binary blob, not a C source file.

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On Fri, Aug 11, 2017 at 10:43 AM, Alexander Bokovoy <[hidden email]> wrote:

> On pe, 11 elo 2017, Richard Sharpe via samba-technical wrote:
>> On Fri, Aug 11, 2017 at 7:30 AM, Richard Sharpe
>> <[hidden email]> wrote:
>> > On Thu, Aug 10, 2017 at 10:47 PM, Andreas Schneider <[hidden email]> wrote:
>> >> On Friday, 11 August 2017 02:06:15 CEST Richard Sharpe wrote:
>> >>> On Wed, Aug 9, 2017 at 9:45 AM, Andreas Schneider via samba-technical
>> >>>
>> >>> <[hidden email]> wrote:
>> >>> > Hi,
>> >>> >
>> >>> > as always, untested code is broken code. So I broke 'net ads
>> >>> > changetrustpw' in Samba 4.6.
>> >>> >
>> >>> > The attached patch fixes the issue and adds a tests which verifies that it
>> >>> > is working.
>> >>> >
>> >>> >
>> >>> > Please review and push if OK.
>> >>>
>> >>> I have a couple of questions. While it looks OK,
>> >>>
>> >>> 1. I can't see why you changed princ to NULL in the following:
>> >>>
>> >>>           ret = krb5_set_password(context,
>> >>>                                                   &creds,
>> >>>                                                   discard_const_p(char,
>> >>> newpw), - princ,
>> >>>                                                   + NULL,
>> >>>                                                   &result_code,
>> >>>                                                   &result_code_string,
>> >>>                                                   &result_string);
>> >>>
>> >>> Perhaps it's because we just got a changepw ticket for the princ, so
>> >>> it is established in the creds who we are changing the password for.
>> >>
>> >> If we 'set' a password the client principal and the target principal are
>> >> different. That isn't the case, we change our own password so we need to pass
>> >> NULL.
>> >>
>> >>>
>> >>> 2. Can you fix the indentation in the call to krb5_set_password?
>> >>
>> >> What indentation? It looks correct to me. You know that the patch tool adds a
>> >> + at the beginning so the indentation with tabs can look strange in patch
>> >> files. Please apply the patch and check the source code :-)
>> >
>> > OK, I can see what is happening. The indentation is wrong for the
>> > whole file, at least in current master (pulled last night) as shown by
>> > the attached screen shot.
>> >
>> > In that case: RB+
>>
>> With attachment this time:
> You have attached a binary blob, not a C source file.
Sigh, Misnamed it. It should have been .jpg

It shows the weird indenting in that the file krb5_setpw.c

--
Regards,
Richard Sharpe
(何以解憂?唯有杜康。--曹操)

krb5_setpw.kpg (211K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix 'net ads changetrustpw'

Samba - samba-technical mailing list
On Fri, 2017-08-11 at 11:52 -0700, Richard Sharpe via samba-technical
wrote:

> On Fri, Aug 11, 2017 at 10:43 AM, Alexander Bokovoy <[hidden email]> wrote:
> > On pe, 11 elo 2017, Richard Sharpe via samba-technical wrote:
> > > On Fri, Aug 11, 2017 at 7:30 AM, Richard Sharpe
> > > <[hidden email]> wrote:
> > > >
> > > > OK, I can see what is happening. The indentation is wrong for the
> > > > whole file, at least in current master (pulled last night) as shown by
> > > > the attached screen shot.
> > > >
> > > > In that case: RB+
> > >
> > > With attachment this time:
> >
> > You have attached a binary blob, not a C source file.
>
> Sigh, Misnamed it. It should have been .jpg
>
> It shows the weird indenting in that the file krb5_setpw.c

Samba uses 8-space tabs, but large chunks of this file does not seem to
comply, probably because the contents were copied from somewhere in
Heimdal. 

I can see now why it burns your eyes ;-)

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


Loading...