AD: Ignore Foreign Security Principals in groups

[ondrejv2]

Currently, group enumeration fails when group contains Foreign security principals.
This code silently ignores them since we can't resolve these anyway (at least ATM).

[sumit-bose]

Hi,

it is not recommended to change the API of a released library. Please move is_domain_sid() and is_principal_sid() to sss_idmap.c and add is_principal_sid() and if you like is_str_sid() to a new library version SSS_IDMAP_0.5.

bye,
Sumit

[ondrejv2]

Hi,

it is not recommended to change the API of a released library. Please move is_domain_sid() and is_principal_sid() to sss_idmap.c and add is_principal_sid() and if you like is_str_sid() to a new library version SSS_IDMAP_0.5.

bye, Sumit

Can't we just move (for backward compatibility) the is_domain_sid() function? I mean is_principal_sid() can stay as inline in sss_idmap.h right?

[sumit-bose]

Hi,
it is not recommended to change the API of a released library. Please move is_domain_sid() and is_principal_sid() to sss_idmap.c and add is_principal_sid() and if you like is_str_sid() to a new library version SSS_IDMAP_0.5.
bye, Sumit

Can't we just move (for backward compatibility) the is_domain_sid() function? I mean is_principal_sid() can stay as inline in sss_idmap.h right?

Hi,

I have to admit that do not like code in header files that much. I know there are a few examples in the SSSD code. But libsss_idmap is a library used by other projects as well and I would prefer to make it clear that a call part of the API or not. So I would like to encourage you to add is_principal_sid() to sss_idmap.c as well. If you think that is_principal_sid() does not belong into libsss_idmap I would suggest to just add it to sdap_async_nested_groups.c as a macro calling is_str_sid().

bye,
Sumit

[sumit-bose]

Hi,

thank you, this looks good now. I just got mixed up with the version numbers SSS_IDMAP_0.5 is the current one. So can you please mofify the change of sss_idmap.exports to

--- a/src/lib/idmap/sss_idmap.exports
+++ b/src/lib/idmap/sss_idmap.exports
@@ -64,3 +64,12 @@ SSS_IDMAP_0.5 {
         sss_idmap_add_auto_domain_ex;
 
 } SSS_IDMAP_0.4;
+
+SSS_IDMAP_0.6 {
+
+    # public functions
+    global:
+
+        is_principal_sid;
+
+} SSS_IDMAP_0.5;

Finally I would like to suggest to add fspdn_len to group_ctx->opts->sdom as well so that strlen(fspdn) has to be calculated only once.

bye,
Sumit

[alexey-tikhonov]

This also need a proper commit message, including a release note (see https://github.com/SSSD/sssd/blob/master/.git-commit-template#L7)

[ondrejv2]

Hello,
I just added another commit which is actually independent of the others and offers another solution of the problem.
In my opinion, it is actually better because:

• it is bullet proof (Foreign security principals are detected regardless of the location)
• it is simpler
• it offers a good starting point for following Foreign security principals in the future

one disadvantage:

• it requires extra ldap search (but we are doing it atm, anyway)

Please consider

[sumit-bose]

Hi,

please fix

Trailing whitespace found:
../src/providers/ldap/sdap_async_nested_groups.c:278:       /* we need to populate the sys_name in the user map so the user is recognized later on */ 
FAIL src/tests/whitespace_test (exit status: 1)

bye,
Sumit

[ondrejv2]

Hi,

please fix

Trailing whitespace found:
../src/providers/ldap/sdap_async_nested_groups.c:278:       /* we need to populate the sys_name in the user map so the user is recognized later on */ 
FAIL src/tests/whitespace_test (exit status: 1)

bye, Sumit

Fixed, thanks for FYI

[sumit-bose]

Hi,

why is it needed to remove the map here?

I think this approach is good as well, but you are doing too many shortcuts with this patch.

Imo it would be better to handle the FSPs in sdap_nested_group_lookup_unknown_send()/recv() by adding a dedicated sdap_nested_group_lookup_fsp_send()/recv() request to handle FSPs in the same way as plain user and group members.

bye,
Sumit

[ondrejv2]

Hi,
We can't use maps for ldapsearch with multiple object classes in the filter due to the design limitation. I wanted to use single ldap search when detecting FSPs/users. To me it's unnecessary to introduce another sdap_nested_group_lookup_fsp_send()/recv() as it would mean another pointless ldapsearch.
Since we only require member uid at this stage, there is no real need for maps and we can squeeze both searches into single one.
Ondrej

[sumit-bose]

Hi,

I see your point. But following this thought would mean to refactor sdap_nested_group_lookup_unknown_send()/recv() to only send a single ldapsearch which would allow all objectclasses (or at least user, group and fsp) and determine the type of the results based on the objectclass. What do you think?

bye,
Sumit

[ondrejv2]

Hi,
Well, yes, that would be an ideal case (as the current code is bit "cumbersome") yes.
The problem is, that in order to search for all objectclasses we need to use unmapped search (I've mentioned this in a different PR).
Using unmapped search means sdap_parse_entry() will not map entries for us so we gotta do it manually. Now I did it for the user objectclass as it's not a big deal (single attr uid), but not sure what we need for group objectclasses.

Ondrej

[sumit-bose]

Hi,

you can just request all attributes and run sdap_parse_entry() after the type is known based on the objectclass. There is an example how it can be done in sdap_asq_search_parse_entry().

bye,
Sumit

[ondrejv2]

I see, but sdap_parse_entry() needs sdap_msg parameter, where do I get it from group_ctx?

[sumit-bose]

Hi,

instead of using sdap_get_generic_send() you can use sdap_get_generic_ext_send() and add your own parser as a callback. This callback will receive sdap_msg.

HTH

bye,
Sumit

[ondrejv2]

Hi,
Sorry, that's bit too complex for me. Can you suggest a suitable code? I can't figure out myself.
Ideally I would like to parse attributes in sdap_nested_group_lookup_unknown_recv() if that's possible.
Namely I need better replacement of this snippet:

       ret = sysdb_attrs_get_string(*_entry, state->group_ctx->opts->user_map[SDAP_AT_USER_NAME].name, &val);
       if (ret != EOK) {
          DEBUG(SSSDBG_TRACE_ALL, "Unable to get username for %s\n",val);
          goto done;
       }
       /* we need to populate the sys_name in the user map so the user is recognized later on */
       sysdb_attrs_add_string(*_entry, state->group_ctx->opts->user_map[SDAP_AT_USER_NAME].sys_name, val);

Once I have the parsing done, then I could possibly do the rest myself.
Thanks.
Ondrej