|
Description
Stef Walter
2012-09-18 06:15:02 UTC
Created attachment 70438 [details] [review] Expose the method for updating an SSSD realm's properties Created attachment 70439 [details] [review] WIP IPA client Testing this, updating patches, and had a crash: https://bugzilla.redhat.com/show_bug.cgi?id=880563 More failures to research. No more time today, but noting here. # /usr/sbin/ipa-client-install --domain idm.lab.bos.redhat.com --realm IDM.LAB.BOS.REDHAT.COM --principal admin -W --mkhomedir --no-ntp --enable-dns-updates --permit Discovery was successful! Hostname: stef-rawhide.thewalter.lan Realm: IDM.LAB.BOS.REDHAT.COM DNS Domain: idm.lab.bos.redhat.com IPA Server: vm-137.idm.lab.bos.redhat.com BaseDN: dc=idm,dc=lab,dc=bos,dc=redhat,dc=com Continue to configure the system with these values? [no]: y Synchronizing time with KDC... Password for admin@IDM.LAB.BOS.REDHAT.COM: Enrolled in IPA realm IDM.LAB.BOS.REDHAT.COM Failed to obtain host TGT. Installation failed. Rolling back changes. IPA client is not configured on this system. Created attachment 70651 [details] [review] Expose the method for updating an SSSD realm's properties Created attachment 70652 [details] [review] Pull out function to build password input out of passwords Created attachment 70653 [details] [review] Refactor out config domain identification logic Created attachment 70654 [details] [review] WIP IPA client Created attachment 74344 [details] [review] Expose the method for updating an SSSD realm's properties Created attachment 74345 [details] [review] Pull out function to build password input out of passwords Created attachment 74346 [details] [review] Refactor out config domain identification logic Created attachment 74347 [details] [review] Use ipa-client-install to provide join functionality for IPA domains Still outstanding: * Documentation: will provide patch * ccache support: will file another bug The first three patches look good to me. The first two are mostly moving stuff around. The third one is more involved, but the most important part (the precedence order of reading the realm) seems correct to me. Please note I only read the diffs and in some cases the surrounding code. I have some comments for patch #4, I'll include them in splinter. (In reply to comment #14) > The third one is more involved, but the most important part > (the precedence order of reading the realm) seems correct to me. Actually scratch that I'll include one comment in the splinter as well. Comment on attachment 74347 [details] [review] Use ipa-client-install to provide join functionality for IPA domains Review of attachment 74347 [details] [review]: ----------------------------------------------------------------- ::: service/realm-sssd-ipa.c @@ +178,5 @@ > + home = realm_sssd_build_default_home (realm_settings_string ("users", "default-home")); > + > + realm_sssd_config_update_domain (realm_sssd_get_config (sssd), > + realm_kerberos_get_name (realm), &error, > + "enumerate", "False", Setting enumerate=False explicitly doesn't hurt, but it's not needed either as it's the default. @@ +181,5 @@ > + realm_kerberos_get_name (realm), &error, > + "enumerate", "False", > + "re_expression", "((?P<name>[^@]+)@(?P<domain>.+$))", > + "full_name_format", "%1$s@%2$s", > + "case_sensitive", "False", I don't quite understand why are IPA domains marked as case insensitive. It would make sense for an AD domain, but I think in IPA the default should be case sensitive. @@ +183,5 @@ > + "re_expression", "((?P<name>[^@]+)@(?P<domain>.+$))", > + "full_name_format", "%1$s@%2$s", > + "case_sensitive", "False", > + "cache_credentials", "True", > + "use_fully_qualified_names", "True", I haven't tested the effect of use_fully_qualified_names together with the specified re_expression, but I would assume that you would *need to* use fully qualified names, the shortnames wouldn't work at all. In other words you'd have to log in as user@ipa.domain. Is that the setting you want to achieve? FWIW The ipa-client-install default is to allow shortnames. @@ +355,5 @@ > + "--password", secret_to_password (secret, &password), > + "--mkhomedir", > + "--no-ntp", > + "--enable-dns-updates", > + "--permit", Are you sure about using --permit? What permit does in the context of IPA client is that instead of using the IPA specific HBAC access control policy stored on the server, the simple access provider is used. On one hand, I think that for IPA clients it would be more typical to rely on the server side access control. On the other hand, I'm not sure how would realm permit/realm deny work then... Comment on attachment 74346 [details] [review] Refactor out config domain identification logic Review of attachment 74346 [details] [review]: ----------------------------------------------------------------- ::: service/realm-sssd-config.c @@ +219,5 @@ > + name = realm_ini_config_get (config, section, field_name); > + if (name == NULL) > + name = realm_ini_config_get (config, section, "krb5_realm"); > + if (name == NULL) > + name = g_strdup (domain); I'm not entirely sure about the bigger context and how would the *realm_name be used later, but if you intend to read the IPA domain name, then why not stop after reading just the SSSD domain name? If, on the other hand, you intent to read them Kerberos realm, then I think krb5_realm should be the most specific match. I just tested an IPA domain that had the following setting: ipa_domain = idm.lab.bos.redhat.com krb5_realm = foobar.com The internal SSSD parser would use foobar.com for the Kerberos realm, then. That said, having a krb5_realm different than the IPA domain is definitely not the typical case and I would bet that 99% of IPA clients don't have the krb5_realm option set at all. First three patches look good. I will include my comments for the fourth patch in the splinter. Comment on attachment 74347 [details] [review] Use ipa-client-install to provide join functionality for IPA domains Review of attachment 74347 [details] [review]: ----------------------------------------------------------------- ::: service/realm-sssd-ipa.c @@ +91,5 @@ > + REALM_KERBEROS_CREDENTIAL_PASSWORD, REALM_KERBEROS_OWNER_ADMIN, > + 0); > + > + realm_kerberos_set_supported_leave_creds (kerberos, supported); > + There is a discrepancy here, in that there are two unenroll functions (leave_password_async, leave_automatic_async), but only one credential type (password) is included in supported_leave_creds. @@ +93,5 @@ > + > + realm_kerberos_set_supported_leave_creds (kerberos, supported); > + > + realm_kerberos_set_suggested_admin (kerberos, "admin"); > + realm_kerberos_set_required_package_sets (kerberos, IPA_PACKAGES); The samba and sssd-ad realm objects include a call to realm_kerberos_set_login_policy() as part of the constructed method. Are login policies not applicable to sssd-ipa realms b/c of HBAC? @@ +116,5 @@ > + EnrollClosure *enroll = data; > + g_object_unref (enroll->invocation); > + g_strfreev (enroll->argv); > + if (enroll->input) > + g_bytes_unref (enroll->input); g_bytes_unref returns immediately if its input is NULL; no need for you to also check before calling g_bytes_unref. @@ +129,5 @@ > + GSimpleAsyncResult *async = G_SIMPLE_ASYNC_RESULT (user_data); > + RealmSssd *sssd = REALM_SSSD (g_async_result_get_source_object (user_data)); > + GError *error = NULL; > + > + realm_service_enable_and_restart_finish (result, &error); Don't we also need to run authconfig ("sssd-enable-logins")? @@ +176,5 @@ > + > + if (error == NULL) { > + home = realm_sssd_build_default_home (realm_settings_string ("users", "default-home")); > + > + realm_sssd_config_update_domain (realm_sssd_get_config (sssd), Why does sssd-ipa call update_domain, while sssd-ad calls add_domain? @@ +250,5 @@ > + const gchar *domain_name; > + const gchar *computer_ou; > + > + domain_name = realm_kerberos_get_name (realm); > + Shouldn't this be realm_kerberos_get_realm_name(realm)? @@ +262,5 @@ > + if (g_variant_lookup (options, REALM_DBUS_OPTION_COMPUTER_OU, "&s", &computer_ou)) { > + g_simple_async_result_set_error (async, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS, > + _("The computer-ou argument is not supported when joining an IPA domain.")); > + g_simple_async_result_complete_in_idle (async); > + Since the realm client also sends REALM_DBUS_OPTION_MEMBERSHIP_SOFTWARE, you need to mark that as unsupported as well. @@ +276,5 @@ > + > + } else { > + realm_packages_install_async (IPA_PACKAGES, invocation, > + on_install_do_join, g_object_ref (async)); > + } If flags include REALM_KERBEROS_ASSUME_PACKAGES, then no packages should be installed. ::: service/realmd-redhat.conf @@ +17,5 @@ > adcli = /usr/sbin/adcli > > +[freeipa-packages] > +freeipa-client = /usr/sbin/ipa-client-install > + Is the value of "freeipa-client" used anywhere? (In reply to comment #16) > Setting enumerate=False explicitly doesn't hurt, but it's not needed either > as it's the default. Good point. Removed. > I don't quite understand why are IPA domains marked as case insensitive. It > would make sense for an AD domain, but I think in IPA the default should be > case sensitive. Makes sense. Removed this line. > @@ +183,5 @@ > > + "re_expression", "((?P<name>[^@]+)@(?P<domain>.+$))", > > + "full_name_format", "%1$s@%2$s", > > + "case_sensitive", "False", > > + "cache_credentials", "True", > > + "use_fully_qualified_names", "True", > > I haven't tested the effect of use_fully_qualified_names together with the > specified re_expression, but I would assume that you would *need to* use > fully qualified names, the shortnames wouldn't work at all. In other words > you'd have to log in as user@ipa.domain. Is that the setting you want to > achieve? FWIW The ipa-client-install default is to allow shortnames. Yes, that is the default behavior we want to achieve. Realmd can't just assume that the domain names magically don't conflict with local names. Do you think we should also support the other behavior, perhaps configurable? I've removed an extra set of parentheses from the re_expression regex. > Are you sure about using --permit? What permit does in the context of IPA > client is that instead of using the IPA specific HBAC access control policy > stored on the server, the simple access provider is used. > > On one hand, I think that for IPA clients it would be more typical to rely > on the server side access control. Makes sense. That's a good default. > On the other hand, I'm not sure how would realm permit/realm deny work > then... That needs to be rounded out now that we have IPA support. I'll work on that in another bug. > ::: service/realm-sssd-config.c > @@ +219,5 @@ > > + name = realm_ini_config_get (config, section, field_name); > > + if (name == NULL) > > + name = realm_ini_config_get (config, section, "krb5_realm"); > > + if (name == NULL) > > + name = g_strdup (domain); > > I'm not entirely sure about the bigger context and how would the *realm_name > be used later, but if you intend to read the IPA domain name, then why not > stop after reading just the SSSD domain name? > > If, on the other hand, you intent to read them Kerberos realm, then I think > krb5_realm should be the most specific match. > > I just tested an IPA domain that had the following setting: > ipa_domain = idm.lab.bos.redhat.com > krb5_realm = foobar.com > > The internal SSSD parser would use foobar.com for the Kerberos realm, then. > > That said, having a krb5_realm different than the IPA domain is definitely > not the typical case and I would bet that 99% of IPA clients don't have the > krb5_realm option set at all. This code is used for AD as well, and I wrote it because we want to be able to recognize AD domains in sssd.conf written before the ad_domain parameter was introduced. Although if you think this is pretty useless, we can change the behavior, by filing another bug. > There is a discrepancy here, in that there are two unenroll functions > (leave_password_async, leave_automatic_async), but only one credential type > (password) is included in supported_leave_creds. Thanks. Fixed. > The samba and sssd-ad realm objects include a call to > realm_kerberos_set_login_policy() as part of the constructed method. Are > login policies not applicable to sssd-ipa realms b/c of HBAC? Yes, I think we want to leave the default for IPA to be to use HBAC (with the tweak above noted by Jakub). > g_bytes_unref returns immediately if its input is NULL; no need for you to > also check before calling g_bytes_unref. Thanks. Changed. > Don't we also need to run authconfig ("sssd-enable-logins")? That's true. Added. > Why does sssd-ipa call update_domain, while sssd-ad calls add_domain? Because ipa-client-install adds the domain to sssd.conf. In the former case we add a few settings, in the latter case we add the whole domain. > @@ +250,5 @@ > > + const gchar *domain_name; > > + const gchar *computer_ou; > > + > > + domain_name = realm_kerberos_get_name (realm); > > + > > Shouldn't this be realm_kerberos_get_realm_name(realm)? No. We use the plain realm name as the sssd.conf domain name. > @@ +262,5 @@ > > + if (g_variant_lookup (options, REALM_DBUS_OPTION_COMPUTER_OU, "&s", &computer_ou)) { > > + g_simple_async_result_set_error (async, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS, > > + _("The computer-ou argument is not supported when joining an IPA domain.")); > > + g_simple_async_result_complete_in_idle (async); > > + > > Since the realm client also sends REALM_DBUS_OPTION_MEMBERSHIP_SOFTWARE, you > need to mark that as unsupported as well. Okay. I've added a check for this option. We support 'freeipa' as the membership software, but others are unsupported. > If flags include REALM_KERBEROS_ASSUME_PACKAGES, then no packages should be > installed. Thanks for catching that. Fixed. > ::: service/realmd-redhat.conf > @@ +17,5 @@ > > adcli = /usr/sbin/adcli > > > > +[freeipa-packages] > > +freeipa-client = /usr/sbin/ipa-client-install > > + > > Is the value of "freeipa-client" used anywhere? Yes, it is the package name to install. Created attachment 74587 [details] [review] Use ipa-client-install to provide join functionality for IPA domains (In reply to comment #20) > > @@ +183,5 @@ > > > + "re_expression", "((?P<name>[^@]+)@(?P<domain>.+$))", > > > + "full_name_format", "%1$s@%2$s", > > > + "case_sensitive", "False", > > > + "cache_credentials", "True", > > > + "use_fully_qualified_names", "True", > > > > I haven't tested the effect of use_fully_qualified_names together with the > > specified re_expression, but I would assume that you would *need to* use > > fully qualified names, the shortnames wouldn't work at all. In other words > > you'd have to log in as user@ipa.domain. Is that the setting you want to > > achieve? FWIW The ipa-client-install default is to allow shortnames. > > Yes, that is the default behavior we want to achieve. Realmd can't just > assume that the domain names magically don't conflict with local names. Do > you think we should also support the other behavior, perhaps configurable? > > I've removed an extra set of parentheses from the re_expression regex. I would assume that when realmd (centralized logins in general) are used, then there are no local accounts to care about. But you're right it's just an assumption and not a safe one, so the default should be safe. Maybe tracking an RFE would make sense. I've already seen one SSSD bug report where the user wanted to remove the re_expression to allow shortnames (and then he ran into unrelated problem, that was the point of the bug report, not the re_expression set by default). > > > Are you sure about using --permit? What permit does in the context of IPA > > client is that instead of using the IPA specific HBAC access control policy > > stored on the server, the simple access provider is used. > > > > On one hand, I think that for IPA clients it would be more typical to rely > > on the server side access control. > > Makes sense. That's a good default. > > > On the other hand, I'm not sure how would realm permit/realm deny work > > then... > > That needs to be rounded out now that we have IPA support. I'll work on that > in another bug. > > > ::: service/realm-sssd-config.c > > @@ +219,5 @@ > > > + name = realm_ini_config_get (config, section, field_name); > > > + if (name == NULL) > > > + name = realm_ini_config_get (config, section, "krb5_realm"); > > > + if (name == NULL) > > > + name = g_strdup (domain); > > > > I'm not entirely sure about the bigger context and how would the *realm_name > > be used later, but if you intend to read the IPA domain name, then why not > > stop after reading just the SSSD domain name? > > > > If, on the other hand, you intent to read them Kerberos realm, then I think > > krb5_realm should be the most specific match. > > > > I just tested an IPA domain that had the following setting: > > ipa_domain = idm.lab.bos.redhat.com > > krb5_realm = foobar.com > > > > The internal SSSD parser would use foobar.com for the Kerberos realm, then. > > > > That said, having a krb5_realm different than the IPA domain is definitely > > not the typical case and I would bet that 99% of IPA clients don't have the > > krb5_realm option set at all. > > This code is used for AD as well, and I wrote it because we want to be able > to recognize AD domains in sssd.conf written before the ad_domain parameter > was introduced. Although if you think this is pretty useless, we can change > the behavior, by filing another bug. > No, that's fine. Thanks for the explanation. (In reply to comment #22) > I would assume that when realmd (centralized logins in general) are used, > then there are no local accounts to care about. But you're right it's just > an assumption and not a safe one, so the default should be safe. > > Maybe tracking an RFE would make sense. Sure. Added here: bug #60637 Created attachment 74604 [details] [review] Add a utility function for updating sssd.conf domains Created attachment 74605 [details] [review] Use ipa-client-install to provide join functionality for IPA domains Added documentation, and some tests for sssd.conf manipulation. So does this look ready to go in now? Comment on attachment 74605 [details] [review] Use ipa-client-install to provide join functionality for IPA domains Review of attachment 74605 [details] [review]: ----------------------------------------------------------------- The changes you made as a result of my previous splinter review look good. I just have a few nits on the doc files here. Once these doc changes are made, the patch looks good to me. ::: doc/internals/realmd-internals.xml @@ +59,2 @@ > </itemizedlist> > </section> There is one other place that needs to be updated. In objects-interfaces section: * s/does not yet implement RealmKerberosMembership/implements RealmKerberosMembership/ @@ +287,1 @@ > </itemizedlist> I think you forgot to add "credentials supported" ::: doc/manual/realmd-guide-freeipa.xml @@ +118,5 @@ > + </informalexample> > + > + <para>The join operation will create or update a computer account > + in the domain. It is possible to </para> > + "It is possible to" clause needs to be removed or expanded. Attachment 74344 [details] pushed as 6778b0e - Expose the method for updating an SSSD realm's properties Attachment 74345 [details] pushed as 91bb537 - Pull out function to build password input out of passwords Attachment 74346 [details] pushed as 9e820c0 - Refactor out config domain identification logic Attachment 74604 [details] pushed as 16c9b7b - Add a utility function for updating sssd.conf domains Attachment 74605 [details] pushed as b89a0f6 - Use ipa-client-install to provide join functionality for IPA domains Thanks. Merged into git master with the changes you noted. This will be a part of the next realmd release. |
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.