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

[Günther Deschner]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.

> - 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 :)

> - 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.).

> 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 ?

>> 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.

> 
>> return WERR_CAN_NOT_COMPLETE; }
> 
> 
> 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?

Cheers,
Guenther

- -- 
Günther Deschner                    GPG-ID: 8EE11688
Red Hat                         gdeschner at redhat.com
Samba Team                              gd at samba.org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAldj5hAACgkQSOk3aI7hFoiLpwCfXxIgskqYjqWq4QUbXf5APBIv
Gv4AnjIc9olzYL8j/76G4TCM+zrNXHdL
=Yr+p
-----END PGP SIGNATURE-----