Re: [Freeipa-devel] [PATCH 154] ipa-kdb: map_groups() consider all results

[Alexander Bokovoy]

On Mon, 01 Feb 2016, Jakub Hrozek wrote:

On Tue, Jan 05, 2016 at 07:55:33PM +0100, Sumit Bose wrote:

Hi,

to find out to which local group a external user is mapped we do a
dereference search over the external groups with the SIDs related to the
external user. If a SID is mapped to more than one external group we
currently consider only the first returned match. With this patch all
results are taken into account. This makes sure all expected local group
memberships are added to the PAC which resolves
https://fedorahosted.org/freeipa/ticket/5573.


I tested with an AD user who was a member of several IPA external groups. All
groups were displayed.  We also have positive feedback from several users
who applied this patch.

The code looks good to me as well, Sumit explained some parts I didn't
understand on IRC.

ACK from me..

... and ACK from me too.
--
/ Alexander Bokovoy

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 154] ipa-kdb: map_groups() consider all results

[Jakub Hrozek]

On Tue, Jan 05, 2016 at 07:55:33PM +0100, Sumit Bose wrote:
> Hi,
> 
> to find out to which local group a external user is mapped we do a
> dereference search over the external groups with the SIDs related to the
> external user. If a SID is mapped to more than one external group we
> currently consider only the first returned match. With this patch all
> results are taken into account. This makes sure all expected local group
> memberships are added to the PAC which resolves
> https://fedorahosted.org/freeipa/ticket/5573.

I tested with an AD user who was a member of several IPA external groups. All
groups were displayed.  We also have positive feedback from several users
who applied this patch.

The code looks good to me as well, Sumit explained some parts I didn't
understand on IRC.

ACK from me..

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 154] ipa-kdb: map_groups() consider all results

[Alexander Bokovoy]

On Tue, 05 Jan 2016, Sumit Bose wrote:

Hi,

to find out to which local group a external user is mapped we do a
dereference search over the external groups with the SIDs related to the
external user. If a SID is mapped to more than one external group we
currently consider only the first returned match. With this patch all
results are taken into account. This makes sure all expected local group
memberships are added to the PAC which resolves
https://fedorahosted.org/freeipa/ticket/5573.

The code looks fine but I have not tested it. Hopefully, will do so in
between FOSDEM and devconf.cz.


--
/ Alexander Bokovoy

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 154] ipa-kdb: map_groups() consider all results

[Petr Vobornik]

On 01/05/2016 07:55 PM, Sumit Bose wrote:

Hi,

to find out to which local group a external user is mapped we do a
dereference search over the external groups with the SIDs related to the
external user. If a SID is mapped to more than one external group we
currently consider only the first returned match. With this patch all
results are taken into account. This makes sure all expected local group
memberships are added to the PAC which resolves
https://fedorahosted.org/freeipa/ticket/5573.

bye,
Sumit



bump for review.
--
Petr Vobornik

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH 154] ipa-kdb: map_groups() consider all results

[Sumit Bose]

Hi,

to find out to which local group a external user is mapped we do a
dereference search over the external groups with the SIDs related to the
external user. If a SID is mapped to more than one external group we
currently consider only the first returned match. With this patch all
results are taken into account. This makes sure all expected local group
memberships are added to the PAC which resolves
https://fedorahosted.org/freeipa/ticket/5573.

bye,
Sumit
From 60748d2da05261df937eba85cee27c2ea0d7e893 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Wed, 16 Dec 2015 12:38:16 +0100
Subject: [PATCH] ipa-kdb: map_groups() consider all results

Resolves https://fedorahosted.org/freeipa/ticket/5573
---
 daemons/ipa-kdb/ipa_kdb_mspac.c | 118 +---
 1 file changed, 61 insertions(+), 57 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 
daa42e369014f2ed401742474453ebb1aadef07c..45721f0dc06d90479f8fc2858c462c3647f7a3c6
 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -1082,68 +1082,72 @@ static int map_groups(TALLOC_CTX *memctx, krb5_context 
kcontext,
 continue;
 }
 
-ldap_derefresponse_free(deref_results);
-ret = ipadb_ldap_deref_results(ipactx->lcontext, lentry, 
_results);
-switch (ret) {
-case ENOENT:
-/* No entry found, try next SID */
-break;
-case 0:
-if (deref_results == NULL) {
-krb5_klog_syslog(LOG_ERR, "No results.");
+do {
+ldap_derefresponse_free(deref_results);
+ret = ipadb_ldap_deref_results(ipactx->lcontext, lentry, 
_results);
+switch (ret) {
+case ENOENT:
+/* No entry found, try next SID */
 break;
-}
+case 0:
+if (deref_results == NULL) {
+krb5_klog_syslog(LOG_ERR, "No results.");
+break;
+}
 
-for (dres = deref_results; dres; dres = dres->next) {
-count++;
-}
+for (dres = deref_results; dres; dres = dres->next) {
+count++;
+}
 
-sids = talloc_realloc(memctx, sids, struct dom_sid, count);
-if (sids == NULL) {
-krb5_klog_syslog(LOG_ERR, "talloc_realloc failed.");
-kerr = ENOMEM;
+sids = talloc_realloc(memctx, sids, struct dom_sid, count);
+if (sids == NULL) {
+krb5_klog_syslog(LOG_ERR, "talloc_realloc failed.");
+kerr = ENOMEM;
+goto done;
+}
+
+for (dres = deref_results; dres; dres = dres->next) {
+gid = 0;
+memset(, '\0', sizeof(struct dom_sid));
+for (dval = dres->attrVals; dval; dval = dval->next) {
+if (strcasecmp(dval->type, "gidNumber") == 0) {
+errno = 0;
+gid = strtoul((char *)dval->vals[0].bv_val,
+  ,10);
+if (gid == 0 || gid >= UINT32_MAX || errno != 
0 ||
+*endptr != '\0') {
+continue;
+}
+}
+if (strcasecmp(dval->type,
+   "ipaNTSecurityIdentifier") == 0) {
+kerr = string_to_sid((char 
*)dval->vals[0].bv_val, );
+if (kerr != 0) {
+continue;
+}
+}
+}
+if (gid != 0 && sid.sid_rev_num != 0) {
+/* TODO: check if gid maps to sid */
+if (sid_index >= count) {
+krb5_klog_syslog(LOG_ERR, "Index larger than "
+  "array, this shoould 
"
+  "never happen.");
+kerr = EFAULT;
+goto done;
+}
+memcpy([sid_index], , sizeof(struct 
dom_sid));
+sid_index++;
+}
+}
+
+break;
+default:
 goto done;
-}
+}
 
-for (dres = deref_results; dres; dres = dres->next) {
-