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
On 04/21/2014 11:15 PM, 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
Ack to all.
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.
On Tue, Apr 22, 2014 at 08:38:46AM -0400, Simo Sorce wrote:
HERE ---^^^^ only 3 elements are NULLed, def_name is missing.
Ah, sorry, I NULL-ed all elements just before sending the patch on the other place, but missed this one.
I see 2 solutions:
- only null .name
- memset(&map[num_entries+nextra], 0, sizeof(struct sdap_attr_map));
They are both fail_safe in case struct sdap_attr_map is changed.
I prefer memset (or some shorthand like Samba's ZERO_STRUCT).
I have another question, though after discussion with Jan (CC-ed).
The way I save the attributes (extra_$name) in this patchset is wrong with respect to how we allow the dbus responder to query the attributes, because the user would have to explicitly allow extra_$name in the attribute ACL which is confusing and would have to be carefully documented.
Also, this approach wouldn't work well with respect to multiple domains with different schemas.
Jan's proposal, which I like, was to change the ldap_user_extra attribute syntax from: ldap_user_extra_attrs = ldap_attr_name1, ldap_attr_name2
to: ldap_user_extra_attrs = ldap_attr_name1:sysdb_attr_name1, ldap_attr_name2:sysdb_attr_name2
The sysdb_attr_name would not be mandatory, if the sysdb name was omitted, then the back end would save the attribute verbatim.
If there was a conflict between the name the user chose (or the original LDAP attribute name), the SSSD would throw an error.
On Tue, 2014-04-22 at 16:18 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 08:38:46AM -0400, Simo Sorce wrote:
HERE ---^^^^ only 3 elements are NULLed, def_name is missing.
Ah, sorry, I NULL-ed all elements just before sending the patch on the other place, but missed this one.
I see 2 solutions:
- only null .name
- memset(&map[num_entries+nextra], 0, sizeof(struct sdap_attr_map));
They are both fail_safe in case struct sdap_attr_map is changed.
I prefer memset (or some shorthand like Samba's ZERO_STRUCT).
I have another question, though after discussion with Jan (CC-ed).
The way I save the attributes (extra_$name) in this patchset is wrong with respect to how we allow the dbus responder to query the attributes, because the user would have to explicitly allow extra_$name in the attribute ACL which is confusing and would have to be carefully documented.
Also, this approach wouldn't work well with respect to multiple domains with different schemas.
Jan's proposal, which I like, was to change the ldap_user_extra attribute syntax from: ldap_user_extra_attrs = ldap_attr_name1, ldap_attr_name2
to: ldap_user_extra_attrs = ldap_attr_name1:sysdb_attr_name1, ldap_attr_name2:sysdb_attr_name2
The sysdb_attr_name would not be mandatory, if the sysdb name was omitted, then the back end would save the attribute verbatim.
If there was a conflict between the name the user chose (or the original LDAP attribute name), the SSSD would throw an error.
I like this a lot, please do it.
Although I wonder, should the order be the reverse ? I think of it as assignments so mentally I would visualize them as: ldap_user_extra_attrs = internal_name_1:ldap_name_1, internal_name_2:ldap_name_2
Simo.
On Tue, Apr 22, 2014 at 02:21:58PM +0000, Simo Sorce wrote:
On Tue, 2014-04-22 at 16:18 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 08:38:46AM -0400, Simo Sorce wrote:
HERE ---^^^^ only 3 elements are NULLed, def_name is missing.
Ah, sorry, I NULL-ed all elements just before sending the patch on the other place, but missed this one.
I see 2 solutions:
- only null .name
- memset(&map[num_entries+nextra], 0, sizeof(struct sdap_attr_map));
They are both fail_safe in case struct sdap_attr_map is changed.
I prefer memset (or some shorthand like Samba's ZERO_STRUCT).
I have another question, though after discussion with Jan (CC-ed).
The way I save the attributes (extra_$name) in this patchset is wrong with respect to how we allow the dbus responder to query the attributes, because the user would have to explicitly allow extra_$name in the attribute ACL which is confusing and would have to be carefully documented.
Also, this approach wouldn't work well with respect to multiple domains with different schemas.
Jan's proposal, which I like, was to change the ldap_user_extra attribute syntax from: ldap_user_extra_attrs = ldap_attr_name1, ldap_attr_name2
to: ldap_user_extra_attrs = ldap_attr_name1:sysdb_attr_name1, ldap_attr_name2:sysdb_attr_name2
The sysdb_attr_name would not be mandatory, if the sysdb name was omitted, then the back end would save the attribute verbatim.
If there was a conflict between the name the user chose (or the original LDAP attribute name), the SSSD would throw an error.
I like this a lot, please do it.
Although I wonder, should the order be the reverse ? I think of it as assignments so mentally I would visualize them as: ldap_user_extra_attrs = internal_name_1:ldap_name_1, internal_name_2:ldap_name_2
I'm not sure, in my mind ldap this config option is a bit like python dictionary, where attribute name is the 'key' and sysdb attribute name is the 'value'...and dictionaries are typically defined as key:value.
But reversing the order would be a trivial change, so I'll just get started on the patch and we can bikeshed about details later :-)
On Tue, Apr 22, 2014 at 02:21:58PM +0000, Simo Sorce wrote:
Also, this approach wouldn't work well with respect to multiple domains with different schemas.
Jan's proposal, which I like, was to change the ldap_user_extra attribute syntax from: ldap_user_extra_attrs = ldap_attr_name1, ldap_attr_name2
to: ldap_user_extra_attrs = ldap_attr_name1:sysdb_attr_name1, ldap_attr_name2:sysdb_attr_name2
The sysdb_attr_name would not be mandatory, if the sysdb name was omitted, then the back end would save the attribute verbatim.
If there was a conflict between the name the user chose (or the original LDAP attribute name), the SSSD would throw an error.
I like this a lot, please do it.
Although I wonder, should the order be the reverse ? I think of it as assignments so mentally I would visualize them as: ldap_user_extra_attrs = internal_name_1:ldap_name_1, internal_name_2:ldap_name_2
How about
ldap_user_extra_attrs = internal_name_1=ldap_name_1, internal_name_2=ldap_name_2
then?
On Tue, Apr 22, 2014 at 05:12:58PM +0200, Jan Pazdziora wrote:
On Tue, Apr 22, 2014 at 02:21:58PM +0000, Simo Sorce wrote:
Also, this approach wouldn't work well with respect to multiple domains with different schemas.
Jan's proposal, which I like, was to change the ldap_user_extra attribute syntax from: ldap_user_extra_attrs = ldap_attr_name1, ldap_attr_name2
to: ldap_user_extra_attrs = ldap_attr_name1:sysdb_attr_name1, ldap_attr_name2:sysdb_attr_name2
The sysdb_attr_name would not be mandatory, if the sysdb name was omitted, then the back end would save the attribute verbatim.
If there was a conflict between the name the user chose (or the original LDAP attribute name), the SSSD would throw an error.
I like this a lot, please do it.
Although I wonder, should the order be the reverse ? I think of it as assignments so mentally I would visualize them as: ldap_user_extra_attrs = internal_name_1:ldap_name_1, internal_name_2:ldap_name_2
How about
ldap_user_extra_attrs = internal_name_1=ldap_name_1, internal_name_2=ldap_name_2
then?
I need to check if this would fly well with libini which uses '=' as the key/value separator.
On Tue, 2014-04-22 at 16:31 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 02:21:58PM +0000, Simo Sorce wrote:
On Tue, 2014-04-22 at 16:18 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 08:38:46AM -0400, Simo Sorce wrote:
HERE ---^^^^ only 3 elements are NULLed, def_name is missing.
Ah, sorry, I NULL-ed all elements just before sending the patch on the other place, but missed this one.
I see 2 solutions:
- only null .name
- memset(&map[num_entries+nextra], 0, sizeof(struct sdap_attr_map));
They are both fail_safe in case struct sdap_attr_map is changed.
I prefer memset (or some shorthand like Samba's ZERO_STRUCT).
I have another question, though after discussion with Jan (CC-ed).
The way I save the attributes (extra_$name) in this patchset is wrong with respect to how we allow the dbus responder to query the attributes, because the user would have to explicitly allow extra_$name in the attribute ACL which is confusing and would have to be carefully documented.
Also, this approach wouldn't work well with respect to multiple domains with different schemas.
Jan's proposal, which I like, was to change the ldap_user_extra attribute syntax from: ldap_user_extra_attrs = ldap_attr_name1, ldap_attr_name2
to: ldap_user_extra_attrs = ldap_attr_name1:sysdb_attr_name1, ldap_attr_name2:sysdb_attr_name2
The sysdb_attr_name would not be mandatory, if the sysdb name was omitted, then the back end would save the attribute verbatim.
If there was a conflict between the name the user chose (or the original LDAP attribute name), the SSSD would throw an error.
I like this a lot, please do it.
Although I wonder, should the order be the reverse ? I think of it as assignments so mentally I would visualize them as: ldap_user_extra_attrs = internal_name_1:ldap_name_1, internal_name_2:ldap_name_2
I'm not sure, in my mind ldap this config option is a bit like python dictionary, where attribute name is the 'key' and sysdb attribute name is the 'value'...and dictionaries are typically defined as key:value.
Well, exactly, in my mind the internal value is the key, the ldap name is whatever the local ldap server uses ...
But reversing the order would be a trivial change, so I'll just get started on the patch and we can bikeshed about details later :-)
Ok.
Simo.
On Tue, 2014-04-22 at 17:16 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 05:12:58PM +0200, Jan Pazdziora wrote:
On Tue, Apr 22, 2014 at 02:21:58PM +0000, Simo Sorce wrote:
Also, this approach wouldn't work well with respect to multiple domains with different schemas.
Jan's proposal, which I like, was to change the ldap_user_extra attribute syntax from: ldap_user_extra_attrs = ldap_attr_name1, ldap_attr_name2
to: ldap_user_extra_attrs = ldap_attr_name1:sysdb_attr_name1, ldap_attr_name2:sysdb_attr_name2
The sysdb_attr_name would not be mandatory, if the sysdb name was omitted, then the back end would save the attribute verbatim.
If there was a conflict between the name the user chose (or the original LDAP attribute name), the SSSD would throw an error.
I like this a lot, please do it.
Although I wonder, should the order be the reverse ? I think of it as assignments so mentally I would visualize them as: ldap_user_extra_attrs = internal_name_1:ldap_name_1, internal_name_2:ldap_name_2
How about
ldap_user_extra_attrs = internal_name_1=ldap_name_1, internal_name_2=ldap_name_2
then?
I need to check if this would fly well with libini which uses '=' as the key/value separator.
I prefer ':' as separator.
Simo.
On Tue, Apr 22, 2014 at 08:38:46AM -0400, Simo Sorce wrote:
- 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)
Sorry, I'm not sure what you mean here, do you just want to move the DEBUG message? The nextra variable is needed to realloc the map to be larger.
On Tue, Apr 22, 2014 at 11:28:16AM -0400, Simo Sorce wrote:
On Tue, 2014-04-22 at 17:16 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 05:12:58PM +0200, Jan Pazdziora wrote:
On Tue, Apr 22, 2014 at 02:21:58PM +0000, Simo Sorce wrote:
Also, this approach wouldn't work well with respect to multiple domains with different schemas.
Jan's proposal, which I like, was to change the ldap_user_extra attribute syntax from: ldap_user_extra_attrs = ldap_attr_name1, ldap_attr_name2
to: ldap_user_extra_attrs = ldap_attr_name1:sysdb_attr_name1, ldap_attr_name2:sysdb_attr_name2
The sysdb_attr_name would not be mandatory, if the sysdb name was omitted, then the back end would save the attribute verbatim.
If there was a conflict between the name the user chose (or the original LDAP attribute name), the SSSD would throw an error.
I like this a lot, please do it.
Although I wonder, should the order be the reverse ? I think of it as assignments so mentally I would visualize them as: ldap_user_extra_attrs = internal_name_1:ldap_name_1, internal_name_2:ldap_name_2
How about
ldap_user_extra_attrs = internal_name_1=ldap_name_1, internal_name_2=ldap_name_2
then?
I need to check if this would fly well with libini which uses '=' as the key/value separator.
I prefer ':' as separator.
Simo.
OK, new patches that use ':' as a separator are attached. This version uses key:value, but that can be swapped.
On Wed, 2014-04-23 at 17:12 +0200, Jakub Hrozek wrote:
OK, new patches that use ':' as a separator are attached. This version uses key:value, but that can be swapped.
Why do you consider the 'key' to be the remote attribute name ? I can't make sense of it :-)
The "key" sssd looks for in it's cache is the sysdb name, no ?
Simo.
On Wed, 2014-04-23 at 16:53 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 08:38:46AM -0400, Simo Sorce wrote:
- 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)
Sorry, I'm not sure what you mean here, do you just want to move the DEBUG message? The nextra variable is needed to realloc the map to be larger.
I am blind :-)
Simo.
On 04/22/2014 11:16 AM, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 05:12:58PM +0200, Jan Pazdziora wrote:
On Tue, Apr 22, 2014 at 02:21:58PM +0000, Simo Sorce wrote:
Also, this approach wouldn't work well with respect to multiple domains with different schemas.
Jan's proposal, which I like, was to change the ldap_user_extra attribute syntax from: ldap_user_extra_attrs = ldap_attr_name1, ldap_attr_name2
to: ldap_user_extra_attrs = ldap_attr_name1:sysdb_attr_name1, ldap_attr_name2:sysdb_attr_name2
The sysdb_attr_name would not be mandatory, if the sysdb name was omitted, then the back end would save the attribute verbatim.
If there was a conflict between the name the user chose (or the original LDAP attribute name), the SSSD would throw an error.
I like this a lot, please do it.
Although I wonder, should the order be the reverse ? I think of it as assignments so mentally I would visualize them as: ldap_user_extra_attrs = internal_name_1:ldap_name_1, internal_name_2:ldap_name_2
How about
ldap_user_extra_attrs = internal_name_1=ldap_name_1, internal_name_2=ldap_name_2
then?
I need to check if this would fly well with libini which uses '=' as the key/value separator. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
INI looks only for the first occurrence of the "=" so the syntax would be fine but I prefer ":" anyways.
On Wed, Apr 23, 2014 at 11:28:51AM -0400, Simo Sorce wrote:
On Wed, 2014-04-23 at 17:12 +0200, Jakub Hrozek wrote:
OK, new patches that use ':' as a separator are attached. This version uses key:value, but that can be swapped.
Why do you consider the 'key' to be the remote attribute name ? I can't make sense of it :-)
The "key" sssd looks for in it's cache is the sysdb name, no ?
Simo.
I don't really mind too much and appears Jan does not either. So I simply swapped the order in the attached patches, now it's sysdb:LDAP.
Man page was changed accordingly.
Jan, could you check if the man page change (src/man/sssd-ldap.5.xml in the second patch) reads good to you?
On (25/04/14 11:42), Jakub Hrozek wrote:
On Wed, Apr 23, 2014 at 11:28:51AM -0400, Simo Sorce wrote:
On Wed, 2014-04-23 at 17:12 +0200, Jakub Hrozek wrote:
OK, new patches that use ':' as a separator are attached. This version uses key:value, but that can be swapped.
Why do you consider the 'key' to be the remote attribute name ? I can't make sense of it :-)
The "key" sssd looks for in it's cache is the sysdb name, no ?
Simo.
I don't really mind too much and appears Jan does not either. So I simply swapped the order in the attached patches, now it's sysdb:LDAP.
Man page was changed accordingly.
Jan, could you check if the man page change (src/man/sssd-ldap.5.xml in the second patch) reads good to you?
From 92acd3f95ef5ae8ce49f398f9ef616a79887193f 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 off-by-one bug in sdap_copy_opts
The sdap_copy_opts function copied all the arguments except for the sentinel.
Makefile.am | 3 +++ src/providers/ldap/sdap.c | 5 +++- src/tests/ipa_ldap_opt-tests.c | 55 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 5dc359596416f1202c5bbf5cc2585b60eeeea6f1..56d8e1df648822a1fe8b3ca5fc8995f3c78a8566 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1215,6 +1215,9 @@ auth_tests_LDADD = \
ipa_ldap_opt_tests_SOURCES = \ src/providers/data_provider_opts.c \
- src/providers/ldap/sdap.c \
- src/providers/ldap/sdap_range.c \
- src/util/sss_ldap.c \ src/tests/ipa_ldap_opt-tests.c
ipa_ldap_opt_tests_CFLAGS = \ $(AM_CFLAGS) \
I appologize for late response. I didn't have a time to review this patch. This simple change causes probelm with linking this test on systems with disabled link_all_deplibs (debian)
Attached patch fix this issue.
LS
On Fri, May 02, 2014 at 11:23:19PM +0200, Lukas Slebodnik wrote:
I appologize for late response. I didn't have a time to review this patch. This simple change causes probelm with linking this test on systems with disabled link_all_deplibs (debian)
Attached patch fix this issue.
LS
Thanks, ACK!
I'm looking forward to a CI engine watching these repetitive tasks for us :)
On Tue, May 06, 2014 at 03:59:41PM +0200, Jakub Hrozek wrote:
On Fri, May 02, 2014 at 11:23:19PM +0200, Lukas Slebodnik wrote:
I appologize for late response. I didn't have a time to review this patch. This simple change causes probelm with linking this test on systems with disabled link_all_deplibs (debian)
Attached patch fix this issue.
LS
Thanks, ACK!
I'm looking forward to a CI engine watching these repetitive tasks for us :)
Pushed to master: d12c95d840ed5de7f34e21002943c48e711a33f4
sssd-devel@lists.fedorahosted.org