[SSSD] [PATCH] Replace space: add some checks

Sumit Bose sbose at redhat.com
Tue Aug 26 15:46:33 UTC 2014


On Tue, Aug 26, 2014 at 03:56:50PM +0200, Jakub Hrozek wrote:
> On Mon, Aug 25, 2014 at 06:27:01PM +0200, Sumit Bose wrote:
> > Hi,
> > 
> > this patch adds some additional checks to the logic or replacing space
> > with a different character. The idea is to not replace the space if we
> > might run into trouble with the modified name and log an error.
> > 
> > As mentioned in the commit message if the original name already contains
> > the replacement character the original name is used and if the input for
> > the revers operation contains both space and the replacement character
> > the input is returned unmodified as well. As an alternative it would be
> > possible to return NULL and cause an error during the lookup. I think
> > returning the unmodified name is more user friendly. The admin might
> > wonder why the space is not replaced but at least the name is resolved.
> > 
> > With this patch a name like 'abc def_ghi' is return unmodified if the
> > replacement character is '_' . But without the check 'abc_def_ghi' is
> > returned which cannot be resolved back which might cause more trouble
> > than the space in the name.
> > 
> > bye,
> > Sumit
> 
> 
> Did you consider shouting to syslog so that the failure is better
> discoverable for the admin?

New version calling sss_log() as well is attached. Please note that
sss_log() might be broken in master, see my other patch for this.

bye,
Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-------------- next part --------------
From 3afe15e22d3cf3d02b5adf232ff3740291a8b6e0 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Mon, 25 Aug 2014 10:36:46 +0200
Subject: [PATCH] Replace space: add some checks

This patch adds some additional checks if the option for replacing
spaces in user and group names is used.

When replacing space with the replacement character it is checked if the
name already contains the replacement character. If it does the
unmodified name is returned because in this case a revers operation
would not be possible.

For the reverse operation is it checked if the input contains both a
space and the replacement character. If this is true the unmodified name
is returned as well, because we have to assume that it is the original
name because otherwise it wouldn't contain both characters.

Additionally a shortcut if the replacement characters is a space and
tests for the new checks are added. The man page is updated accordingly.

Related to https://fedorahosted.org/sssd/ticket/1854 and
https://fedorahosted.org/sssd/ticket/2397 .
---
 src/man/sssd.conf.5.xml              |  8 +++++---
 src/tests/cmocka/test_string_utils.c |  4 ++++
 src/util/string_utils.c              | 26 +++++++++++++++++++++++---
 src/util/util.h                      |  2 +-
 4 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index 81a46ed..ad01f37 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -334,9 +334,11 @@
                             </para>
                             <para>
                                 Please note it is a configuration error to use
-                                a replacement character that might be used by
-                                another LDAP object. In that case, result of
-                                a lookup is undefined.
+                                a replacement character that might be used in
+                                user or group names. If a name contains the
+                                replacement character SSSD tries to return the
+                                unmodified name but in general the result of a
+                                lookup is undefined.
                             </para>
                             <para>
                                 Default: not set (spaces will not be replaced)
diff --git a/src/tests/cmocka/test_string_utils.c b/src/tests/cmocka/test_string_utils.c
index 93d7d71..e446387 100644
--- a/src/tests/cmocka/test_string_utils.c
+++ b/src/tests/cmocka/test_string_utils.c
@@ -47,6 +47,7 @@ void test_replace_whitespaces(void **state)
         { "    ", "    ", ' ' },
         { "abcd", "abcd", ' ' },
         { "a b c d", "a b c d", ' ' },
+        { "a b^c d", "a b^c d", '^' },
         { NULL, NULL, '\0' },
     };
 
@@ -92,6 +93,8 @@ void test_reverse_replace_whitespaces(void **state)
         { "abcd", "abcd", '-' },
         { "a-b-c-d", "a b c d", '-' },
         { "-a-b-c-d-", " a b c d ", '-' },
+        { "a b c d", "a b c d", '-' },
+        { " a b c d ", " a b c d ", '-' },
         { "^", " ", '^' },
         { "^^^^", "    ", '^' },
         { "abcd", "abcd", '^' },
@@ -102,6 +105,7 @@ void test_reverse_replace_whitespaces(void **state)
         { "abcd", "abcd", ' ' },
         { "a b c d", "a b c d", ' ' },
         { " a b c d ", " a b c d ", ' ' },
+        { "a b^c d", "a b^c d", '^' },
         { NULL, NULL, '\0' },
     };
 
diff --git a/src/util/string_utils.c b/src/util/string_utils.c
index ec4cc68..a39b950 100644
--- a/src/util/string_utils.c
+++ b/src/util/string_utils.c
@@ -48,18 +48,38 @@ char * sss_replace_space(TALLOC_CTX *mem_ctx,
                          const char *orig_name,
                          const char subst)
 {
-    if (subst == '\0') {
+    if (subst == '\0' || subst == ' ') {
         return talloc_strdup(mem_ctx, orig_name);
     }
+
+    if (strchr(orig_name, subst) != NULL) {
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              "Input [%s] already contains replacement character [%c].\n",
+              orig_name, subst);
+        sss_log(SSS_LOG_CRIT,
+                "Name [%s] already contains replacement character [%c]. " \
+                "No replacement will be done.\n",
+                orig_name, subst);
+        return talloc_strdup(mem_ctx, orig_name);
+    }
+
     return replace_char(mem_ctx, orig_name, ' ', subst);
 }
 
 char * sss_reverse_replace_space(TALLOC_CTX *mem_ctx,
-                                 char *orig_name,
+                                 const char *orig_name,
                                  const char subst)
 {
-    if (subst == '\0') {
+    if (subst == '\0' || subst == ' ') {
         return talloc_strdup(mem_ctx, orig_name);
     }
+
+    if (strchr(orig_name, subst) != NULL && strchr(orig_name, ' ') != NULL) {
+        DEBUG(SSSDBG_TRACE_FUNC,
+              "Input [%s] contains replacement character [%c] and space.\n",
+              orig_name, subst);
+        return talloc_strdup(mem_ctx, orig_name);
+    }
+
     return replace_char(mem_ctx, orig_name, subst, ' ');
 }
diff --git a/src/util/util.h b/src/util/util.h
index ca740d0..d3b746b 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -570,7 +570,7 @@ char * sss_replace_space(TALLOC_CTX *mem_ctx,
                          const char *orig_name,
                          const char replace_char);
 char * sss_reverse_replace_space(TALLOC_CTX *mem_ctx,
-                                 char *orig_name,
+                                 const char *orig_name,
                                  const char replace_char);
 
 #endif /* __SSSD_UTIL_H__ */
-- 
1.8.3.1



More information about the sssd-devel mailing list