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 <sbose(a)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.