[PATCH] Correctly handle !authoritative in the rpc-based auth backends

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

[PATCH] Correctly handle !authoritative in the rpc-based auth backends

Volker Lendecke-3
Hi!

This is independently correct, but is quite ineffective so far. The
core auth backend loops don't do this yet, but I want to make the
final patchset smaller.

Review appreciated!

Thanks, Volker

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Andrew Bartlett
On Thu, 2017-03-09 at 13:36 +0100, Volker Lendecke wrote:
> Hi!
>
> This is independently correct, but is quite ineffective so far. The
> core auth backend loops don't do this yet, but I want to make the
> final patchset smaller.
>
> Review appreciated!

Looks good to me!

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

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
|  
Report Content as Inappropriate

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Volker Lendecke-3
On Fri, Mar 10, 2017 at 07:30:27AM +1300, Andrew Bartlett wrote:

> On Thu, 2017-03-09 at 13:36 +0100, Volker Lendecke wrote:
> > Hi!
> >
> > This is independently correct, but is quite ineffective so far. The
> > core auth backend loops don't do this yet, but I want to make the
> > final patchset smaller.
> >
> > Review appreciated!
>
> Looks good to me!
>
> Reviewed-by: Andrew Bartlett <[hidden email]>
>
> Thanks!
Thank you for looking :-)

Because you asked earlier: Attached find the whole patchset I'm
working on. The local netlogond use of winbind makes it easier in the
rodc case. The routing is clear, we don't need to special-case in the
auth methods for that. This version has not yet survived a full
autobuild. Without the last 3 ones it failed at the very end in the
rodc tests, so I added them again. Now those tests pass individually.

BTW, there's already precedence for connecting to netlogond locally:
In 5602377fc we've added that for a different call, but it's there.

We need to talk about full bisectability with make test. That will be
difficult to achieve for this patchset. We need to mark many tests as
failing in between and unmark them again. Or one big patch.

Volker

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Andrew Bartlett
On Thu, 2017-03-09 at 19:48 +0100, Volker Lendecke wrote:

> On Fri, Mar 10, 2017 at 07:30:27AM +1300, Andrew Bartlett wrote:
> > On Thu, 2017-03-09 at 13:36 +0100, Volker Lendecke wrote:
> > > Hi!
> > >
> > > This is independently correct, but is quite ineffective so far.
> > > The
> > > core auth backend loops don't do this yet, but I want to make the
> > > final patchset smaller.
> > >
> > > Review appreciated!
> >
> > Looks good to me!
> >
> > Reviewed-by: Andrew Bartlett <[hidden email]>
> >
> > Thanks!
>
> Thank you for looking :-)
>
> Because you asked earlier: Attached find the whole patchset I'm
> working on. The local netlogond use of winbind makes it easier in the
> rodc case.

OK.  I can see that being the case for our current (rather poor) tests,
and I do expect this would improve things in the real world.
  
Honestly the tests really need to improve (and that is one of the jobs
I have coming up). 

Just for thought: If we put an RODC on (say) a busy squid proxy, how
would ntlm_auth go to the sam.ldb for cached passwords, but fallback to
the real DC for others?

The other case we have to support, which is more difficult, is that
when we are an RODC or even a full DC, we should present wrong password
to the PDC emulator.  On the RODC this allows the bad password count to
be updated (otherwise it can't be stored anywhere), and on an RW DC it
is intended to give the user a second chance to authenticate with their
new password before replication catches up.

> The routing is clear, we don't need to special-case in the
> auth methods for that. This version has not yet survived a full
> autobuild. Without the last 3 ones it failed at the very end in the
> rodc tests, so I added them again. Now those tests pass individually.

OK, that is very interesting.

I totally agree on the need for clear routing.  The previous design was
(sadly) a deliberate obfuscation in places, designed to pretend the AD
DC didn't exist (hidden behind auth methods) in winbindd.  I'm glad to
get past that.

I would really like a clear, different route for SMB or NETLOGON
(trusted domain) authentication assistance (perhaps via IRPC from
auth_winbind, which could be used to make the auth stack async) as for
the ntlm_auth or wbinfo -a handler.  

The SMB or NETLOGON case has already checked the password locally, so
should never talk to the auth subsystem again, it must just go directly
to a remote DC.  The ntlm_auth case might be trying a local login.

> BTW, there's already precedence for connecting to netlogond locally:
> In 5602377fc we've added that for a different call, but it's there.
>
> We need to talk about full bisectability with make test. That will be
> difficult to achieve for this patchset. We need to mark many tests as
> failing in between and unmark them again. Or one big patch.

Thanks for the explanation and the patches.  They look really good!  It
is great to get this dealt with properly, and I look forward to the
final set.

I take it that "auth methods" now only controls the smb connection
handler?

The pdbtest patch looks wrong, we have been testing the different auth
methods via that tool, so fixing it to 'sam' seems to be limiting what
we are testing.

Regarding the last patch, my main thought is that I want the call to
netlogon not to happen at all, unless we are servicing wbinfo or
ntlm_auth.  I certainly appreciate that by going to netlogon the auth
module list can be fixed, which certainly has advantages.  

When checking local passwords, I like the API call for the ability to
pass in additional data (like the source IP of the auth I've added in
the logging patches), but we could:
 - use a private custom IRPC
 - or just not worry for this particular case

It would be good to also consider the trusted domains case in netlogon,
even if that is only experimental at this point.

Thank you so much for sharing your work with progress with me.  I hope
you take these comments in the constructive manner in which they were
intended.

Thanks,

Andrew Bartlett


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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Volker Lendecke-3
On Fri, Mar 10, 2017 at 05:46:58PM +1300, Andrew Bartlett wrote:
> Just for thought: If we put an RODC on (say) a busy squid proxy, how
> would ntlm_auth go to the sam.ldb for cached passwords, but fallback to
> the real DC for others?

We have two entry points into winbind. One for ntlm_auth/smbd, and one
for netlogond. ntlm_auth/smbd right now go through the pipe, but this
is not set in stone, the main point is that netlogond has a special
entry into winbind, the irpc interface.

The flow would be:

ntlm_auth goes to winbind through the pipe. As part of the pipe
handler we ask netlogond over NCACN_UNIX. netlogond looks at dsdb,
finds that the password is cached, returns with ok/failure and
authoritative=1. If password is not cached, it returns
no_such_user/authoritative=0. Then winbind can go to the rw-dc. This
is a decision that netlogond has to make.

It is basically the same decision that netlogond has to do for domain
name and usn suffix searches. A non-cached password from this
perspective is the same as an unknown domain name.

Now what to do if netlogond has to authenticate a real client from the
network: netlogond can see which transport was used for the auth
request. When it's not local unix, then in the domain unknown /
password not cached case it asks winbind over the irpc interface. The
handler for this interface does not have the "look at local
netlogond" code.

Essentially what we need is a way for winbind to tell netlogond that
it should not ask winbind over irpc in the unkown domain / uncached
pwd case but return NO_SUCH_USER/!authoritative. How we do that is
mechanics. Different socket name for ncacn_unix, different schannel
type, metze even mentioned that a new, private dcerpc interface idl
would be possible. A new rpc interface could also enable passing more
information to netlogon for logging purposes. Client IP address for
example comes to mind.

> The other case we have to support, which is more difficult, is that
> when we are an RODC or even a full DC, we should present wrong password
> to the PDC emulator.  On the RODC this allows the bad password count to
> be updated (otherwise it can't be stored anywhere), and on an RW DC it
> is intended to give the user a second chance to authenticate with their
> new password before replication catches up.

Every logon failure against a DC via netlogon triggers a callback to
the PDC emulator done by the DC? Wow, I did not know that. Need to
think about it.

> I would really like a clear, different route for SMB or NETLOGON
> (trusted domain) authentication assistance (perhaps via IRPC from
> auth_winbind, which could be used to make the auth stack async) as for
> the ntlm_auth or wbinfo -a handler.  

If you look at one of the patches it has a comment:

                /*
                 * This is the place to asynchronously do a winbind
                 * trusted domain query
                 */

For netlogond4 I would do the irpc call asynchronously directly at
this piece of code. We don't have to make auth_check_password async to
get the trusted domains async without a nested event loop for this.

> The SMB or NETLOGON case has already checked the password locally, so
> should never talk to the auth subsystem again, it must just go directly
> to a remote DC.  The ntlm_auth case might be trying a local login.

Please explain. For the AD DC, smbd would go to winbind even for local
authentication. If this turns out to be a performance problem, we can
introduce a call from smbd directly into netlogond ncacn_unix. On an
AD DC, we have the huge advantage to always have winbind around, and
we should make use of this.

> Thanks for the explanation and the patches.  They look really good!  It
> is great to get this dealt with properly, and I look forward to the
> final set.
>
> I take it that "auth methods" now only controls the smb connection
> handler?

Yep. If you ask me, I would ditch the "auth methods" parameter from
smb.conf. I would really like to see the use cases that make them
necessary.

> The pdbtest patch looks wrong, we have been testing the different auth
> methods via that tool, so fixing it to 'sam' seems to be limiting what
> we are testing.

Well, it does survive autobuild.

> Regarding the last patch, my main thought is that I want the call to
> netlogon not to happen at all, unless we are servicing wbinfo or
> ntlm_auth.  I certainly appreciate that by going to netlogon the auth
> module list can be fixed, which certainly has advantages.  
>
> When checking local passwords, I like the API call for the ability to
> pass in additional data (like the source IP of the auth I've added in
> the logging patches), but we could:
>  - use a private custom IRPC
>  - or just not worry for this particular case

See above.

My main reason for winbind to do netlogon rpc locally for sam auth is
that it reduces the number of places we call into the auth subsystem
loop. I really want to have canonical ways into the auth subsystem and
get them right and fast once and for all cases. And we have to get the
SamLogon call 100% right and fast anyway, so why not use it locally
too?

From a progress perspective I'd call the patchset pretty much done and
ready for master. It survives autobuild and enables correct fallback
for !authoritative. Right, for ntlm_auth in the rodc case we don't look
locally, but the rodc is not a fully supported configuration yet.

The deeper reason why I want this is in rather sooner than later is
the removal of the call to is_trusted_domain() in the source3 auth
path. That call is just wrong, but to get it through autobuild I had
to cleanup the DC side of things.

Volker

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Stefan Metzmacher-2
Hi,

> We have two entry points into winbind. One for ntlm_auth/smbd, and one
> for netlogond. ntlm_auth/smbd right now go through the pipe, but this
> is not set in stone, the main point is that netlogond has a special
> entry into winbind, the irpc interface.
>
> The flow would be:
>
> ntlm_auth goes to winbind through the pipe. As part of the pipe
> handler we ask netlogond over NCACN_UNIX.

Volker, I think you should use 'NCALRPC' instead of NCACN_UNIX_STREAM
in your patchset :-)

> Essentially what we need is a way for winbind to tell netlogond that
> it should not ask winbind over irpc in the unkown domain / uncached
> pwd case but return NO_SUCH_USER/!authoritative. How we do that is
> mechanics. Different socket name for ncacn_unix, different schannel
> type, metze even mentioned that a new, private dcerpc interface idl
> would be possible. A new rpc interface could also enable passing more
> information to netlogon for logging purposes. Client IP address for
> example comes to mind.

I think it would be better to use custom calls in both directions,
from winbindd to the netlogon server and the other direction.

BTW: what's the error message from an RODC for an existing user
without cached password if no RWDC is reachable?

>> The other case we have to support, which is more difficult, is that
>> when we are an RODC or even a full DC, we should present wrong password
>> to the PDC emulator.  On the RODC this allows the bad password count to
>> be updated (otherwise it can't be stored anywhere), and on an RW DC it
>> is intended to give the user a second chance to authenticate with their
>> new password before replication catches up.
>
> Every logon failure against a DC via netlogon triggers a callback to
> the PDC emulator done by the DC? Wow, I did not know that. Need to
> think about it.
Andrew, are you referring to the netr_LogonSendToSam() calls shown in
[MS-ADOD] ?

3.2.5 Example 5: Change a User Account's Password Against a Non-PDC DC
https://msdn.microsoft.com/en-us/library/hh872148.aspx

and

3.2.6 Example 6: Update the User's lastLogOnTimeStamp Against an RODC
When the User Binds to an LDAP Server
https://msdn.microsoft.com/en-us/library/hh872036.aspx

I guess logonCount, badPwdCount and badPasswordTime are still maintained
on the RODC itself as these attributes are marked with
FLAG_ATTR_NOT_REPLICATED.

It would be interesting to see what a non-pdc emulator will do with
failing LogonSamLogon calls and if there's a difference between
interactive and network
logons.

metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Andrew Bartlett
In reply to this post by Volker Lendecke-3
On Fri, 2017-03-10 at 15:08 +0100, Volker Lendecke wrote:
> On Fri, Mar 10, 2017 at 05:46:58PM +1300, Andrew Bartlett wrote:
> >
> > The pdbtest patch looks wrong, we have been testing the different
> > auth
> > methods via that tool, so fixing it to 'sam' seems to be limiting
> > what
> > we are testing.
>
> Well, it does survive autobuild.

Sure, but that is because you remove what it is testing.  pdbtest is
acting as the driver for a sort of unit test of the auth subsystem, as
controlled by 'auth methods'.  The tests set auth methods to various
values to try and test those modules.

This was added to ensure we didn't have untested code in the auth
subsystem and to avoid relying on indirect tests.

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
|  
Report Content as Inappropriate

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
On Sat, Mar 11, 2017 at 08:31:36AM +1300, Andrew Bartlett wrote:

> On Fri, 2017-03-10 at 15:08 +0100, Volker Lendecke wrote:
> > On Fri, Mar 10, 2017 at 05:46:58PM +1300, Andrew Bartlett wrote:
> > >
> > > The pdbtest patch looks wrong, we have been testing the different
> > > auth
> > > methods via that tool, so fixing it to 'sam' seems to be limiting
> > > what
> > > we are testing.
> >
> > Well, it does survive autobuild.
>
> Sure, but that is because you remove what it is testing.  pdbtest is
> acting as the driver for a sort of unit test of the auth subsystem, as
> controlled by 'auth methods'.  The tests set auth methods to various
> values to try and test those modules.
>
> This was added to ensure we didn't have untested code in the auth
> subsystem and to avoid relying on indirect tests.

https://git.samba.org/?p=vl/samba.git/.git;h=refs/heads/auth

has fixes for this issue.

Comments?

Volker

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
In reply to this post by Stefan Metzmacher-2
On Fri, 2017-03-10 at 16:31 +0100, Stefan Metzmacher wrote:
> Hi,

> > Essentially what we need is a way for winbind to tell netlogond
> > that
> > it should not ask winbind over irpc in the unkown domain / uncached
> > pwd case but return NO_SUCH_USER/!authoritative. How we do that is
> > mechanics. Different socket name for ncacn_unix, different schannel
> > type, metze even mentioned that a new, private dcerpc interface idl
> > would be possible. A new rpc interface could also enable passing
> > more
> > information to netlogon for logging purposes. Client IP address for
> > example comes to mind.
>
> I think it would be better to use custom calls in both directions,
> from winbindd to the netlogon server and the other direction.
>
> BTW: what's the error message from an RODC for an existing user
> without cached password if no RWDC is reachable?
I don't know, but Douglas did a pile of work building RODC tests last
year.  Sadly we didn't get them to a point where they could be part of
make test (particularly because of difficulty making the RWDC
unreachable on demand), but it is on our 'soon' agenda to work on.

> > > The other case we have to support, which is more difficult, is
> > > that
> > > when we are an RODC or even a full DC, we should present wrong
> > > password
> > > to the PDC emulator.  On the RODC this allows the bad password
> > > count to
> > > be updated (otherwise it can't be stored anywhere), and on an RW
> > > DC it
> > > is intended to give the user a second chance to authenticate with
> > > their
> > > new password before replication catches up.
> >
> > Every logon failure against a DC via netlogon triggers a callback
> > to
> > the PDC emulator done by the DC? Wow, I did not know that. Need to
> > think about it.
>
> Andrew, are you referring to the netr_LogonSendToSam() calls shown in
> [MS-ADOD] ?
>
> 3.2.5 Example 5: Change a User Account's Password Against a Non-PDC
> DC
> https://msdn.microsoft.com/en-us/library/hh872148.aspx
>
> and
>
> 3.2.6 Example 6: Update the User's lastLogOnTimeStamp Against an RODC
> When the User Binds to an LDAP Server
> https://msdn.microsoft.com/en-us/library/hh872036.aspx
>
> I guess logonCount, badPwdCount and badPasswordTime are still
> maintained
> on the RODC itself as these attributes are marked with
> FLAG_ATTR_NOT_REPLICATED.
>
> It would be interesting to see what a non-pdc emulator will do with
> failing LogonSamLogon calls and if there's a difference between
> interactive and network
> logons.
I've not looked up the docs for the mechanics, but I have seen it
clearly in traces that a wrong password is forwarded to the DC.
LogonSendToSam certainly looks like a very interesting and important
call to support.

Thanks,

Andrew Bartlett

signature.asc (879 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Sat, 2017-03-11 at 14:40 +0100, Volker Lendecke wrote:

> On Sat, Mar 11, 2017 at 08:31:36AM +1300, Andrew Bartlett wrote:
> > On Fri, 2017-03-10 at 15:08 +0100, Volker Lendecke wrote:
> > > On Fri, Mar 10, 2017 at 05:46:58PM +1300, Andrew Bartlett wrote:
> > > >
> > > > The pdbtest patch looks wrong, we have been testing the
> > > > different
> > > > auth
> > > > methods via that tool, so fixing it to 'sam' seems to be
> > > > limiting
> > > > what
> > > > we are testing.
> > >
> > > Well, it does survive autobuild.
> >
> > Sure, but that is because you remove what it is testing.  pdbtest
> > is
> > acting as the driver for a sort of unit test of the auth subsystem,
> > as
> > controlled by 'auth methods'.  The tests set auth methods to
> > various
> > values to try and test those modules.
> >
> > This was added to ensure we didn't have untested code in the auth
> > subsystem and to avoid relying on indirect tests.
>
> https://git.samba.org/?p=vl/samba.git/.git;h=refs/heads/auth
>
> has fixes for this issue.
>
> Comments?

That addresses my specific concern here regarding pdbtest.

For the change in winbind_pam could we do:

char *auth_methods = "sam";

if (lp_server_role() == ROLE_ACTIVE_DIRECTORY_DC) {
  auth_methods = "samba4:sam";
}

That would keep this patch self-contained for the purpose it declares,
without swapping the auth backend in use.  I realise that you swap it
implicitly later with https://git.samba.org/?p=vl/samba.git/.git;a=comm
itdiff;h=b420cf0a648b420256284390f7e51eb5c1a2c794 but that isn't in the
same patch, so in the meantime we would try to run the source3 auth
stack against pdb_samba_dsdb.  

Not doing that should help with the bisect-ability desire.

Thanks,

Andrew Bartlett

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
In reply to this post by Volker Lendecke-3
On Thu, 2017-03-09 at 19:48 +0100, Volker Lendecke wrote:
>
> We need to talk about full bisectability with make test. That will be
> difficult to achieve for this patchset. We need to mark many tests as
> failing in between and unmark them again. Or one big patch.

Yeah, I've got the same issue with the auth logging patches, so I feel
your pain.  Metze asked that I ensure that we don't fail in the earlier
 patches (we are quite confident it all works out in the end), so I'm
going to try and get a testsuite that we can run against each, to at
least walk the codepaths.  I guess I'll know how well that works out
soon...

I'm personally not a stickler for full bisectabilty, but I try for it
when I can.

Andrew Bartlett

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
In reply to this post by Volker Lendecke-3
On Fri, 2017-03-10 at 15:08 +0100, Volker Lendecke wrote:

> On Fri, Mar 10, 2017 at 05:46:58PM +1300, Andrew Bartlett wrote:
> >
> > The SMB or NETLOGON case has already checked the password locally,
> > so
> > should never talk to the auth subsystem again, it must just go
> > directly
> > to a remote DC.  The ntlm_auth case might be trying a local login. 
>
> Please explain. For the AD DC, smbd would go to winbind even for
> local
> authentication. If this turns out to be a performance problem, we can
> introduce a call from smbd directly into netlogond ncacn_unix. On an
> AD DC, we have the huge advantage to always have winbind around, and
> we should make use of this.

I'm certainly concerned about performance, but also consistency.  I'll
explain a little more below.

> > Thanks for the explanation and the patches.  They look really
> > good!  It
> > is great to get this dealt with properly, and I look forward to the
> > final set.
> >
> > I take it that "auth methods" now only controls the smb connection
> > handler?
>
> Yep. If you ask me, I would ditch the "auth methods" parameter from
> smb.conf. I would really like to see the use cases that make them
> necessary.

I tend to agree.   The only users doing interesting things that I've
heard of were patching Samba anyway, like Apple back when they ran
Samba.  I did try and write a new auth module recently for a client to
authenticate against Open Directory, but I never got it to work (OD
restrictions).

> > Regarding the last patch, my main thought is that I want the call
> > to
> > netlogon not to happen at all, unless we are servicing wbinfo or
> > ntlm_auth.  I certainly appreciate that by going to netlogon the
> > auth
> > module list can be fixed, which certainly has advantages.  
> >
> > When checking local passwords, I like the API call for the ability
> > to
> > pass in additional data (like the source IP of the auth I've added
> > in
> > the logging patches), but we could:
> >  - use a private custom IRPC
> >  - or just not worry for this particular case
>
> See above.
>
> My main reason for winbind to do netlogon rpc locally for sam auth is
> that it reduces the number of places we call into the auth subsystem
> loop. I really want to have canonical ways into the auth subsystem
> and
> get them right and fast once and for all cases. And we have to get
> the
> SamLogon call 100% right and fast anyway, so why not use it locally
> too?

Is this just for ntlm_auth?  Above you seem to suggest you want smbd to
use winbindd as well.

It isn't in your patch set, but I think you are saying that we should
call:
 smbd -> winbindd -> netlogond4 for every authentication

That may be OK for SMB, but for LDAP where we we can get hammered with
simple bind requests, and are currently stuck in a single process, it
would be:

 ldapsrv (single process) -> winbind (multi-process in parts) ->
netlogond4 (multi-process)

As ldapsrv is currently blocking on authentication and is a single
process, this would seem to be undesirable.  

Now, you might ask why do I bring LDAP in to it?  I very strongly feel
that the auth process on SMB needs to be the same as DCE/RPC and LDAP,
because I fear subtle variations between the authentication result on
these different protocols will be a nightmare to debug when all
integrated into the same AD DC.  

It is this I feel most strongly about, I'm not as fussed about the in-
process/inter-process split as long as it brings technical advantages.

> From a progress perspective I'd call the patchset pretty much done
> and
> ready for master. It survives autobuild and enables correct fallback
> for !authoritative. Right, for ntlm_auth in the rodc case we don't
> look
> locally, but the rodc is not a fully supported configuration yet.

Correct, but I would really like not to move backwards, and keep an eye
on what we need to do to improve it, as I'll be re-starting work on
this soon.

> The deeper reason why I want this is in rather sooner than later is
> the removal of the call to is_trusted_domain() in the source3 auth
> path. That call is just wrong, but to get it through autobuild I had
> to cleanup the DC side of things.

Indeed, and I do thank you for your patience and persistence here.

As the above is more general, I've replied with some more specific
concerns in another mail.

Thanks,

Andrew Bartlett

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, 2017-03-13 at 10:17 +1300, Andrew Bartlett wrote:

> On Sat, 2017-03-11 at 14:40 +0100, Volker Lendecke wrote:
> > On Sat, Mar 11, 2017 at 08:31:36AM +1300, Andrew Bartlett wrote:
> > > On Fri, 2017-03-10 at 15:08 +0100, Volker Lendecke wrote:
> > > > On Fri, Mar 10, 2017 at 05:46:58PM +1300, Andrew Bartlett
> > > > wrote:
> > > > >
> > > > > The pdbtest patch looks wrong, we have been testing the
> > > > > different
> > > > > auth
> > > > > methods via that tool, so fixing it to 'sam' seems to be
> > > > > limiting
> > > > > what
> > > > > we are testing.
> > > >
> > > > Well, it does survive autobuild.
> > >
> > > Sure, but that is because you remove what it is testing.  pdbtest
> > > is
> > > acting as the driver for a sort of unit test of the auth
> > > subsystem,
> > > as
> > > controlled by 'auth methods'.  The tests set auth methods to
> > > various
> > > values to try and test those modules.
> > >
> > > This was added to ensure we didn't have untested code in the auth
> > > subsystem and to avoid relying on indirect tests.
> >
> > https://git.samba.org/?p=vl/samba.git/.git;h=refs/heads/auth
> >
> > has fixes for this issue.
> >
> > Comments?
>
> That addresses my specific concern here regarding pdbtest.
>
> For the change in winbind_pam could we do:
>
> char *auth_methods = "sam";
>
> if (lp_server_role() == ROLE_ACTIVE_DIRECTORY_DC) {
>   auth_methods = "samba4:sam";
> }
>
> That would keep this patch self-contained for the purpose it
> declares,
> without swapping the auth backend in use.  I realise that you swap it
> implicitly later with https://git.samba.org/?p=vl/samba.git/.git;a=co
> mm
> itdiff;h=b420cf0a648b420256284390f7e51eb5c1a2c794 but that isn't in
> the
> same patch, so in the meantime we would try to run the source3 auth
> stack against pdb_samba_dsdb.  
>
> Not doing that should help with the bisect-ability desire. 

I tried implementing that, and along the way I found some other issues:

This patch:
auth3: Return NT_STATUS_NOT_IMPLEMENTED from auth_check_ntlm_password
https://git.samba.org/?p=vl/samba.git/.git;a=commitdiff;h=4b3f6a614a24c
7a4747dec91dc3b90e05cc0675b

Does not cover all the auth subsystem callers.  

If you want to put the check at this layer, then
auth_check_password_session_info() also needs to do that check, and as
above, pdbtest also makes a call here.

My thoughts are that this is an internal auth subsystem detail that
shouldn't leak out like that.  Indeed, perhaps we should just make
authoritative it an additional return parameter.

In the meantime, I think it is better to keep a flag like
USER_INFO_LOCAL_SAM_ONLY and specify it in netlogon and (at this point
in the series at least) winbindd_pam.

Finally, I think we need to carefully consider the right way to signal
'user found but no password (need to forward)' compared with 'I don't
know the domain'.  At the moment they have been using the same return
value, and that is why the RODC tests failed until your latest patch.  
(However it isn't at all clear to my how your latest patch - pushing to
the local netlogon server - fixes that).

Sorry,

Andrew Bartlett

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, Mar 13, 2017 at 01:54:51PM +1300, Andrew Bartlett wrote:
> Indeed, and I do thank you for your patience and persistence here.
>
> As the above is more general, I've replied with some more specific
> concerns in another mail.

As you have a LOT of concerns that will take me months to fix, I am sure
you have patches handy to fix the !authoritative NT_STATUS_NO_SUCH_USER
bug we have. I seem to have taken an a lot too complex approach.  Can you
share your patch with me, so that I can get what I really need: Fix the
source3 client.

Thanks, Volker

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, Mar 13, 2017 at 01:54:51PM +1300, Andrew Bartlett wrote:
> Is this just for ntlm_auth?  Above you seem to suggest you want smbd to
> use winbindd as well.
>
> It isn't in your patch set, but I think you are saying that we should
> call:
>  smbd -> winbindd -> netlogond4 for every authentication

Yes.

> That may be OK for SMB, but for LDAP where we we can get hammered with
> simple bind requests, and are currently stuck in a single process, it
> would be:
>
>  ldapsrv (single process) -> winbind (multi-process in parts) ->
> netlogond4 (multi-process)
>
> As ldapsrv is currently blocking on authentication and is a single
> process, this would seem to be undesirable.  

How do you plan to handle NTLM to trusted domains in this setup?

Volker

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, Mar 13, 2017 at 02:05:02PM +1300, Andrew Bartlett wrote:
> My thoughts are that this is an internal auth subsystem detail that
> shouldn't leak out like that.  Indeed, perhaps we should just make
> authoritative it an additional return parameter.

That would change a LOT of code. This is so deeply embedded everywhere
that this would be a much larger change code-wise.

> In the meantime, I think it is better to keep a flag like
> USER_INFO_LOCAL_SAM_ONLY and specify it in netlogon and (at this point
> in the series at least) winbindd_pam.

To me it is much more understandable to not pass flags down that
subtly change behaviour. But that is just my limited intellectual
capacity that makes this necessary.

> Finally, I think we need to carefully consider the right way to signal
> 'user found but no password (need to forward)' compared with 'I don't
> know the domain'.  At the moment they have been using the same return
> value, and that is why the RODC tests failed until your latest patch.  
> (However it isn't at all clear to my how your latest patch - pushing to
> the local netlogon server - fixes that).

What return values do you propose?

Thanks,

Volker


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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
On Mon, 2017-03-13 at 10:03 +0100, Volker Lendecke wrote:

> On Mon, Mar 13, 2017 at 02:05:02PM +1300, Andrew Bartlett wrote:
> > My thoughts are that this is an internal auth subsystem detail that
> > shouldn't leak out like that.  Indeed, perhaps we should just make
> > authoritative it an additional return parameter.
>
> That would change a LOT of code. This is so deeply embedded
> everywhere
> that this would be a much larger change code-wise.
>
> > In the meantime, I think it is better to keep a flag like
> > USER_INFO_LOCAL_SAM_ONLY and specify it in netlogon and (at this
> > point
> > in the series at least) winbindd_pam.
>
> To me it is much more understandable to not pass flags down that
> subtly change behaviour. But that is just my limited intellectual
> capacity that makes this necessary.
>
> > Finally, I think we need to carefully consider the right way to
> > signal
> > 'user found but no password (need to forward)' compared with 'I
> > don't
> > know the domain'.  At the moment they have been using the same
> > return
> > value, and that is why the RODC tests failed until your latest
> > patch.  
> > (However it isn't at all clear to my how your latest patch -
> > pushing to
> > the local netlogon server - fixes that). 
>
> What return values do you propose?

NT_STATUS_WRONG_PASSWORD with *authoriative=0 would do it nicely I
think.

If we do the same with NO_SUCH_USER then the confusing mappings outside
the auth subsytem go away, and we can probably dispense with the flag
you so dislike (as then I think the different auth module lists would
work).

That is, break out of the auth module loop based on *authoriative, not
NT_STATUS_NOT_IMPLEMENTED.  

That way we have no need for flag based changes to return values, and
callers like ntlm and ntlmssp can just ignore it, while netlogon can
honour it.  

I hope this helps,

Andrew Bartlett


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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
On Tue, Mar 14, 2017 at 12:51:31PM +1300, Andrew Bartlett via samba-technical wrote:

> On Mon, 2017-03-13 at 10:03 +0100, Volker Lendecke wrote:
> > On Mon, Mar 13, 2017 at 02:05:02PM +1300, Andrew Bartlett wrote:
> > > My thoughts are that this is an internal auth subsystem detail that
> > > shouldn't leak out like that.  Indeed, perhaps we should just make
> > > authoritative it an additional return parameter.
> >
> > That would change a LOT of code. This is so deeply embedded
> > everywhere
> > that this would be a much larger change code-wise.
> >
> > > In the meantime, I think it is better to keep a flag like
> > > USER_INFO_LOCAL_SAM_ONLY and specify it in netlogon and (at this
> > > point
> > > in the series at least) winbindd_pam.
> >
> > To me it is much more understandable to not pass flags down that
> > subtly change behaviour. But that is just my limited intellectual
> > capacity that makes this necessary.
> >
> > > Finally, I think we need to carefully consider the right way to
> > > signal
> > > 'user found but no password (need to forward)' compared with 'I
> > > don't
> > > know the domain'.  At the moment they have been using the same
> > > return
> > > value, and that is why the RODC tests failed until your latest
> > > patch.  
> > > (However it isn't at all clear to my how your latest patch -
> > > pushing to
> > > the local netlogon server - fixes that). 
> >
> > What return values do you propose?
>
> NT_STATUS_WRONG_PASSWORD with *authoriative=0 would do it nicely I
> think.
>
> If we do the same with NO_SUCH_USER then the confusing mappings outside
> the auth subsytem go away, and we can probably dispense with the flag
> you so dislike (as then I think the different auth module lists would
> work).
>
> That is, break out of the auth module loop based on *authoriative, not
> NT_STATUS_NOT_IMPLEMENTED.  
>
> That way we have no need for flag based changes to return values, and
> callers like ntlm and ntlmssp can just ignore it, while netlogon can
> honour it.  
>
> I hope this helps,

Just been following from the sidelines so I'm sure Volker can comment
with *authoriative=1 :-), but that looks like a workable plan to excise
USER_INFO_LOCAL_SAM_ONLY.

Thanks Andrew !

Jeremy.

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
On Mon, 2017-03-13 at 17:19 -0700, Jeremy Allison wrote:

> On Tue, Mar 14, 2017 at 12:51:31PM +1300, Andrew Bartlett via samba-
> technical wrote:
> > On Mon, 2017-03-13 at 10:03 +0100, Volker Lendecke wrote:
> > >
> > > What return values do you propose?
> >
> > NT_STATUS_WRONG_PASSWORD with *authoriative=0 would do it nicely I
> > think.
> >
> > If we do the same with NO_SUCH_USER then the confusing mappings
> > outside
> > the auth subsytem go away, and we can probably dispense with the
> > flag
> > you so dislike (as then I think the different auth module lists
> > would
> > work).
> >
> > That is, break out of the auth module loop based on *authoriative,
> > not
> > NT_STATUS_NOT_IMPLEMENTED.  
> >
> > That way we have no need for flag based changes to return values,
> > and
> > callers like ntlm and ntlmssp can just ignore it, while netlogon
> > can
> > honour it.  
> >
> > I hope this helps,
>
> Just been following from the sidelines so I'm sure Volker can comment
> with *authoriative=1 :-), but that looks like a workable plan to
> excise
> USER_INFO_LOCAL_SAM_ONLY.
>
> Thanks Andrew !

I just wanted to write down, while I still remember them, my guidance
on how we can get this to a conclusion:

 - make changes in sync between the two auth subsystems (the current
patch set removes the offensive flag, but only in auth3)
 - not attempt a change to inter-process communication in the same
patch set (eg move to "sam" and "samba4:sam" if specifying auth module
lists in winbindd)
 - clearly distinguish between the 'smbd as client' and
'ntlm_auth/wbinfo as client' cases in winbindd.
 - use *authoritative as the indicator.
 - have tests (both for the specific change desired, and for the other
areas touched like rodc)
 - be bisectable

I realise this remains a large task, but we need changes here done
carefully and clearly tested.  Sadly given the issues I found during my
review, that this patch set passed autobuild only showed the deficiency
of our current tests.

Thanks,

Andrew Bartlett

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

Re: [PATCH] Correctly handle !authoritative in the rpc-based auth backends

Samba - samba-technical mailing list
On Thu, Mar 16, 2017 at 04:06:22PM +1300, Andrew Bartlett via samba-technical wrote:

> I just wanted to write down, while I still remember them, my guidance
> on how we can get this to a conclusion:
>
>  - make changes in sync between the two auth subsystems (the current
> patch set removes the offensive flag, but only in auth3)
>  - not attempt a change to inter-process communication in the same
> patch set (eg move to "sam" and "samba4:sam" if specifying auth module
> lists in winbindd)
>  - clearly distinguish between the 'smbd as client' and
> 'ntlm_auth/wbinfo as client' cases in winbindd.
>  - use *authoritative as the indicator.
>  - have tests (both for the specific change desired, and for the other
> areas touched like rodc)
>  - be bisectable
>
> I realise this remains a large task, but we need changes here done
> carefully and clearly tested.  Sadly given the issues I found during my
> review, that this patch set passed autobuild only showed the deficiency
> of our current tests.

The one I really care about from a personal perspective is the patch
to remove "map untrusted to domain". winbind must be changed to not
enumerate trusted domains. Everything else was just necessary to push
this one through autobuild. The main blocker is bug 2976 in the AD DC.

So if you could provide a quick fix for this bug from more than a
decade ago that re-surfaced with the release of the AD DC, I will keep
my fingers off source4/auth and let this be handled better by your
team at Catalyst.

Thanks,

Volker

1234
Loading...