On Fri, Jul 15, 2016 at 09:13:01PM +0200, Lukas Slebodnik wrote: > 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.
OK, thanks. I removed one unused variable that I didn't notice before and pushed the patches upstream as: * 70673115c03c37ddc64c951b53d92df9d3310762 * 17dccc24e4490dfda2820d46b62a029b14ba2359 * 35fa5a83ce8badf6bc868937047f44c3f32b7c28 * 20348a30feb4be619b3b691c24c9be8131507c46 * 132b31fd5fb74a7627896cdceaf29c7601ed4795 * 39f21d2b61685362642d42bc2f94f829671cd5ef CI: http://sssd-ci.duckdns.org/logs/job/49/87/summary.html _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
