[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