[PATCH] Simplify auth_check_ntlm_password

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

[PATCH] Simplify auth_check_ntlm_password

Volker Lendecke-3
Hi!

While working on the auth code I had a hard time following the logic
in auth_check_ntlm_password. The attached patchset is supposed to make
that more traceable. It does not change behaviour, those patches will
come at a later step and will make the routine even simpler. The
behaviour change will be to always break out of the loop if

!NT_STATUS_EQUAL(nt_status,NT_STATUS_NOT_IMPLEMENTED);

Review appreciated!

Thanks, Volker

patch.txt (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Andrew Bartlett
On Tue, 2017-03-07 at 14:28 +0100, Volker Lendecke wrote:

> Hi!
>
> While working on the auth code I had a hard time following the logic
> in auth_check_ntlm_password. The attached patchset is supposed to
> make
> that more traceable. It does not change behaviour, those patches will
> come at a later step and will make the routine even simpler. The
> behaviour change will be to always break out of the loop if
>
> !NT_STATUS_EQUAL(nt_status,NT_STATUS_NOT_IMPLEMENTED);
>
> Review appreciated!

Thanks Volker, these look like a helpful start to making this stuff
simpler.  

I looked at this set last night, to see how it works with the
authentication audit code (so I could integrate into that series it as
promised).  Putting it in before causes rebase pain, so I'll take a
look at adding it after and see if it is any less difficult.

Otherwise I'll take the concepts and apply them once we are done.  

I hope it is OK to delay these a little, the auth logging patch series
does great things, but hasn't yet got the required review (and is still
improving).  

I know we normally work on a 'race to master' principle, but if you can
wait on this one it will be most appreciated, we don't expect to be
more than a few more days.

Thanks,

Andrew Bartlett


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Jeremy Allison
On Wed, Mar 08, 2017 at 10:04:19AM +1300, Andrew Bartlett wrote:

> On Tue, 2017-03-07 at 14:28 +0100, Volker Lendecke wrote:
> > Hi!
> >
> > While working on the auth code I had a hard time following the logic
> > in auth_check_ntlm_password. The attached patchset is supposed to
> > make
> > that more traceable. It does not change behaviour, those patches will
> > come at a later step and will make the routine even simpler. The
> > behaviour change will be to always break out of the loop if
> >
> > !NT_STATUS_EQUAL(nt_status,NT_STATUS_NOT_IMPLEMENTED);
> >
> > Review appreciated!
>
> Thanks Volker, these look like a helpful start to making this stuff
> simpler.  
>
> I looked at this set last night, to see how it works with the
> authentication audit code (so I could integrate into that series it as
> promised).  Putting it in before causes rebase pain, so I'll take a
> look at adding it after and see if it is any less difficult.
>
> Otherwise I'll take the concepts and apply them once we are done.  

Really ? Does this cause much rebase pain ? Yeah, it'll cause
some, but this really does clean up these code paths.

Looking at the previously posted logging patch I can only
find two places where the logging patch touches source3/auth/auth.c:

-------------------------------------------------------------------
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 50d0188..c1e82a2 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -310,6 +310,10 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
 
        /* failed authentication; check for guest lapping */
 
+       /*
+        * Please try not to change this string, it is probably in use
+        * in audit logging tools
+        */
-------------------------------------------------------------------

and:

-------------------------------------------------------------------
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index c1e82a2..7ca1aae 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -264,6 +264,7 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
        /* successful authentication */
 
        if (NT_STATUS_IS_OK(nt_status)) {
+               struct dom_sid sid = {0};
                unix_username = (*pserver_info)->unix_name;
 
                /* We skip doing this step if the caller asked us not to */
@@ -305,6 +306,19 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
                               unix_username));
                }
 
