When running 'net ads join' we currently use the samba registry to pass and retrieve parameters. The main parameter we need back out is the 'workgroup'. 'net ads join' expects that either the configuration is writable (as the registry is) or that this is set correctly. So in order to change this, we need to discover the workgroup ahead of time, build our own smb.conf that contains it and pass that to 'net ads join'.
In addition Marko brought up: > * when using net to join it creates krb5.conf for the domain since > "create krb5 conf = no" is not set in smb.conf, I think it would be best > to disable the on-the-fly generated config file and thus rely on the > defaults or the previously created system krb5.conf This one is a hard call, because I've seen 'net ads join' fail in strange ways when it couldn't create its own kerberos configuration. But worth looking into. Marko, what was the specific use case that this broke?
> This one is a hard call, because I've seen 'net ads join' fail in strange > ways when it couldn't create its own kerberos configuration. What kind of failures you have seen? If the system default / system krb5.conf are correct then it should not fail, at least because of that setting. You can investigate the generated krb5.conf under /var/lib/samba to see what it contains, it will be used by the krb5 library when running net if in use instead of the system one. > But worth looking into. Marko, what was the specific use case that this broke? Using "create krb5 conf = no" would be preferable already for the reason that the system would not have two different configuration files for the same thing, administrators will get quickly confused if changes to the system krb5.conf do not affect Samba components. The case I had was that I needed special mappings for realm/domain which is not possible with the generated krb5.conf. FWIW, see this thread what Samba developers think about this: https://lists.samba.org/archive/samba-technical/2012-April/083176.html Thanks.
Created attachment 77062 [details] [review] Cleanup our samba net ads join process
Yassir, would you be interested in reviewing the attached patch?
Created attachment 77105 [details] [review] Cleanup our samba net ads join process
Comment on attachment 77105 [details] [review] Cleanup our samba net ads join process Review of attachment 77105 [details] [review]: ----------------------------------------------------------------- Great patch! The samba enrollment implementation is now much easier to understand! With regard to doing another DNS discovery if there was no DNS discovery done before the join starts, I wonder if the adcli enrollment implementation might benefit from that as well. ::: service/realm-samba-enroll.c @@ +84,5 @@ > > + join->config = realm_ini_config_new (REALM_INI_NO_WATCH | REALM_INI_PRIVATE); > + realm_ini_config_set (join->config, REALM_SAMBA_CONFIG_GLOBAL, "security", "ads"); > + realm_ini_config_set (join->config, REALM_SAMBA_CONFIG_GLOBAL, "kerberos method", "system keytab"); > + realm_ini_config_set (join->config, REALM_SAMBA_CONFIG_GLOBAL, "realm", join->realm); Since all of the "net" commands make use of custom_smb_conf, we should write out join->config to custom_smb_conf at this point (and then later insert workgroup when we have it). Currently, we write custom_smb_conf after the call to "net ads lookup", which means that this patch will introduce a new requirement that a default smb.conf file MUST exist and be populated with at least a realm (otherwise "net ads lookup" will fail). @@ +250,5 @@ > + free (workgroup); > + > + /* Write out the config file for use by various net commands */ > + join->custom_smb_conf = g_build_filename (g_get_tmp_dir (), "realmd-smb-conf.XXXXXX", NULL); > + temp_fd = g_mkstemp_full (join->custom_smb_conf, O_WRONLY, S_IRUSR | S_IWUSR); If the temporary file is being created write-only (O_WRONLY), why do include read permission in the mode (S_IRUSR)? @@ +346,3 @@ > begin_net_process (join, NULL, > + on_net_ads_lookup, g_object_ref (async), > + "ads", "lookup", "-S", kdcs[0], NULL); Calling "net ads lookup" and then parsing the results for specific strings seems somewhat fragile, since the samba code may change those strings at any time. Can we use "net ads workgroup" instead? @@ +443,4 @@ > > if (settings != NULL) { > join = g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (result)); > + *settings = realm_ini_config_get_all (join->config, REALM_SAMBA_CONFIG_GLOBAL); The callers of realm_samba_enroll_join_finish() are only going to use the "settings" hashtable that is passed back in order to extract the workgroup parameter. As such, it would be more efficient to simply return the workgroup parameter, rather than going through the trouble of creating a full "settings" hashtable. The callers are on_join_do_sssd and on_join_do_winbind. This comment is not related to the changes in the current patch. Just something I noticed. ::: service/realm-samba.c @@ +21,1 @@ > #include "realm-discovery.h" This comment is not related to this patch, but I noticed that realm_samba_constructed() calls realm_kerberos_set_supported_join_creds and set_supported_leave_creds with the following data: {PASSWORD, ADMIN}, {PASSWORD, USER}, {AUTOMATIC, NONE}. However, realm_kerberos_membership_iface() indicates that {AUTOMATIC} is not supported for joins (b/c there is no enroll_automatic_async function). ::: service/realm-sssd-ad.c @@ +340,3 @@ > g_assert (join->user_name != NULL); > g_assert (join->user_password != NULL); > realm_samba_enroll_join_async (join->realm_name, join->user_name, join->user_password, you should add "g_assert(join->realm_name != NULL)", since realm_samba_enroll_join_async requires (realm != null)
(In reply to comment #6) > ::: service/realm-samba-enroll.c > @@ +84,5 @@ > > > > + join->config = realm_ini_config_new (REALM_INI_NO_WATCH | REALM_INI_PRIVATE); > > + realm_ini_config_set (join->config, REALM_SAMBA_CONFIG_GLOBAL, "security", "ads"); > > + realm_ini_config_set (join->config, REALM_SAMBA_CONFIG_GLOBAL, "kerberos method", "system keytab"); > > + realm_ini_config_set (join->config, REALM_SAMBA_CONFIG_GLOBAL, "realm", join->realm); > > Since all of the "net" commands make use of custom_smb_conf, we should write > out > join->config to custom_smb_conf at this point (and then later insert > workgroup when we have it). Currently, we write custom_smb_conf after the > call to "net ads lookup", which means that this patch will introduce a new > requirement that a default smb.conf file MUST exist and be populated with at > least a realm (otherwise "net ads lookup" will fail). Are you sure? It doesn't fail for me. In any case the newly attached patch implements this change. > @@ +250,5 @@ > > + free (workgroup); > > + > > + /* Write out the config file for use by various net commands */ > > + join->custom_smb_conf = g_build_filename (g_get_tmp_dir (), "realmd-smb-conf.XXXXXX", NULL); > > + temp_fd = g_mkstemp_full (join->custom_smb_conf, O_WRONLY, S_IRUSR | S_IWUSR); > > If the temporary file is being created write-only (O_WRONLY), why do include > read permission in the mode (S_IRUSR)? We only want to write to temp_fd, but want the current user (root) to be able to read the file (although obviously not through the that fd). > @@ +346,3 @@ > > begin_net_process (join, NULL, > > + on_net_ads_lookup, g_object_ref (async), > > + "ads", "lookup", "-S", kdcs[0], NULL); > > Calling "net ads lookup" and then parsing the results for specific strings > seems somewhat fragile, since the samba code may change those strings at any > time. Can we use "net ads workgroup" instead? Good find. That's just what we need. Although in reality we need 'net ads join' not to be so dumb and insist on a valid workgroup in the config to complete the join :S > @@ +443,4 @@ > > > > if (settings != NULL) { > > join = g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (result)); > > + *settings = realm_ini_config_get_all (join->config, REALM_SAMBA_CONFIG_GLOBAL); > > The callers of realm_samba_enroll_join_finish() are only going to use the > "settings" hashtable that is passed back in order to extract the workgroup > parameter. As such, it would be more efficient to simply return the > workgroup parameter, rather than going through the trouble of creating a > full "settings" hashtable. The callers are on_join_do_sssd and > on_join_do_winbind. This comment is not related to the changes in the > current patch. Just something I noticed. That makes sense. I'd prefer to keep this as is though. As we get into configuring samba properly, we'll want to pass further discovered parameters up through here. > ::: service/realm-samba.c > @@ +21,1 @@ > > #include "realm-discovery.h" > > This comment is not related to this patch, but I noticed that > realm_samba_constructed() calls realm_kerberos_set_supported_join_creds and > set_supported_leave_creds with the following data: {PASSWORD, ADMIN}, > {PASSWORD, USER}, {AUTOMATIC, NONE}. However, > realm_kerberos_membership_iface() indicates that {AUTOMATIC} is not > supported for joins (b/c there is no enroll_automatic_async function). Indeed. I've removed the line. > ::: service/realm-sssd-ad.c > @@ +340,3 @@ > > g_assert (join->user_name != NULL); > > g_assert (join->user_password != NULL); > > realm_samba_enroll_join_async (join->realm_name, join->user_name, join->user_password, > > you should add "g_assert(join->realm_name != NULL)", since > realm_samba_enroll_join_async requires (realm != null) Sure. But it's assumed that join->realm_name is always non-NULL. The asserts aren't really a statement about what realm_samba_enroll_join_async() needs, but rather what assumptions about what should be true if that branch of the big if block is taken.
Created attachment 77737 [details] [review] Cleanup our samba net ads join process
Attachment 77737 [details] pushed as 4d81337 - Cleanup our samba net ads join process Pushed with the above changes.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.