[PATCH] IPA_PROVIDER: Explicit no handle of services

[Petr Cech]

Hello,

please see first attempt of patch for:
https://fedorahosted.org/sssd/ticket/2747

Regards

Petr

PS:
# reproducer
getent services -s sss [e-mail]

[Jakub Hrozek]

On Mon, Nov 09, 2015 at 04:56:32PM +0100, Petr Cech wrote:
> Hello,
>
> please see first attempt of patch for:
> https://fedorahosted.org/sssd/ticket/2747

Hi, I think it's a good idea to only say we don't handle services for
IPA subdomains. But I also think it would be better to shortcut the
request sooner, in ipa_subdomain_account_send() to avoid even sending an
LDAP query.

[Petr Cech]

On 11/11/2015 02:42 PM, Jakub Hrozek wrote:
> Hi, I think it's a good idea to only say we don't handle services for
> IPA subdomains. But I also think it would be better to shortcut the
> request sooner, in ipa_subdomain_account_send() to avoid even sending an
> LDAP query.

Hi Jakub,

new patch is attached. During the testing... I found out, that I use 
wrong set up. Subdomains are connected to FreeIPA with trusted AD.

So... patch is here, but I would like set up my environment properly and 
then I will inform you :-)

Regards

Petr

[Jakub Hrozek]

On Thu, Nov 12, 2015 at 04:54:21PM +0100, Petr Cech wrote:
> On 11/11/2015 02:42 PM, Jakub Hrozek wrote:
> > Hi, I think it's a good idea to only say we don't handle services for
> > IPA subdomains. But I also think it would be better to shortcut the
> > request sooner, in ipa_subdomain_account_send() to avoid even sending an
> > LDAP query.
>
> Hi Jakub,
>
> new patch is attached. During the testing... I found out, that I use wrong
> set up. Subdomains are connected to FreeIPA with trusted AD.

What's wrong about it? I think we /should/ test this setup..
> So... patch is here, but I would like set up my environment properly and
> then I will inform you :-)
>
> Regards
>
> Petr
> From a7d1a734489434df07d7663deb201bac10f01891 Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Mon, 9 Nov 2015 09:51:05 -0500
> Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
>
> Services for IPA subdomains aren't handled by SSSD. This patch add
> quick shortcut to avoid sending an LDAP query.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2747
>
> src/providers/ipa/ipa_subdomains_id.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c
> index 472985d4ab4f785aa9c4af94bf8021829ca1c3c8..66898eb136dd09da5ca034f0e7ba0f54b075fcab 100644
> --- a/src/providers/ipa/ipa_subdomains_id.c
> +++ b/src/providers/ipa/ipa_subdomains_id.c
> @@ -81,6 +81,12 @@ struct tevent_req *ipa_subdomain_account_send(TALLOC_CTX *memctx,
>      struct tevent_req *subreq;
>      int ret;
> if ((ar->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_SERVICES)  {
>    DEBUG(SSSDBG_OP_FAILURE,
>          "Services for IPA subdomains aren't handled by SSSD.\n");
>    return NULL;

(I know this is undocumented tribal knowledge)

I would prefer if the function, instead of returning NULL created the
request and then just marked it as finished (either with success and no
entries or ENOENT, not sure which is better in this case)

We normally use NULL return from _send() tevent functions only in OOM
situations.
> }
> req = tevent_req_create(memctx, &state, struct ipa_subdomain_account_state);
>    if (req == NULL) {
>        DEBUG(SSSDBG_OP_FAILURE, "tevent_req_create failed.\n");
>
> -- 
> 2.4.3
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Petr Cech]

On 11/12/2015 06:04 PM, Jakub Hrozek wrote:
> On Thu, Nov 12, 2015 at 04:54:21PM +0100, Petr Cech wrote:
> > > On 11/11/2015 02:42 PM, Jakub Hrozek wrote:
> > > > > Hi, I think it's a good idea to only say we don't handle services for
> > > > > IPA subdomains. But I also think it would be better to shortcut the
> > > > > request sooner, in ipa_subdomain_account_send() to avoid even sending an
> > > > > LDAP query.
> > >
> > > Hi Jakub,
> > >
> > > new patch is attached. During the testing... I found out, that I use wrong
> > > set up. Subdomains are connected to FreeIPA with trusted AD.
>
> What's wrong about it? I think we/should/  test this setup..

Hi Jakub,

I was confused about it. I haven't got such environment with IP trust. 
But now I have one.
> > > So... patch is here, but I would like set up my environment properly and
> > > then I will inform you:-)
> > >
> > > Regards
> > >
> > > Petr

But I still do something wrong. I set up environment:

AD server: dom-78.abc...
FreeIPA server: mirfak.persei.dev
FreeIPA client: algol.persei.dev

I set up the trust between IPA and AD. And then I tried on ipa clinet:
# getent services ldap/[e-mail]

The result of 'tail -f *.log' in /var/log/sssd is attached.

Please, does anybody know what I do wrong?

Petr

PS: <astronomy>Mirfak is the brightest star in Perseus and Algol is the 
second one.</astronomy>

[Petr Cech]

On 11/19/2015 05:57 PM, Petr Cech wrote:
> On 11/12/2015 06:04 PM, Jakub Hrozek wrote:
> > On Thu, Nov 12, 2015 at 04:54:21PM +0100, Petr Cech wrote:
> > > > On 11/11/2015 02:42 PM, Jakub Hrozek wrote:
> > > > > > Hi, I think it's a good idea to only say we don't handle services
> > > >
> > > > for
> > > > > > IPA subdomains. But I also think it would be better to shortcut the
> > > > > > request sooner, in ipa_subdomain_account_send() to avoid even
> > > >
> > > > sending an
> > > > > > LDAP query.
> > > >
> > > > Hi Jakub,
> > > >
> > > > new patch is attached. During the testing... I found out, that I use
> > >
> > > wrong
> > > > set up. Subdomains are connected to FreeIPA with trusted AD.
> >
> > What's wrong about it? I think we/should/  test this setup..
>
> Hi Jakub,
>
> I was confused about it. I haven't got such environment with IP trust.
> But now I have one.
> > > > So... patch is here, but I would like set up my environment properly
> > >
> > > and
> > > > then I will inform you:-)
> > > >
> > > > Regards
> > > >
> > > > Petr
>
> But I still do something wrong. I set up environment:
>
> AD server: dom-78.abc...
> FreeIPA server: mirfak.persei.dev
> FreeIPA client: algol.persei.dev
>
> I set up the trust between IPA and AD. And then I tried on ipa clinet:
> # getent services ldap/[e-mail]
>
> The result of 'tail -f *.log' in /var/log/sssd is attached.
>
> Please, does anybody know what I do wrong?
>
> Petr
>
> PS: <astronomy>Mirfak is the brightest star in Perseus and Algol is the
> second one.</astronomy>

bump

[Jakub Hrozek]

On Thu, Nov 19, 2015 at 05:57:56PM +0100, Petr Cech wrote:
> On 11/12/2015 06:04 PM, Jakub Hrozek wrote:
> > On Thu, Nov 12, 2015 at 04:54:21PM +0100, Petr Cech wrote:
> > > > On 11/11/2015 02:42 PM, Jakub Hrozek wrote:
> > > > > > Hi, I think it's a good idea to only say we don't handle services for
> > > > > > IPA subdomains. But I also think it would be better to shortcut the
> > > > > > request sooner, in ipa_subdomain_account_send() to avoid even sending an
> > > > > > LDAP query.
> > > >
> > > > Hi Jakub,
> > > >
> > > > new patch is attached. During the testing... I found out, that I use wrong
> > > > set up. Subdomains are connected to FreeIPA with trusted AD.
> >
> > What's wrong about it? I think we/should/  test this setup..
>
> Hi Jakub,
>
> I was confused about it. I haven't got such environment with IP trust. But
> now I have one.
> > > > So... patch is here, but I would like set up my environment properly and
> > > > then I will inform you:-)
> > > >
> > > > Regards
> > > >
> > > > Petr
>
> But I still do something wrong. I set up environment:
>
> AD server: dom-78.abc...
> FreeIPA server: mirfak.persei.dev
> FreeIPA client: algol.persei.dev
>
> I set up the trust between IPA and AD. And then I tried on ipa clinet:
> # getent services ldap/[e-mail]

This is not a different kind of service (a Kerberos service principal).
The ticket is about services resolvable via libc's NSS interface:
    https://www.gnu.org/software/libc/manual/html_node/Name-Service-Switch.html#...

See man 5 nsswitch conf for some more info. The API used for resolving
services is described in man 3 getservent and similar. Normally services
are stored on the filesystem, similar to local users, see cat
/etc/services. But custom services can also be stored in LDAP or other
sources. And while we support looking up services, the IPA back end does
not and currently if you do:
    getent services -s sss dns

The logs would show:
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [be_get_account_info] (0x0200): Got request for [0x1005][FAST BE_REQ_SERVICES][1][name=dns]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [be_req_set_domain] (0x0400): Changing request domain from [ipa.test] to [ipa.test]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [services_get_send] (0x1000): Preparing to search for services with filter [(&(cn=dns)(objectclass=ipService))]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_id_op_connect_step] (0x4000): reusing cached connection
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_services_next_base] (0x0400): Searching for services with base [cn=accounts,dc=ipa,dc=test]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_print_server] (0x2000): Searching 192.168.122.100
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x0400): calling ldap_search_ext with [(&(cn=dns)(objectclass=ipService))][cn=accounts,dc=ipa,dc=test].
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [objectClass]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [cn]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServicePort]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServiceProtocol]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [entryUSN]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x2000): ldap_search_ext called, msgid = 16
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_op_add] (0x2000): New operation 16 timeout 2
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_process_result] (0x2000): Trace: sh[0xeedc00], connected[1], ops[0xf1aca0], ldap[0xef0bb0]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_RESULT]
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_op_finished] (0x0400): Search result: Success(0), no errmsg set
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_op_destructor] (0x2000): Operation 16 finished
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_services_process] (0x0400): Search for services, returned 0 results.
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_id_op_done] (0x4000): releasing operation connection
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): start ldb transaction (nesting: 0)
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sysdb_search_services] (0x2000): Search services with filter: (&(objectclass=service)(&(serviceProtocol=*)(|(nameAlias=dns)(name=dns))))
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Added timed event "ltdb_callback": 0xf06ba0

(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Added timed event "ltdb_timeout": 0xf06c60

(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Running timer event 0xf06ba0 "ltdb_callback"

(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Destroying timer event 0xf06c60 "ltdb_timeout"

(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Ending timer event 0xf06ba0 "ltdb_callback"

(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sysdb_search_services] (0x2000): No such entry
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): cancel ldb transaction (nesting: 0)
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [get_object_from_cache] (0x0020): Unexpected entry type [5].
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ipa_id_get_account_info_orig_done] (0x0040): get_object_from_cache failed.
(Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ipa_account_info_error_text] (0x0020): Bug: dp_error is OK on failed request

The problem is that software that tries to locate service entries via NSS
might ask SSSD and then we get these confusing error messages..

I think the question we need to ask is if we want to support admins who
add service records to IPA manually. If we do, then I think we should
allow service lookups for the IPA domain itself and then don't error out
in get_object_from_cache. Trusted domain lookups can just end. If we decide
that service lookups are not supported at all with IPA, then we can always
just return "not found".

[Petr Cech]

On 11/27/2015 10:35 AM, Jakub Hrozek wrote:
> On Thu, Nov 19, 2015 at 05:57:56PM +0100, Petr Cech wrote:
> > On 11/12/2015 06:04 PM, Jakub Hrozek wrote:
> > > On Thu, Nov 12, 2015 at 04:54:21PM +0100, Petr Cech wrote:
> > > > > On 11/11/2015 02:42 PM, Jakub Hrozek wrote:
> > > > > > > Hi, I think it's a good idea to only say we don't handle services for
> > > > > > > IPA subdomains. But I also think it would be better to shortcut the
> > > > > > > request sooner, in ipa_subdomain_account_send() to avoid even sending an
> > > > > > > LDAP query.
> > > > >
> > > > > Hi Jakub,
> > > > >
> > > > > new patch is attached. During the testing... I found out, that I use wrong
> > > > > set up. Subdomains are connected to FreeIPA with trusted AD.
> > >
> > > What's wrong about it? I think we/should/  test this setup..
> >
> > Hi Jakub,
> >
> > I was confused about it. I haven't got such environment with IP trust. But
> > now I have one.
> > > > > So... patch is here, but I would like set up my environment properly and
> > > > > then I will inform you:-)
> > > > >
> > > > > Regards
> > > > >
> > > > > Petr
> >
> > But I still do something wrong. I set up environment:
> >
> > AD server: dom-78.abc...
> > FreeIPA server: mirfak.persei.dev
> > FreeIPA client: algol.persei.dev
> >
> > I set up the trust between IPA and AD. And then I tried on ipa clinet:
> > # getent services ldap/[e-mail]
>
> This is not a different kind of service (a Kerberos service principal).
> The ticket is about services resolvable via libc's NSS interface:
>      https://www.gnu.org/software/libc/manual/html_node/Name-Service-Switch.html#...

Hi Jakub,

thank you for documentation.
> See man 5 nsswitch conf for some more info. The API used for resolving
> services is described in man 3 getservent and similar. Normally services
> are stored on the filesystem, similar to local users, see cat
> /etc/services. But custom services can also be stored in LDAP or other
> sources. And while we support looking up services, the IPA back end does
> not and currently if you do:
>      getent services -s sss dns

This command is usefull. I have used:
# getent services -s sss [e-mail]
> The logs would show:
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [be_get_account_info] (0x0200): Got request for [0x1005][FAST BE_REQ_SERVICES][1][name=dns]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [be_req_set_domain] (0x0400): Changing request domain from [ipa.test] to [ipa.test]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [services_get_send] (0x1000): Preparing to search for services with filter [(&(cn=dns)(objectclass=ipService))]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_id_op_connect_step] (0x4000): reusing cached connection
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_services_next_base] (0x0400): Searching for services with base [cn=accounts,dc=ipa,dc=test]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_print_server] (0x2000): Searching 192.168.122.100
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x0400): calling ldap_search_ext with [(&(cn=dns)(objectclass=ipService))][cn=accounts,dc=ipa,dc=test].
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [objectClass]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [cn]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServicePort]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServiceProtocol]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [entryUSN]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x2000): ldap_search_ext called, msgid = 16
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_op_add] (0x2000): New operation 16 timeout 2
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_process_result] (0x2000): Trace: sh[0xeedc00], connected[1], ops[0xf1aca0], ldap[0xef0bb0]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_RESULT]
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_op_finished] (0x0400): Search result: Success(0), no errmsg set
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_op_destructor] (0x2000): Operation 16 finished
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_services_process] (0x0400): Search for services, returned 0 results.
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_id_op_done] (0x4000): releasing operation connection
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): start ldb transaction (nesting: 0)
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sysdb_search_services] (0x2000): Search services with filter: (&(objectclass=service)(&(serviceProtocol=*)(|(nameAlias=dns)(name=dns))))
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Added timed event "ltdb_callback": 0xf06ba0
>
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Added timed event "ltdb_timeout": 0xf06c60
>
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Running timer event 0xf06ba0 "ltdb_callback"
>
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Destroying timer event 0xf06c60 "ltdb_timeout"
>
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Ending timer event 0xf06ba0 "ltdb_callback"
>
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sysdb_search_services] (0x2000): No such entry
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): cancel ldb transaction (nesting: 0)

If I understand, those lines are misleading:
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [get_object_from_cache] (0x0020): Unexpected entry type [5].
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ipa_id_get_account_info_orig_done] (0x0040): get_object_from_cache failed.
> (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ipa_account_info_error_text] (0x0020): Bug: dp_error is OK on failed request

New attached patch removes them. There are logs:

(Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] 
[get_object_from_cache] (0x0040): Services for IPA subdomains aren't 
handled by SSSD.
(Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] 
[ipa_id_get_account_info_orig_done] (0x0080): Object not found, ending 
request
(Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [acctinfo_callback] 
(0x0100): Request processed. Returned 3,0,Account info lookup failed
(Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [sdap_process_result] 
(0x2000): Trace: sh[0x89f8d0], connected[1], ops[(nil)], ldap[0x89c340]
(Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [sdap_process_result] 
(0x2000): Trace: ldap_result found nothing!
> The problem is that software that tries to locate service entries via NSS
> might ask SSSD and then we get these confusing error messages..
>
> I think the question we need to ask is if we want to support admins who
> add service records to IPA manually. If we do, then I think we should
> allow service lookups for the IPA domain itself and then don't error out
> in get_object_from_cache. Trusted domain lookups can just end. If we decide
> that service lookups are not supported at all with IPA, then we can always
> just return "not found".

That's the question. I think that better is no support for service 
lookup. Because such services were added manually by admins, so there is 
no check, standardization.

I am not sure if I have done whole task. If we need another shortcut, 
please, write me.

Regards

Petr

[Jakub Hrozek]

On Tue, Dec 01, 2015 at 05:02:39PM +0100, Petr Cech wrote:
> On 11/27/2015 10:35 AM, Jakub Hrozek wrote:
> > On Thu, Nov 19, 2015 at 05:57:56PM +0100, Petr Cech wrote:
> > > On 11/12/2015 06:04 PM, Jakub Hrozek wrote:
> > > > On Thu, Nov 12, 2015 at 04:54:21PM +0100, Petr Cech wrote:
> > > > > > On 11/11/2015 02:42 PM, Jakub Hrozek wrote:
> > > > > > >>Hi, I think it's a good idea to only say we don't handle services for
> > > > > > >>IPA subdomains. But I also think it would be better to shortcut the
> > > > > > >>request sooner, in ipa_subdomain_account_send() to avoid even sending an
> > > > > > >>LDAP query.
> > > > > > Hi Jakub,
> > > > > >
> > > > > > new patch is attached. During the testing... I found out, that I use wrong
> > > > > > set up. Subdomains are connected to FreeIPA with trusted AD.
> > > >
> > > > What's wrong about it? I think we/should/  test this setup..
> > >
> > > Hi Jakub,
> > >
> > > I was confused about it. I haven't got such environment with IP trust. But
> > > now I have one.
> > > > > > So... patch is here, but I would like set up my environment properly and
> > > > > > then I will inform you:-)
> > > > > >
> > > > > > Regards
> > > > > >
> > > > > > Petr
> > >
> > > But I still do something wrong. I set up environment:
> > >
> > > AD server: dom-78.abc...
> > > FreeIPA server: mirfak.persei.dev
> > > FreeIPA client: algol.persei.dev
> > >
> > > I set up the trust between IPA and AD. And then I tried on ipa clinet:
> > > # getent services ldap/[e-mail]
> >
> > This is not a different kind of service (a Kerberos service principal).
> > The ticket is about services resolvable via libc's NSS interface:
> >     https://www.gnu.org/software/libc/manual/html_node/Name-Service-Switch.html#...
>
> Hi Jakub,
>
> thank you for documentation.
> > See man 5 nsswitch conf for some more info. The API used for resolving
> > services is described in man 3 getservent and similar. Normally services
> > are stored on the filesystem, similar to local users, see cat
> > /etc/services. But custom services can also be stored in LDAP or other
> > sources. And while we support looking up services, the IPA back end does
> > not and currently if you do:
> >     getent services -s sss dns
>
> This command is usefull. I have used:
> # getent services -s sss [e-mail]
> > The logs would show:
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [be_get_account_info] (0x0200): Got request for [0x1005][FAST BE_REQ_SERVICES][1][name=dns]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [be_req_set_domain] (0x0400): Changing request domain from [ipa.test] to [ipa.test]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [services_get_send] (0x1000): Preparing to search for services with filter [(&(cn=dns)(objectclass=ipService))]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_id_op_connect_step] (0x4000): reusing cached connection
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_services_next_base] (0x0400): Searching for services with base [cn=accounts,dc=ipa,dc=test]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_print_server] (0x2000): Searching 192.168.122.100
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x0400): calling ldap_search_ext with [(&(cn=dns)(objectclass=ipService))][cn=accounts,dc=ipa,dc=test].
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [objectClass]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [cn]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServicePort]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServiceProtocol]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [entryUSN]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x2000): ldap_search_ext called, msgid = 16
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_op_add] (0x2000): New operation 16 timeout 2
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_process_result] (0x2000): Trace: sh[0xeedc00], connected[1], ops[0xf1aca0], ldap[0xef0bb0]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_RESULT]
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_op_finished] (0x0400): Search result: Success(0), no errmsg set
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_op_destructor] (0x2000): Operation 16 finished
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_services_process] (0x0400): Search for services, returned 0 results.
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_id_op_done] (0x4000): releasing operation connection
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): start ldb transaction (nesting: 0)
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sysdb_search_services] (0x2000): Search services with filter: (&(objectclass=service)(&(serviceProtocol=*)(|(nameAlias=dns)(name=dns))))
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Added timed event "ltdb_callback": 0xf06ba0
> >
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Added timed event "ltdb_timeout": 0xf06c60
> >
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Running timer event 0xf06ba0 "ltdb_callback"
> >
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Destroying timer event 0xf06c60 "ltdb_timeout"
> >
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Ending timer event 0xf06ba0 "ltdb_callback"
> >
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sysdb_search_services] (0x2000): No such entry
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): cancel ldb transaction (nesting: 0)
>
> If I understand, those lines are misleading:
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [get_object_from_cache] (0x0020): Unexpected entry type [5].
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ipa_id_get_account_info_orig_done] (0x0040): get_object_from_cache failed.
> > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ipa_account_info_error_text] (0x0020): Bug: dp_error is OK on failed request
>
> New attached patch removes them. There are logs:
>
> (Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [get_object_from_cache]
> (0x0040): Services for IPA subdomains aren't handled by SSSD.
> (Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]]
> [ipa_id_get_account_info_orig_done] (0x0080): Object not found, ending
> request
> (Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [acctinfo_callback]
> (0x0100): Request processed. Returned 3,0,Account info lookup failed
> (Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [sdap_process_result]
> (0x2000): Trace: sh[0x89f8d0], connected[1], ops[(nil)], ldap[0x89c340]
> (Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [sdap_process_result]
> (0x2000): Trace: ldap_result found nothing!
> > The problem is that software that tries to locate service entries via NSS
> > might ask SSSD and then we get these confusing error messages..
> >
> > I think the question we need to ask is if we want to support admins who
> > add service records to IPA manually. If we do, then I think we should
> > allow service lookups for the IPA domain itself and then don't error out
> > in get_object_from_cache. Trusted domain lookups can just end. If we decide
> > that service lookups are not supported at all with IPA, then we can always
> > just return "not found".
>
> That's the question. I think that better is no support for service lookup.
> Because such services were added manually by admins, so there is no check,
> standardization.

Hmm, I'm not sure I agree. I mean, is there a downsite to supporting the
service lookups?

The check and standardization should be done on the server side by the
schema, then it's only about a proper client configu.
> I am not sure if I have done whole task. If we need another shortcut,
> please, write me.

The current patch still marks the request as failed, which is something
I wanted to avoid:
    (Thu Dec  3 15:57:33 2015) [sssd[be[ipa.test]]]
    [get_object_from_cache] (0x0040): Services for IPA subdomains aren't
    handled by SSSD.
    (Thu Dec  3 15:57:33 2015) [sssd[be[ipa.test]]]
    [ipa_id_get_account_info_orig_done] (0x0080): Object not found,
    ending request
    (Thu Dec  3 15:57:33 2015) [sssd[be[ipa.test]]] [acctinfo_callback]
    (0x0100): Request processed. Returned 3,0,Account info lookup failed
Moreover, even though the debug message states service lookups are not
supported, the lookups runs and the service object would be cached at
that point, so all the work is more or less done.

So we have two options:
    1) shortcut the service lookups completely
    2) allow service lookups, but don't apply the override magic. I
    personally prefer this option (mostly because I don't see any
    downside), but I'm fine discussing this.

Implementing 1) would mean that we would check if the request is for
services in ipa_account_info_handler(). If yes, then we would just mark
the request as done with a debug message. This would prevent the search
from running (and note that the search here also includes the cache
save).

