Re: [SSSD] [PATCHES] fix minor memory leaks

2015-09-04 Thread Pavel Reichl 



On 09/04/2015 01:56 PM, Jakub Hrozek wrote:

On Fri, Sep 04, 2015 at 01:09:36PM +0200, Pavel Reichl wrote:

Hello,

please see simple patch set fixing minor memory leaks of providers. I'm not 
aware of any user hitting those currently.

Thanks!



 From ef62f0245cde314fd11b1cd2584589e018ede050 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 4 Sep 2015 07:02:42 -0400
Subject: [PATCH 1/4] AD: fix minor memory leak

---
  src/providers/ad/ad_common.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index 
130cdeb613aae3843f7453a478815daaae6aab77..aca550581708e6501c99f1c69e3c5ec9303d3b8c
 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -658,7 +658,7 @@ ad_failover_init(TALLOC_CTX *mem_ctx, struct be_ctx *bectx,
  TALLOC_CTX *tmp_ctx;
  struct ad_service *service;

-tmp_ctx = talloc_new(mem_ctx);
+tmp_ctx = talloc_new(NULL);


I don't think we should do this kind of change, it's quite possible
we'll start using talloc pools soon which will make us migrate from
using NULL context.
OK, I just find the usage of tmp_ctx somewhat awkward in this function. I think function would be better off without it completely - service would be allocated directly on mem_ctx. But I agree this is not a bug and possibly just a matter of personal 
preference, so we can ignore that.




Just make sure tmp_ctx is freed as appropriate.

I haven't reviewed the rest of the patches.


  if (!tmp_ctx) return ENOMEM;

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCHES] fix minor memory leaks

2015-09-04 Thread Jakub Hrozek 
On Fri, Sep 04, 2015 at 01:09:36PM +0200, Pavel Reichl wrote:
> Hello,
> 
> please see simple patch set fixing minor memory leaks of providers. I'm not 
> aware of any user hitting those currently.
> 
> Thanks!

> From ef62f0245cde314fd11b1cd2584589e018ede050 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Fri, 4 Sep 2015 07:02:42 -0400
> Subject: [PATCH 1/4] AD: fix minor memory leak
> 
> ---
>  src/providers/ad/ad_common.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
> index 
> 130cdeb613aae3843f7453a478815daaae6aab77..aca550581708e6501c99f1c69e3c5ec9303d3b8c
>  100644
> --- a/src/providers/ad/ad_common.c
> +++ b/src/providers/ad/ad_common.c
> @@ -658,7 +658,7 @@ ad_failover_init(TALLOC_CTX *mem_ctx, struct be_ctx 
> *bectx,
>  TALLOC_CTX *tmp_ctx;
>  struct ad_service *service;
>  
> -tmp_ctx = talloc_new(mem_ctx);
> +tmp_ctx = talloc_new(NULL);

I don't think we should do this kind of change, it's quite possible
we'll start using talloc pools soon which will make us migrate from
using NULL context.

Just make sure tmp_ctx is freed as appropriate.

I haven't reviewed the rest of the patches.

>  if (!tmp_ctx) return ENOMEM;
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] Temporarily mark subdomain as inactive instead marking the whole back end offline

2015-09-04 Thread Jakub Hrozek 
On Fri, Sep 04, 2015 at 02:35:49PM +0200, Jakub Hrozek wrote:
> From 591d07855b70aacbb4488ba9a54438ee9ded48b5 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Fri, 4 Sep 2015 09:27:17 +0200
> Subject: [PATCH 2/8] DP: Provide a way to mark subdomain as disabled and
>  auto-enable it later with offline_timeout
> 
> https://fedorahosted.org/sssd/ticket/2637
> 
> Adds a new Data Provider function be_mark_dom_offline() that is a
> replacement for be_mark_offline(). When called, the function would
> either set the whole back end offline, just like be_mark_offline or just
> set the subdomain status to inactive.
> 
> When a subdomain is inactive, there is a singleton timed task that would
> re-set the subdomin after offline_timeout seconds.
> ---
>  src/providers/data_provider_be.c | 99 
> 
>  src/providers/dp_backend.h   |  1 +
>  2 files changed, 91 insertions(+), 9 deletions(-)

One note -- I'm adding a unit test for this patch:
https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=oneway

But I still think it makes sense to send the patches for review while
I'm working on the test..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCHES] fix minor memory leaks

2015-09-04 Thread Pavel Reichl 

Hello,

please see simple patch set fixing minor memory leaks of providers. I'm not 
aware of any user hitting those currently.

Thanks!
>From ef62f0245cde314fd11b1cd2584589e018ede050 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 4 Sep 2015 07:02:42 -0400
Subject: [PATCH 1/4] AD: fix minor memory leak

