On (15/07/16 21:05), Sumit Bose wrote: >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 <[email protected]> >> > > > > 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. > I plan to push sssd-1.14.1 into fedora 23+ and there might be users which will use sssd with this patch against freeipa-4.3(fedora 24, ubuntu16.04) freipa-4.2(fedora-23 and rhel7.2)
I assume that there should not be problems with these versions of freeipa because they do not have such feature on serve side. IMHO, it still better to use freipa(git master) then official freeipa-4.4.0 these days due to many bug fixes. Therefore I think we should not be any problem if we push patch to upstream. LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