+               nt_status = get_user_sid_info3_and_extra((*pserver_info)->info3,
+                                                        &(*pserver_info)->extra,
+                                                        &sid);
+               if (!NT_STATUS_IS_OK(nt_status)) {
+                       static const struct dom_sid null_sid = {0};
+                       sid = null_sid;
+               }
+              
+               log_authentication_event(user_info, nt_status,
+                                        (*pserver_info)->info3->base.logon_domain.string,
+                                        (*pserver_info)->info3->base.account_name.string,
+                                        unix_username, &sid);
+
                return nt_status;
        }
 
@@ -317,6 +331,9 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
        DEBUG(2, ("check_ntlm_password:  Authentication for user [%s] -> [%s] FAILED with error %s\n",
                  user_info->client.account_name, user_info->mapped.account_name,
                  nt_errstr(nt_status)));
+      
+       log_authentication_event(user_info, nt_status, NULL, NULL, NULL, NULL);
+      
        ZERO_STRUCTP(pserver_info);
 
        return nt_status;
-------------------------------------------------------------------

These should be a 2 minute clean-up in a rebase to add
them to the now 2 authoritative places where this function
now collects all the possible returns.

If you can point me at the current logging patchset I'll
even do the rebase for you on top of this patch and send
it as a new patchset to you.

I understand not wanting conflicts, but this one really
should be trivial to solve.

Cheers,

        Jeremy.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Volker Lendecke-3
On Tue, Mar 07, 2017 at 01:52:28PM -0800, Jeremy Allison wrote:
> +               if (!NT_STATUS_IS_OK(nt_status)) {
> +                       static const struct dom_sid null_sid = {0};
> +                       sid = null_sid;
> +               }

While there -- please don't ever add this kind of static. We have a
null sid, we also can do sid = (struct dom_sid) {0}. But not this kind
of static please.

Thanks,

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Jeremy Allison
On Tue, Mar 07, 2017 at 11:39:15PM +0100, Volker Lendecke wrote:
> On Tue, Mar 07, 2017 at 01:52:28PM -0800, Jeremy Allison wrote:
> > +               if (!NT_STATUS_IS_OK(nt_status)) {
> > +                       static const struct dom_sid null_sid = {0};
> > +                       sid = null_sid;
> > +               }
>
> While there -- please don't ever add this kind of static. We have a
> null sid, we also can do sid = (struct dom_sid) {0}. But not this kind
> of static please.

Yeah, I noticed the 'static' too when looking at the
needed rebase. But this is a review comment once it
gets to that stage - it's not been proposed for merge
yet :-).

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Andrew Bartlett
In reply to this post by Volker Lendecke-3
On Tue, 2017-03-07 at 23:39 +0100, Volker Lendecke wrote:

> On Tue, Mar 07, 2017 at 01:52:28PM -0800, Jeremy Allison wrote:
> > +               if (!NT_STATUS_IS_OK(nt_status)) {
> > +                       static const struct dom_sid null_sid = {0};
> > +                       sid = null_sid;
> > +               }
>
> While there -- please don't ever add this kind of static. We have a
> null sid, we also can do sid = (struct dom_sid) {0}. But not this
> kind
> of static please.

Thanks, that is a much nicer construct!

Andrew Bartlett

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Andrew Bartlett
In reply to this post by Jeremy Allison
On Tue, 2017-03-07 at 13:52 -0800, Jeremy Allison wrote:

> On Wed, Mar 08, 2017 at 10:04:19AM +1300, Andrew Bartlett wrote:
> > On Tue, 2017-03-07 at 14:28 +0100, Volker Lendecke wrote:
> > > Hi!
> > >
> > > While working on the auth code I had a hard time following the
> > > logic
> > > in auth_check_ntlm_password. The attached patchset is supposed to
> > > make
> > > that more traceable. It does not change behaviour, those patches
> > > will
> > > come at a later step and will make the routine even simpler. The
> > > behaviour change will be to always break out of the loop if
> > >
> > > !NT_STATUS_EQUAL(nt_status,NT_STATUS_NOT_IMPLEMENTED);
> > >
> > > Review appreciated!
> >
> > Thanks Volker, these look like a helpful start to making this stuff
> > simpler.  
> >
> > I looked at this set last night, to see how it works with the
> > authentication audit code (so I could integrate into that series it
> > as
> > promised).  Putting it in before causes rebase pain, so I'll take a
> > look at adding it after and see if it is any less difficult.
> >
> > Otherwise I'll take the concepts and apply them once we are done.  
>
> Really ? Does this cause much rebase pain ? Yeah, it'll cause
> some, but this really does clean up these code paths.

I'm not objecting to the patch, just saying I will try and add it on
top of the branch rather than underneath it.  I did actually try the
rebase last night, and perhaps you have better luck, but I've had
enough of these go bad on me that I would just like to apply it on top,
if that is OK.

> Looking at the previously posted logging patch I can only
> find two places where the logging patch touches source3/auth/auth.c:

We keep our repo public, but we haven't been spamming the list as we
go.  The current code is in auth-logging-ntlm in the catalyst git repo.

git://git.catalyst.net.nz/samba.git auth-logging-ntlm

> These should be a 2 minute clean-up in a rebase to add
> them to the now 2 authoritative places where this function
> now collects all the possible returns.
>
> If you can point me at the current logging patchset I'll
> even do the rebase for you on top of this patch and send
> it as a new patchset to you.

Thanks, that would be most appreciated.

> I understand not wanting conflicts, but this one really
> should be trivial to solve.

I hope so - and the offer stands to apply it after:  I'm not objecting
to the patches, just trying to get as much audit logging done as
possible in the available time.  You know how long we have been waiting
for it :-)

Thanks,

Andrew Bartlett

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Jeremy Allison
On Wed, Mar 08, 2017 at 01:59:18PM +1300, Andrew Bartlett wrote:

> On Tue, 2017-03-07 at 13:52 -0800, Jeremy Allison wrote:
> > On Wed, Mar 08, 2017 at 10:04:19AM +1300, Andrew Bartlett wrote:
> > > On Tue, 2017-03-07 at 14:28 +0100, Volker Lendecke wrote:
> > > > Hi!
> > > >
> > > > While working on the auth code I had a hard time following the
> > > > logic
> > > > in auth_check_ntlm_password. The attached patchset is supposed to
> > > > make
> > > > that more traceable. It does not change behaviour, those patches
> > > > will
> > > > come at a later step and will make the routine even simpler. The
> > > > behaviour change will be to always break out of the loop if
> > > >
> > > > !NT_STATUS_EQUAL(nt_status,NT_STATUS_NOT_IMPLEMENTED);
> > > >
> > > > Review appreciated!
> > >
> > > Thanks Volker, these look like a helpful start to making this stuff
> > > simpler.  
> > >
> > > I looked at this set last night, to see how it works with the
> > > authentication audit code (so I could integrate into that series it
> > > as
> > > promised).  Putting it in before causes rebase pain, so I'll take a
> > > look at adding it after and see if it is any less difficult.
> > >
> > > Otherwise I'll take the concepts and apply them once we are done.  
> >
> > Really ? Does this cause much rebase pain ? Yeah, it'll cause
> > some, but this really does clean up these code paths.
>
> I'm not objecting to the patch, just saying I will try and add it on
> top of the branch rather than underneath it.  I did actually try the
> rebase last night, and perhaps you have better luck, but I've had
> enough of these go bad on me that I would just like to apply it on top,
> if that is OK.
>
> > Looking at the previously posted logging patch I can only
> > find two places where the logging patch touches source3/auth/auth.c:
>
> We keep our repo public, but we haven't been spamming the list as we
> go.  The current code is in auth-logging-ntlm in the catalyst git repo.
>
> git://git.catalyst.net.nz/samba.git auth-logging-ntlm
Here is your patch rebased on top of Volker's work applied to
master.

It only took 10 mins or so (so I exaggerated about 2 mins :-).

I didn't fix the 'static' issue as I only wanted to show your
exact code on top of what I'm proposing to push.