---
 src/providers/ad/ad_common.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index 130cdeb613aae3843f7453a478815daaae6aab77..aca550581708e6501c99f1c69e3c5ec9303d3b8c 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -658,7 +658,7 @@ ad_failover_init(TALLOC_CTX *mem_ctx, struct be_ctx *bectx,
 TALLOC_CTX *tmp_ctx;
 struct ad_service *service;
 
-tmp_ctx = talloc_new(mem_ctx);
+tmp_ctx = talloc_new(NULL);
 if (!tmp_ctx) return ENOMEM;
 
 service = talloc_zero(tmp_ctx, struct ad_service);
@@ -745,7 +745,7 @@ ad_failover_init(TALLOC_CTX *mem_ctx, struct be_ctx *bectx,
 ret = be_add_online_cb(bectx, bectx, ad_online_cb, service, NULL);
 if (ret != EOK) {
 DEBUG(SSSDBG_CRIT_FAILURE, "Could not set up AD online callback\n");
-return ret;
+goto done;
 }
 
 ret = be_fo_service_add_callback(mem_ctx, bectx, ad_service,
@@ -797,7 +797,8 @@ ad_resolve_callback(void *private_data, struct fo_server *server)
 sdata = fo_get_server_user_data(server);
 if (fo_is_srv_lookup(server) == false && sdata == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE, "No user data?\n");
-return;
+ret = EINVAL;
+goto done;
 }
 
 service = talloc_get_type(private_data, struct ad_service);
-- 
2.4.3

>From 7533dde984c08c0580b924c0f480e6fd17c34395 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 4 Sep 2015 07:03:45 -0400
Subject: [PATCH 2/4] IPA: fix minor memory leak

---
 src/providers/ipa/ipa_hbac_rules.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/providers/ipa/ipa_hbac_rules.c b/src/providers/ipa/ipa_hbac_rules.c
index ffef6dc4ce4229f2063d1b00308892bd3765f398..6ce1f76bb656352ec61332007a6fb62568374203 100644
--- a/src/providers/ipa/ipa_hbac_rules.c
+++ b/src/providers/ipa/ipa_hbac_rules.c
@@ -86,7 +86,7 @@ ipa_hbac_rule_info_send(TALLOC_CTX *mem_ctx,
 req = tevent_req_create(mem_ctx, , struct ipa_hbac_rule_state);
 if (req == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create failed.\n");
-return NULL;
+goto error;
 }
 
 state->ev = ev;
-- 
2.4.3

>From 26433a65fef11bc40bb7e5bed1e9acd9e2adbdcf Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 4 Sep 2015 07:04:10 -0400
Subject: [PATCH 3/4] SDAP: fix minor memory leak

---
 src/providers/ldap/sdap_async_groups.c | 2 +-
 src/providers/ldap/sdap_idmap.c| 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
index 525c6fa09553d8c0232ce2317751184f83632d86..52487e3cbc6c67c41966cd6a3665c53d30cd4055 100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -592,7 +592,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
 if (ret != EOK) {
 DEBUG(SSSDBG_OP_FAILURE,
   "Error: Failed to mark group as non-posix!\n");
-return ret;
+goto done;
 }
 }
 
diff --git a/src/providers/ldap/sdap_idmap.c b/src/providers/ldap/sdap_idmap.c
index dd959b2c133b342f105f76c26c889d678ce40391..36d529836eb7e4becd27721df15cfbccf035ae3b 100644
--- a/src/providers/ldap/sdap_idmap.c
+++ b/src/providers/ldap/sdap_idmap.c
@@ -206,7 +206,8 @@ sdap_idmap_init(TALLOC_CTX *mem_ctx,
 if (err != IDMAP_SUCCESS) {
 /* This should never happen */
 DEBUG(SSSDBG_CRIT_FAILURE, "sss_idmap_ctx corrupted\n");
-return EIO;
+ret = EIO;
+goto done;
 }
 
 
-- 
2.4.3

>From 4227817656ce966ecf82fd120f83b623cf31ea7a Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 4 Sep 2015 07:04:40 -0400
Subject: [PATCH 4/4] PROXY: fix minor memory leak

---
 src/providers/proxy/proxy_services.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/providers/proxy/proxy_services.c b/src/providers/proxy/proxy_services.c
index 2aa44dbf7f061b99a551b199d0265393e5399a06..ee545cedc9f8dc36981eb3190ff16763e1da390f 100644
--- a/src/providers/proxy/proxy_services.c
+++ b/src/providers/proxy/proxy_services.c
@@ -130,7 +130,7 @@ get_serv_byname(struct proxy_id_ctx *ctx,
 if (status != NSS_STATUS_SUCCESS && status != NSS_STATUS_NOTFOUND) {
 DEBUG(SSSDBG_MINOR_FAILURE,
   "getservbyname_r failed for service [%s].\n", name);
-return ret;
+goto done;
 }
 
 if (status == NSS_STATUS_NOTFOUND) {
@@ -183,7 +183,7 @@ get_serv_byport(struct proxy_id_ctx *ctx,

[SSSD] [PATCH] Temporarily mark subdomain as inactive instead marking the whole back end offline

2015-09-04 Thread Jakub Hrozek 
Hi,

the attached patches implement https://fedorahosted.org/sssd/ticket/2637

There are two main use-cases:
1) If AD DCs are not reachable on the IPA server itself, we should
avoid going offline completely, at least the IPA domain should be
still reachable.
2) If SSSD is connected to a non-root AD DC, we still try to contact
the forest root because only the forest root normally knows all the
subdomains. But in many setups, the forest root is not reachable
due to network restrictions.

The full design is described here:
https://fedorahosted.org/sssd/wiki/DesignDocs/OneWayTrusts#Subdomainofflinestatuschanges

There is one area that I'm not sure about myself -- in the
ad_subdomains.c changes, I always set ignore_mark_offline to true for
the root DC. I think it's safe because in this case the root domain is a
subdomain and we don't want a subdomain to allow marking the be as
offline, but I would like to ask that this change is double-checked.

I mostly checked by pausing AD DC VMs in my test setups and making sure
that the backend stays offline after lookup error, a subsequent lookup
is answered as if we were online on the backend side and that the
inactive status is reset. Also, main domain failures still must mark
sssd as offline.
>From cde1bf93e93003e10220b4c5e10e066dae06767d Mon Sep 17 00:00:00 2001
From: Jakub Hrozek 
Date: Tue, 18 Aug 2015 15:15:44 +
Subject: [PATCH 1/8] UTIL: Convert domain->disabled into tri-state with domain
 states

Required for:
https://fedorahosted.org/sssd/ticket/2637

This is a first step towards making it possible for domain to be around,
but not contacted by Data Provider.

Also explicitly create domains as enabled, previously we only relied on
talloc_zero marking dom->disabled as false.
---
 src/confdb/confdb.c  |  2 ++
 src/confdb/confdb.h  |  8 +++-
 src/db/sysdb_subdomains.c|  7 +--
 src/providers/ad/ad_subdomains.c |  2 +-
 src/providers/ipa/ipa_subdomains.c   |  2 +-
 src/responder/common/responder_common.c  |  5 +++--
 src/tests/cmocka/test_sysdb_subdomains.c |  6 +-
 src/tests/cmocka/test_utils.c|  6 +++---
 src/util/domain_info_utils.c | 17 ++---
 src/util/util.h  |  3 +++
 src/util/util_errors.c   |  1 +
 src/util/util_errors.h   |  1 +
 12 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 
3a8a1c01b92e62302ac4f787ccd085be9d8f05c3..a207ce3ba55eec50ba6392d14ccf835c6ba7b578
 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -1342,6 +1342,8 @@ static int confdb_get_domain_internal(struct confdb_ctx 
*cdb,
 domain->has_views = false;
 domain->view_name = NULL;
 
+domain->state = DOM_ENABLED;
+
 *_domain = domain;
 ret = EOK;
 done:
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index 
427c309a290dc9b02270311dacfcf308bd78430c..7456e0c6c226b3bc6ea3a64808fba0f732329265
 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -216,6 +216,12 @@
 struct confdb_ctx;
 struct config_file_ctx;
 
+enum sss_domain_state {
+DOM_ENABLED,
+DOM_DISABLED,
+DOM_INACTIVE,
+};
+
 /**
  * Data structure storing all of the basic features
  * of a domain.
@@ -278,7 +284,7 @@ struct sss_domain_info {
 struct sss_domain_info *prev;
 struct sss_domain_info *next;
 
-bool disabled;
+enum sss_domain_state state;
 char **sd_inherit;
 
 /* Do not use the forest pointer directly in new code, but rather the
diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c
index 
142520c1836d74ef7bc5c5269487b8971f261b88..ffcb4235cd9e3510eeedc28030332b88a4ead3be
 100644
--- a/src/db/sysdb_subdomains.c
+++ b/src/db/sysdb_subdomains.c
@@ -111,6 +111,8 @@ struct sss_domain_info *new_subdomain(TALLOC_CTX *mem_ctx,
 dom->enumerate = enumerate;
 dom->fqnames = true;
 dom->mpg = mpg;
+dom->state = DOM_ENABLED;
+
 /* If the parent domain filters out group members, the subdomain should
  * as well if configured */
 inherit_option = string_in_list(CONFDB_DOMAIN_IGNORE_GROUP_MEMBERS,
@@ -268,7 +270,7 @@ errno_t sysdb_update_subdomains(struct sss_domain_info 
*domain)
 /* disable all domains,
  * let the search result refresh any that are still valid */
 for (dom = domain->subdomains; dom; dom = get_next_domain(dom, false)) {
-dom->disabled = true;
+sss_domain_set_state(dom, DOM_DISABLED);
 }
 
 if (res->count == 0) {
@@ -312,7 +314,8 @@ errno_t sysdb_update_subdomains(struct sss_domain_info 
*domain)
 /* explicitly use dom->next as we need to check 'disabled' domains */
 for (dom = domain->subdomains; dom; dom = dom->next) {
 if (strcasecmp(dom->name, name) == 0) {
-dom->disabled = false;
+sss_domain_set_state(dom, DOM_ENABLED);
+

Re: [SSSD] I think #2637 is out of scope of 1.13

2015-09-04 Thread Jakub Hrozek 
On Wed, Sep 02, 2015 at 12:42:17PM +0200, Jakub Hrozek wrote:
> On Wed, Sep 02, 2015 at 10:11:46AM +0200, Sumit Bose wrote:
> > On Tue, Sep 01, 2015 at 08:19:06PM +0200, Jakub Hrozek wrote:
> > > Hi,
> > > 
> > > ssia...
> > > 
> > > I was working on implementing https://fedorahosted.org/sssd/ticket/2637
> > > and while for most occurences it was quite easy to set domain status
> > > instead of back end status, I hit the sdap_id_op module..
> > > 
> > > The sdap_id_op operation sets the offline status directly, in a bit of
> > > a hackish way by accessing the be_ctx through sdap_id_ctx. It's not
> > > trivial to change the request to operate on domains, except changing
> > > sdap_id_op_create() to also accept domain as a parameter..but we already
> > > have 37 calls to sdap_id_op_create().
> > > 
> > > A better way would be to change the whole sdap_id_op request to return an
> > > error code and don't touch the backend status, but that's even more work
> > > -- we need to do that and it's something we need to do in the next weeks
> > > anyway, I just think it's not something that should land in 1.13..
> > > 
> > > Thoughts? Should I go ahead and change sdap_id_op_create() to accept
> > > domain in 1.13 or do refactor the sdap_id_op for 1.14? I would
> > > personally prefer the latter..
> > 
> > I agree that sdap_id_op should not change the status on its own and that
> > it should be fixed with the refactoring planned for 1.14.
> > 
> > Nevertheless I wonder if you already have a patch which covers all the
> > other cases if it wouldn't be an improvement to the current state to add
> > it?
> > 
> > I would assume that most issue are caused by error during the DNS
> > lookup, initial connect or authentication which iirc are not handled by
> > sdap_id_op.
> 
> The LDAP connection and the failover during the connection is handled by
> sdap_id_op. That's the crux of the problem I didn't realize when doing
> the initial design (frankly I didn't think the sdap operation was in the
> business of changing failover status and offline status for the backend..)
> 
> > I would assume that error during sdap_id_op, e.g. a server
> > shutdown, are more rare and it would be acceptable to let those errors
> > switch the whole backend to offline until the refactoring is done.
> 
> Hmm perhaps you're right that we can try solve at least the use-cases
> while we have the code in mind. I think we mostly care about:
> - don't bring the whole AD backend offline if there are connection
>   problems with discovering subdomains when AD client is connected
>   to a subdomain
> - don't bring the whole IPA backend offline if there are connection
>   problems with AD server in server mode
> - don't bring the while IPA backend offline if there are connection
>   problems with AD server with IPA client
> 
> I think these are solvable without major hacks. I'll see about fixing
> these use-cases.
> 
> The rest needs to be fixed when I work on the LDAP code and Pavel
> Brezina works on the failover changes.

The patches are on list now. Given the changes, I no longer think the
patches are out of scope for 1.13.x -- downstreams would really benefit
from them -- but I don't think we should block 1.13.1 over the patches
either, we should rather test them out well.

FreeIPA would like us to release 1.13.1 on Monday, so I'll concentrate
on reviewing your patch for #2692 and respinning the patch for #2723 so
that we can do the release on Mon.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] Fix #2275 nested netgroups do not work in IPA provider

2015-09-04 Thread Petr Cech 

On 09/04/2015 03:24 PM, Petr Cech wrote:

On 09/03/2015 03:45 PM, Sumit Bose wrote:

I tried both case. I used only originalMemberOf and I had right
hostgroups,
>no user groups. Then I used only memberOf and I had no hostgroups,
right
>user groups.
>
>So I did little hack, we could use both memberOf. The patch is
attached and
>it works for me.

Hi Petr,

thank you for the patch I haven't tested it yet. But I think I now
understand the issue better. Currently we store the originalMemberOf
attribute for users and hosts but not for POSIX/user groups (we do not
even read it from LDAP). So an alternative fix might be to add memberOf
attribute to the list of attribute read from LDAP for POSIX groups and
save the result in originalMemberOf in the cache. The using only
originalMemberOf should be sufficient for the netgroups lookup.

Would you mind to try this? For a test is shoult de sufficient to add a
line like

 { "ldap_group_member_of", "memberOf", SYSDB_MEMBEROF, NULL }

to all 'struct sdap_attr_map *_group_map[]' lists and a corresponding
entry to 'enum sdap_group_attrs'.

bye,
Sumit



Hello Sumit,

I tried your alternative way (thanks for it). Patch is attached.
I added some lines like:
#  { "ldap_user_member_of", "memberOf", SYSDB_ORIG_MEMBEROF, NULL }
and it works for me.

I hope that meaning of this patch is saving user/POSIX group memberOf
attribute to originalMemberOf attribute.

Regards,

Petr

And there is version with ticket number.



___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

>From 0207fbc11e56efea8796b88e8fa449f82c4628fe Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Fri, 4 Sep 2015 09:09:25 -0400
Subject: [PATCH] IPA PROVIDER: Resolve nested netgroup membership

Informations about posix/user group membership are stored in memberOf
attribute. And informations about hostgroup membership are stored
in originalMemberOf.
Netgroup membership process looks only into originalMemberOf.
This patch adds saving of posix/user group memberOf attribute to
originalMemberOf storage.

Resolves:
https://fedorahosted.org/sssd/ticket/2275
---
 src/providers/ad/ad_opts.h | 1 +
 src/providers/ipa/ipa_opts.h   | 1 +
 src/providers/ldap/ldap_opts.h | 3 +++
 src/providers/ldap/sdap.h  | 1 +
 4 files changed, 6 insertions(+)

diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h
index 00586a7ada63ad4c89630e9589d3ff75d1726703..7917e8fc5e60ed27e7ed1248550d1e65d2d159d2 100644
--- a/src/providers/ad/ad_opts.h
+++ b/src/providers/ad/ad_opts.h
@@ -192,6 +192,7 @@ struct sdap_attr_map ad_2008r2_user_map[] = {
 { "ldap_user_principal", "userPrincipalName", SYSDB_UPN, NULL },
 { "ldap_user_fullname", "name", SYSDB_FULLNAME, NULL },
 { "ldap_user_member_of", "memberOf", SYSDB_MEMBEROF, NULL },
+{ "ldap_user_member_of", "memberOf", SYSDB_ORIG_MEMBEROF, NULL },
 { "ldap_user_uuid", "objectGUID", SYSDB_UUID, NULL },
 { "ldap_user_objectsid", "objectSID", SYSDB_SID, NULL },
 { "ldap_user_primary_group", "primaryGroupID", SYSDB_PRIMARY_GROUP, NULL },
diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h
index 78949e3ddec95f7f4303eab905bbbf6ec14ed6ae..9b5fdd138fbdf09f3d3662c011ea792f6272b7a6 100644
--- a/src/providers/ipa/ipa_opts.h
+++ b/src/providers/ipa/ipa_opts.h
@@ -180,6 +180,7 @@ struct sdap_attr_map ipa_user_map[] = {
 { "ldap_user_principal", "krbPrincipalName", SYSDB_UPN, NULL },
 { "ldap_user_fullname", "cn", SYSDB_FULLNAME, NULL },
 { "ldap_user_member_of", "memberOf", SYSDB_MEMBEROF, NULL },
+{ "ldap_user_member_of", "memberOf", SYSDB_ORIG_MEMBEROF, NULL },
 { "ldap_user_uuid", "ipaUniqueID", SYSDB_UUID, NULL },
 { "ldap_user_objectsid", "ipaNTSecurityIdentifier", SYSDB_SID_STR, NULL },
 { "ldap_user_primary_group", NULL, SYSDB_PRIMARY_GROUP, NULL },
diff --git a/src/providers/ldap/ldap_opts.h b/src/providers/ldap/ldap_opts.h
index 9f58db5bd9eef1391e97c1890cbff94c2a5406d6..db7bc560f430331462470b2825f6319dbaaf9141 100644
--- a/src/providers/ldap/ldap_opts.h
+++ b/src/providers/ldap/ldap_opts.h
@@ -156,6 +156,7 @@ struct sdap_attr_map rfc2307_user_map[] = {
 { "ldap_user_principal", "krbPrincipalName", SYSDB_UPN, NULL },
 { "ldap_user_fullname", "cn", SYSDB_FULLNAME, NULL },
 { "ldap_user_member_of", NULL, SYSDB_MEMBEROF, NULL },
+{ "ldap_user_member_of", NULL, SYSDB_ORIG_MEMBEROF, NULL },
 { "ldap_user_uuid", NULL, SYSDB_UUID, NULL },
 { "ldap_user_objectsid", NULL, SYSDB_SID, NULL },
 { "ldap_user_primary_group", NULL, SYSDB_PRIMARY_GROUP, NULL },
@@ -212,6 +213,7 @@ struct sdap_attr_map rfc2307bis_user_map[] = {
 { "ldap_user_principal", "krbPrincipalName", SYSDB_UPN, NULL },
 { "ldap_user_fullname", "cn", SYSDB_FULLNAME, NULL },
 { "ldap_user_member_of", "memberOf", SYSDB_MEMBEROF, NULL },
+{ "ldap_user_member_of", "memberOf",

Re: [SSSD] [PATCH] [HBAC]: Better libhbac debuging

2015-09-04 Thread Pavel Reichl 

All my concerns were addressed.
CI passed: http://sssd-ci.duckdns.org/logs/job/24/37/summary.html
Code is fine by me and my testing passed.

ACK

I think that this patch will be a real improvement for debugging HBAC issues, 
thanks Petr.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] I think #2637 is out of scope of 1.13

2015-09-04 Thread Jakub Hrozek 
On Fri, Sep 04, 2015 at 04:17:54PM +0200, Jakub Hrozek wrote:
> > I would say more realistic is in the middle of next week.

btw we need to do the release on Monday otherwise we can't rebase for
Fedora 23 beta, sorry.

> > 
> > tbabej found some issues with authenticating a newly created user
> > to FreeIPA 4.2.
> 
> Do we have a ticket already?

I just tested the simplest scenario and it works OK to me, so we need a
ticket with logs or even better accesss to a machine where we can see
the bug.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] SDAP: Remove unused function

2015-09-04 Thread Pavel Reichl 



On 09/02/2015 04:21 PM, Pavel Reichl wrote:


On 09/02/2015 04:16 PM, Jakub Hrozek wrote:

Unused code is broken code, remove it :)



___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


Should we improve comment in remove_ldap_connection_callbacks() which mentions 
sdap_mark_offline()?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


OK, I don't want to stall the patch.

CI passed (with exception of unrelated issues on rawhide):
http://sssd-ci.duckdns.org/logs/job/24/38/summary.html

ACK
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] Fix #2275 nested netgroups do not work in IPA provider

2015-09-04 Thread Petr Cech 

On 09/03/2015 03:45 PM, Sumit Bose wrote:

I tried both case. I used only originalMemberOf and I had right hostgroups,
>no user groups. Then I used only memberOf and I had no hostgroups, right
>user groups.
>
>So I did little hack, we could use both memberOf. The patch is attached and
>it works for me.

Hi Petr,

thank you for the patch I haven't tested it yet. But I think I now
understand the issue better. Currently we store the originalMemberOf
attribute for users and hosts but not for POSIX/user groups (we do not
even read it from LDAP). So an alternative fix might be to add memberOf
attribute to the list of attribute read from LDAP for POSIX groups and
save the result in originalMemberOf in the cache. The using only
originalMemberOf should be sufficient for the netgroups lookup.

Would you mind to try this? For a test is shoult de sufficient to add a
line like

 { "ldap_group_member_of", "memberOf", SYSDB_MEMBEROF, NULL }

to all 'struct sdap_attr_map *_group_map[]' lists and a corresponding
entry to 'enum sdap_group_attrs'.

bye,
Sumit



Hello Sumit,

I tried your alternative way (thanks for it). Patch is attached.
I added some lines like:
#  { "ldap_user_member_of", "memberOf", SYSDB_ORIG_MEMBEROF, NULL }
and it works for me.

I hope that meaning of this patch is saving user/POSIX group memberOf 
attribute to originalMemberOf attribute.


Regards,

Petr
>From a5b0e35de6a9cb6e9e1881deaae9fa55701aa33a Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Fri, 4 Sep 2015 09:09:25 -0400
Subject: [PATCH] IPA PROVIDER: Resolve nested netgroup membership

Informations about posix/user group membership are stored in memberOf
attribute. And informations about hostgroup membership are stored
in originalMemberOf.
Netgroup membership process looks only into originalMemberOf.
This patch adds saving of posix/user group memberOf attribute to
originalMemberOf storage.

Resolves:
https://fedorahosted.org/sssd/ticket/
---
 src/providers/ad/ad_opts.h | 1 +
 src/providers/ipa/ipa_opts.h   | 1 +
 src/providers/ldap/ldap_opts.h | 3 +++
 src/providers/ldap/sdap.h  | 1 +
 4 files changed, 6 insertions(+)

diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h
index 00586a7ada63ad4c89630e9589d3ff75d1726703..7917e8fc5e60ed27e7ed1248550d1e65d2d159d2 100644
--- a/src/providers/ad/ad_opts.h
+++ b/src/providers/ad/ad_opts.h
@@ -192,6 +192,7 @@ struct sdap_attr_map ad_2008r2_user_map[] = {
 { "ldap_user_principal", "userPrincipalName", SYSDB_UPN, NULL },
 { "ldap_user_fullname", "name", SYSDB_FULLNAME, NULL },
 { "ldap_user_member_of", "memberOf", SYSDB_MEMBEROF, NULL },
+{ "ldap_user_member_of", "memberOf", SYSDB_ORIG_MEMBEROF, NULL },
 { "ldap_user_uuid", "objectGUID", SYSDB_UUID, NULL },
 { "ldap_user_objectsid", "objectSID", SYSDB_SID, NULL },
 { "ldap_user_primary_group", "primaryGroupID", SYSDB_PRIMARY_GROUP, NULL },
diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h
index 78949e3ddec95f7f4303eab905bbbf6ec14ed6ae..9b5fdd138fbdf09f3d3662c011ea792f6272b7a6 100644
--- a/src/providers/ipa/ipa_opts.h
+++ b/src/providers/ipa/ipa_opts.h
@@ -180,6 +180,7 @@ struct sdap_attr_map ipa_user_map[] = {
 { "ldap_user_principal", "krbPrincipalName", SYSDB_UPN, NULL },
 { "ldap_user_fullname", "cn", SYSDB_FULLNAME, NULL },
 { "ldap_user_member_of", "memberOf", SYSDB_MEMBEROF, NULL },
+{ "ldap_user_member_of", "memberOf", SYSDB_ORIG_MEMBEROF, NULL },
 { "ldap_user_uuid", "ipaUniqueID", SYSDB_UUID, NULL },
 { "ldap_user_objectsid", "ipaNTSecurityIdentifier", SYSDB_SID_STR, NULL },
 { "ldap_user_primary_group", NULL, SYSDB_PRIMARY_GROUP, NULL },
diff --git a/src/providers/ldap/ldap_opts.h b/src/providers/ldap/ldap_opts.h
index 9f58db5bd9eef1391e97c1890cbff94c2a5406d6..db7bc560f430331462470b2825f6319dbaaf9141 100644
--- a/src/providers/ldap/ldap_opts.h
+++ b/src/providers/ldap/ldap_opts.h
@@ -156,6 +156,7 @@ struct sdap_attr_map rfc2307_user_map[] = {
 { "ldap_user_principal", "krbPrincipalName", SYSDB_UPN, NULL },
 { "ldap_user_fullname", "cn", SYSDB_FULLNAME, NULL },
 { "ldap_user_member_of", NULL, SYSDB_MEMBEROF, NULL },
+{ "ldap_user_member_of", NULL, SYSDB_ORIG_MEMBEROF, NULL },
 { "ldap_user_uuid", NULL, SYSDB_UUID, NULL },
 { "ldap_user_objectsid", NULL, SYSDB_SID, NULL },
 { "ldap_user_primary_group", NULL, SYSDB_PRIMARY_GROUP, NULL },
@@ -212,6 +213,7 @@ struct sdap_attr_map rfc2307bis_user_map[] = {
 { "ldap_user_principal", "krbPrincipalName", SYSDB_UPN, NULL },
 { "ldap_user_fullname", "cn", SYSDB_FULLNAME, NULL },
 { "ldap_user_member_of", "memberOf", SYSDB_MEMBEROF, NULL },
+{ "ldap_user_member_of", "memberOf", SYSDB_ORIG_MEMBEROF, NULL },
 { "ldap_user_uuid", NULL, SYSDB_UUID, NULL },
 { "ldap_user_objectsid", NULL, SYSDB_SID, NULL },
 { "ldap_user_primary_group", NULL, SYSDB_PRIMARY_GROUP, NULL },
@@ -268,6 +270,7 @@ struct sdap_attr_map

Re: [SSSD] I think #2637 is out of scope of 1.13

2015-09-04 Thread Lukas Slebodnik 
On (04/09/15 14:38), Jakub Hrozek wrote:
>On Wed, Sep 02, 2015 at 12:42:17PM +0200, Jakub Hrozek wrote:
>> On Wed, Sep 02, 2015 at 10:11:46AM +0200, Sumit Bose wrote:
>> > On Tue, Sep 01, 2015 at 08:19:06PM +0200, Jakub Hrozek wrote:
>> > > Hi,
>> > > 
>> > > ssia...
>> > > 
>> > > I was working on implementing https://fedorahosted.org/sssd/ticket/2637
>> > > and while for most occurences it was quite easy to set domain status
>> > > instead of back end status, I hit the sdap_id_op module..
>> > > 
>> > > The sdap_id_op operation sets the offline status directly, in a bit of
>> > > a hackish way by accessing the be_ctx through sdap_id_ctx. It's not
>> > > trivial to change the request to operate on domains, except changing
>> > > sdap_id_op_create() to also accept domain as a parameter..but we already
>> > > have 37 calls to sdap_id_op_create().
>> > > 
>> > > A better way would be to change the whole sdap_id_op request to return an
>> > > error code and don't touch the backend status, but that's even more work
>> > > -- we need to do that and it's something we need to do in the next weeks
>> > > anyway, I just think it's not something that should land in 1.13..
>> > > 
>> > > Thoughts? Should I go ahead and change sdap_id_op_create() to accept
>> > > domain in 1.13 or do refactor the sdap_id_op for 1.14? I would
>> > > personally prefer the latter..
>> > 
>> > I agree that sdap_id_op should not change the status on its own and that
>> > it should be fixed with the refactoring planned for 1.14.
>> > 
>> > Nevertheless I wonder if you already have a patch which covers all the
>> > other cases if it wouldn't be an improvement to the current state to add
>> > it?
>> > 
>> > I would assume that most issue are caused by error during the DNS
>> > lookup, initial connect or authentication which iirc are not handled by
>> > sdap_id_op.
>> 
>> The LDAP connection and the failover during the connection is handled by
>> sdap_id_op. That's the crux of the problem I didn't realize when doing
>> the initial design (frankly I didn't think the sdap operation was in the
>> business of changing failover status and offline status for the backend..)
>> 
>> > I would assume that error during sdap_id_op, e.g. a server
>> > shutdown, are more rare and it would be acceptable to let those errors
>> > switch the whole backend to offline until the refactoring is done.
>> 
>> Hmm perhaps you're right that we can try solve at least the use-cases
>> while we have the code in mind. I think we mostly care about:
>> - don't bring the whole AD backend offline if there are connection
>>   problems with discovering subdomains when AD client is connected
>>   to a subdomain
>> - don't bring the whole IPA backend offline if there are connection
>>   problems with AD server in server mode
>> - don't bring the while IPA backend offline if there are connection
>>   problems with AD server with IPA client
>> 
>> I think these are solvable without major hacks. I'll see about fixing
>> these use-cases.
>> 
>> The rest needs to be fixed when I work on the LDAP code and Pavel
>> Brezina works on the failover changes.
>
>The patches are on list now. Given the changes, I no longer think the
>patches are out of scope for 1.13.x -- downstreams would really benefit
>from them -- but I don't think we should block 1.13.1 over the patches
>either, we should rather test them out well.
>
>FreeIPA would like us to release 1.13.1 on Monday, so I'll concentrate
>on reviewing your patch for #2692 and respinning the patch for #2723 so
>that we can do the release on Mon.
I would say more realistic is in the middle of next week.

tbabej found some issues with authenticating a newly created user
to FreeIPA 4.2.

sssd-1.13.0 works fine; there is just a problem with sssd master

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] DATA_PROVIDER: BE_REQ as string in log message

2015-09-04 Thread Pavel Reichl 

On 08/28/2015 04:31 PM, Petr Cech wrote:

+  "Got request for [%#x][%s][%d][%s]\n", type, be_req2str(type),
+  attr_type, filter);


Petr do you think it could be useful to print attr_type as a string?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] I think #2637 is out of scope of 1.13

2015-09-04 Thread Lukas Slebodnik 
On (04/09/15 16:22), Jakub Hrozek wrote:
>On Fri, Sep 04, 2015 at 04:17:54PM +0200, Jakub Hrozek wrote:
>> > I would say more realistic is in the middle of next week.
>
>btw we need to do the release on Monday otherwise we can't rebase for
>Fedora 23 beta, sorry.
>
I would rather have stable version in beta
rather then broken version.

I do not see a reason why FreeIPA should require 1.13.1

>> > 
>> > tbabej found some issues with authenticating a newly created user
>> > to FreeIPA 4.2.
>> 
>> Do we have a ticket already?
>
>I just tested the simplest scenario and it works OK to me, so we need a
>ticket with logs or even better accesss to a machine where we can see
>the bug.
I asked him to file a ticket.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] I think #2637 is out of scope of 1.13

2015-09-04 Thread Jakub Hrozek 
On Fri, Sep 04, 2015 at 04:34:35PM +0200, Lukas Slebodnik wrote:
> On (04/09/15 16:22), Jakub Hrozek wrote:
> >On Fri, Sep 04, 2015 at 04:17:54PM +0200, Jakub Hrozek wrote:
> >> > I would say more realistic is in the middle of next week.
> >
> >btw we need to do the release on Monday otherwise we can't rebase for
> >Fedora 23 beta, sorry.
> >
> I would rather have stable version in beta
> rather then broken version.

yes..

> 
> I do not see a reason why FreeIPA should require 1.13.1

OK,then we need to talk to freeipa before they tag the new version. We
can sit down on Monday and look at the fixes to backport.

> 
> >> > 
> >> > tbabej found some issues with authenticating a newly created user
> >> > to FreeIPA 4.2.
> >> 
> >> Do we have a ticket already?
> >
> >I just tested the simplest scenario and it works OK to me, so we need a
> >ticket with logs or even better accesss to a machine where we can see
> >the bug.
> I asked him to file a ticket.
> 
> LS
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel