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