Implementing 2) would be a bit more involved. It would require to run
the LDAP search, but not the special IPA-specific override
functionality.

Once the flow arrives to the sssd_be process, the lookup goes like this:
    ipa_account_info_handler ->
     ipa_id_get_account_info_send ->
      ipa_id_get_account_info_get_original_step -> 
        sdap_handle_acct_req_send ->
            ipa_id_get_account_info_orig_done

In ipa_id_get_account_info_orig_done(), after we call
sdap_handle_acct_req_recv() we could check if the object is
"overridable". If yes, we would run the current code. If not, we would
mark the request as done. The function to check for objects that support
overrides might be also useful for starting the request -- in
ipa_id_get_account_info_send() we could use it to decide whether to go
directly to ipa_id_get_account_info_get_original_step() or first check
the overrides with the other branch.

Sorry for the long mail, I hope it makes some sense now :)

[Petr Cech]

On 12/03/2015 05:26 PM, Jakub Hrozek wrote:
> On Tue, Dec 01, 2015 at 05:02:39PM +0100, Petr Cech wrote:
> > On 11/27/2015 10:35 AM, Jakub Hrozek wrote:
> > > On Thu, Nov 19, 2015 at 05:57:56PM +0100, Petr Cech wrote:
> > > > On 11/12/2015 06:04 PM, Jakub Hrozek wrote:
> > > > > On Thu, Nov 12, 2015 at 04:54:21PM +0100, Petr Cech wrote:
> > > > > > > On 11/11/2015 02:42 PM, Jakub Hrozek wrote:
> > > > > > >>> Hi, I think it's a good idea to only say we don't handle services for
> > > > > > >>> IPA subdomains. But I also think it would be better to shortcut the
> > > > > > >>> request sooner, in ipa_subdomain_account_send() to avoid even sending an
> > > > > > >>> LDAP query.
> > > > > > > Hi Jakub,
> > > > > > >
> > > > > > > new patch is attached. During the testing... I found out, that I use wrong
> > > > > > > set up. Subdomains are connected to FreeIPA with trusted AD.
> > > > >
> > > > > What's wrong about it? I think we/should/  test this setup..
> > > >
> > > > Hi Jakub,
> > > >
> > > > I was confused about it. I haven't got such environment with IP trust. But
> > > > now I have one.
> > > > > > >
> > > > > > > So... patch is here, but I would like set up my environment properly and
> > > > > > > then I will inform you:-)
> > > > > > >
> > > > > > > Regards
> > > > > > >
> > > > > > > Petr
> > > >
> > > > But I still do something wrong. I set up environment:
> > > >
> > > > AD server: dom-78.abc...
> > > > FreeIPA server: mirfak.persei.dev
> > > > FreeIPA client: algol.persei.dev
> > > >
> > > > I set up the trust between IPA and AD. And then I tried on ipa clinet:
> > > > # getent services ldap/[e-mail]
> > >
> > > This is not a different kind of service (a Kerberos service principal).
> > > The ticket is about services resolvable via libc's NSS interface:
> > >      https://www.gnu.org/software/libc/manual/html_node/Name-Service-Switch.html#...
> >
> > Hi Jakub,
> >
> > thank you for documentation.
> > > See man 5 nsswitch conf for some more info. The API used for resolving
> > > services is described in man 3 getservent and similar. Normally services
> > > are stored on the filesystem, similar to local users, see cat
> > > /etc/services. But custom services can also be stored in LDAP or other
> > > sources. And while we support looking up services, the IPA back end does
> > > not and currently if you do:
> > >      getent services -s sss dns
> >
> > This command is usefull. I have used:
> > # getent services -s sss [e-mail]
> > > The logs would show:
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [be_get_account_info] (0x0200): Got request for [0x1005][FAST BE_REQ_SERVICES][1][name=dns]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [be_req_set_domain] (0x0400): Changing request domain from [ipa.test] to [ipa.test]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [services_get_send] (0x1000): Preparing to search for services with filter [(&(cn=dns)(objectclass=ipService))]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_id_op_connect_step] (0x4000): reusing cached connection
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_services_next_base] (0x0400): Searching for services with base [cn=accounts,dc=ipa,dc=test]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_print_server] (0x2000): Searching 192.168.122.100
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x0400): calling ldap_search_ext with [(&(cn=dns)(objectclass=ipService))][cn=accounts,dc=ipa,dc=test].
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [objectClass]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [cn]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServicePort]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServiceProtocol]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [entryUSN]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_ext_step] (0x2000): ldap_search_ext called, msgid = 16
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_op_add] (0x2000): New operation 16 timeout 2
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_process_result] (0x2000): Trace: sh[0xeedc00], connected[1], ops[0xf1aca0], ldap[0xef0bb0]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_RESULT]
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_generic_op_finished] (0x0400): Search result: Success(0), no errmsg set
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_op_destructor] (0x2000): Operation 16 finished
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_get_services_process] (0x0400): Search for services, returned 0 results.
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sdap_id_op_done] (0x4000): releasing operation connection
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): start ldb transaction (nesting: 0)
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sysdb_search_services] (0x2000): Search services with filter: (&(objectclass=service)(&(serviceProtocol=*)(|(nameAlias=dns)(name=dns))))
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Added timed event "ltdb_callback": 0xf06ba0
> > >
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Added timed event "ltdb_timeout": 0xf06c60
> > >
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Running timer event 0xf06ba0 "ltdb_callback"
> > >
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Destroying timer event 0xf06c60 "ltdb_timeout"
> > >
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): Ending timer event 0xf06ba0 "ltdb_callback"
> > >
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [sysdb_search_services] (0x2000): No such entry
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ldb] (0x4000): cancel ldb transaction (nesting: 0)
> >
> > If I understand, those lines are misleading:
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [get_object_from_cache] (0x0020): Unexpected entry type [5].
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ipa_id_get_account_info_orig_done] (0x0040): get_object_from_cache failed.
> > > (Fri Nov 27 09:31:13 2015) [sssd[be[ipa.test]]] [ipa_account_info_error_text] (0x0020): Bug: dp_error is OK on failed request
> >
> > New attached patch removes them. There are logs:
> >
> > (Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [get_object_from_cache]
> > (0x0040): Services for IPA subdomains aren't handled by SSSD.
> > (Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]]
> > [ipa_id_get_account_info_orig_done] (0x0080): Object not found, ending
> > request
> > (Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [acctinfo_callback]
> > (0x0100): Request processed. Returned 3,0,Account info lookup failed
> > (Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [sdap_process_result]
> > (0x2000): Trace: sh[0x89f8d0], connected[1], ops[(nil)], ldap[0x89c340]
> > (Tue Dec  1 16:53:50 2015) [sssd[be[persei.dev]]] [sdap_process_result]
> > (0x2000): Trace: ldap_result found nothing!
> > > The problem is that software that tries to locate service entries via NSS
> > > might ask SSSD and then we get these confusing error messages..
> > >
> > > I think the question we need to ask is if we want to support admins who
> > > add service records to IPA manually. If we do, then I think we should
> > > allow service lookups for the IPA domain itself and then don't error out
> > > in get_object_from_cache. Trusted domain lookups can just end. If we decide
> > > that service lookups are not supported at all with IPA, then we can always
> > > just return "not found".
> >
> > That's the question. I think that better is no support for service lookup.
> > Because such services were added manually by admins, so there is no check,
> > standardization.
>
> Hmm, I'm not sure I agree. I mean, is there a downsite to supporting the
> service lookups?

No, there isn't if I understand the problem.
> The check and standardization should be done on the server side by the
> schema, then it's only about a proper client configu.
> > I am not sure if I have done whole task. If we need another shortcut,
> > please, write me.
>
> The current patch still marks the request as failed, which is something
> I wanted to avoid:
>      (Thu Dec  3 15:57:33 2015) [sssd[be[ipa.test]]]
>      [get_object_from_cache] (0x0040): Services for IPA subdomains aren't
>      handled by SSSD.
>      (Thu Dec  3 15:57:33 2015) [sssd[be[ipa.test]]]
>      [ipa_id_get_account_info_orig_done] (0x0080): Object not found,
>      ending request
>      (Thu Dec  3 15:57:33 2015) [sssd[be[ipa.test]]] [acctinfo_callback]
>      (0x0100): Request processed. Returned 3,0,Account info lookup failed
> Moreover, even though the debug message states service lookups are not
> supported, the lookups runs and the service object would be cached at
> that point, so all the work is more or less done.
>
> So we have two options:
>      1) shortcut the service lookups completely
>      2) allow service lookups, but don't apply the override magic. I
>      personally prefer this option (mostly because I don't see any
>      downside), but I'm fine discussing this.
>
> Implementing 1) would mean that we would check if the request is for
> services in ipa_account_info_handler(). If yes, then we would just mark
> the request as done with a debug message. This would prevent the search
> from running (and note that the search here also includes the cache
> save).
>
> Implementing 2) would be a bit more involved. It would require to run
> the LDAP search, but not the special IPA-specific override
> functionality.
>
> Once the flow arrives to the sssd_be process, the lookup goes like this:
>      ipa_account_info_handler ->
>       ipa_id_get_account_info_send ->
>        ipa_id_get_account_info_get_original_step ->
>          sdap_handle_acct_req_send ->
>              ipa_id_get_account_info_orig_done
>
> In ipa_id_get_account_info_orig_done(), after we call
> sdap_handle_acct_req_recv() we could check if the object is
> "overridable". If yes, we would run the current code. If not, we would
> mark the request as done. The function to check for objects that support
> overrides might be also useful for starting the request -- in
> ipa_id_get_account_info_send() we could use it to decide whether to go
> directly to ipa_id_get_account_info_get_original_step() or first check
> the overrides with the other branch.
>
> Sorry for the long mail, I hope it makes some sense now :)

So, I have choose the 2) option and the patch is attached.
And something more:
  a.bak ... log before patch
  b.bak ... log after patch

Regards

Petr

[Jakub Hrozek]

On Fri, Dec 04, 2015 at 04:33:51PM +0100, Petr Cech wrote:
> So, I have choose the 2) option and the patch is attached.

I think this patch goes in the right direction, but still needs more
work. See inline.
> And something more:
>  a.bak ... log before patch
>  b.bak ... log after patch
>
> Regards
>
> Petr
> From 12f03f5510cdfc56efd6d24bf06db0ad0e6d6368 Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Mon, 9 Nov 2015 09:51:05 -0500
> Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
>
> Services for IPA subdomains aren't handled by SSSD. This patch
> adds quick shortcut to avoid sending an LDAP query to cache.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2747
>
> src/providers/ipa/ipa_id.c            | 28 +++++++++++++++++++++++++++-
>  src/providers/ipa/ipa_subdomains_id.c |  7 +++++++
>  2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
> index e81ccb34dd6eb44618538593f5473fbe5e89d896..efc00d2ae0b2086a0a94137f998d303c69a85d83 100644
> --- a/src/providers/ipa/ipa_id.c
> +++ b/src/providers/ipa/ipa_id.c
> @@ -30,6 +30,25 @@
>  #include "providers/ldap/sdap_async.h"
>  #include "providers/ipa/ipa_id.h"
>
> +static bool is_object_overridable(struct be_acct_req *ar)
> +{
> bool ret = false;
> switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> case BE_REQ_USER:
>    ret = true;
>    break;
> case BE_REQ_GROUP:
>    ret = true;
>    break;

I know what you tried to do here, but please keep in mind that other types
of request can also yield user. At least BE_REQ_INITGROUPS, BE_REQ_BY_SECID,
BE_REQ_USER_AND_GROUP, BE_REQ_BY_UUID, BE_REQ_BY_CERT.

We can also collapse the case: statements so that the switch is more
compact.
> default:
>    break;
> }
> return ret;
>
> +}
>
> static const char *ipa_account_info_error_text(int ret, int *dp_error,
>                                                 const char *default_text)
>  {
> @@ -638,7 +657,8 @@ ipa_id_get_account_info_send(TALLOC_CTX *memctx, struct tevent_context *ev,
>              || state->ar->filter_type == BE_FILTER_SECID
>              || state->ar->extra_value == NULL
>              || strcmp(state->ar->extra_value,
>                  EXTRA_INPUT_MAYBE_WITH_VIEW) != 0 ) {
>                  EXTRA_INPUT_MAYBE_WITH_VIEW) != 0
>        || ! is_object_overridable(state->ar)) {
>    ret = ipa_id_get_account_info_get_original_step(req, ar);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_OP_FAILURE,
>
> @@ -820,6 +840,12 @@ static void ipa_id_get_account_info_orig_done(struct tevent_req *subreq)
>          goto fail;
>      }
> if (! is_object_overridable(state->ar)) {
>    state->dp_error = DP_ERR_OK;
>    tevent_req_done(req);
>    return;
> }
> ret = get_object_from_cache(state, state->domain, state->ar,
>                                &state->obj_msg);
>    if (ret == ENOENT) {
>
> diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c
> index 472985d4ab4f785aa9c4af94bf8021829ca1c3c8..7966a763ee860bbf426f3e308a3f1457a8c8d5fa 100644
> --- a/src/providers/ipa/ipa_subdomains_id.c
> +++ b/src/providers/ipa/ipa_subdomains_id.c
> @@ -915,6 +915,13 @@ errno_t get_object_from_cache(TALLOC_CTX *mem_ctx,
>                              NULL };
>      char *name;
> if ((ar->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_SERVICES)  {
>    DEBUG(SSSDBG_OP_FAILURE,
>          "Services for IPA subdomains aren't handled by SSSD.\n");
>    ret = ENOENT;
>    goto done;
> }

With adding is_object_overridable(), I don't think this branch is
reachable.
>  if (ar->filter_type == BE_FILTER_SECID) {
>      ret = sysdb_search_object_by_sid(mem_ctx, dom, ar->filter_value, attrs,
>                                       &res);
>
> -- 
> 2.4.3
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [be_get_account_info] (0x0200): Got request for [0x1005][FAST BE_REQ_SERVICES][1][name=dns]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [be_req_set_domain] (0x0400): Changing request domain from [persei.dev] to [persei.dev]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [services_get_send] (0x1000): Preparing to search for services with filter [(&(cn=dns)(objectclass=ipService))]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_id_op_connect_step] (0x4000): reusing cached connection
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_get_services_next_base] (0x0400): Searching for services with base [cn=accounts,dc=persei,dc=dev]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_print_server] (0x2000): Searching 10.34.58.89
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x0400): calling ldap_search_ext with [(&(cn=dns)(objectclass=ipService))][cn=accounts,dc=persei,dc=dev].
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [objectClass]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [cn]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServicePort]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServiceProtocol]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [entryUSN]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x2000): ldap_search_ext called, msgid = 14
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_op_add] (0x2000): New operation 14 timeout 6
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_process_result] (0x2000): Trace: sh[0x13f08d0], connected[1], ops[0x1429ae0], ldap[0x13ed340]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_RESULT]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_get_generic_op_finished] (0x0400): Search result: Success(0), no errmsg set
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_op_destructor] (0x2000): Operation 14 finished
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_get_services_process] (0x0400): Search for services, returned 0 results.
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_id_op_done] (0x4000): releasing operation connection
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): start ldb transaction (nesting: 0)
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sysdb_search_services] (0x2000): Search services with filter: (&(objectclass=service)(&(serviceProtocol=*)(|(nameAlias=dns)(name=dns))))
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): Added timed event "ltdb_callback": 0x1413950
>
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): Added timed event "ltdb_timeout": 0x13fb3f0
>
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): Running timer event 0x1413950 "ltdb_callback"
>
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): Destroying timer event 0x13fb3f0 "ltdb_timeout"
>
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): Ending timer event 0x1413950 "ltdb_callback"
>
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sysdb_search_services] (0x2000): No such entry
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): cancel ldb transaction (nesting: 0)
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [get_object_from_cache] (0x0020): Unexpected entry type [5].
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [ipa_id_get_account_info_orig_done] (0x0040): get_object_from_cache failed.
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [ipa_account_info_error_text] (0x0020): Bug: dp_error is OK on failed request
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [acctinfo_callback] (0x0100): Request processed. Returned 3,22,Account info lookup failed
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_process_result] (0x2000): Trace: sh[0x13f08d0], connected[1], ops[(nil)], ldap[0x13ed340]
> (Fri Dec  4 15:53:50 2015) [sssd[be[persei.dev]]] [sdap_process_result] (0x2000): Trace: ldap_result found nothing!
> (Fri Dec  4 15:53:55 2015) [sssd[be[persei.dev]]] [sbus_dispatch] (0x4000): dbus conn: 0x13d1980
> (Fri Dec  4 15:53:55 2015) [sssd[be[persei.dev]]] [sbus_dispatch] (0x4000): Dispatching.
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [be_get_account_info] (0x0200): Got request for [0x1005][FAST BE_REQ_SERVICES][1][name=dns]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [be_req_set_domain] (0x0400): Changing request domain from [persei.dev] to [persei.dev]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [services_get_send] (0x1000): Preparing to search for services with filter [(&(cn=dns)(objectclass=ipService))]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_id_op_connect_step] (0x4000): reusing cached connection
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_get_services_next_base] (0x0400): Searching for services with base [cn=accounts,dc=persei,dc=dev]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_print_server] (0x2000): Searching 10.34.58.89
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x0400): calling ldap_search_ext with [(&(cn=dns)(objectclass=ipService))][cn=accounts,dc=persei,dc=dev].
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [objectClass]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [cn]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServicePort]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [ipServiceProtocol]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [entryUSN]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_get_generic_ext_step] (0x2000): ldap_search_ext called, msgid = 14
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_op_add] (0x2000): New operation 14 timeout 6
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_process_result] (0x2000): Trace: sh[0xb8f8d0], connected[1], ops[0xbb7d90], ldap[0xb8c340]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_RESULT]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_get_generic_op_finished] (0x0400): Search result: Success(0), no errmsg set
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_op_destructor] (0x2000): Operation 14 finished
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_get_services_process] (0x0400): Search for services, returned 0 results.
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_id_op_done] (0x4000): releasing operation connection
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): start ldb transaction (nesting: 0)
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sysdb_search_services] (0x2000): Search services with filter: (&(objectclass=service)(&(serviceProtocol=*)(|(nameAlias=dns)(name=dns))))
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): Added timed event "ltdb_callback": 0xbc7880
>
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): Added timed event "ltdb_timeout": 0xb9e260
>
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): Running timer event 0xbc7880 "ltdb_callback"
>
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): Destroying timer event 0xb9e260 "ltdb_timeout"
>
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): Ending timer event 0xbc7880 "ltdb_callback"
>
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sysdb_search_services] (0x2000): No such entry
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [ldb] (0x4000): cancel ldb transaction (nesting: 0)
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [acctinfo_callback] (0x0100): Request processed. Returned 0,0,Success
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_process_result] (0x2000): Trace: sh[0xb8f8d0], connected[1], ops[(nil)], ldap[0xb8c340]
> (Fri Dec  4 15:52:36 2015) [sssd[be[persei.dev]]] [sdap_process_result] (0x2000): Trace: ldap_result found nothing!
> (Fri Dec  4 15:52:40 2015) [sssd[be[persei.dev]]] [sbus_dispatch] (0x4000): dbus conn: 0xb70980
> (Fri Dec  4 15:52:40 2015) [sssd[be[persei.dev]]] [sbus_dispatch] (0x4000): Dispatching.
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Petr Cech]

