[PATCH] addon patches for domain join failure (Re: [SCM] Samba Shared Repository - branch master updated)

[Michael Adam]

On 2016-06-17 at 13:59 +0200, Günther Deschner wrote:
> Hi Michael,
> 
> On 16/06/16 11:30, Michael Adam wrote:
> > - The place for early-success-out in the flow of this function is
> > further up where valid_realm is set. This is the error treatment
> > path of the function.
> > 
> > ==> So this patch is *wrong* since it will let the config check 
> > succeed when the workgroup is wrong!
> 
> Good catch, thanks! We certainly need to stop once lp_workgroup does
> not match.
> 
> > - The comment is kind-of redundant: the actual code says it all!
> > :-)
> 
> Well, kind of yes. But I think it is fine to add an additional comment
> to explain why this is done.

A comment that adds extra reasoning or explanation might be
good, but the current comment is redundant. :-)

> > - The comment and debug msg are not quite correct: This will not
> > only warn when realm is unset, but also when it is set to a
> > different value than the detected dns domain name
> > (r->out.dns_domain_name), which I think is OK.
> 
> Yes, this is intended, and I think it is OK as well :)

Hm? The incorrect comment and debug msg are intended?

> > - I question the content of actual warning message: Why *should* I
> > configure a realm when using 'security=domain' ?
> 
> Because a lot of code relies on lp_realm() when talking to AD since we
> initially moved all AD and RPC join routines to the libnetjoin
> interfaces (e.g. the gensec gse_krb5 backend, some of the namelookup
> code, etc.).

So as explained in the thread about 'winbind rpc only',
I was confused about what 'security = domain' is intended to do,
so that's the reason behind this question.

Rephrasing it : Why would I configure a realm when doing
'winbind rpc only = yes' ?

> > I could buy this recommendation when also 'winbind rpc only = no'. 
> > So if we add a warning, we should make it dependent on the value of
> > this parameter, imho.
> > 
> > ==> this makes me remember that I would like to change the default
> > of 'winbind rpc only' to 'yes'. Sending a patch separately...
> 
> ??? Sorry, but why should we change the default of this ?

==> see the other thread. Ancient misunderstanding on my end. ;-)

> >> libnet_join_set_error_string(mem_ctx, r, "Invalid configuration
> >> (%s) and configuration modification " "was not requested",
> >> wrong_conf); +
> > 
> > Sneaking in an extra blank line? :-)
> 
> I think it reads nicer with having a newline here.

I was commenting on patch hygiene here.
IMHO, that should have been omitted or put up as a separate
patch, since it is not in the lines changed anyways.
But that's just me and maybe I am too priestly... ;-)

> > Attached find 3 patches I propose for the fix:
> > 
> > #1 reverts this patch #2 achieves the same intended difference in
> > behavior #3 adds the debug message in early-success path
> 
> I think the other proposed patchset is not correct as it would then
> emit the warning regardless whether we deal with AD or not, right?

What is 'the other proposed patchset' ?

My patchset would only print the warning
if the domain is AD, and security is domain
and the ream has been ignored.


I guess in my patchset, the possible improvement I currently see
based on the increased understanding I have gained
through the discussion about security=domain and
winbind rpc only is to make the valid_realm = true
dependent on if (lp_winbind_rpc_only())

so :

instead of:

 	case SEC_DOMAIN:
+		valid_realm = true;
 	case SEC_ADS:

I might propose:

 	case SEC_DOMAIN:
+		if (lp_winbind_rpc_only()) {
+			valid_realm = true;
+		}
 	case SEC_ADS:


Cheers - Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160617/82c4c70c/signature.sig>