On Fri, Jul 15, 2016 at 08:42:50PM +0200, Jakub Hrozek wrote: > On Wed, Jul 06, 2016 at 06:48:25PM +0200, Jakub Hrozek wrote: > > On Tue, Jul 05, 2016 at 10:06:15PM +0200, Sumit Bose wrote: > > > On Tue, Jul 05, 2016 at 07:15:03PM +0200, Jakub Hrozek wrote: > > > > On Tue, Jul 05, 2016 at 12:37:22PM +0200, Sumit Bose wrote: > > > > > Hi, > > > > > > > > > > this patch set should solve https://fedorahosted.org/sssd/ticket/3018 > > > > > by looking up the additional UPN suffixes on the IPA server. If some > > > > > were found, enterprise principals are enabled if they are not > > > > > explicitly > > > > > disabled in sssd.conf. > > > > > > > > > > The first patch read the attributes. The second and third patch store > > > > > the found suffixes in the cached object of the corresponding domain. > > > > > So > > > > > far this is not strictly needed but maybe it might be handy at some > > > > > later time if this data is around. The fourth and fifth patch just add > > > > > some getter-calls because some internal data is needed to allow the > > > > > sub-domain provider to modify the configuration of the auth provider. > > > > > Finally the sixth patch sets the enterprise principal option to true > > > > > if > > > > > there are UPN suffixes and enterprise principals are not explicitly > > > > > disabled in sssd.conf. > > > > > > > > Thanks for the patches, they look OK to me, although I still haven't > > > > refreshed my dev environment to run IPA master. > > > > > > > > So far I only have two comments. One is a Coverity warning: > > > > Error: CHECKED_RETURN (CWE-252): > > > > sssd-1.13.92/src/responder/secrets/local.c:627: check_return: Calling > > > > "unlink" without checking return value (as is done elsewhere 22 out of > > > > 23 times). > > > > sssd-1.13.92/src/confdb/confdb_setup.c:385: example_assign: Example 1: > > > > Assigning: "ret" = return value from "unlink(cdb_file)". > > > > sssd-1.13.92/src/confdb/confdb_setup.c:386: example_checked: Example 1 > > > > (cont.): "ret" has its value checked in "ret != 0". > > > > sssd-1.13.92/src/db/sysdb_init.c:755: example_assign: Example 2: > > > > Assigning: "ret" = return value from "unlink(sysdb->ldb_ts_file)". > > > > sssd-1.13.92/src/db/sysdb_init.c:756: example_checked: Example 2 > > > > (cont.): "ret" has its value checked in "ret != 0". > > > > sssd-1.13.92/src/monitor/monitor.c:1575: example_assign: Example 3: > > > > Assigning: "ret" = return value from "unlink("/var/run/sssd.pid")". > > > > sssd-1.13.92/src/monitor/monitor.c:1576: example_checked: Example 3 > > > > (cont.): "ret" has its value checked in "ret == -1". > > > > sssd-1.13.92/src/providers/ipa/ipa_init.c:428: example_assign: Example > > > > 4: Assigning: "ret" = return value from > > > > "unlink("/var/lib/sss/pubconf/pam_preauth_available")". > > > > sssd-1.13.92/src/providers/ipa/ipa_init.c:429: example_checked: Example > > > > 4 (cont.): "ret" has its value checked in "ret != 0". > > > > sssd-1.13.92/src/providers/ipa/ipa_subdomains_server.c:451: > > > > example_assign: Example 5: Assigning: "ret" = return value from > > > > "unlink(keytab_path)". > > > > sssd-1.13.92/src/providers/ipa/ipa_subdomains_server.c:452: > > > > example_checked: Example 5 (cont.): "ret" has its value checked in "ret > > > > == -1". > > > > # 625| close(fd); > > > > # 626| if (rsize != size) { > > > > # 627|-> unlink(filename); > > > > # 628| return EFAULT; > > > > # 629| } > > > > I'm not sure why Coveriy started complaining now, but I still wonder if > > > > we > > > > should check the return value and just warn to silence the static > > > > analyzer. > > > > > > I'm all for checking the return values, but iirc the attached patches do > > > not add an unlink() call. > > > > > > > > > > > The other is inline: > > > > > From d2c50ee770f0f0c95b6b1a41ada99d4db55c5c77 Mon Sep 17 00:00:00 2001 > > > > > From: Sumit Bose <sb...@redhat.com> > > > > > Date: Fri, 1 Jul 2016 18:18:14 +0200 > > > > > Subject: [PATCH 6/6] IPA: enable enterprise principals if server > > > > > supports them > > > > > > > > > > If there are alternative UPN suffixes found on the server we can > > > > > safely > > > > > assume that the IPA server supports enterprise principals. > > > > > > > > > > Resolves https://fedorahosted.org/sssd/ticket/3018 > > > > > > > > [...] > > > > > > > > > + ret = confdb_get_param(be_ctx->cdb, tmp_ctx, be_ctx->conf_path, > > > > > + > > > > > ipa_def_krb5_opts[KRB5_USE_ENTERPRISE_PRINCIPAL].opt_name, > > > > > + &vals); > > > > > > > > Did you find it impractical here to check the dp_opts structure? The > > > > code > > > > is not wrong, it's just that normally we read that structure instead of > > > > looking directly at confdb. > > > > > > Afaik with dp_opts there is no way to see if the value came from > > > sssd.conf, i.e. is explicitly set, or if it is just the default value. > > > In the first case I always keep the value from sssd.conf no matter what > > > it is. > > > > The patches seem to work well on the SSSD side, but it seems there is an > > issue on the IPA side with enterprise principals. > > > > AD users login works with these patches and I can see the enterprise > > principal is being requested. > > > > CI: http://sssd-ci.duckdns.org/logs/job/47/32/summary.html > > > > Provisional ACK, but we should fix the IPA issue as well. > > Hi, > > the IPA patches were pushed, so I was wondering if we can push these > patches to SSSD as well. > > I tested them again with logins of IPA and AD users and both worked. > > I know IPA didn't release a new tarball, but the 4.4 tarball wasn't > built even for Fedora, only for the next RHEL release..so at the very > least, we can patch downstream.
I think we can push the patches to master and use them for downstream as well. I would just wait with putting them into a new SSSD release until FreeIPA 4.4.1 is release. bye, Sumit > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