On 12/07/2015 03:43 PM, Jakub Hrozek wrote:
> On Fri, Dec 04, 2015 at 04:33:51PM +0100, Petr Cech wrote:
> > So, I have choose the 2) option and the patch is attached.
>
> I think this patch goes in the right direction, but still needs more
> work. See inline.

Hello Jakub,
thank you for careful review.
> > And something more:
> >   a.bak ... log before patch
> >   b.bak ... log after patch
> >
> > Regards
> >
> > Petr
> > From 12f03f5510cdfc56efd6d24bf06db0ad0e6d6368 Mon Sep 17 00:00:00 2001
> > From: Petr Cech 
> > Date: Mon, 9 Nov 2015 09:51:05 -0500
> > Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
> >
> > Services for IPA subdomains aren't handled by SSSD. This patch
> > adds quick shortcut to avoid sending an LDAP query to cache.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2747

...
> > +static bool is_object_overridable(struct be_acct_req *ar)
> > +{
> > bool ret = false;
> > switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> > case BE_REQ_USER:
> >    ret = true;
> >    break;
> > case BE_REQ_GROUP:
> >    ret = true;
> >    break;
>
> I know what you tried to do here, but please keep in mind that other types
> of request can also yield user. At least BE_REQ_INITGROUPS, BE_REQ_BY_SECID,
> BE_REQ_USER_AND_GROUP, BE_REQ_BY_UUID, BE_REQ_BY_CERT.

I did cases for those REQs, but I don't know if it is all.
> We can also collapse the case: statements so that the switch is more
> compact.

Addressed.

...
> > diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c
> > index 472985d4ab4f785aa9c4af94bf8021829ca1c3c8..7966a763ee860bbf426f3e308a3f1457a8c8d5fa 100644
> > --- a/src/providers/ipa/ipa_subdomains_id.c
> > +++ b/src/providers/ipa/ipa_subdomains_id.c
> > @@ -915,6 +915,13 @@ errno_t get_object_from_cache(TALLOC_CTX *mem_ctx,
> >                               NULL };
> >       char *name;
> > if ((ar->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_SERVICES)  {
> >    DEBUG(SSSDBG_OP_FAILURE,
> >          "Services for IPA subdomains aren't handled by SSSD.\n");
> >    ret = ENOENT;
> >    goto done;
> > }
>
> With adding is_object_overridable(), I don't think this branch is
> reachable.

I agree. I have been surprised that this message wasn't been in logs.
> >   if (ar->filter_type == BE_FILTER_SECID) {
> >       ret = sysdb_search_object_by_sid(mem_ctx, dom, ar->filter_value, attrs,
> >                                        &res);

Regards

Petr

[Jakub Hrozek]

On Tue, Dec 08, 2015 at 04:57:02PM +0100, Petr Cech wrote:
> On 12/07/2015 03:43 PM, Jakub Hrozek wrote:
> > On Fri, Dec 04, 2015 at 04:33:51PM +0100, Petr Cech wrote:
> > > So, I have choose the 2) option and the patch is attached.
> >
> > I think this patch goes in the right direction, but still needs more
> > work. See inline.
>
> Hello Jakub,
> thank you for careful review.
> > > And something more:
> > >  a.bak ... log before patch
> > >  b.bak ... log after patch
> > >
> > > Regards
> > >
> > > Petr
> > > From 12f03f5510cdfc56efd6d24bf06db0ad0e6d6368 Mon Sep 17 00:00:00 2001
> > > From: Petr Cech 
> > > Date: Mon, 9 Nov 2015 09:51:05 -0500
> > > Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
> > >
> > > Services for IPA subdomains aren't handled by SSSD. This patch
> > > adds quick shortcut to avoid sending an LDAP query to cache.
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2747
>
> ...
> > > +static bool is_object_overridable(struct be_acct_req *ar)
> > > +{
> > > bool ret = false;
> > > switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> > > case BE_REQ_USER:
> > >    ret = true;
> > >    break;
> > > case BE_REQ_GROUP:
> > >    ret = true;
> > >    break;
> >
> > I know what you tried to do here, but please keep in mind that other types
> > of request can also yield user. At least BE_REQ_INITGROUPS, BE_REQ_BY_SECID,
> > BE_REQ_USER_AND_GROUP, BE_REQ_BY_UUID, BE_REQ_BY_CERT.
>
> I did cases for those REQs, but I don't know if it is all.
> > We can also collapse the case: statements so that the switch is more
> > compact.
>
> Addressed.
>
> ...
> > > diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c
> > > index 472985d4ab4f785aa9c4af94bf8021829ca1c3c8..7966a763ee860bbf426f3e308a3f1457a8c8d5fa 100644
> > > --- a/src/providers/ipa/ipa_subdomains_id.c
> > > +++ b/src/providers/ipa/ipa_subdomains_id.c
> > > @@ -915,6 +915,13 @@ errno_t get_object_from_cache(TALLOC_CTX *mem_ctx,
> > >                              NULL };
> > >      char *name;
> > > if ((ar->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_SERVICES)  {
> > >    DEBUG(SSSDBG_OP_FAILURE,
> > >          "Services for IPA subdomains aren't handled by SSSD.\n");
> > >    ret = ENOENT;
> > >    goto done;
> > > }
> >
> > With adding is_object_overridable(), I don't think this branch is
> > reachable.
>
> I agree. I have been surprised that this message wasn't been in logs.

One style nitpick and then we can push :)
> > >  if (ar->filter_type == BE_FILTER_SECID) {
> > >      ret = sysdb_search_object_by_sid(mem_ctx, dom, ar->filter_value, attrs,
> > >                                       &res);
>
> Regards
>
> Petr
> From e024fd15dc9c6735b291f5566ba435da7fc496bd Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Mon, 9 Nov 2015 09:51:05 -0500
> Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
>
> Function get_object_from_cache() does not handle services.
> This patch adds quick shortcut to avoid sending an LDAP query
> to cache.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2747
>
> src/providers/ipa/ipa_id.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
> index e81ccb34dd6eb44618538593f5473fbe5e89d896..dd32d7cf5fb2e6b4793b0009924949801517a809 100644
> --- a/src/providers/ipa/ipa_id.c
> +++ b/src/providers/ipa/ipa_id.c
> @@ -30,6 +30,31 @@
>  #include "providers/ldap/sdap_async.h"
>  #include "providers/ipa/ipa_id.h"
>
> +static bool is_object_overridable(struct be_acct_req *ar)
> +{
> bool ret = false;
> switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> case BE_REQ_USER: ret = true;
>    break;
> case BE_REQ_GROUP: ret = true;
>    break;
> case BE_REQ_INITGROUPS: ret = true;
>    break;
> case BE_REQ_BY_SECID: ret = true;
>    break;
> case BE_REQ_USER_AND_GROUP: ret = true;
>    break;
> case BE_REQ_BY_UUID: ret = true;
>    break;
> case BE_REQ_BY_CERT: ret = true;
>    break;
> default: break;
> }

We can make the code more compact with:
    case BE_REQ_USER:
    case BE_REQ_GROUP:
        /* And also all the others.. */
        ret = true;
        break;
> return ret;
>
> +}
>
> static const char *ipa_account_info_error_text(int ret, int *dp_error,
>                                                 const char *default_text)
>  {

[Lukas Slebodnik]

On (10/12/15 11:26), Jakub Hrozek wrote:
> On Tue, Dec 08, 2015 at 04:57:02PM +0100, Petr Cech wrote:
> > On 12/07/2015 03:43 PM, Jakub Hrozek wrote:
> > > On Fri, Dec 04, 2015 at 04:33:51PM +0100, Petr Cech wrote:
> > > > So, I have choose the 2) option and the patch is attached.
> > >
> > > I think this patch goes in the right direction, but still needs more
> > > work. See inline.
> >
> > Hello Jakub,
> > thank you for careful review.
> > > > And something more:
> > > >  a.bak ... log before patch
> > > >  b.bak ... log after patch
> > > >
> > > > Regards
> > > >
> > > > Petr
> > > > From 12f03f5510cdfc56efd6d24bf06db0ad0e6d6368 Mon Sep 17 00:00:00 2001
> > > > From: Petr Cech 
> > > > Date: Mon, 9 Nov 2015 09:51:05 -0500
> > > > Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
> > > >
> > > > Services for IPA subdomains aren't handled by SSSD. This patch
> > > > adds quick shortcut to avoid sending an LDAP query to cache.
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/2747
> >
> > ...
> > > > +static bool is_object_overridable(struct be_acct_req *ar)
> > > > +{
> > > > bool ret = false;
> > > > switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> > > > case BE_REQ_USER:
> > > >    ret = true;
> > > >    break;
> > > > case BE_REQ_GROUP:
> > > >    ret = true;
> > > >    break;
> > >
> > > I know what you tried to do here, but please keep in mind that other types
> > > of request can also yield user. At least BE_REQ_INITGROUPS, BE_REQ_BY_SECID,
> > > BE_REQ_USER_AND_GROUP, BE_REQ_BY_UUID, BE_REQ_BY_CERT.
> >
> > I did cases for those REQs, but I don't know if it is all.
> > > We can also collapse the case: statements so that the switch is more
> > > compact.
> >
> > Addressed.
> >
> > ...
> > > > diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c
> > > > index 472985d4ab4f785aa9c4af94bf8021829ca1c3c8..7966a763ee860bbf426f3e308a3f1457a8c8d5fa 100644
> > > > --- a/src/providers/ipa/ipa_subdomains_id.c
> > > > +++ b/src/providers/ipa/ipa_subdomains_id.c
> > > > @@ -915,6 +915,13 @@ errno_t get_object_from_cache(TALLOC_CTX *mem_ctx,
> > > >                              NULL };
> > > >      char *name;
> > > > if ((ar->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_SERVICES)  {
> > > >    DEBUG(SSSDBG_OP_FAILURE,
> > > >          "Services for IPA subdomains aren't handled by SSSD.\n");
> > > >    ret = ENOENT;
> > > >    goto done;
> > > > }
> > >
> > > With adding is_object_overridable(), I don't think this branch is
> > > reachable.
> >
> > I agree. I have been surprised that this message wasn't been in logs.
>
> One style nitpick and then we can push :)
> > > >  if (ar->filter_type == BE_FILTER_SECID) {
> > > >      ret = sysdb_search_object_by_sid(mem_ctx, dom, ar->filter_value, attrs,
> > > >                                       &res);
> >
> > Regards
> >
> > Petr
> > From e024fd15dc9c6735b291f5566ba435da7fc496bd Mon Sep 17 00:00:00 2001
> > From: Petr Cech 
> > Date: Mon, 9 Nov 2015 09:51:05 -0500
> > Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
> >
> > Function get_object_from_cache() does not handle services.
> > This patch adds quick shortcut to avoid sending an LDAP query
> > to cache.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2747
> >
> > src/providers/ipa/ipa_id.c | 34 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
> > index e81ccb34dd6eb44618538593f5473fbe5e89d896..dd32d7cf5fb2e6b4793b0009924949801517a809 100644
> > --- a/src/providers/ipa/ipa_id.c
> > +++ b/src/providers/ipa/ipa_id.c
> > @@ -30,6 +30,31 @@
> >  #include "providers/ldap/sdap_async.h"
> >  #include "providers/ipa/ipa_id.h"
> >
> > +static bool is_object_overridable(struct be_acct_req *ar)
> > +{
> > bool ret = false;
> > switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> > case BE_REQ_USER: ret = true;
> >    break;
> > case BE_REQ_GROUP: ret = true;
> >    break;
> > case BE_REQ_INITGROUPS: ret = true;
> >    break;
> > case BE_REQ_BY_SECID: ret = true;
> >    break;
> > case BE_REQ_USER_AND_GROUP: ret = true;
> >    break;
> > case BE_REQ_BY_UUID: ret = true;
> >    break;
> > case BE_REQ_BY_CERT: ret = true;
> >    break;
> > default: break;
> > }
>
> We can make the code more compact with:
>    case BE_REQ_USER:
>    case BE_REQ_GROUP:
>        /* And also all the others.. */
>        ret = true;

we can also use
         return true;
>    break;
>
> > return ret;
> >
> > +}
> >
> > static const char *ipa_account_info_error_text(int ret, int *dp_error,
> >                                                 const char *default_text)
> >  {
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Petr Cech]

On 12/10/2015 11:33 AM, Lukas Slebodnik wrote:
> On (10/12/15 11:26), Jakub Hrozek wrote:
> > On Tue, Dec 08, 2015 at 04:57:02PM +0100, Petr Cech wrote:
> > > On 12/07/2015 03:43 PM, Jakub Hrozek wrote:
> > > > On Fri, Dec 04, 2015 at 04:33:51PM +0100, Petr Cech wrote:
> > One style nitpick and then we can push :)
> > > +static bool is_object_overridable(struct be_acct_req *ar)
> > > +{
> > > bool ret = false;
> > > switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> > > case BE_REQ_USER: ret = true;
> > >    break;
> > > case BE_REQ_GROUP: ret = true;
> > >    break;
> > > case BE_REQ_INITGROUPS: ret = true;
> > >    break;
> > > case BE_REQ_BY_SECID: ret = true;
> > >    break;
> > > case BE_REQ_USER_AND_GROUP: ret = true;
> > >    break;
> > > case BE_REQ_BY_UUID: ret = true;
> > >    break;
> > > case BE_REQ_BY_CERT: ret = true;
> > >    break;
> > > default: break;
> > > }
> >
> > We can make the code more compact with:
> >     case BE_REQ_USER:
> >     case BE_REQ_GROUP:
> >         /* And also all the others.. */
> >         ret = true;
>       we can also use
>       return true;
>
> >     break;
> >
> > > return ret;
> > >
> > > +}

Thank you for review.
I prefer one return point per function.

Petr

PS: New patch is attached.

[Jakub Hrozek]

On Thu, Dec 10, 2015 at 01:52:48PM +0100, Petr Cech wrote:
> Thank you for review.
> I prefer one return point per function.
>
> Petr
>
> PS: New patch is attached.

ACK

[Lukas Slebodnik]

On (11/12/15 12:41), Jakub Hrozek wrote:
> On Thu, Dec 10, 2015 at 01:52:48PM +0100, Petr Cech wrote:
> > Thank you for review.
> > I prefer one return point per function.
> >
> > Petr
> >
> > PS: New patch is attached.
>
> ACK

master:
* 565e6d91814884054ec0dc4d770804d7bf472d3f

sssd-1-13:
* 5869cb41be8b577105773d2defaba6b51d271575

LS