On 07/17/2014 06:12 PM, Pavel Reichl wrote:
On Wed, 2014-07-16 at 15:40 +0200, Michal Židek wrote:
Hi,
patches for ticket
https://fedorahosted.org/sssd/ticket/2367
are in attachment.
Michal
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi Michal,
I haven't tested the patches yet. I just wanted to share some nitpicks
with you:
From 071b3ca8cd9a194c8cb287f9abca2fe7c58323a2 Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
Date: Tue, 15 Jul 2014 12:00:36 -0400
Subject: [PATCH 1/3] Add function confdb_set_string.
---
src/confdb/confdb.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
src/confdb/confdb.h | 6 +++++
2 files changed, 76 insertions(+)
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 15de961..79c89b7 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -369,6 +369,76 @@ done:
return ret;
}
+int confdb_set_string(struct confdb_ctx *cdb,
+ const char *section,
+ const char *attribute,
+ char *val)
+{
+ TALLOC_CTX *tmp_ctx;
+ struct ldb_dn *dn;
+ char *secdn;
+ struct ldb_message *msg;
+ int ret, lret;
+
+ tmp_ctx = talloc_new(NULL);
+ if (!tmp_ctx)
+ return ENOMEM;
Although there's not a consensus about the form of checking allocated
pointers ( !tmp_ctx vs. tmp_ctx != NULL ) among SSSD developers, still I
think there is an agreement that 'if' should be followed by block or
condition and action should be one-liner.
Could you change the code to something like?
if (!tmp_ctx) return ENOMEM;
or
if (!tmp_ctx) {
return ENOMEM;
}
Fixed.
In the second patch you use 2 forms of testing result of strcasecmp
!strcasecmp(tmp, "true")
strcasecmp(domain->provider, "local") == 0
Could you please just use one of them? From my POV the second is
preferred.
Fixed.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thank you Pavel!
New version is attached.
Michal
>From bb62af3c02a539930352a2d9d0e350d217fc443d Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
Date: Tue, 15 Jul 2014 12:00:36 -0400
Subject: [PATCH 1/3] Add function confdb_set_string.
Part of fix for:
https://fedorahosted.org/sssd/ticket/2367
---
src/confdb/confdb.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
src/confdb/confdb.h | 7 +++++-
2 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 15de961..b97e0bf 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -369,6 +369,77 @@ done:
return ret;
}
+int confdb_set_string(struct confdb_ctx *cdb,
+ const char *section,
+ const char *attribute,
+ char *val)
+{
+ TALLOC_CTX *tmp_ctx;
+ struct ldb_dn *dn;
+ char *secdn;
+ struct ldb_message *msg;
+ int ret, lret;
+
+ tmp_ctx = talloc_new(NULL);
+ if (!tmp_ctx) {
+ return ENOMEM;
+ }
+
+ ret = parse_section(tmp_ctx, section, &secdn, NULL);
+ if (ret != EOK) {
+ goto done;
+ }
+
+ dn = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
+ if (!dn) {
+ ret = EIO;
+ goto done;
+ }
+
+ msg = ldb_msg_new(tmp_ctx);
+ if (!msg) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ msg->dn = dn;
+
+ lret = ldb_msg_add_empty(msg, attribute, LDB_FLAG_MOD_REPLACE, NULL);
+ if (lret != LDB_SUCCESS) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "ldb_msg_add_empty failed: [%s]\n", ldb_strerror(lret));
+ ret = EIO;
+ goto done;
+ }
+
+ 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;
+ }
+
+
+ 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;
+}
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;
+ 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);
+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
>From 570fce327957f08927c799ddea9aefccc90ccca0 Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
Date: Tue, 15 Jul 2014 12:10:34 -0400
Subject: [PATCH 2/3] case_sensitivity = preserving
If case_sensitivity is set to 'preserving', getXXnam
returns name attribute in the same format as
stored in LDAP.
Fixes:
https://fedorahosted.org/sssd/ticket/2367
---
src/confdb/confdb.c | 27 +++++++++++++++++++++------
src/confdb/confdb.h | 1 +
src/providers/ad/ad_common.c | 30 +++++++++++++++++++++++++++---
src/providers/ipa/ipa_selinux.c | 2 +-
src/responder/nss/nsssrv_cmd.c | 4 ++--
5 files changed, 52 insertions(+), 12 deletions(-)
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index b97e0bf..f3dcfc8 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -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;
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
>From b23eecca5341c41e6ffae573a588824c4b7be5a3 Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
Date: Tue, 15 Jul 2014 13:16:28 -0400
Subject: [PATCH 3/3] MAN: case_sensitivity man page update
Fixes:
https://fedorahosted.org/sssd/ticket/2367
---
src/man/sssd.conf.5.xml | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index 58ccc30..b0f110b 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -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
--
1.9.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