I haven't compiled it yet :-).

Doing the rebase really isn't that hard - it'd be good for
Gary to learn this skill now rather than later !

Cheers,

        Jeremy.

rebase-log-patch1 (280K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Andrew Bartlett
On Tue, 2017-03-07 at 17:46 -0800, Jeremy Allison wrote:

> On Wed, Mar 08, 2017 at 01:59:18PM +1300, Andrew Bartlett wrote:
> > On Tue, 2017-03-07 at 13:52 -0800, Jeremy Allison wrote:
> > > On Wed, Mar 08, 2017 at 10:04:19AM +1300, Andrew Bartlett wrote:
> > > > On Tue, 2017-03-07 at 14:28 +0100, Volker Lendecke wrote:
> > > > > Hi!
> > > > >
> > > > > While working on the auth code I had a hard time following
> > > > > the
> > > > > logic
> > > > > in auth_check_ntlm_password. The attached patchset is
> > > > > supposed to
> > > > > make
> > > > > that more traceable. It does not change behaviour, those
> > > > > patches
> > > > > will
> > > > > come at a later step and will make the routine even simpler.
> > > > > The
> > > > > behaviour change will be to always break out of the loop if
> > > > >
> > > > > !NT_STATUS_EQUAL(nt_status,NT_STATUS_NOT_IMPLEMENTED);
> > > > >
> > > > > Review appreciated!
> > > >
> > > > Thanks Volker, these look like a helpful start to making this
> > > > stuff
> > > > simpler.  
> > > >
> > > > I looked at this set last night, to see how it works with the
> > > > authentication audit code (so I could integrate into that
> > > > series it
> > > > as
> > > > promised).  Putting it in before causes rebase pain, so I'll
> > > > take a
> > > > look at adding it after and see if it is any less difficult.
> > > >
> > > > Otherwise I'll take the concepts and apply them once we are
> > > > done.  
> > >
> > > Really ? Does this cause much rebase pain ? Yeah, it'll cause
> > > some, but this really does clean up these code paths.
> >
> > I'm not objecting to the patch, just saying I will try and add it
> > on
> > top of the branch rather than underneath it.  I did actually try
> > the
> > rebase last night, and perhaps you have better luck, but I've had
> > enough of these go bad on me that I would just like to apply it on
> > top,
> > if that is OK.
> >
> > > Looking at the previously posted logging patch I can only
> > > find two places where the logging patch touches
> > > source3/auth/auth.c:
> >
> > We keep our repo public, but we haven't been spamming the list as
> > we
> > go.  The current code is in auth-logging-ntlm in the catalyst git
> > repo.
> >
> > git://git.catalyst.net.nz/samba.git auth-logging-ntlm
>
> Here is your patch rebased on top of Volker's work applied to
> master.
>
> It only took 10 mins or so (so I exaggerated about 2 mins :-).
>
> I didn't fix the 'static' issue as I only wanted to show your
> exact code on top of what I'm proposing to push.
>
> I haven't compiled it yet :-).
>
> Doing the rebase really isn't that hard - it'd be good for
> Gary to learn this skill now rather than later !

Thanks for trying.  Sadly it looks like our repo and what you rebased
have already diverged, and I can't make much sense of it.  I've tried a
number of tacks, but I'm going to have to ask you take a look at it
again later, as I'm just loosing time.

Sorry,

Andrew Bartlett


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Jeremy Allison
On Wed, Mar 08, 2017 at 04:02:53PM +1300, Andrew Bartlett wrote:

> > I didn't fix the 'static' issue as I only wanted to show your
> > exact code on top of what I'm proposing to push.
> >
> > I haven't compiled it yet :-).
> >
> > Doing the rebase really isn't that hard - it'd be good for
> > Gary to learn this skill now rather than later !
>
> Thanks for trying.  Sadly it looks like our repo and what you rebased
> have already diverged, and I can't make much sense of it.  I've tried a
> number of tacks, but I'm going to have to ask you take a look at it
> again later, as I'm just loosing time.

What ? I just did a git clone of your repo, did a git checkout of
the branch you specified and then rebased the code from that on top
of the master branch with Volker's patch applied.

e.g.

git clone git://git.catalyst.net.nz/samba.git auth-logging-ntlm
cd auth-logging-ntlm
git checkout auth-logging-ntlm

Then did the rebase work. Did I misunderstand
how to get the correct code you're working
on ?

As I said, it took about 10 minutes. Did
I screw up getting the right code ?

If you have a current master, you should
now be able to do:

git am /tmp/vl-patch
git am /tmp/rebase-log-patch1

and you should end up with your patchset
(rebase-log-patch1) on top of Volker's.

I'll try and take a look tomorrow when
I'm back in at work, but this is a problem
rebasing on a changing master) that your
team will need to learn to cope with going
forward because it will keep happening with
large out-of-tree patchsets.

