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 <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.
>
>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: