<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 07/21/2014 06:29 PM, Michal Židek
wrote:<br>
</div>
<blockquote cite="mid:53CD3FD2.8010208@redhat.com" type="cite">On
07/21/2014 05:54 PM, Michal Židek wrote:
<br>
<blockquote type="cite">On 07/18/2014 03:41 PM, Pavel Reichl
wrote:
<br>
<blockquote type="cite">
<br>
Thanks for the quick update, I have some more concerns and
questions
<br>
about the patches.
<br>
<br>
1st patch:
<br>
<blockquote type="cite">+int confdb_set_string(struct
confdb_ctx *cdb,
<br>
+ const char *section,
<br>
+ const char *attribute,
<br>
+ char *val)
<br>
+{
<br>
</blockquote>
[snip]
<br>
<blockquote type="cite">+ lret = ldb_msg_add_string(msg,
attribute, val);
<br>
+ if (lret != LDB_SUCCESS) {
<br>
+ DEBUG(SSSDBG_MINOR_FAILURE,
<br>
+ "ldb_msg_add_string failed: [%s]\n",
ldb_strerror(lret));
<br>
+ ret = EIO;
<br>
+ goto done;
<br>
+ }
<br>
+
<br>
+
<br>
</blockquote>
Two empty lines.
<br>
</blockquote>
<br>
Fixed.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">+ lret = ldb_modify(cdb->ldb,
msg);
<br>
+ if (lret != LDB_SUCCESS) {
<br>
+ DEBUG(SSSDBG_MINOR_FAILURE,
<br>
+ "ldb_modify failed: [%s]\n",
ldb_strerror(lret));
<br>
+ ret = EIO;
<br>
+ goto done;
<br>
+ }
<br>
+
<br>
+ ret = EOK;
<br>
+
<br>
+done:
<br>
+ talloc_free(tmp_ctx);
<br>
+ if (ret != EOK) {
<br>
+ DEBUG(SSSDBG_CRIT_FAILURE,
<br>
+ "Failed to set [%s] from [%s], error [%d]
(%s)\n",
<br>
+ attribute, section, ret, strerror(ret));
<br>
+ }
<br>
+ return ret;
<br>
+}
<br>
</blockquote>
<br>
missing new line here
<br>
</blockquote>
<br>
Fixed.
<br>
<br>
<blockquote type="cite">
<br>
<blockquote type="cite"> int confdb_get_string(struct
confdb_ctx *cdb, TALLOC_CTX *ctx,
<br>
const char *section, const char
*attribute,
<br>
const char *defstr, char **result)
<br>
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
<br>
index ba33ea5..f81c6d4 100644
<br>
--- a/src/confdb/confdb.h
<br>
+++ b/src/confdb/confdb.h
<br>
@@ -216,7 +216,7 @@ struct sss_domain_info {
<br>
<br>
bool cache_credentials;
<br>
bool legacy_passwords;
<br>
- bool case_sensitive;
<br>
</blockquote>
<br>
Why do you remove case_sensitive here and add it back in
second patch? I
<br>
think this 'ping-pong' is confusing and needless, could you
fix it,
<br>
please?
<br>
<br>
</blockquote>
<br>
Sorry, bad rebasing. Fixed.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">+ bool case_preserve;
<br>
<br>
gid_t override_gid;
<br>
const char *override_homedir;
<br>
@@ -459,6 +459,11 @@ int confdb_set_bool(struct confdb_ctx
*cdb,
<br>
const char *attribute,
<br>
bool val);
<br>
<br>
</blockquote>
<br>
Would you consider adding doxygen comment here? All functions
with
<br>
exception of confdb_set_bool() have one.
<br>
But maybe it's not worth it, what do you think?
<br>
</blockquote>
<br>
I do not think it is necessary in this case, but I added it
<br>
anyway.
<br>
</blockquote>
</blockquote>
<br>
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. <br>
<br>
<blockquote cite="mid:53CD3FD2.8010208@redhat.com" type="cite">
<blockquote type="cite">
<br>
<blockquote type="cite">
<br>
<blockquote type="cite">+int confdb_set_string(struct
confdb_ctx *cdb,
<br>
+ const char *section,
<br>
+ const char *attribute,
<br>
+ char *val);
<br>
+
<br>
/**
<br>
* @brief Convenience function to retrieve a single-valued
attribute
<br>
as a
<br>
* null-terminated array of strings
<br>
--
<br>
1.9.3
<br>
</blockquote>
<br>
Generally, I think there's custom do remove any unused
function from
<br>
code-base when they aren't called. So I think you should
remove
<br>
confdb_set_string(). I guess you will need a new patch for
that to keep
<br>
every commit compilable.
<br>
</blockquote>
<br>
You mean confdb_set_bool? I added new patch to remove
<br>
that function.
<br>
<br>
</blockquote>
</blockquote>
Yes, thanks.<br>
<blockquote cite="mid:53CD3FD2.8010208@redhat.com" type="cite">
<blockquote type="cite">
<blockquote type="cite">
<br>
2nd patch:
<br>
<br>
<blockquote type="cite">@@ -1218,12 +1218,27 @@ static int
confdb_get_domain_internal(struct
<br>
confdb_ctx *cdb,
<br>
}
<br>
}
<br>
<br>
- ret = get_entry_as_bool(res->msgs[0],
&domain->case_sensitive,
<br>
- CONFDB_DOMAIN_CASE_SENSITIVE,
true);
<br>
- if(ret != EOK) {
<br>
- DEBUG(SSSDBG_FATAL_FAILURE,
<br>
- "Invalid value for %s\n",
CONFDB_DOMAIN_CASE_SENSITIVE);
<br>
- goto done;
<br>
+ tmp = ldb_msg_find_attr_as_string(res->msgs[0],
<br>
+ CONFDB_DOMAIN_CASE_SENSITIVE, "true");
<br>
+ if (tmp != NULL) {
<br>
+ if (strcasecmp(tmp, "true") == 0) {
<br>
+ domain->case_sensitive = true;
<br>
+ domain->case_preserve = true;
<br>
+ } else if (strcasecmp(tmp, "false") == 0) {
<br>
+ domain->case_sensitive = false;
<br>
+ domain->case_preserve = false;
<br>
+ } else if (strcasecmp(tmp, "preserving") == 0) {
<br>
+ domain->case_sensitive = false;
<br>
+ domain->case_preserve = true;
<br>
+ } else {
<br>
+ DEBUG(SSSDBG_FATAL_FAILURE,
<br>
+ "Invalid value for %s\n",
<br>
CONFDB_DOMAIN_CASE_SENSITIVE);
<br>
+ goto done;
<br>
+ }
<br>
+ } else {
<br>
+ /* default */
<br>
+ domain->case_sensitive = true;
<br>
+ domain->case_preserve = true;
<br>
}
<br>
if (domain->case_sensitive == false &&
<br>
strcasecmp(domain->provider, "local") == 0) {
<br>
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
<br>
index f81c6d4..8a642b3 100644
<br>
--- a/src/confdb/confdb.h
<br>
+++ b/src/confdb/confdb.h
<br>
@@ -216,6 +216,7 @@ struct sss_domain_info {
<br>
<br>
bool cache_credentials;
<br>
bool legacy_passwords;
<br>
+ bool case_sensitive;
<br>
bool case_preserve;
<br>
<br>
gid_t override_gid;
<br>
diff --git a/src/providers/ad/ad_common.c
b/src/providers/ad/ad_common.c
<br>
index 67ded36..672a1e1 100644
<br>
--- a/src/providers/ad/ad_common.c
<br>
+++ b/src/providers/ad/ad_common.c
<br>
@@ -263,6 +263,7 @@ ad_get_common_options(TALLOC_CTX
*mem_ctx,
<br>
char *realm;
<br>
char *ad_hostname;
<br>
char hostname[HOST_NAME_MAX + 1];
<br>
+ char *tmp;
<br>
</blockquote>
<br>
I personally dislike general variable names, it's my opinion
that if you
<br>
are creating variable for specific use then you should name it
<br>
accordingly.
<br>
Of course there are exceptions from this rule, do you see any
not to
<br>
name it 'case_sensitive_str' or something like this?
<br>
</blockquote>
<br>
I wanted to have the same name as in confdb.c, but ok, I will
<br>
change the name in the ad specific code to case_sensitive_opt.
<br>
<br>
<blockquote type="cite">
<br>
<blockquote type="cite">
<br>
opts = talloc_zero(mem_ctx, struct ad_options);
<br>
if (!opts) return ENOMEM;
<br>
@@ -333,13 +334,36 @@ ad_get_common_options(TALLOC_CTX
*mem_ctx,
<br>
}
<br>
<br>
/* Active Directory is always case-insensitive */
<br>
- dom->case_sensitive = false;
<br>
+ ret = confdb_get_string(cdb, mem_ctx, conf_path,
<br>
+ CONFDB_DOMAIN_CASE_SENSITIVE,
"false",
<br>
+ &tmp);
<br>
+ if (ret != EOK) {
<br>
+ DEBUG(SSSDBG_CRIT_FAILURE, "condb_get_string
failed.\n");
<br>
+ goto done;
<br>
+ }
<br>
+
<br>
+ if (strcasecmp(tmp, "true") == 0) {
<br>
+ DEBUG(SSSDBG_CRIT_FAILURE,
<br>
+ "Warning: AD domain can not be set as
<br>
case-sensitive.\n");
<br>
+ dom->case_sensitive = false;
<br>
+ dom->case_preserve = false;
<br>
+ } else if (strcasecmp(tmp, "false") == 0) {
<br>
+ dom->case_sensitive = false;
<br>
+ dom->case_preserve = false;
<br>
+ } else if (strcasecmp(tmp, "preserving") == 0) {
<br>
+ dom->case_sensitive = false;
<br>
+ dom->case_preserve = true;
<br>
+ } else {
<br>
+ DEBUG(SSSDBG_FATAL_FAILURE,
<br>
+ "Invalid value for %s\n",
CONFDB_DOMAIN_CASE_SENSITIVE);
<br>
+ goto done;
<br>
+ }
<br>
<br>
/* Set this in the confdb so that the responders pick
it
<br>
* up when they start up.
<br>
*/
<br>
- ret = confdb_set_bool(cdb, conf_path, "case_sensitive",
<br>
- dom->case_sensitive);
<br>
+ ret = confdb_set_string(cdb, conf_path,
"case_sensitive",
<br>
+ tmp);
<br>
if (ret != EOK) {
<br>
DEBUG(SSSDBG_CRIT_FAILURE,
<br>
"Could not set domain case-sensitive:
[%s]\n",
<br>
diff --git a/src/providers/ipa/ipa_selinux.c
<br>
b/src/providers/ipa/ipa_selinux.c
<br>
index 927e545..3cd1cc1 100644
<br>
--- a/src/providers/ipa/ipa_selinux.c
<br>
+++ b/src/providers/ipa/ipa_selinux.c
<br>
@@ -757,7 +757,7 @@ static errno_t
write_selinux_login_file(const char
<br>
*orig_name,
<br>
/* pam_selinux needs the username in the same format
getpwnam()
<br>
would
<br>
* return it
<br>
*/
<br>
- username = sss_get_cased_name(tmp_ctx, orig_name,
<br>
dom->case_sensitive);
<br>
+ username = sss_get_cased_name(tmp_ctx, orig_name,
<br>
dom->case_preserve);
<br>
if (username == NULL) {
<br>
ret = ENOMEM;
<br>
goto done;
<br>
diff --git a/src/responder/nss/nsssrv_cmd.c
<br>
b/src/responder/nss/nsssrv_cmd.c
<br>
index a168a3e..c201d3a 100644
<br>
--- a/src/responder/nss/nsssrv_cmd.c
<br>
+++ b/src/responder/nss/nsssrv_cmd.c
<br>
@@ -365,7 +365,7 @@ static int fill_pwent(struct sss_packet
*packet,
<br>
packet_initialized = true;
<br>
}
<br>
<br>
- tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
<br>
dom->case_sensitive);
<br>
+ tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
<br>
dom->case_preserve);
<br>
if (tmpstr == NULL) {
<br>
DEBUG(SSSDBG_CRIT_FAILURE,
<br>
"sss_get_cased_name failed, skipping\n");
<br>
@@ -2492,7 +2492,7 @@ static int fill_grent(struct
sss_packet *packet,
<br>
}
<br>
}
<br>
<br>
- tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
<br>
dom->case_sensitive);
<br>
+ tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
<br>
dom->case_preserve);
<br>
if (tmpstr == NULL) {
<br>
DEBUG(SSSDBG_CRIT_FAILURE,
<br>
"sss_get_cased_name failed, skipping\n");
<br>
--
<br>
1.9.3
<br>
</blockquote>
<br>
I don't like very much the code duplication around parsing the
option
<br>
value case_sensitive. Would you consider creating some
utility function
<br>
that would do so and call it from ad_get_common_options() and
<br>
confdb_get_domain_internal()?
<br>
Something like:
<br>
<br>
int sss_parse_case_sensitivity_option(const char* str)
<br>
{
<br>
if (strcasecmp(str, "true") == 0) {
<br>
return CASE_INSENSITIVE;
<br>
} else if (strcasecmp(str, "false") == 0) {
<br>
return CASE_INSENSITIVE;
<br>
....
<br>
<br>
<br>
</blockquote>
<br>
I do not think this will help. If we added such function
<br>
than the only think that would change would be, that
<br>
the
<br>
'if (strcasecmp(tmp, "something") == 0)'
<br>
lines would be replaced with
<br>
'if (ret == SSS_SOMETHING)'.
<br>
Or maybe switch-case might be used, but the
<br>
lines would still have to be there. See that
<br>
I set different values in the AD specific code
<br>
and print additional debug message in one case
<br>
so it is not really duplication and would still
<br>
have to be handled outside of this utility
<br>
function, which is the reason why IMO such
<br>
function would only add code but would not be
<br>
very helpful -- what do you think?).
<br>
<br>
</blockquote>
</blockquote>
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. <br>
<br>
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.<br>
<blockquote cite="mid:53CD3FD2.8010208@redhat.com" type="cite">
<blockquote type="cite">
<blockquote type="cite">
<br>
3rd patch:
<br>
<br>
<blockquote type="cite">@@ -1817,12 +1817,16 @@
fallback_homedir = /home/%u
<br>
</varlistentry>
<br>
<br>
<varlistentry>
<br>
- <term>case_sensitive
(boolean)</term>
<br>
+ <term>case_sensitive
(string)</term>
<br>
<listitem>
<br>
<para>
<br>
Treat user and group names as
case
<br>
sensitive. At
<br>
the moment, this option is not
supported in
<br>
- the local provider.
<br>
+ the local provider. Possible
options are:
<br>
+ True, False, Preserving.
Preserving is the
<br>
+ same as False (case
insensitive), but does
<br>
+ not lowercase names in the
output of
<br>
getpwnam
<br>
+ and getgrnam.
<br>
</para>
<br>
<para>
<br>
Default: True
<br>
</blockquote>
I wonder if it were better to list each option value on its
own line.
<br>
Something like:
<br>
<br>
<para>
<br>
Treat user and group names as
case
<br>
sensitive. At
<br>
the moment, this option is not
supported in
<br>
- the local provider. Possible
options are:
<br>
- True, False, Preserving.
Preserving is the
<br>
- same as False (case insensitive),
but does
<br>
+ the local provider.
<br>
+ </para>
<br>
+ <para>
<br>
+ Supported values:
<br>
+ </para>
<br>
+ <para>
<br>
+ True: case insensitive
<br>
+ </para>
<br>
+ <para>
<br>
+ True: case sensitive
<br>
+ </para>
<br>
+ <para>
<br>
+ Preserving: case insensitive, but
does
<br>
not lowercase names in the
output of
<br>
getpwnam
<br>
and g
<br>
<br>
But I think this is a matter of personal taste so I leave it
up to you.
<br>
</blockquote>
<br>
Ok, I added a list of possible options.
<br>
</blockquote>
</blockquote>
Thanks, I hope you agree it's better now.<br>
<br>
<blockquote type="cite">+ the local
provider. Possible options are:<br>
</blockquote>
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. <br>
<br>
<blockquote type="cite">+
<variablelist><br>
+ <varlistentry><br>
+ <br>
</blockquote>
<br>
Could you also mention AD specifics for this option?<br>
<br>
<blockquote cite="mid:53CD3FD2.8010208@redhat.com" type="cite">
<blockquote type="cite">
<br>
<blockquote type="cite">Anyway, I think that man page changes
must be acked-by native speaker
<br>
(Stephen),
<br>
<br>
Thanks,
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
The 4th patch had wrong author in description. Sending the
<br>
patches again.
<br>
<br>
Michal
<br>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
sssd-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
<a class="moz-txt-link-freetext" href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
</pre>
</blockquote>
<br>
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. <br>
<br>
I was not able to create an user in IPA with some upper case so I
had to change it directly in LDAP.<br>
<br>
This is what I see when using ipa:<br>
<br>
ipa user-find max<br>
--------------<br>
1 user matched<br>
--------------<br>
User login: MaX<br>
First name: xx<br>
Last name: AAA<br>
Home directory: /home/max<br>
Login shell: /bin/sh<br>
Email address: <a class="moz-txt-link-abbreviated" href="mailto:max@ipa.work">max@ipa.work</a><br>
UID: 1994000038<br>
GID: 1994000038<br>
Account disabled: False<br>
Password: False<br>
Kerberos keys available: False<br>
----------------------------<br>
Number of entries returned 1<br>
----------------------------<br>
<br>
This is what I see when using sssd as ipa client:<br>
<br>
case_sensitive = preserving:<br>
getent group <a class="moz-txt-link-abbreviated" href="mailto:MaX@ipa.work">MaX@ipa.work</a><br>
<a class="moz-txt-link-abbreviated" href="mailto:max@ipa.work:*:1994000038">max@ipa.work:*:1994000038</a>:<br>
<br>
case_sensitive = false:<br>
getent group <a class="moz-txt-link-abbreviated" href="mailto:MaX@ipa.work">MaX@ipa.work</a><br>
<a class="moz-txt-link-abbreviated" href="mailto:max@ipa.work:*:1994000038">max@ipa.work:*:1994000038</a>:<br>
<br>
case_sensitive = true:<br>
does not return anything<br>
<br>
Was the patch working with IPA provider to you? <br>
<br>
Thanks!<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
</body>
</html>