As a procedural issue we've never really
allowed people to 'reserve' an area of
code as being worked on unless there are
extreme (i.e. security) circumstances. I
don't think this is good engineering practice
either. Once a patch is ready and reviewed,
it's the responsibility of the out-of-tree code
maintainer to rebase on top of what goes
into master.

Volker's patchset is simple, clean and
does a cleanup of the function it works
on, so I really think it needs to go in
first, and your logging patchset should
be rebased on top.

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Jeremy Allison
On Tue, Mar 07, 2017 at 08:54:04PM -0800, Jeremy Allison wrote:

> On Wed, Mar 08, 2017 at 04:02:53PM +1300, Andrew Bartlett wrote:
> > > I didn't fix the 'static' issue as I only wanted to show your
> > > exact code on top of what I'm proposing to push.
> > >
> > > I haven't compiled it yet :-).
> > >
> > > Doing the rebase really isn't that hard - it'd be good for
> > > Gary to learn this skill now rather than later !
> >
> > Thanks for trying.  Sadly it looks like our repo and what you rebased
> > have already diverged, and I can't make much sense of it.  I've tried a
> > number of tacks, but I'm going to have to ask you take a look at it
> > again later, as I'm just loosing time.
>
> What ? I just did a git clone of your repo, did a git checkout of
> the branch you specified and then rebased the code from that on top
> of the master branch with Volker's patch applied.
>
> e.g.
>
> git clone git://git.catalyst.net.nz/samba.git auth-logging-ntlm
> cd auth-logging-ntlm
> git checkout auth-logging-ntlm
>
> Then did the rebase work. Did I misunderstand
> how to get the correct code you're working
> on ?
>
> As I said, it took about 10 minutes. Did
> I screw up getting the right code ?

FYI, In case I'm not being direct enough - I
*really* *do* want the logging patchset finished :-),
and I'll do the work to rebase it for you guys
this time so it works on top of master+vl-cleanup
(but please learn to do it yourselves for next
time please).

Can you send me an idiot-proof 'cut-and-paste'
command set I can use to make sure I get the
guarenteed correct code you want me to rebase ?

It won't take me long to get something useful
for you once I know I'm working on the right
code :-).

Cheers,

        Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Andreas Schneider-15
On Wednesday, 8 March 2017 05:59:59 CET Jeremy Allison wrote:

