Re: [SSSD] [PATCH] Extend the LDAP attribute map with custom options

On Mon, 2014-04-21 at 23:15 +0200, Jakub Hrozek wrote:

Hi,

the attached patches implement ticket #2073 -- the possibility to extend the LDAP attribute map with custom attributes.

All attributes are saved to sysdb with the 'extra_' prefix so that the custom attributes can be distinguished from the sysdb schema.

The first patch fixes a mostly unrelated bug, which I found when writing unit tests.

[PATCH 1/3] LDAP: Fix one-by-one bug in sdap_copy_opts The sdap_copy_opts function copied all the arguments except for the sentinel.

[PATCH 2/3] LDAP: Make it possible to extend an attribute map https://fedorahosted.org/sssd/ticket/2073

This commit adds a new option ldap_user_extra_attrs that is unset by default. When set, the option contains a list of LDAP attributes the LDAP provider would download and store in addition to the usual set.

The attributes are fetched in sysdb with the 'extra_' prefix to not conflict with the sssd sdap maps.

[PATCH 3/3] Make LDAP extra attributes available to IPA and AD https://fedorahosted.org/sssd/ticket/2073

Looks good from 10000 feet, however I saw at least one issue, inline

From cc4b347c955f93d640772fad197a55a1b9034525 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 21 Apr 2014 21:33:36 +0200 Subject: [PATCH 1/3] LDAP: Fix one-by-one bug in sdap_copy_opts

[..]

• /* Include the sentinel */
• map[num_entries].def_name = NULL;
• map[num_entries].sys_name = NULL;
• map[num_entries].opt_name = NULL;
• map[num_entries].name = NULL;

Here you explicitly mark all 4 components as NULL.

[..]

Then in the next patch ...

From 40329ebff77b0d05c8051d3d4a2af2f6545c8d86 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 31 Jul 2013 10:59:43 +0200 Subject: [PATCH 2/3] LDAP: Make it possible to extend an attribute map

[..]

• for (nextra = 0; extra_attrs[nextra]; nextra++) ;
• DEBUG(SSSDBG_FUNC_DATA, "%zu extra attributes\n", nextra);

(aside: is this really worth here, you can print it after the next loop as nextra = i; at less cost)

• map = talloc_realloc(memctx, src_map, struct sdap_attr_map,

•                     num_entries + nextra + 1);
  

• if (map == NULL) {

•    return ENOMEM;
  

• }
• for (i = 0; extra_attrs[i]; i++) {

•    map[num_entries+i].opt_name = talloc_strdup(map,
  

extra_attrs[i]);

•    map[num_entries+i].sys_name = talloc_asprintf(map,
  

SYSDB_EXTRA_ATTR_TMPL,

•                                            map[num_entries
  

+i].opt_name);

•    map[num_entries+i].name = talloc_strdup(map,
  

•                                            map[num_entries
  

+i].opt_name);

•    map[num_entries+i].def_name = talloc_strdup(map,
  

•                                            map[num_entries
  

+i].opt_name);

•    if (map[num_entries+i].opt_name == NULL ||
  

•        map[num_entries+i].sys_name == NULL ||
  

•        map[num_entries+i].name == NULL ||
  

•        map[num_entries+i].def_name == NULL) {
  

•        return ENOMEM;
  

•    }
  

•    DEBUG(SSSDBG_TRACE_FUNC, "Extending map with %s\n",
  

extra_attrs[i]);

• }
• /* Sentinel */
• map[num_entries+nextra].opt_name = NULL;
• map[num_entries+nextra].sys_name = NULL;
• map[num_entries+nextra].name = NULL;

HERE ---^^^^ only 3 elements are NULLed, def_name is missing.

I see 2 solutions: 1) only null .name 2) memset(&map[num_entries+nextra], 0, sizeof(struct sdap_attr_map));

They are both fail_safe in case struct sdap_attr_map is changed.

Simo.