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