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

[Andrew Bartlett]

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