> On Tue, Mar 07, 2017 at 08:54:04PM -0800, Jeremy Allison wrote:
> > On Wed, Mar 08, 2017 at 04:02:53PM +1300, Andrew Bartlett wrote:
> > > > I didn't fix the 'static' issue as I only wanted to show your
> > > > exact code on top of what I'm proposing to push.
> > > >
> > > > I haven't compiled it yet :-).
> > > >
> > > > Doing the rebase really isn't that hard - it'd be good for
> > > > Gary to learn this skill now rather than later !
> > >
> > > Thanks for trying.  Sadly it looks like our repo and what you rebased
> > > have already diverged, and I can't make much sense of it.  I've tried a
> > > number of tacks, but I'm going to have to ask you take a look at it
> > > again later, as I'm just loosing time.
> >
> > What ? I just did a git clone of your repo, did a git checkout of
> > the branch you specified and then rebased the code from that on top
> > of the master branch with Volker's patch applied.
> >
> > e.g.
> >
> > git clone git://git.catalyst.net.nz/samba.git auth-logging-ntlm
> > cd auth-logging-ntlm
> > git checkout auth-logging-ntlm
> >
> > Then did the rebase work. Did I misunderstand
> > how to get the correct code you're working
> > on ?
> >
> > As I said, it took about 10 minutes. Did
> > I screw up getting the right code ?
>
> FYI, In case I'm not being direct enough - I
> *really* *do* want the logging patchset finished :-),
> and I'll do the work to rebase it for you guys
> this time so it works on top of master+vl-cleanup
> (but please learn to do it yourselves for next
> time please).
>
> Can you send me an idiot-proof 'cut-and-paste'
> command set I can use to make sure I get the
> guarenteed correct code you want me to rebase ?
>
> It won't take me long to get something useful
> for you once I know I'm working on the right
> code :-).

Wow, if I just had known this earlier. I would have let *you* do all the
rebases of the master-mit-kdc branch over the last years.

I didn't know what you offer these rebase services!



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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Jeremy Allison
On Wed, Mar 08, 2017 at 08:21:53AM +0100, Andreas Schneider wrote:

> On Wednesday, 8 March 2017 05:59:59 CET Jeremy Allison wrote:
> > On Tue, Mar 07, 2017 at 08:54:04PM -0800, Jeremy Allison wrote:
> > > On Wed, Mar 08, 2017 at 04:02:53PM +1300, Andrew Bartlett wrote:
> > > > > I didn't fix the 'static' issue as I only wanted to show your
> > > > > exact code on top of what I'm proposing to push.
> > > > >
> > > > > I haven't compiled it yet :-).
> > > > >
> > > > > Doing the rebase really isn't that hard - it'd be good for
> > > > > Gary to learn this skill now rather than later !
> > > >
> > > > Thanks for trying.  Sadly it looks like our repo and what you rebased
> > > > have already diverged, and I can't make much sense of it.  I've tried a
> > > > number of tacks, but I'm going to have to ask you take a look at it
> > > > again later, as I'm just loosing time.
> > >
> > > What ? I just did a git clone of your repo, did a git checkout of
> > > the branch you specified and then rebased the code from that on top
> > > of the master branch with Volker's patch applied.
> > >
> > > e.g.
> > >
> > > git clone git://git.catalyst.net.nz/samba.git auth-logging-ntlm
> > > cd auth-logging-ntlm
> > > git checkout auth-logging-ntlm
> > >
> > > Then did the rebase work. Did I misunderstand
> > > how to get the correct code you're working
> > > on ?
> > >
> > > As I said, it took about 10 minutes. Did
> > > I screw up getting the right code ?
> >
> > FYI, In case I'm not being direct enough - I
> > *really* *do* want the logging patchset finished :-),
> > and I'll do the work to rebase it for you guys
> > this time so it works on top of master+vl-cleanup
> > (but please learn to do it yourselves for next
> > time please).
> >
> > Can you send me an idiot-proof 'cut-and-paste'
> > command set I can use to make sure I get the
> > guarenteed correct code you want me to rebase ?
> >
> > It won't take me long to get something useful
> > for you once I know I'm working on the right
> > code :-).
>
> Wow, if I just had known this earlier. I would have let *you* do all the
> rebases of the master-mit-kdc branch over the last years.
>
> I didn't know what you offer these rebase services!

It's a once-only-training offer. You can get your
free coupon from me at SambaXP :-).

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Andrew Bartlett
In reply to this post by Jeremy Allison
On Tue, 2017-03-07 at 20:59 -0800, Jeremy Allison wrote:

