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. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
