#534 RFE: Add SASL mappings fallback
Closed: Fixed None Opened 7 years ago by simo.

Currently once a SASL mapping regex matches, the server attempts to find an entry in the directory as configure within the matched rule.
If no entry is found then the mapping fails.

It would be useful if a missing mapping would cause DS to try the next matching SASL mapping.

This enhancement would allow FreeIPA to fix https://fedorahosted.org/freeipa/ticket/3291


There are actually multiple issues with SASL mappings that we should address:

  • Mappings are not dynamically updatable.
  • Allow mapping evaluation order to be defined.
  • Allow mappings to fall through in the case where a regex matches, but no bind target is found.

Followed Nathan's suggestions and added:

  • Mapping fallback
  • Prioritized mappings

    • Added new config attribute: nsSaslMapPriority: [1 - 100], where 1 is the highest priority
  • Allow online mapping modifications

Sending out for review...

Very minor comments. Overall, your patch looks good to me.

1) If priority_str is bogus, the priority would be 100. I guess that's intentional?
{{{
382 if(priority_str){
383 priority = atoi(priority_str);
384 } else {
385 priority = 100;
386 }
387 if(priority == 0 || priority > 100){
388 priority = 100;
389 }
}}}
It would be nice if you clearly state that 100 is the lowest priority value?

nsSaslMapPrioirty (Integer) - priority of mapping, where 1 is the highest priority

2) There's no chance in the current code, but to avoid the NULL reference, it'll be nice to have a check for "if (map)", as well.
{{{
677 if(map == NULL)
678
map = priv->map_data_list;
}}}

Replying to [comment:6 nhosoi]:

Very minor comments. Overall, your patch looks good to me.

1) If priority_str is bogus, the priority would be 100. I guess that's intentional?
Yes
{{{
382 if(priority_str){
383 priority = atoi(priority_str);
384 } else {
385 priority = 100;
386 }
387 if(priority == 0 || priority > 100){
388 priority = 100;
389 }
}}}
It would be nice if you clearly state that 100 is the lowest priority value?

nsSaslMapPrioirty (Integer) - priority of mapping, where 1 is the highest priority
Agreed, and changed!

2) There's no chance in the current code, but to avoid the NULL reference, it'll be nice to have a check for "if (map)", as well.
{{{
677 if(map == NULL)
678
map = priv->map_data_list;
}}}
done.

Attached new patch.

need to free priority_str in sasl_map_config_parse_entry() - otherwise, looks good

git merge ticket534
Updating ebd8c40..83f292c
Fast-forward
ldap/schema/01core389.ldif | 3 +-
ldap/servers/slapd/fe.h | 21 ++++-
ldap/servers/slapd/fedse.c | 67 +++++++-------
ldap/servers/slapd/sasl_map.c | 208 +++++++++++++++++++++++++++++------------
ldap/servers/slapd/saslbind.c | 62 +++++++-----
5 files changed, 239 insertions(+), 122 deletions(-)

[mareynol@localhost servers]$ git push origin master
Counting objects: 21, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (11/11), done.
Writing objects: 100% (11/11), 3.43 KiB, done.
Total 11 (delta 9), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
ebd8c40..83f292c master -> master

Fix memory leak:

git merge ticket534
Updating 83f292c..a0986e6
Fast-forward
ldap/servers/slapd/sasl_map.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
[mareynol@localhost servers]$ git push origin master
Counting objects: 11, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (6/6), done.
Writing objects: 100% (6/6), 615 bytes, done.
Total 6 (delta 4), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
83f292c..a0986e6 master -> master

IPA team requested to make the "fallback" configurable. Reopened ticket, and working on new patch...

Do you need to set the init_sasl_mapping_fallback value?

Added new config setting to turn the "fallback" feature on or off
0001-Ticket-534-Add-SASL-mappings-fallback.patch

Yes I do, change made, new patch attached. Thanks.

git merge ticket534
Updating 93b6cef..a15416d
Fast-forward
ldap/ldif/template-dse.ldif.in | 1 +
ldap/schema/01core389.ldif | 1 +
ldap/servers/slapd/libglobs.c | 31 ++++++++++++++++++++++++++++++-
ldap/servers/slapd/proto-slap.h | 2 ++
ldap/servers/slapd/saslbind.c | 3 ++-
ldap/servers/slapd/slap.h | 2 ++
6 files changed, 38 insertions(+), 2 deletions(-)

git push origin master
Counting objects: 25, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (13/13), done.
Writing objects: 100% (13/13), 1.84 KiB, done.
Total 13 (delta 10), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
93b6cef..a15416d master -> master

The function for removing entries from the mapping list is incorrect. This patch fixes it.
0001-Ticket-534-RFE-Add-SASL-mappings-fallback.2.patch

Reopening the ticket so that this is not forgotten.

Replying to [comment:19 mkosek]:

Reopening the ticket so that this is not forgotten.

Why is this reopened exactly? The fix was finished along time ago and committed to the source tree.

Just curious,
Mark

I thought that jcholast's fix (comment 19) for a DS crash is not in, so I reopened it so that it does not get lost. If the fix is in, you are of course free to close this ticket.

git merge ticket534
Updating a4c4daa..2e5a30d
Fast-forward
ldap/servers/slapd/sasl_map.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)

git push origin master
Counting objects: 11, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (6/6), done.
Writing objects: 100% (6/6), 760 bytes, done.
Total 6 (delta 4), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
a4c4daa..2e5a30d master -> master

Metadata Update from @mreynolds:
- Issue assigned to mreynolds
- Issue set to the milestone: 1.3.1

3 years ago

Login to comment on this ticket.

Metadata