> On Tue, Mar 07, 2017 at 08:54:04PM -0800, Jeremy Allison wrote:
> > On Wed, Mar 08, 2017 at 04:02:53PM +1300, Andrew Bartlett wrote:
> > > > I didn't fix the 'static' issue as I only wanted to show your
> > > > exact code on top of what I'm proposing to push.
> > > >
> > > > I haven't compiled it yet :-).
> > > >
> > > > Doing the rebase really isn't that hard - it'd be good for
> > > > Gary to learn this skill now rather than later !
> > >
> > > Thanks for trying.  Sadly it looks like our repo and what you
> > > rebased
> > > have already diverged, and I can't make much sense of it.  I've
> > > tried a
> > > number of tacks, but I'm going to have to ask you take a look at
> > > it
> > > again later, as I'm just loosing time.
> >
> > What ? I just did a git clone of your repo, did a git checkout of
> > the branch you specified and then rebased the code from that on top
> > of the master branch with Volker's patch applied.
> >
> > e.g.
> >
> > git clone git://git.catalyst.net.nz/samba.git auth-logging-ntlm
> > cd auth-logging-ntlm
> > git checkout auth-logging-ntlm
> >
> > Then did the rebase work. Did I misunderstand
> > how to get the correct code you're working
> > on ?
> >
> > As I said, it took about 10 minutes. Did
> > I screw up getting the right code ?
>
> FYI, In case I'm not being direct enough - I
> *really* *do* want the logging patchset finished :-),
> and I'll do the work to rebase it for you guys
> this time so it works on top of master+vl-cleanup
> (but please learn to do it yourselves for next
> time please).
>
> Can you send me an idiot-proof 'cut-and-paste'
> command set I can use to make sure I get the
> guarenteed correct code you want me to rebase ?
>
> It won't take me long to get something useful
> for you once I know I'm working on the right
> code :-).

I think we were literally in a race condition during the NZ work day,
and I didn't expect you were intending to work on it until the CA
morning, so didn't try to coordinate more closely.  (It looks like we
re-shuffled some patches just as you were working).

Normally this kind of collaboration works best with fix-up patches, but
that isn't a viable approach for rebase failures.

The commands you ran look right, I'll let you know when we are done
today.

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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Jeremy Allison
On Thu, Mar 09, 2017 at 07:12:59AM +1300, Andrew Bartlett wrote:

> >
> > FYI, In case I'm not being direct enough - I
> > *really* *do* want the logging patchset finished :-),
> > and I'll do the work to rebase it for you guys
> > this time so it works on top of master+vl-cleanup
> > (but please learn to do it yourselves for next
> > time please).
> >
> > Can you send me an idiot-proof 'cut-and-paste'
> > command set I can use to make sure I get the
> > guarenteed correct code you want me to rebase ?
> >
> > It won't take me long to get something useful
> > for you once I know I'm working on the right
> > code :-).
>
> I think we were literally in a race condition during the NZ work day,
> and I didn't expect you were intending to work on it until the CA
> morning, so didn't try to coordinate more closely.  (It looks like we
> re-shuffled some patches just as you were working).
>
> Normally this kind of collaboration works best with fix-up patches, but
> that isn't a viable approach for rebase failures.
>
> The commands you ran look right, I'll let you know when we are done
> today.
>
> Thanks!

No worries. I might not get to the rebase today (I have
to host the winners of a "hacking" contest at Google
today :-) but tomorrow should work fine.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Simplify auth_check_ntlm_password

Andreas Schneider-15
In reply to this post by Jeremy Allison
On Wednesday, 8 March 2017 17:51:20 CET Jeremy Allison wrote:
> > Wow, if I just had known this earlier. I would have let *you* do all the
> > rebases of the master-mit-kdc branch over the last years.
> >
> > I didn't know what you offer these rebase services!
>
> It's a once-only-training offer. You can get your
> free coupon from me at SambaXP :-).

I will ask you there! ;-)