On Fri, 2012-02-24 at 14:17 +0100, Jan Cholasta wrote:
> On 22.2.2012 09:13, Jan Zelený wrote:
>>> On Tue, 2012-02-21 at 16:48 +0100, Jakub Hrozek wrote:
>>>> On Tue, Feb 21, 2012 at 10:49:54AM +0100, Jan Zelený wrote:
>>>>>> On Thu, Feb 09, 2012 at 06:05:30PM +0100, Jan Zelený wrote:
>>>>>>>>> On Thu, 2012-02-09 at 13:46 +0100, Jakub Hrozek
wrote:
>>>>>>>>>> On Tue, Feb 07, 2012 at 01:40:39PM +0100, Jan
Zelený wrote:
>>>>>>>>>>> With all these changes happening in last two
weeks, the IPA
>>>>>>>>>>> hosts code was messy at best. This patch
sorts out some of
>>>>>>>>>>> the mess. I already did some testing: SELinux
and HBAC
>>>>>>>>>>> seem to be working correctly. Honza, please
take a look
>>>>>>>>>>> and test this patch with your SSH-related
code. That is
>>>>>>>>>>> the last code that uses host fetching.
>>>>>>>>>>>
>>>>>>>>>>> If this patch makes it to master, I plan to
do some more
>>>>>>>>>>> cleanup in the HBAC code which is closely
related to the
>>>>>>>>>>> code this patch cleans.
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Jan
>>>>>>>>>>
>>>>>>>>>> None of the new options is documented or has a
configAPI
>>>>>>>>>> entry. If that's intentional to avoid
breaking string
>>>>>>>>>> freeze, then there should be a ticket to track
adding them.
>>>>>>>>>
>>>>>>>>> They still need to be in the sssd.api.d/sssd-ipa.conf
file. We
>>>>>>>>> can omit the strings from SSSDConfig.py to avoid
breaking
>>>>>>>>> string freeze I guess. But the API needs to not choke
on them
>>>>>>>>> if they're specified in the config file.
>>>>>>>>>
>>>>>>>>> Is this change necessary for the 1.8.0 release, or
can we just
>>>>>>>>> fix it properly (with option changes) in 1.9.0?
>>>>>>>>
>>>>>>>> I guess we can leave it for 1.9, the current state
isn't breaking
>>>>>>>> anything AFAIK.
>>>>>>>>
>>>>>>>>>> ...or...
>>>>>>>>>>
>>>>>>>>>> I was actually thinking if we want to document
the IPA
>>>>>>>>>> attribute maps at all. The options clutter the
manual page
>>>>>>>>>> and their value is questionable. Would anyone
oppose
>>>>>>>>>> *removing* all the attribute maps from the IPA
manual page?
>>>>>>>>>> Or at the very least moving them into some
section down at
>>>>>>>>>> the bottom where noone would be confused by the
options.
>>>>>>>>>
>>>>>>>>> I'm in favor of removing any manpage entries that
are of
>>>>>>>>> neglibible utility (in 1.9.0, not 1.8.0).
>>>>>>>>
>>>>>>>> Agreed. I'll file a ticket for that.
>>>>>>>>
>>>>>>>> I'll send updated patch for master in a moment
>>>>>>>
>>>>>>> Filed ticket
https://fedorahosted.org/sssd/ticket/1187
>>>>>>>
>>>>>>> Patches are in attachment.
>>>>>>>
>>>>>>> Jan
>>>>>>
>>>>>> The patch works fine but have you considered moving
"sdap_attr_map
>>>>>> *selinuxuser_map", host_map and hostgroup_map into struct
>>>>>> ipa_options?
>>>>>>
>>>>>> It seems odd that the search bases are in struct ipa_options but
maps
>>>>>> are in sdap_options. Also ipa_options seems like a better fit
for
>>>>>> IPA-specific data.
>>>>>
>>>>> Done, patches are in attachment.
>>>>>
>>>>> Jan
>>>>
>>>> Ack
>>>
>>> Nack. This patch fails to compile with the error:
>>>
>>> /bin/sh ./libtool --tag=CC --mode=compile gcc -DHAVE_CONFIG_H -I.
>>> -I/home/sgallagh/workspace/sssd -Wall -Iinclude -I..
>>> -I/home/sgallagh/workspace/sssd/include
>>> -I/home/sgallagh/workspace/sssd/src/sss_client
>>> -I/home/sgallagh/workspace/sssd/src -Iinclude -I.
>>> -I/usr/include/dbus-1.0 -I/usr/lib64/dbus-1.0/include -DLIBDIR=
>>> \"/usr/lib64\" -DVARDIR=\"/var\"
-DSHLIBEXT=\"\" -DSSSD_LIBEXEC_PATH=
>>> \"/usr/libexec/sssd\" -DSSSD_INTROSPECT_PATH=\"\"
-DSSSD_CONF_DIR=
>>> \"/etc/sssd\"
-DSSS_NSS_SOCKET_NAME=\"/var/lib/sss/pipes/nss\"
>>> -DSSS_PAM_SOCKET_NAME=\"/var/lib/sss/pipes/pam\"
>>> -DSSS_PAM_PRIV_SOCKET_NAME=\"/var/lib/sss/pipes/private/pam\"
>>> -DSSS_SUDO_SOCKET_NAME=\"/var/lib/sss/pipes/sudo\"
>>> -DSSS_AUTOFS_SOCKET_NAME=\"/var/lib/sss/pipes/autofs\"
>>> -DSSS_SSH_SOCKET_NAME=\"/var/lib/sss/pipes/ssh\" -DLOCALEDIR=
>>> \"/usr/share/locale\" -Wall -Wshadow -Wstrict-prototypes
>>> -Wpointer-arith -Wcast-qual -Wcast-align -Wwrite-strings
>>> -Werror-implicit-function-declaration -fno-strict-aliasing -ggdb3 -O0
>>> -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wformat-security
>>> -Werror -MT src/providers/ipa/libsss_ipa_la-ipa_hostid.lo -MD -MP -MF
>>> src/providers/ipa/.deps/libsss_ipa_la-ipa_hostid.Tpo -c -o
>>> src/providers/ipa/libsss_ipa_la-ipa_hostid.lo `test -f
>>> 'src/providers/ipa/ipa_hostid.c' || echo
>>> '/home/sgallagh/workspace/sssd/'`src/providers/ipa/ipa_hostid.c
>>> libtool: compile: gcc -DHAVE_CONFIG_H -I.
>>> -I/home/sgallagh/workspace/sssd -Wall -Iinclude -I..
>>> -I/home/sgallagh/workspace/sssd/include
>>> -I/home/sgallagh/workspace/sssd/src/sss_client
>>> -I/home/sgallagh/workspace/sssd/src -Iinclude -I.
>>> -I/usr/include/dbus-1.0 -I/usr/lib64/dbus-1.0/include -DLIBDIR=
>>> \"/usr/lib64\" -DVARDIR=\"/var\"
-DSHLIBEXT=\"\" -DSSSD_LIBEXEC_PATH=
>>> \"/usr/libexec/sssd\" -DSSSD_INTROSPECT_PATH=\"\"
-DSSSD_CONF_DIR=
>>> \"/etc/sssd\"
-DSSS_NSS_SOCKET_NAME=\"/var/lib/sss/pipes/nss\"
>>> -DSSS_PAM_SOCKET_NAME=\"/var/lib/sss/pipes/pam\"
>>> -DSSS_PAM_PRIV_SOCKET_NAME=\"/var/lib/sss/pipes/private/pam\"
>>> -DSSS_SUDO_SOCKET_NAME=\"/var/lib/sss/pipes/sudo\"
>>> -DSSS_AUTOFS_SOCKET_NAME=\"/var/lib/sss/pipes/autofs\"
>>> -DSSS_SSH_SOCKET_NAME=\"/var/lib/sss/pipes/ssh\" -DLOCALEDIR=
>>> \"/usr/share/locale\" -Wall -Wshadow -Wstrict-prototypes
-Wpointer-arith
>>> -Wcast-qual -Wcast-align -Wwrite-strings
>>> -Werror-implicit-function-declaration -fno-strict-aliasing -ggdb3 -O0
>>> -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wformat-security
>>> -Werror -MT src/providers/ipa/libsss_ipa_la-ipa_hostid.lo -MD -MP -MF
>>> src/providers/ipa/.deps/libsss_ipa_la-ipa_hostid.Tpo
>>> -c /home/sgallagh/workspace/sssd/src/providers/ipa/ipa_hostid.c -fPIC
>>> -DPIC -o src/providers/ipa/.libs/libsss_ipa_la-ipa_hostid.o
>>> /home/sgallagh/workspace/sssd/src/providers/ipa/ipa_hostid.c: In
>>> function 'hosts_get_connect_done':
>>> /home/sgallagh/workspace/sssd/src/providers/ipa/ipa_hostid.c:231:42:
>>> error: 'struct sdap_options' has no member named 'host_map'
>>> /home/sgallagh/workspace/sssd/src/providers/ipa/ipa_hostid.c:232:42:
>>> error: 'struct sdap_options' has no member named
'hostgroup_map'
>>> make[1]: *** [src/providers/ipa/libsss_ipa_la-ipa_hostid.lo] Error 1
>>> make[1]: Leaving directory `/home/sgallagh/workspace/sssd/x86_64'
>>> make: *** [check-recursive] Error 1
>>>
>>>
>>>
>>> While fixing this, please rebase it atop my patch "[PATCH] LDAP: Only
>>> use paging control on requests for multiple entries" so we don't
have to
>>> worry about merging it later.
>>
>> Thanks for catching that, my build script doesn't use experimental features
at
>> the moment. Rebased and fixed.
>>
>> Jan
>>
>
> Two things:
>
> In ipa_common.c:
>
> + { "ipa_host_name", "cn", SYSDB_NAME, NULL },
> + { "ipa_host_fqdn", "fqdn", SYSDB_FQDN, NULL },
>
> In IPA fqdn is the primary key of hosts, shouldn't it be mapped to
> SYSDB_NAME instead of cn?
>
It probably should have been, but we didn't do it that way initially and
it'll be too much trouble to change it (particularly for existing
deployments).
> In ipa_hostid.c:
>
> subreq = ipa_host_info_send(state, state->ev, state->sysdb,
> sdap_id_op_handle(state->op),
> - ctx->opts, state->name,
> - state->attrs, ctx->opts->host_map,
> - IPA_OPTS_HOST, false,
> + state->ctx->sdap_id_ctx->opts,
state->name,
> + state->ctx->ipa_opts->host_map,
> + state->ctx->ipa_opts->hostgroup_map,
> state->ctx->host_search_bases);
>
> Hostgroups aren't needed for SSH, so I think the
> state->ctx->ipa_opts->hostgroup_map bit should be replaced with NULL.
This is a nice-to-have for the future, but I don't think it's worth
holding up this patch. I'm going to push it.
Please open a ticket to track this change.
I have included the change in my SSH cleanup patchset.
Honza
--
Jan Cholasta