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:
Followed Nathan's suggestions and added:
Prioritized mappings
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; }}}
revision 0001-Ticket-534-RFE-Add-SASL-mappings-fallback.patch
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.
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
fix memory leak 0001-Ticket-534-sasl-mapping-fallback.patch
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
schema oid update 0001-Ticket-534-SASL-Mapping-Fallback.patch
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=918709
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]:
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
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/534
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.