[PATCH] addon patches for domain join failure (Re: [SCM] Samba Shared Repository - branch master updated)
Michael Adam
obnox at samba.org
Fri Jun 17 12:53:05 UTC 2016
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>
More information about the samba-technical
mailing list