[SSSD] [PATCHES] sss_case = preserving

Pavel Reichl preichl at redhat.com
Tue Jul 22 12:03:18 UTC 2014


On 07/21/2014 06:29 PM, Michal Židek wrote:
> On 07/21/2014 05:54 PM, Michal Židek wrote:
>> On 07/18/2014 03:41 PM, Pavel Reichl wrote:
>>>
>>> Thanks for the quick update, I have some more concerns and questions
>>> about the patches.
>>>
>>> 1st patch:
>>>> +int confdb_set_string(struct confdb_ctx *cdb,
>>>> +                      const char *section,
>>>> +                      const char *attribute,
>>>> +                      char *val)
>>>> +{
>>> [snip]
>>>> +    lret = ldb_msg_add_string(msg, attribute, val);
>>>> +    if (lret != LDB_SUCCESS) {
>>>> +        DEBUG(SSSDBG_MINOR_FAILURE,
>>>> +              "ldb_msg_add_string failed: [%s]\n", 
>>>> ldb_strerror(lret));
>>>> +        ret = EIO;
>>>> +        goto done;
>>>> +    }
>>>> +
>>>> +
>>> Two empty lines.
>>
>> Fixed.
>>
>>>> +    lret = ldb_modify(cdb->ldb, msg);
>>>> +    if (lret != LDB_SUCCESS) {
>>>> +        DEBUG(SSSDBG_MINOR_FAILURE,
>>>> +              "ldb_modify failed: [%s]\n", ldb_strerror(lret));
>>>> +        ret = EIO;
>>>> +        goto done;
>>>> +    }
>>>> +
>>>> +    ret = EOK;
>>>> +
>>>> +done:
>>>> +    talloc_free(tmp_ctx);
>>>> +    if (ret != EOK) {
>>>> +        DEBUG(SSSDBG_CRIT_FAILURE,
>>>> +              "Failed to set [%s] from [%s], error [%d] (%s)\n",
>>>> +               attribute, section, ret, strerror(ret));
>>>> +    }
>>>> +    return ret;
>>>> +}
>>>
>>> missing new line here
>>
>> Fixed.
>>
>>>
>>>>  int confdb_get_string(struct confdb_ctx *cdb, TALLOC_CTX *ctx,
>>>>                        const char *section, const char *attribute,
>>>>                        const char *defstr, char **result)
>>>> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
>>>> index ba33ea5..f81c6d4 100644
>>>> --- a/src/confdb/confdb.h
>>>> +++ b/src/confdb/confdb.h
>>>> @@ -216,7 +216,7 @@ struct sss_domain_info {
>>>>
>>>>      bool cache_credentials;
>>>>      bool legacy_passwords;
>>>> -    bool case_sensitive;
>>>
>>> Why do you remove case_sensitive here and add it back in second 
>>> patch? I
>>> think this 'ping-pong' is confusing and needless, could you fix it,
>>> please?
>>>
>>
>> Sorry, bad rebasing. Fixed.
>>
>>>> +    bool case_preserve;
>>>>
>>>>      gid_t override_gid;
>>>>      const char *override_homedir;
>>>> @@ -459,6 +459,11 @@ int confdb_set_bool(struct confdb_ctx *cdb,
>>>>                       const char *attribute,
>>>>                       bool val);
>>>>
>>>
>>> Would you consider adding doxygen comment here? All functions with
>>> exception of confdb_set_bool() have one.
>>> But maybe it's not worth it, what do you think?
>>
>> I do not think it is necessary in this case, but I added it
>> anyway.

Michal you didn't have to do that if you didn't like it.  I was just 
suggesting the idea. Sorry if you felt like I made you to do that.

>>
>>>
>>>> +int confdb_set_string(struct confdb_ctx *cdb,
>>>> +                      const char *section,
>>>> +                      const char *attribute,
>>>> +                      char *val);
>>>> +
>>>>  /**
>>>>   * @brief Convenience function to retrieve a single-valued attribute
>>>> as a
>>>>   * null-terminated array of strings
>>>> -- 
>>>> 1.9.3
>>>
>>> Generally, I think there's custom do remove any unused function from
>>> code-base when they aren't called. So I think you should remove
>>> confdb_set_string(). I guess you will need a new patch for that to keep
>>> every commit compilable.
>>
>> You mean confdb_set_bool? I added new patch to remove
>> that function.
>>
Yes, thanks.
>>>
>>> 2nd patch:
>>>
>>>> @@ -1218,12 +1218,27 @@ static int confdb_get_domain_internal(struct
>>>> confdb_ctx *cdb,
>>>>          }
>>>>      }
>>>>
>>>> -    ret = get_entry_as_bool(res->msgs[0], &domain->case_sensitive,
>>>> -                            CONFDB_DOMAIN_CASE_SENSITIVE, true);
>>>> -    if(ret != EOK) {
>>>> -        DEBUG(SSSDBG_FATAL_FAILURE,
>>>> -              "Invalid value for %s\n", 
>>>> CONFDB_DOMAIN_CASE_SENSITIVE);
>>>> -        goto done;
>>>> +    tmp = ldb_msg_find_attr_as_string(res->msgs[0],
>>>> + CONFDB_DOMAIN_CASE_SENSITIVE, "true");
>>>> +    if (tmp != NULL) {
>>>> +        if (strcasecmp(tmp, "true") == 0) {
>>>> +            domain->case_sensitive = true;
>>>> +            domain->case_preserve = true;
>>>> +        } else if (strcasecmp(tmp, "false") == 0) {
>>>> +            domain->case_sensitive = false;
>>>> +            domain->case_preserve = false;
>>>> +        } else if (strcasecmp(tmp, "preserving") == 0) {
>>>> +            domain->case_sensitive = false;
>>>> +            domain->case_preserve = true;
>>>> +        } else {
>>>> +            DEBUG(SSSDBG_FATAL_FAILURE,
>>>> +                  "Invalid value for %s\n",
>>>> CONFDB_DOMAIN_CASE_SENSITIVE);
>>>> +            goto done;
>>>> +        }
>>>> +    } else {
>>>> +        /* default */
>>>> +        domain->case_sensitive = true;
>>>> +        domain->case_preserve = true;
>>>>      }
>>>>      if (domain->case_sensitive == false &&
>>>>          strcasecmp(domain->provider, "local") == 0) {
>>>> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
>>>> index f81c6d4..8a642b3 100644
>>>> --- a/src/confdb/confdb.h
>>>> +++ b/src/confdb/confdb.h
>>>> @@ -216,6 +216,7 @@ struct sss_domain_info {
>>>>
>>>>      bool cache_credentials;
>>>>      bool legacy_passwords;
>>>> +    bool case_sensitive;
>>>>      bool case_preserve;
>>>>
>>>>      gid_t override_gid;
>>>> diff --git a/src/providers/ad/ad_common.c 
>>>> b/src/providers/ad/ad_common.c
>>>> index 67ded36..672a1e1 100644
>>>> --- a/src/providers/ad/ad_common.c
>>>> +++ b/src/providers/ad/ad_common.c
>>>> @@ -263,6 +263,7 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
>>>>      char *realm;
>>>>      char *ad_hostname;
>>>>      char hostname[HOST_NAME_MAX + 1];
>>>> +    char *tmp;
>>>
>>> I personally dislike general variable names, it's my opinion that if 
>>> you
>>> are creating variable for specific use then you should name it
>>> accordingly.
>>> Of course there are exceptions from this rule, do you see any not to
>>> name it 'case_sensitive_str' or something like this?
>>
>> I wanted to have the same name as in confdb.c, but ok, I will
>> change the name in the ad specific code to case_sensitive_opt.
>>
>>>
>>>>
>>>>      opts = talloc_zero(mem_ctx, struct ad_options);
>>>>      if (!opts) return ENOMEM;
>>>> @@ -333,13 +334,36 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
>>>>      }
>>>>
>>>>      /* Active Directory is always case-insensitive */
>>>> -    dom->case_sensitive = false;
>>>> +    ret = confdb_get_string(cdb, mem_ctx, conf_path,
>>>> +                            CONFDB_DOMAIN_CASE_SENSITIVE, "false",
>>>> +                            &tmp);
>>>> +    if (ret != EOK) {
>>>> +        DEBUG(SSSDBG_CRIT_FAILURE, "condb_get_string failed.\n");
>>>> +        goto done;
>>>> +    }
>>>> +
>>>> +    if (strcasecmp(tmp, "true") == 0) {
>>>> +        DEBUG(SSSDBG_CRIT_FAILURE,
>>>> +              "Warning: AD domain can not be set as
>>>> case-sensitive.\n");
>>>> +        dom->case_sensitive = false;
>>>> +        dom->case_preserve = false;
>>>> +    } else if (strcasecmp(tmp, "false") == 0) {
>>>> +        dom->case_sensitive = false;
>>>> +        dom->case_preserve = false;
>>>> +    } else if (strcasecmp(tmp, "preserving") == 0) {
>>>> +        dom->case_sensitive = false;
>>>> +        dom->case_preserve = true;
>>>> +    } else {
>>>> +        DEBUG(SSSDBG_FATAL_FAILURE,
>>>> +              "Invalid value for %s\n", 
>>>> CONFDB_DOMAIN_CASE_SENSITIVE);
>>>> +        goto done;
>>>> +    }
>>>>
>>>>      /* Set this in the confdb so that the responders pick it
>>>>       * up when they start up.
>>>>       */
>>>> -    ret = confdb_set_bool(cdb, conf_path, "case_sensitive",
>>>> -                          dom->case_sensitive);
>>>> +    ret = confdb_set_string(cdb, conf_path, "case_sensitive",
>>>> +                            tmp);
>>>>      if (ret != EOK) {
>>>>          DEBUG(SSSDBG_CRIT_FAILURE,
>>>>                "Could not set domain case-sensitive: [%s]\n",
>>>> diff --git a/src/providers/ipa/ipa_selinux.c
>>>> b/src/providers/ipa/ipa_selinux.c
>>>> index 927e545..3cd1cc1 100644
>>>> --- a/src/providers/ipa/ipa_selinux.c
>>>> +++ b/src/providers/ipa/ipa_selinux.c
>>>> @@ -757,7 +757,7 @@ static errno_t write_selinux_login_file(const char
>>>> *orig_name,
>>>>      /* pam_selinux needs the username in the same format getpwnam()
>>>> would
>>>>       * return it
>>>>       */
>>>> -    username = sss_get_cased_name(tmp_ctx, orig_name,
>>>> dom->case_sensitive);
>>>> +    username = sss_get_cased_name(tmp_ctx, orig_name,
>>>> dom->case_preserve);
>>>>      if (username == NULL) {
>>>>          ret = ENOMEM;
>>>>          goto done;
>>>> diff --git a/src/responder/nss/nsssrv_cmd.c
>>>> b/src/responder/nss/nsssrv_cmd.c
>>>> index a168a3e..c201d3a 100644
>>>> --- a/src/responder/nss/nsssrv_cmd.c
>>>> +++ b/src/responder/nss/nsssrv_cmd.c
>>>> @@ -365,7 +365,7 @@ static int fill_pwent(struct sss_packet *packet,
>>>>              packet_initialized = true;
>>>>          }
>>>>
>>>> -        tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
>>>> dom->case_sensitive);
>>>> +        tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
>>>> dom->case_preserve);
>>>>          if (tmpstr == NULL) {
>>>>              DEBUG(SSSDBG_CRIT_FAILURE,
>>>>                    "sss_get_cased_name failed, skipping\n");
>>>> @@ -2492,7 +2492,7 @@ static int fill_grent(struct sss_packet *packet,
>>>>              }
>>>>          }
>>>>
>>>> -        tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
>>>> dom->case_sensitive);
>>>> +        tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
>>>> dom->case_preserve);
>>>>          if (tmpstr == NULL) {
>>>>              DEBUG(SSSDBG_CRIT_FAILURE,
>>>>                    "sss_get_cased_name failed, skipping\n");
>>>> -- 
>>>> 1.9.3
>>>
>>> I don't like very much the code duplication around parsing the option
>>> value case_sensitive.  Would you consider creating some utility 
>>> function
>>> that would do so and call it from ad_get_common_options() and
>>> confdb_get_domain_internal()?
>>> Something like:
>>>
>>> int sss_parse_case_sensitivity_option(const char* str)
>>> {
>>>          if (strcasecmp(str, "true") == 0) {
>>>              return CASE_INSENSITIVE;
>>>          } else if (strcasecmp(str, "false") == 0) {
>>>              return CASE_INSENSITIVE;
>>> ....
>>>
>>>
>>
>> I do not think this will help. If we added such function
>> than the only think that would change would be, that
>> the
>> 'if (strcasecmp(tmp, "something") == 0)'
>> lines would be replaced with
>> 'if (ret == SSS_SOMETHING)'.
>> Or maybe switch-case might be used, but the
>> lines would still have to be there. See that
>> I set different values in the AD specific code
>> and print additional debug message in one case
>> so it is not really duplication and would still
>> have to be handled outside of this utility
>> function, which is the reason why IMO such
>> function would only add code but would not be
>> very helpful -- what do you think?).
>>
I agree with your concerns about my proposal, however I think you could 
reduce number of lines by setting default values for case_sensitive and 
case_preserve and then update them just for case where default value is 
not the right value.

But generally I don't think this is crucial for the patch. I'm fine if 
you don't do any further changes with this part of code. But thanks for 
considering.
>>>
>>> 3rd patch:
>>>
>>>> @@ -1817,12 +1817,16 @@ fallback_homedir = /home/%u
>>>>                  </varlistentry>
>>>>
>>>>                  <varlistentry>
>>>> -                    <term>case_sensitive (boolean)</term>
>>>> +                    <term>case_sensitive (string)</term>
>>>>                      <listitem>
>>>>                          <para>
>>>>                              Treat user and group names as case
>>>> sensitive. At
>>>>                              the moment, this option is not 
>>>> supported in
>>>> -                            the local provider.
>>>> +                            the local provider. Possible options are:
>>>> +                            True, False, Preserving. Preserving is 
>>>> the
>>>> +                            same as False (case insensitive), but 
>>>> does
>>>> +                            not lowercase names in the output of
>>>> getpwnam
>>>> +                            and getgrnam.
>>>>                          </para>
>>>>                          <para>
>>>>                              Default: True
>>> I wonder if it were better to list each option value on its own line.
>>> Something like:
>>>
>>>                            <para>
>>>                               Treat user and group names as case
>>> sensitive. At
>>>                               the moment, this option is not 
>>> supported in
>>> -                            the local provider. Possible options are:
>>> -                            True, False, Preserving. Preserving is the
>>> -                            same as False (case insensitive), but does
>>> +                            the local provider.
>>> +                        </para>
>>> +                        <para>
>>> +                            Supported values:
>>> +                        </para>
>>> +                        <para>
>>> +                            True: case insensitive
>>> +                        </para>
>>> +                        <para>
>>> +                            True: case sensitive
>>> +                        </para>
>>> +                        <para>
>>> +                            Preserving: case insensitive, but does
>>>                               not lowercase names in the output of
>>> getpwnam
>>>                               and g
>>>
>>> But I think this is a matter of personal taste so I leave it up to you.
>>
>> Ok, I added a list of possible options.
Thanks, I hope you agree it's better now.

> +                            the local provider. Possible options are:
Could you change it to something like: "the local provider. Possible 
option values are:"? Hmm I'm not really sure with wording, I just think 
word 'value' should be part of the sentence.

> + <variablelist>
> +                            <varlistentry>
> +

Could you also mention AD specifics for this option?

>>
>>> Anyway, I think that man page changes must be acked-by native speaker
>>> (Stephen),
>>>
>>> Thanks,
>>>
>>
>
> The 4th patch had wrong author in description. Sending the
> patches again.
>
> Michal
>
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

I finally tested the patches and it seems to me to be working with AD 
and LDAP provider, but does not seem to work with IPA provider.

I was not able to create an user in IPA with some upper case so I had to 
change it directly in LDAP.

This is what I see when using ipa:

ipa user-find max
--------------
1 user matched
--------------
   User login: MaX
   First name: xx
   Last name: AAA
   Home directory: /home/max
   Login shell: /bin/sh
   Email address: max at ipa.work
   UID: 1994000038
   GID: 1994000038
   Account disabled: False
   Password: False
   Kerberos keys available: False
----------------------------
Number of entries returned 1
----------------------------

This is what I see when using sssd as ipa client:

case_sensitive = preserving:
getent group MaX at ipa.work
max at ipa.work:*:1994000038:

case_sensitive = false:
getent group MaX at ipa.work
max at ipa.work:*:1994000038:

case_sensitive = true:
does not return anything

Was the patch working with IPA provider to you?

Thanks!







-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140722/6960b486/attachment.html>


More information about the sssd-devel mailing list