Re: [SSSD] [PATCH] INIT: Drop syslog.target from service file

2015-11-10 Thread Lukas Slebodnik 
On (10/11/15 16:23), Jakub Hrozek wrote:
>On Tue, Nov 10, 2015 at 09:43:55AM +0100, Lukas Slebodnik wrote:
>> ehlo,
>> 
>> debian has pach which removes syslog.target from service.file
>> http://anonscm.debian.org/cgit/pkg-sssd/sssd.git/tree/debian/patches/fix-obsolete-target.diff
>> and this target is not available on el7.1with quite old systemd.
>> 
>> I had a small discussion with systemd guys and we can drop
>> it without any problem. It should work out-of-the-box.
>> 
>> LS
>
>As a matter of fact, I discussed the same with the rsyslogd Red Hat
>maintainer, so I agree with the change.
>
>ACK

This part cannot be tested by CI.
So we can omit CI link.

master:
* 46ef3da071401904a8c4930df4f2b1103c309c25

pushed also to stable branch. So downstream(s) can drop custom patch
sssd-1-13:
* d68024ffdf47e355c76d6113a38c7080f30b6e88

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

Re: [SSSD] [PATCH] BE: Add IFP to known clients

2015-11-10 Thread Jakub Hrozek 
On Tue, Nov 10, 2015 at 02:50:57PM +0100, Lukas Slebodnik wrote:
> On (10/11/15 12:10), Pavel Březina wrote:
> >This gets rid of confusing debug message:
> >[be_client_destructor] (0x0020): Unknown client removed ...
> 
> >From 17b1d8216bab3770c58c79cf51c571cb184e8ab4 Mon Sep 17 00:00:00 2001
> >From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> >Date: Tue, 10 Nov 2015 11:47:30 +0100
> >Subject: [PATCH] BE: Add IFP to known clients
> >
> >This gets rid of confusing debug message:
> >[be_client_destructor] (0x0020): Unknown client removed ...
> >---
> > src/providers/data_provider_be.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> >diff --git a/src/providers/data_provider_be.c 
> >b/src/providers/data_provider_be.c
> >index 
> >effa185f905097e0be1e03596b5fc6da75a7e382..104016ca19c093dcbb4b0c1011f06e4bb4e2a49c
> > 100644
> >--- a/src/providers/data_provider_be.c
> >+++ b/src/providers/data_provider_be.c
> >@@ -2107,6 +2107,9 @@ static int be_client_destructor(void *ctx)
> > } else if (becli->bectx->pac_cli == becli) {
> > DEBUG(SSSDBG_TRACE_FUNC, "Removed PAC client\n");
> > becli->bectx->pac_cli = NULL;
> >+} else if (becli->bectx->ifp_cli == becli) {
> >+DEBUG(SSSDBG_TRACE_FUNC, "Removed IFP client\n");
> >+becli->bectx->ifp_cli = NULL;
> > } else {
> > DEBUG(SSSDBG_CRIT_FAILURE, "Unknown client removed ...\n");
> > }
> 
> ACK
> http://sssd-ci.duckdns.org/logs/job/32/41/
> 
> The simples reproducer is
>   pkill -9 sssD_ifp
> 
> LS

* master: b2d7301516a8a6ca69e38999170da8a0ecb2bdba
* sssd-1-13: ac7b15daccebe3a088ebef490ad1fed89d107e5b (It's better to
  emit loud warnings in the stable branch)
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] INIT: Drop syslog.target from service file

2015-11-10 Thread Jakub Hrozek 
On Tue, Nov 10, 2015 at 09:43:55AM +0100, Lukas Slebodnik wrote:
> ehlo,
> 
> debian has pach which removes syslog.target from service.file
> http://anonscm.debian.org/cgit/pkg-sssd/sssd.git/tree/debian/patches/fix-obsolete-target.diff
> and this target is not available on el7.1with quite old systemd.
> 
> I had a small discussion with systemd guys and we can drop
> it without any problem. It should work out-of-the-box.
> 
> LS

As a matter of fact, I discussed the same with the rsyslogd Red Hat
maintainer, so I agree with the change.

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

Re: [SSSD] [PATCH] SSSD: Add a new command diag_cmd

2015-11-10 Thread Jakub Hrozek 
On Tue, Nov 10, 2015 at 01:22:54PM +0100, Lukas Slebodnik wrote:
> On (10/11/15 13:15), Jakub Hrozek wrote:
> >On Mon, Nov 09, 2015 at 11:32:30AM +0100, Petr Cech wrote:
> >> On 11/04/2015 11:24 AM, Jakub Hrozek wrote:
> >> >Hi,
> >> >
> >> >I created this patch to try to diagnose an issue where sssd would
> >> >randomly restart on any of machines in a VM cluster without giving too
> >> >much advise why. I think it might be useful to merge in general.
> >> 
> >> Hi Jakub,
> >> 
> >> I reviewed the patch. Code looks good to me.
> >> CI tests passed: http://sssd-ci.duckdns.org/logs/job/32/25/summary.html
> >> 
> >> Then I tried to test new functionality.
> >> 
> >> Man pages are right, I found diag_cmd in sssd.conf.
> >> 
> >> And I really got the right message when I kill sss_pam:
> >> # (Mon Nov  9 04:30:47 2015) [sssd] [svc_child_info] (0x0040): Child 
> >> [25767]
> >> terminated with signal [9]
> >> 
> >> I would like to see output of pstack, but I don't know, how to get the 
> >> right
> >> state of SSSD. Can you help me, please?
> >
> >I tested the patch by setting a low 'timeout' in the 'domain' section
> >and then setting the diag_cmd:
> >[domain/foo]
> >timeout = 2
> >diag_cmd = pstack %p
> >
> >then I stopped the back end:
> ># kill -STOP $(pidof sssd_be)
> >
> >You should see the pstack output in /var/log/sssd/sssd.log, also the
> >debug_level must be increased in the [sssd] section. You might also need
> >to set SELinux to Permissive, otherwise sssd might not be able to fork
> >an exec pstack..
> So in this case I would prefer if this opton was not documented.
> or it should be documented issues with SELinux
> 

Then I would prefer undocumented. It matches how we (don't) document the
"command" option.

A new patch is attached.
>From fb1b8c5fd9fbec475c036563640d7e320d526620 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek 
Date: Mon, 2 Nov 2015 11:41:31 +0100
Subject: [PATCH] SSSD: Add a new command diag_cmd

This command is an optional one that is run when a sbus ping times out
and before a SIGKILL commans is sent.

This command supports a single template substitution that expands to the
PID of the service being signaled.
---
 src/confdb/confdb.h  |   1 +
 src/config/SSSDConfig/__init__.py.in |   1 +
 src/config/SSSDConfigTest.py |   1 +
 src/config/etc/sssd.api.conf |   1 +
 src/monitor/monitor.c| 215 +++
 5 files changed, 197 insertions(+), 22 deletions(-)

diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index 
37b5fd7c7629e2618a1699e3ffd58110171db605..0ef7268f9cdc2c18482bbf7b8dbe19d3ef6b7bbf
 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -71,6 +71,7 @@
 #define CONFDB_MONITOR_DEFAULT_DOMAIN "default_domain_suffix"
 #define CONFDB_MONITOR_OVERRIDE_SPACE "override_space"
 #define CONFDB_MONITOR_USER_RUNAS "user"
+#define CONFDB_MONITOR_PRE_KILL_CMD "diag_cmd"
 
 /* Both monitor and domains */
 #define CONFDB_NAME_REGEX   "re_expression"
diff --git a/src/config/SSSDConfig/__init__.py.in 
b/src/config/SSSDConfig/__init__.py.in
index 
bf61c402796122050fa43cf41128faec4771c5d2..60129e6e7fbc96d11c539323346c22a7db6d7f23
 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -50,6 +50,7 @@ option_strings = {
 'reconnection_retries' : _('Number of times to attempt connection to Data 
Providers'),
 'fd_limit' : _('The number of file descriptors that may be opened by this 
responder'),
 'client_idle_timeout' : _('Idle time before automatic disconnection of a 
client'),
+'diag_cmd' : _('The command to run when a service ping times out'),
 
 # [sssd]
 'services' : _('SSSD Services to start'),
diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
index 
45562214da5d227b45914abbcb298e043048adf5..abd4a39258e060f27db62eb2352450b6c405930c
 100755
--- a/src/config/SSSDConfigTest.py
+++ b/src/config/SSSDConfigTest.py
@@ -307,6 +307,7 @@ class SSSDConfigTestSSSDService(unittest.TestCase):
 'reconnection_retries',
 'fd_limit',
 'client_idle_timeout',
+'diag_cmd',
 'description']
 
 self.assertTrue(type(options) == dict,
diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
index 
72abb8b3f427e9bcf3cf3dcaa67aaf4e3e50226c..0c03625bd5f0d3fbf8948971b015d4e8255f0009
 100644
--- a/src/config/etc/sssd.api.conf
+++ b/src/config/etc/sssd.api.conf
@@ -13,6 +13,7 @@ fd_limit = int, None, false
 client_idle_timeout = int, None, false
 force_timeout = int, None, false
 description = str, None, false
+diag_cmd = str, None, false
 
 [sssd]
 # Monitor service
diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index 
89ac882d38652e071fdc619495c3252807527da0..5ff5f85527a6aa91e220ddd07e8c8738dba0e829
 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -116,6 +116,7 @@ struct mt_svc {
 char *identity;

Re: [SSSD] [PATCH] TESTS: extend PAM responder unit test

2015-11-10 Thread Lukas Slebodnik 
On (05/11/15 17:01), Pavel Reichl wrote:
>On 11/05/2015 09:17 AM, Lukas Slebodnik wrote:
>>Let's image following use case:
>>* cached authentication is enabled.
>>* user "pamuser" has never authenticated to the machine and thus
>>   password is not cached
>>* for the first time the the data provider should be contacted.
>>   ( storing cached password is done in DP, so you still need to
>> call sysdb_cache_password)
>>* cached authentication shoudl be used for second following authentication.
>>   and attribute lastOnlineAuthWithCurrentToken should be set be 1st
>>   authentication so you needn't call 
>> pam_set_last_online_auth_with_curr_token.
>>
>>This test-case would test that attribute lastOnlineAuthWithCurrentToken is
>>properly set.
>>
>>Is it clear?
>
>I hope so, thanks.
>
>Updated patches attached.

>From 931190aa5faca0833fe285c6815ab955fd706f2d Mon Sep 17 00:00:00 2001
>From: Pavel Reichl 
>Date: Tue, 20 Oct 2015 08:07:02 -0400
>Subject: [PATCH 1/2] pam-srv-tests: split pam_test_setup() so it can be reused
>
>Split pam_test_setup() so domain and pam parameters can be easily set
>distinctly for each test.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2697
>---
ACK

>From c9d24e577470f3b459c79f7653e7866754266a57 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl 
>Date: Tue, 20 Oct 2015 09:10:30 -0400
>Subject: [PATCH 2/2] pam-srv-tests: Add UT for cached 'online' auth.
>
>Extend PAM responder unit test to check 'online' cached authentication.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2697
>---
> src/responder/pam/pamsrv.h  |   5 ++
> src/responder/pam/pamsrv_cmd.c  |   2 +-
> src/tests/cmocka/test_pam_srv.c | 195 
> 3 files changed, 201 insertions(+), 1 deletion(-)
>
>diff --git a/src/responder/pam/pamsrv.h b/src/responder/pam/pamsrv.h
>index 
>59831f2e73f923053e53cad838fac715330546dd..64a7d85735ac3eb20e4504899916a49a280d4959
> 100644
>--- a/src/responder/pam/pamsrv.h
>+++ b/src/responder/pam/pamsrv.h
>@@ -95,4 +95,9 @@ errno_t add_pam_cert_response(struct pam_data *pd, const 
>char *user,
>   const char *token_name);
> 
> bool may_do_cert_auth(struct pam_ctx *pctx, struct pam_data *pd);
>+
>+errno_t
>+pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>+ const char *username,
>+ uint64_t value);
> #endif /* __PAMSRV_H__ */
>diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
>index 
>4bb3e27b102584656e32546f9ad0035750eaeb06..80095cc0bf1c4c92dd04344c069b666ab76a3714
> 100644
>--- a/src/responder/pam/pamsrv_cmd.c
>+++ b/src/responder/pam/pamsrv_cmd.c
>@@ -1925,7 +1925,7 @@ struct sss_cmd_table *get_pam_cmds(void)
> return sss_cmds;
> }
> 
>-static errno_t
>+errno_t
> pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>  const char *username,
>  uint64_t value)
>diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c
>index 
>30fbbc6eaafa7828afd7c37d5bbb581851e5bd97..8a8aa4a7e472242a7b640dd5772d6bbf39640a04
> 100644
>--- a/src/tests/cmocka/test_pam_srv.c
>+++ b/src/tests/cmocka/test_pam_srv.c
>@@ -71,6 +71,9 @@
> "zotpoBIZmdH+ipYsu58HohHVlM9Wi5H4QmiiXl+Soldkq7eXYlafcmT7wv8+cKwz" \
> "Nz0Tm3+eYpFqRo3skr6QzXi525Jkg3r6r+kkhxU=" \
> 
>+#define CACHED_AUTH_TIMEOUT_STR "2"
>+#define CACHED_AUTH_TIMEOUT 2
It's more type safe to use constants.
static char CACHED_AUTH_TIMEOUT_STR[] = "2";
static const int CACHED_AUTH_TIMEOUT = 2;

>+
> struct pam_test_ctx {
> struct sss_test_ctx *tctx;
> struct sss_domain_info *subdom;
>@@ -82,6 +85,7 @@ struct pam_test_ctx {
> 
> int ncache_hits;
> int exp_pam_status;
>+bool provider_contacted;
> };
> 
> /* Must be global because it is needed in some wrappers */
>@@ -301,6 +305,26 @@ static int pam_test_setup(void **state)
> return 0;
> }
> 
>+static int pam_cached_test_setup(void **state)
>+{
>+struct sss_test_conf_param dom_params[] = {
>+{ "enumerate", "false" },
>+{ "cache_credentials", "true" },
>+{ "cached_auth_timeout", CACHED_AUTH_TIMEOUT_STR },
>+{ NULL, NULL }, /* Sentinel */
>+};
>+
>+struct sss_test_conf_param pam_params[] = {
>+{ "p11_child_timeout", "30"},
 ^
 missing space

>+{ NULL, NULL }, /* Sentinel */
>+};
>+
>+test_pam_setup(dom_params, pam_params, state);
>+
>+pam_test_setup_common();
>+return 0;
>+}
>+
> static int pam_test_teardown(void **state)
> {
> int ret;
>@@ -383,6 +407,7 @@ static void set_cmd_cb(cmd_cb_fn_t fn)
> 
> int __wrap_pam_dp_send_req(struct pam_auth_req *preq, int timeout)
> {
>+pam_test_ctx->provider_contacted = true;
> 
> /* Set expected status */
>

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-10 Thread Petr Cech 

On 11/10/2015 08:29 AM, Pavel Reichl wrote:



On 11/05/2015 05:29 PM, Petr Cech wrote:

+void test_groups_by_recent_filter_valid(void **state)
+{
+struct cache_req_test_ctx *test_ctx = NULL;
+TALLOC_CTX *req_mem_ctx = NULL;
+struct tevent_req *req = NULL;
+const char **group_names = NULL;
+const char **ldb_results = NULL;
+const char *ldbname = NULL;
+void *tmp_ctx = NULL;

Could you use TALLOC_CTX?

Yes, I could :-)



+errno_t ret;
+
+test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
+test_ctx->create_group1 = true;
+test_ctx->create_group2 = true;
+
+ret = sysdb_store_group(test_ctx->tctx->dom, TEST_GROUP_NAME2,
+1001, NULL, 1001, time(NULL));
+assert_int_equal(ret, EOK);
+
+sleep(1);
+
+req_mem_ctx = talloc_new(global_talloc_context);
+check_leaks_push(req_mem_ctx);
+
+/* Filters always go to DP */
+will_return(__wrap_sss_dp_get_account_send, test_ctx);
+mock_account_recv_simple();
+
+/* Group TEST_GROUP1 and TEST_GROUP2 are created with a DP
callback. */
+req = cache_req_group_by_filter_send(req_mem_ctx,
test_ctx->tctx->ev,
+ test_ctx->rctx,
+ test_ctx->tctx->dom->name,
+ TEST_USER_PREFIX);
+assert_non_null(req);
+
+tevent_req_set_callback(req, cache_req_group_by_filter_test_done,
test_ctx);
+
+ret = test_ev_loop(test_ctx->tctx);
+assert_int_equal(ret, ERR_OK);
+assert_true(check_leaks_pop(req_mem_ctx));
+
+assert_non_null(test_ctx->result);
+assert_int_equal(test_ctx->result->count, 2);
+
+tmp_ctx = talloc_zero(NULL, void *);


Why not to use talloc_new(parent_ctx)?


+
+group_names = talloc_array(tmp_ctx, const char *, 2);
+assert_non_null(group_names);
+group_names[0] = TEST_GROUP_NAME;
+group_names[1] = TEST_GROUP_NAME2;
+
+ldb_results = talloc_array(tmp_ctx, const char *, 2);
+assert_non_null(ldb_results);
+for (int i = 0; i < 2; ++i) {
+ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[i],
+  SYSDB_NAME, NULL);
+assert_non_null(ldbname);
+ldb_results[i] = ldbname;
+}
+
+assert_string_not_equal(ldb_results[0], ldb_results[1]);
+
+assert_true(are_values_in_ldb_result(ldb_results, group_names));
+
+talloc_zfree(tmp_ctx);
+}


Thanks!


Your comments will be addressed in nex patchset.

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

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-10 Thread Petr Cech 

On 11/10/2015 08:37 AM, Lukas Slebodnik wrote:

On (10/11/15 08:29), Pavel Reichl wrote:



On 11/05/2015 05:29 PM, Petr Cech wrote:

+void test_groups_by_recent_filter_valid(void **state)
+{
+struct cache_req_test_ctx *test_ctx = NULL;
+TALLOC_CTX *req_mem_ctx = NULL;
+struct tevent_req *req = NULL;
+const char **group_names = NULL;
+const char **ldb_results = NULL;
+const char *ldbname = NULL;
+void *tmp_ctx = NULL;

Could you use TALLOC_CTX?


Why do we need two different talloc context in a test?
"TALLOC_CTX *req_mem_ctx", "void *tmp_ctx"

If we properly release resources we can use single talloc context.
It's the best way how to catch memory leaks.

LS


Right, I will change void *tmp_ctx to TALLOC_CTX *tmp_ctx and I will 
create it under req_mem_ctx. I feel it will be more clear and readable.


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

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-10 Thread Petr Cech 

On 11/09/2015 04:28 PM, Jakub Hrozek wrote:

On Thu, Nov 05, 2015 at 05:29:25PM +0100, Petr Cech wrote:

>On 11/04/2015 11:11 AM, Jakub Hrozek wrote:

> >Hi,
> >
> >Sorry it took so long to get back to the review.  I only have some minor
> >comments, see inline..
> >
> >Because the group patches are more or less equivalent, I'll just comment
> >here. If you agree with the comments, please also change the group tests
> >and resend in a single set.
> >
> >Thanks for the tests!
> >

> >>> From e3dd543eec09f6e4386bfe6f1505538575fe5356 Mon Sep 17 00:00:00 2001
> >>>From: Petr Cech
> >>>Date: Fri, 2 Oct 2015 07:34:08 -0400
> >>>Subject: [PATCH 1/3] TEST: Add test_user_by_recent_filter_valid
> >>>
> >>>Test users_by_filter_valid() was removed in past. We will add two new
> >>>tests instead of it. Logic of those tests is connected to RECENT
> >>>filter. It returns only records which have been wrote or updated after
> >>>filter was created (or another given time).
> >>>
> >>>users_by_filter_valid() --> user_by_recent_filter_valid()
> >>> users_by_recent_filter_valid()
> >>>
> >>>The first of new tests, user_by_recent_filter_valid(), counts with two
> >>>users. One is stored before filter request creation and the second user
> >>>is stored after filter request creation. So filter returns only one
> >>>user.
> >>>
> >>>The second of new tests, users_by_recent_filter_valid(), counts with
> >>>three users. One is stored before filter request creation and two users
> >>>are stored after filter request creation. So filter returns two users.
> >>>
> >>>This patch adds user_by_recent_filter_valid().
> >>>
> >>>Resolves:
> >>>https://fedorahosted.org/sssd/ticket/2730
> >>>---
> >>>  src/tests/cmocka/test_responder_cache_req.c | 50 
+
> >>>  1 file changed, 50 insertions(+)
> >>>
> >>>diff --git a/src/tests/cmocka/test_responder_cache_req.c 
b/src/tests/cmocka/test_responder_cache_req.c
> >>>index 
744c8f4a8f7aa4e08f82aca5aea003438b5b59da..3379b17f7feea521966d6c8646afd9859a3c5255 100644
> >>>--- a/src/tests/cmocka/test_responder_cache_req.c
> >>>+++ b/src/tests/cmocka/test_responder_cache_req.c
> >>>@@ -1239,6 +1239,53 @@ static void 
cache_req_user_by_filter_test_done(struct tevent_req *req)
> >>>  ctx->tctx->done = true;
> >>>  }
> >>>
> >>>+void test_user_by_recent_filter_valid(void **state)
> >>>+{
> >>>+struct cache_req_test_ctx *test_ctx = NULL;
> >>>+TALLOC_CTX *req_mem_ctx = NULL;
> >>>+struct tevent_req *req = NULL;
> >>>+const char *ldbname = NULL;
> >>>+errno_t ret;
> >>>+
> >>>+test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
> >>>+test_ctx->create_user = true;
> >>>+
> >>>+ret = sysdb_store_user(test_ctx->tctx->dom, TEST_USER_NAME2, "pwd", 
1001, 1001,
> >>>+   NULL, NULL, NULL, 
"cn="TEST_USER_NAME2",dc=test", NULL,
> >>>+   NULL, 1000, time(NULL));
> >>>+assert_int_equal(ret, EOK);
> >>>+
> >>>+sleep(1);

> >The purpose of the sleep() here is just to make sure the entry was
> >created in the past, right? Would it be equally safe to create the user
> >with timestamp time(NULL)-1 to make the test faster?
> >

> >>>+
> >>>+req_mem_ctx = talloc_new(test_ctx->tctx);
> >>>+check_leaks_push(req_mem_ctx);
> >>>+
> >>>+/* Filters always go to DP */
> >>>+will_return(__wrap_sss_dp_get_account_send, test_ctx);
> >>>+mock_account_recv_simple();

> >Can you add a comment that the TEST_USER is created with a DP callback
> >here?
> >

> >>>+
> >>>+req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
> >>>+test_ctx->rctx,
> >>>+test_ctx->tctx->dom->name,
> >>>+"test*");

> >It would read nicer if we had a constant TEST_USER_PREFIX "test_user" 
#defined,
> >or even TEST_USER_FILTER with the asterist.
> >

> >>>+assert_non_null(req);
> >>>+
> >>>+tevent_req_set_callback(req, cache_req_user_by_filter_test_done, 
test_ctx);
> >>>+
> >>>+ret = test_ev_loop(test_ctx->tctx);
> >>>+assert_int_equal(ret, ERR_OK);
> >>>+assert_true(check_leaks_pop(req_mem_ctx));
> >>>+
> >>>+assert_non_null(test_ctx->result);
> >>>+assert_int_equal(test_ctx->result->count, 1);
> >>>+
> >>>+ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[0],
> >>>+  SYSDB_NAME, NULL);
> >>>+assert_non_null(ldbname);
> >>>+assert_string_equal(ldbname, TEST_USER_NAME);
> >>>+}
> >>> From c2e87544dfbc0667e1b935394d697322b34dddeb Mon Sep 17 00:00:00 2001
> >>>From: Petr Cech
> >>>Date: Tue, 27 Oct 2015 03:53:18 -0400
> >>>Subject: [PATCH 2/3] TEST: Refactor of test_responder_cache_req.c
> >>>
> >>>We need little more in background of responder_cache_req tests. There
> >>>will be tests which will use three test users. This

Re: [SSSD] [PATCH] tools: Don't shadow 'exit'

2015-11-10 Thread Lukas Slebodnik 
On (10/11/15 08:59), Lukas Slebodnik wrote:
>On (09/11/15 15:33), Jakub Hrozek wrote:
>>On Mon, Nov 09, 2015 at 10:44:49AM +0100, Lukas Slebodnik wrote:
>>> Obvious ACK
>>
>>While reviewing your patches, I found one more place I forgot to fix,
>>see the attached patch.
>
>>From b275e167c4c802c838d8b3d4d2d419e8af6fa99b Mon Sep 17 00:00:00 2001
>>From: Jakub Hrozek 
>>Date: Mon, 9 Nov 2015 10:27:41 +0100
>>Subject: [PATCH] tools: Don't shadow 'exit'
>>MIME-Version: 1.0
>>Content-Type: text/plain; charset=UTF-8
>>Content-Transfer-Encoding: 8bit
>>
>>Fixes:
>>/sssd/src/tools/sss_override.c: In function ‘override_user_import’:
>>/sssd/src/tools/sss_override.c:1471: warning: declaration of ‘exit’ 
>> shadows a global declaration
>>/usr/include/stdlib.h:544: warning: shadowed declaration is here
>>/sssd/src/tools/sss_override.c: In function ‘override_group_import’:
>>/sssd/src/tools/sss_override.c:1737: warning: declaration of ‘exit’ 
>> shadows a global declaration
>>/usr/include/stdlib.h:544: warning: shadowed declaration is here
>>---
>ACK
>
>http://sssd-ci.duckdns.org/logs/job/32/36/summary.html
>
master:

* 3b9a62badec2478f978ac28d2e3b72a7dd16a6e5

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

[SSSD] [PATCH] INIT: Drop syslog.target from service file

2015-11-10 Thread Lukas Slebodnik 
ehlo,

debian has pach which removes syslog.target from service.file
http://anonscm.debian.org/cgit/pkg-sssd/sssd.git/tree/debian/patches/fix-obsolete-target.diff
and this target is not available on el7.1with quite old systemd.

I had a small discussion with systemd guys and we can drop
it without any problem. It should work out-of-the-box.

LS
>From 022e5c9fbfa2097d4a57d038a4750658fad1a047 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Tue, 10 Nov 2015 07:53:47 +0100
Subject: [PATCH] INIT: Drop syslog.target from service file

The syslog.target is not part of systemd anymore.
The non-socket-activated syslog daemons are not supported in systemd >= 35
and in the same version it was recomemded to not use this target in service
files.

http://www.freedesktop.org/wiki/Software/systemd/syslog/
---
 src/sysv/systemd/sssd.service.in | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/sysv/systemd/sssd.service.in b/src/sysv/systemd/sssd.service.in
index 
06445ea12e748028658feee0eb4e60040c40e8d1..a4f9125b58e72429cc3ac1e679271367ada27f3c
 100644
--- a/src/sysv/systemd/sssd.service.in
+++ b/src/sysv/systemd/sssd.service.in
@@ -1,7 +1,5 @@
 [Unit]
 Description=System Security Services Daemon
-# SSSD will not be started until syslog is
-After=syslog.target
 # SSSD must be running before we permit user sessions
 Before=systemd-user-sessions.service nss-user-lookup.target
 Wants=nss-user-lookup.target
-- 
2.5.0

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

Re: [SSSD] [PATCH] tools: Don't shadow 'exit'

2015-11-10 Thread Lukas Slebodnik 
On (09/11/15 20:53), Jakub Hrozek wrote:
>On Mon, Nov 09, 2015 at 06:35:05PM +0100, Lukas Slebodnik wrote:
>> BTW which version do you prefer?
>> a) signl
>> b) sig
>> c) a_signal
>
>I don't care :) Feel free to use a_signal since it came first.

Updated patches are attached.

LS
>From a2a1495247ed992074f606c488f165e20f6b082f Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Mon, 9 Nov 2015 10:40:11 +0100
Subject: [PATCH 1/5] cache_req: Fix warning -Wshadow

src/responder/common/responder_cache_req.c: In function 
'cache_req_input_set_name':
src/responder/common/responder_cache_req.c:199: warning: declaration of 'dup' 
shadows a global declaration
/usr/include/unistd.h:528: warning: shadowed declaration is here
---
 src/responder/common/responder_cache_req.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/responder/common/responder_cache_req.c 
b/src/responder/common/responder_cache_req.c
index fc63f84..68206e4 100644
--- a/src/responder/common/responder_cache_req.c
+++ b/src/responder/common/responder_cache_req.c
@@ -196,15 +196,15 @@ static errno_t
 cache_req_input_set_name(struct cache_req_input *input,
  const char *name)
 {
-const char *dup;
+const char *dup_name;
 
-dup = talloc_strdup(input, name);
-if (dup == NULL) {
+dup_name = talloc_strdup(input, name);
+if (dup_name == NULL) {
 return ENOMEM;
 }
 
 talloc_zfree(input->name);
-input->name = dup;
+input->name = dup_name;
 
 return EOK;
 }
-- 
2.4.3

>From 8bb8c2f42218ee83eba71acde55203cba310b128 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Mon, 9 Nov 2015 10:48:55 +0100
Subject: [PATCH 2/5] SBUS: Fix warnings -Wshadow
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

src/sbus/sssd_dbus_invokers.c  -fPIC -DPIC -o 
src/sbus/.libs/libsss_util_la-sssd_dbus_invokers.o
src/sbus/sssd_dbus_introspect.c: In function 'sbus_introspect_generate_signals':
src/sbus/sssd_dbus_introspect.c:206: warning: declaration of 'signal' shadows a 
global declaration
/usr/include/signal.h:101: warning: shadowed declaration is here

src/sbus/sssd_dbus_introspect.c: In function 
'sbus_introspect_generate_properties':
src/sbus/sssd_dbus_introspect.c:243: warning: declaration of 'access' shadows a 
global declaration
/usr/include/unistd.h:288: warning: shadowed declaration is here

src/sbus/sssd_dbus_signals.c:29: warning: declaration of 'signal' shadows a 
global declaration
/usr/include/signal.h:101: warning: shadowed declaration is here
src/sbus/sssd_dbus_signals.c: In function 'sbus_new_incoming_signal':
src/sbus/sssd_dbus_signals.c:39: warning: declaration of 'signal' shadows a 
global declaration
/usr/include/signal.h:101: warning: shadowed declaration is here
src/sbus/sssd_dbus_signals.c: In function 'sbus_incoming_signal_hash_add':
src/sbus/sssd_dbus_signals.c:73: warning: declaration of 'signal' shadows a 
global declaration
/usr/include/signal.h:101: warning: shadowed declaration is here
src/sbus/sssd_dbus_signals.c: In function 'sbus_incoming_signal_hash_lookup':
src/sbus/sssd_dbus_signals.c:134: warning: declaration of 'signal' shadows a 
global declaration
/usr/include/signal.h:101: warning: shadowed declaration is here
src/sbus/sssd_dbus_signals.c: In function 'sbus_signal_listen':
src/sbus/sssd_dbus_signals.c:168: warning: declaration of 'signal' shadows a 
global declaration
/usr/include/signal.h:101: warning: shadowed declaration is here
src/sbus/sssd_dbus_signals.c: In function 'sbus_signal_handler':
src/sbus/sssd_dbus_signals.c:227: warning: declaration of 'signal' shadows a 
global declaration
/usr/include/signal.h:101: warning: shadowed declaration is here
src/sbus/sssd_dbus_signals.c: In function 'sbus_signal_handler_got_caller_id':
src/sbus/sssd_dbus_signals.c:264: warning: declaration of 'signal' shadows a 
global declaration
/usr/include/signal.h:101: warning: shadowed declaration is here
src/sbus/sssd_dbus_common_signals.c: In function 
'sbus_signal_name_owner_changed':
src/sbus/sssd_dbus_common_signals.c:30: warning: declaration of 'signal' 
shadows a global declaration
/usr/include/signal.h:101: warning: shadowed declaration is here

src/sbus/sssd_dbus_common_signals.c: In function 
‘sbus_signal_name_owner_changed’:
src/sbus/sssd_dbus_common_signals.c:30: warning: declaration of ‘signal’ 
shadows a global declaration
/usr/include/signal.h:101: warning: shadowed declaration is here
---
 src/sbus/sssd_dbus_common_signals.c |  4 +--
 src/sbus/sssd_dbus_introspect.c | 19 +-
 src/sbus/sssd_dbus_signals.c| 72 ++---
 3 files changed, 48 insertions(+), 47 deletions(-)

diff --git a/src/sbus/sssd_dbus_common_signals.c 
b/src/sbus/sssd_dbus_common_signals.c
index 2950c35..c153e46 100644
--- a/src/sbus/sssd_dbus_common_signals.c
+++ b/src/sbus/sssd_dbus_common_signals.c
@@ -27,10 +27,10 @@
 #include

[SSSD] [PATCH] FAIL_OVER: Fix warning value computed is not used

2015-11-10 Thread Lukas Slebodnik 
ehlo,

another warning on rhel6.

BTW different solution would be to cast to void.

LS
>From 2516dadde6940fe8cd7bf0ad769f81bbf2c23b7e Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Tue, 10 Nov 2015 07:41:10 +
Subject: [PATCH 1/2] FAIL_OVER: Fix warning value computed is not used
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

src/providers/fail_over.c: In function ‘fo_ref_server’:
src/providers/fail_over.c:861: warning: value computed is not used
---
 src/providers/fail_over.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c
index 
24aed9dfa469ad730d176244eb329522a43c6fd8..c60310d8f17b129be115a4d3724ac7e54549820e
 100644
--- a/src/providers/fail_over.c
+++ b/src/providers/fail_over.c
@@ -858,7 +858,7 @@ void fo_ref_server(TALLOC_CTX *ref_ctx,
struct fo_server *server)
 {
 if (server) {
-rc_reference(ref_ctx, struct fo_server, server);
+server = rc_reference(ref_ctx, struct fo_server, server);
 }
 }
 
-- 
2.5.0

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

Re: [SSSD] [PATCH] SSSD: Add a new command diag_cmd

2015-11-10 Thread Petr Cech 

On 11/09/2015 07:17 PM, Stephen Gallagher wrote:

There are problems inherent with passing the PID to the child process.
There's no guarantee that the process still exists. In the worst-case,
the PID could actually be reassigned to a new process and the output
you got back from something like pstack could be reading from a
different executable entirely.

+1

I am sorry I didn't see big picture.

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

[SSSD] Fix warning may be used uninitialized

2015-11-10 Thread Lukas Slebodnik 
ehlo,

and the last one patch to fix warnings on el6

LS
>From 84e149ddba1cebfbc37bf7b6d3769f9851bce446 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Tue, 10 Nov 2015 07:42:59 +
Subject: [PATCH 2/2] DP_PTASK: Fix warning may be used uninitialized
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It could be unitialized only in case if we add new enum be_ptask_schedule
Currently, we have only BE_PTASK_SCHEDULE_FROM_NOW and
BE_PTASK_SCHEDULE_FROM_LAST which are properly covered in switch case.

src/providers/dp_ptask.c:200: warning: ‘tv’ may be used uninitialized in this 
function
---
 src/providers/dp_ptask.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/providers/dp_ptask.c b/src/providers/dp_ptask.c
index 
0f28dee1ed8e6683dbdb82d9274d18a10108cbd6..51800ab57b5649380c0603f1d602dfa81d1f5919
 100644
--- a/src/providers/dp_ptask.c
+++ b/src/providers/dp_ptask.c
@@ -197,7 +197,7 @@ static void be_ptask_schedule(struct be_ptask *task,
   enum be_ptask_delay delay_type,
   enum be_ptask_schedule from)
 {
-struct timeval tv;
+struct timeval tv = { 0, };
 time_t delay = 0;
 
 if (!task->enabled) {
-- 
2.5.0

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

[SSSD] CONFIGURE: Bump AM_GNU_GETTEXT_VERSION

2015-11-10 Thread Lukas Slebodnik 
ehlo,

The function gettext was not detected properly with strict
cflags even thought it was part of glibc.

sh$ CFLAGS="-Werror" ./configure

sh$ grep gt_cv_func_gnugettext config.log
gt_cv_func_gnugettext1_libc=no
gt_cv_func_gnugettext1_libintl=no

sh$ objdump -T /lib64/libc.so.6 | grep gettext
0002fc60  w   DF .text  0010  GLIBC_2.2.5 dcngettext
0002dc70  w   DF .text  000f  GLIBC_2.2.5 dcgettext
0002fc80  w   DF .text  0016  GLIBC_2.2.5 ngettext
0002dc90  w   DF .text  000f  GLIBC_2.2.5 gettext
0002dc70 gDF .text  000f  GLIBC_2.2.5 __dcgettext
0002dc80  w   DF .text  000a  GLIBC_2.2.5 dgettext
0002dc80 gDF .text  000a  GLIBC_2.2.5 __dgettext

With attached patch situation is better.
q
sh$ autoreconf
sh$ CFLAGS="-Werror" ./configure

sh$ grep gt_cv_func_gnugettext config.log
gt_cv_func_gnugettext1_libc=yes

LS
>From c60c2e870d140e127bca69eb03bba30988c1dec4 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Tue, 10 Nov 2015 10:39:07 +0100
Subject: [PATCH] CONFIGURE: Bump AM_GNU_GETTEXT_VERSION

The function gettext was not detected properly with strict
cflags even thought it was part of glibc.

sh$ CFLAGS="-Werror" ./configure

sh$ grep gt_cv_func_gnugettext config.log
gt_cv_func_gnugettext1_libc=no
gt_cv_func_gnugettext1_libintl=no

sh$ objdump -T /lib64/libc.so.6 | grep gettext
0002fc60  w   DF .text  0010  GLIBC_2.2.5 dcngettext
0002dc70  w   DF .text  000f  GLIBC_2.2.5 dcgettext
0002fc80  w   DF .text  0016  GLIBC_2.2.5 ngettext
0002dc90  w   DF .text  000f  GLIBC_2.2.5 gettext
0002dc70 gDF .text  000f  GLIBC_2.2.5 __dcgettext
0002dc80  w   DF .text  000a  GLIBC_2.2.5 dgettext
0002dc80 gDF .text  000a  GLIBC_2.2.5 __dgettext
0002fc70  w   DF .text  000b  GLIBC_2.2.5 dngettext
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 
51c0b0da8a7cfb692eabf38a71e42fbb0e40b4b0..16f14ae041e47fa9ac18c57d6a16cee115d35840
 100644
--- a/configure.ac
+++ b/configure.ac
@@ -26,7 +26,7 @@ m4_ifdef([AC_PROG_MKDIR_P],
 LT_LIB_DLLOAD
 AC_CONFIG_MACRO_DIR([m4])
 AM_GNU_GETTEXT([external])
-AM_GNU_GETTEXT_VERSION([0.14])
+AM_GNU_GETTEXT_VERSION([0.14.4])
 
 AC_SUBST([PRERELEASE_VERSION],
  PRERELEASE_VERSION_NUMBER)
-- 
2.5.0

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

[SSSD] [PATCH] p11: enable ocsp checks

2015-11-10 Thread Sumit Bose 
Hi,

this patch switches on the Online Certificate Status Protocol (OCSP)
checks while validation the certificate. This is done by calling
CERT_EnableOCSPChecking() before doing the validation. The main part of
the patch makes this configurable.

Since I expect that certificate validation will need more tuning option
in future I didn't add a new option of switch OCSP off and on but a more
generic one called 'certificate_verification' which accepts a comma
separated list of parameters. Currently only 'no_ocsp' is supported.

Currently this option is tested indirectly because I generated the test
certificates with IPA they contain the OCSP data. But since the OCSP
check, which is now on by default, requires on-line access to the
referenced OCSP server the OCSP check must be disabled for the
unit-test.

bye,
Sumit
From 64701f85a3c76ce420b974b6fd94186007e53257 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Thu, 5 Nov 2015 18:20:27 +0100
Subject: [PATCH] p11: enable ocsp checks

This patch enables the Online Certificate Status Protocol in NSS and
adds an option to disable it if needed. To make further tuning of
certificate verification more easy it is not an option on its own but an
option to the new certificate_verification configuration option.

Resolves https://fedorahosted.org/sssd/ticket/2812
---
 Makefile.am  |  5 ++--
 src/confdb/confdb.h  |  1 +
 src/config/SSSDConfig/__init__.py.in |  1 +
 src/config/SSSDConfigTest.py |  3 ++-
 src/config/etc/sssd.api.conf |  1 +
 src/man/sssd.conf.5.xml  | 28 ++
 src/p11_child/p11_child_nss.c| 25 ++--
 src/responder/pam/pamsrv.h   |  1 +
 src/responder/pam/pamsrv_cmd.c   | 14 ++-
 src/responder/pam/pamsrv_p11.c   | 21 ++--
 src/responder/ssh/sshsrv_cmd.c   | 15 +++-
 src/tests/cmocka/test_cert_utils.c   |  2 +-
 src/tests/cmocka/test_pam_srv.c  | 34 --
 src/util/cert.h  |  3 +++
 src/util/cert/cert_common.c  | 46 
 src/util/cert/libcrypto/cert.c   |  1 +
 src/util/cert/nss/cert.c | 20 
 17 files changed, 199 insertions(+), 22 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 
a0abb8fb3a9cc19cfabad8066c6c65d78e78d206..ee97d095e07f6d485232a1afd87f9e4057688f22
 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2579,8 +2579,7 @@ test_cert_utils_LDADD = \
 $(CRYPTO_LIBS) \
 libsss_debug.la \
 libsss_test_common.la \
-libsss_cert.la \
-libsss_crypt.la \
+libsss_util.la \
 $(NULL)
 
 test_data_provider_be_SOURCES = \
@@ -3156,6 +3155,7 @@ proxy_child_LDADD = \
 p11_child_SOURCES = \
 src/p11_child/p11_child_nss.c \
 src/util/atomic_io.c \
+src/util/util.c \
 $(NULL)
 p11_child_CFLAGS = \
 $(AM_CFLAGS) \
@@ -3168,6 +3168,7 @@ p11_child_LDADD = \
 $(POPT_LIBS) \
 $(NSS_LIBS) \
 libsss_crypt.la \
+libsss_cert.la \
 $(NULL)
 
 memberof_la_SOURCES = \
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index 
37b5fd7c7629e2618a1699e3ffd58110171db605..c6a5e3f61d8bfd045eb2699d0f5e279cb7d89f86
 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -71,6 +71,7 @@
 #define CONFDB_MONITOR_DEFAULT_DOMAIN "default_domain_suffix"
 #define CONFDB_MONITOR_OVERRIDE_SPACE "override_space"
 #define CONFDB_MONITOR_USER_RUNAS "user"
+#define CONFDB_MONITOR_CERT_VERIFICATION "certificate_verification"
 
 /* Both monitor and domains */
 #define CONFDB_NAME_REGEX   "re_expression"
diff --git a/src/config/SSSDConfig/__init__.py.in 
b/src/config/SSSDConfig/__init__.py.in
index 
bf61c402796122050fa43cf41128faec4771c5d2..2cb857013fe4bddfd2e79e589d3ba9721dc3ca4f
 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -60,6 +60,7 @@ option_strings = {
 'krb5_rcache_dir' : _('Directory on the filesystem where SSSD should store 
Kerberos replay cache files.'),
 'default_domain_suffix' : _('Domain to add to names without a domain 
component.'),
 'user' : _('The user to drop privileges to'),
+'certificate_verification' : _('Tune certificate verification'),
 
 # [nss]
 'enum_cache_timeout' : _('Enumeration cache timeout length (seconds)'),
diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
index 
45562214da5d227b45914abbcb298e043048adf5..8277d1ee4198f342b5cff50f8d44c973fce4a29b
 100755
--- a/src/config/SSSDConfigTest.py
+++ b/src/config/SSSDConfigTest.py
@@ -307,7 +307,8 @@ class SSSDConfigTestSSSDService(unittest.TestCase):
 'reconnection_retries',
 'fd_limit',
 'client_idle_timeout',
-'description']
+'description',
+'certificate_verification']
 
 self.assertTrue(type(options) == dict,
 "Options should be a dictionary")
diff --git

[SSSD] [PATCH] p11: check if cert is valid before selecting it

2015-11-10 Thread Sumit Bose 
Hi,

currently the p11_child does not continue to search for more
certificates if the first suitable certificate cannot be verified. With
this patch p11_child will continue until a valid certificate is found
(or all are checked).

As said before I'm working on improving the handling of the certificates
for the unit-test and would prefer to not spend time to add a test for
this case with the old scheme. If you agree I would open a ticket to
remind me to add tests for this case with the new scheme.

bye,
Sumit
From 26898bf92af4d28ef12cd9ae3a5ffb513d38183b Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Thu, 5 Nov 2015 17:43:52 +0100
Subject: [PATCH] p11: check if cert is valid before selecting it

Currently the first certificate was selected and if it was not valid
p11_child just returned an error. With this patch the validity is
checked first and the first valid certificate is selected.

Resolves https://fedorahosted.org/sssd/ticket/2801
---
 src/p11_child/p11_child_nss.c | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
index 
8a383a044dd295c911a1b12c6caeeffc57d00be6..22e449d547c64c94a3ee9e29939377c98443b47d
 100644
--- a/src/p11_child/p11_child_nss.c
+++ b/src/p11_child/p11_child_nss.c
@@ -272,6 +272,18 @@ int do_work(TALLOC_CTX *mem_ctx, const char *nss_db, const 
char *slot_name_in,
  cert_list_node->cert->nickname,
  cert_list_node->cert->subjectName);
 
+rv = CERT_VerifyCertificateNow(handle, cert_list_node->cert,
+   PR_TRUE, certificateUsageSSLClient,
+   NULL, NULL);
+if (rv != SECSuccess) {
+DEBUG(SSSDBG_OP_FAILURE,
+  "Certificate [%s][%s] not valid [%d], skipping.\n",
+  cert_list_node->cert->nickname,
+  cert_list_node->cert->subjectName, PR_GetError());
+break;
+}
+
+
 if (found_cert == NULL) {
 found_cert = cert_list_node->cert;
 } else {
@@ -291,16 +303,6 @@ int do_work(TALLOC_CTX *mem_ctx, const char *nss_db, const 
char *slot_name_in,
 goto done;
 }
 
-rv = CERT_VerifyCertificateNow(handle, found_cert, PR_TRUE,
-   certificateUsageSSLClient, NULL, NULL);
-if (rv != SECSuccess) {
-DEBUG(SSSDBG_OP_FAILURE,
-  "CERT_VerifyCertificateNow failed [%d].\n",
-  PR_GetError());
-ret = EIO;
-goto done;
-}
-
 if (mode == OP_AUTH) {
 rv = PK11_GenerateRandom(random_value, sizeof(random_value));
 if (rv != SECSuccess) {
-- 
2.1.0

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

[SSSD] [PATCH] cache_req: check all domains for lookups by certificate

2015-11-10 Thread Sumit Bose 
Hi,

this patch is the equivalent of 374268c5eda35e8bbc2fef30752299199439cffe
"fix upn cache_req for sub-domain users" for lookups by certificates.

bye,
Sumit
From 78ac47bfdbb9c91dddefb4de06dcdf41e7035c6a Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 12 Oct 2015 13:00:28 +0200
Subject: [PATCH] cache_req: check all domains for lookups by certificate

Like lookup by ID or by UPN the match for lookups by certificate can be
found in any domain and all sub-domains must be included in the search.
---
 src/responder/common/responder_cache_req.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/responder/common/responder_cache_req.c 
b/src/responder/common/responder_cache_req.c
index 
fc63f84f1725b98398d383983c385c01508c73b6..a6fff2253c4057eb7239373be18ab15ba161b3dc
 100644
--- a/src/responder/common/responder_cache_req.c
+++ b/src/responder/common/responder_cache_req.c
@@ -982,6 +982,7 @@ static errno_t cache_req_next_domain(struct tevent_req *req)
 * qualified names instead. */
 while (state->domain != NULL && state->check_next
 && state->domain->fqnames
+&& state->input->type != CACHE_REQ_USER_BY_CERT
 && !cache_req_input_is_upn(state->input)) {
 state->domain = get_next_domain(state->domain, 0);
 }
@@ -1010,9 +1011,9 @@ static errno_t cache_req_next_domain(struct tevent_req 
*req)
 
 /* we will continue with the following domain the next time */
 if (state->check_next) {
-if (cache_req_input_is_upn(state->input)) {
-state->domain = get_next_domain(state->domain,
-SSS_GND_DESCEND);
+if (cache_req_input_is_upn(state->input)
+|| state->input->type != CACHE_REQ_USER_BY_CERT ) {
+state->domain = get_next_domain(state->domain, 
SSS_GND_DESCEND);
 } else {
 state->domain = get_next_domain(state->domain, 0);
 }
-- 
2.1.0

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

Re: [SSSD] [PATCH] SSSD: Add a new command diag_cmd

2015-11-10 Thread Jakub Hrozek 
On Mon, Nov 09, 2015 at 11:32:30AM +0100, Petr Cech wrote:
> On 11/04/2015 11:24 AM, Jakub Hrozek wrote:
> >Hi,
> >
> >I created this patch to try to diagnose an issue where sssd would
> >randomly restart on any of machines in a VM cluster without giving too
> >much advise why. I think it might be useful to merge in general.
> 
> Hi Jakub,
> 
> I reviewed the patch. Code looks good to me.
> CI tests passed: http://sssd-ci.duckdns.org/logs/job/32/25/summary.html
> 
> Then I tried to test new functionality.
> 
> Man pages are right, I found diag_cmd in sssd.conf.
> 
> And I really got the right message when I kill sss_pam:
> # (Mon Nov  9 04:30:47 2015) [sssd] [svc_child_info] (0x0040): Child [25767]
> terminated with signal [9]
> 
> I would like to see output of pstack, but I don't know, how to get the right
> state of SSSD. Can you help me, please?

I tested the patch by setting a low 'timeout' in the 'domain' section
and then setting the diag_cmd:
[domain/foo]
timeout = 2
diag_cmd = pstack %p

then I stopped the back end:
# kill -STOP $(pidof sssd_be)

You should see the pstack output in /var/log/sssd/sssd.log, also the
debug_level must be increased in the [sssd] section. You might also need
to set SELinux to Permissive, otherwise sssd might not be able to fork
an exec pstack..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] p11: enable ocsp checks

2015-11-10 Thread Lukas Slebodnik 
On (10/11/15 11:36), Sumit Bose wrote:
>Hi,
>
>this patch switches on the Online Certificate Status Protocol (OCSP)
>checks while validation the certificate. This is done by calling
>CERT_EnableOCSPChecking() before doing the validation. The main part of
>the patch makes this configurable.
>
>Since I expect that certificate validation will need more tuning option
>in future I didn't add a new option of switch OCSP off and on but a more
>generic one called 'certificate_verification' which accepts a comma
>separated list of parameters. Currently only 'no_ocsp' is supported.
>
>Currently this option is tested indirectly because I generated the test
>certificates with IPA they contain the OCSP data. But since the OCSP
>check, which is now on by default, requires on-line access to the
>referenced OCSP server the OCSP check must be disabled for the
>unit-test.
>
>bye,
>Sumit

Just few "build-related" comments.

>From 64701f85a3c76ce420b974b6fd94186007e53257 Mon Sep 17 00:00:00 2001
>From: Sumit Bose 
>Date: Thu, 5 Nov 2015 18:20:27 +0100
>Subject: [PATCH] p11: enable ocsp checks
>
>This patch enables the Online Certificate Status Protocol in NSS and
>adds an option to disable it if needed. To make further tuning of
>certificate verification more easy it is not an option on its own but an
>option to the new certificate_verification configuration option.
>
>Resolves https://fedorahosted.org/sssd/ticket/2812
>---
> Makefile.am  |  5 ++--
> src/confdb/confdb.h  |  1 +
> src/config/SSSDConfig/__init__.py.in |  1 +
> src/config/SSSDConfigTest.py |  3 ++-
> src/config/etc/sssd.api.conf |  1 +
> src/man/sssd.conf.5.xml  | 28 ++
> src/p11_child/p11_child_nss.c| 25 ++--
> src/responder/pam/pamsrv.h   |  1 +
> src/responder/pam/pamsrv_cmd.c   | 14 ++-
> src/responder/pam/pamsrv_p11.c   | 21 ++--
> src/responder/ssh/sshsrv_cmd.c   | 15 +++-
> src/tests/cmocka/test_cert_utils.c   |  2 +-
> src/tests/cmocka/test_pam_srv.c  | 34 --
> src/util/cert.h  |  3 +++
> src/util/cert/cert_common.c  | 46 
> src/util/cert/libcrypto/cert.c   |  1 +
> src/util/cert/nss/cert.c | 20 
> 17 files changed, 199 insertions(+), 22 deletions(-)
>
>diff --git a/Makefile.am b/Makefile.am
>index 
>a0abb8fb3a9cc19cfabad8066c6c65d78e78d206..ee97d095e07f6d485232a1afd87f9e4057688f22
> 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -2579,8 +2579,7 @@ test_cert_utils_LDADD = \
> $(CRYPTO_LIBS) \
> libsss_debug.la \
> libsss_test_common.la \
>-libsss_cert.la \
>-libsss_crypt.la \
>+libsss_util.la \
> $(NULL)
libss_cert and libsss_crypt chould not be removed.
Otherwise I cannot build the test.
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function 
`test_sss_cert_der_to_pem':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:193: undefined 
reference to `sss_cert_der_to_pem'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:196: undefined 
reference to `sss_cert_der_to_pem'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function 
`test_sss_cert_pem_to_der':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:213: undefined 
reference to `sss_cert_pem_to_der'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:216: undefined 
reference to `sss_cert_pem_to_der'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function 
`test_sss_cert_derb64_to_pem':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:232: undefined 
reference to `sss_cert_derb64_to_pem'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:235: undefined 
reference to `sss_cert_derb64_to_pem'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function 
`test_sss_cert_pem_to_derb64':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:250: undefined 
reference to `sss_cert_pem_to_derb64'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:253: undefined 
reference to `sss_cert_pem_to_derb64'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function 
`test_bin_to_ldap_filter_value':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:279: undefined 
reference to `bin_to_ldap_filter_value'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:283: undefined 
reference to `bin_to_ldap_filter_value'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function 
`test_sss_cert_derb64_to_ldap_filter':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:300: undefined 
reference to `sss_cert_derb64_to_ldap_filter'
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:303: undefined 
reference to `sss_cert_derb64_to_ldap_filter'
src/tests/cmocka/test_cert_utils-test_cert_utils.o: In function 
`test_cert_to_ssh_key':
/dev/shm/gcc/../sssd/src/tests/cmocka/test_cert_utils.c:352: undefined 
reference

[SSSD] [PATCH] p11: allow p11_child to run completely unprivileged

2015-11-10 Thread Sumit Bose 
Hi,

this patch removes the requirement to install p11_child with SETUID or
SETGID bit set. The needed privileges can be tuned with the help of
policy-kit so p11_child can either run as root or as SSSD user depending
on the SSSD configuration without the need to gain extra user or group
privileges.

With this patch the needed policy-kit configuration snippet is created
if the --with-sssd-user configure option is used. Since the contributed
spec file implicitly assumes that the SSSD user is 'sssd' I added the
option here. I added 'Requires: polkit' as well to make sure
%{_datadir}/polkit-1/rules.d/ exists. This might not be the best
solution for minimal installations where e.g. only the SSSD LDAP
provider is needed. But distribution can modify it to suit their needs.
I did it this way because I'm not sure if all distribution allow
directories to be owned by multiple packages. If someone can confirm
that this is allowed by the majority of the distribution I'd be happy to
modify the patch so that the directory is created and owned by the SSSD
package and drop the polkit requirement.

Since dropping the SETUID and SETGID bits improves the general security
it would be nice if this patch can be include in 1.13.x so the
distributions can pick it and improve already released packages.

Unfortunately I think to test the functionality, i.e. access a Smartcard
with p11_child as an unprivileged user, real hardware is needed. If you
want to test it, please ping me, maybe we can arrange something.

bye,
Sumit
From 38fb2635aaec4dca335227d532965dfbadbca2ef Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Fri, 30 Oct 2015 16:29:31 +0100
Subject: [PATCH] p11: allow p11_child to run completely unprivileged

To only operation of p11_child which requires special privileges is the
communication to pcscd which handles the Smartcard access. pcscd uses
policy-kit for access control so access can easily be configured by
dropping config snippets into the right directory.

If SSSD is configured to run as un-privileged user this patch creates
the needed config snippet for policy-kit and installs it in a suitable
directory. As a result p11_child does not have to be installed with
SETUID or SETGID bits set.

Resolves https://fedorahosted.org/sssd/ticket/2755 by making it obsolete
---
 Makefile.am   |  8 
 configure.ac  |  1 +
 contrib/sssd-pcsc.rules.in| 15 +++
 contrib/sssd.spec.in  |  6 +-
 src/p11_child/p11_child_nss.c | 27 ++-
 5 files changed, 31 insertions(+), 26 deletions(-)
 create mode 100644 contrib/sssd-pcsc.rules.in

diff --git a/Makefile.am b/Makefile.am
index 
ee97d095e07f6d485232a1afd87f9e4057688f22..57bece64c5fca058dcf57efffb149770ac3b773e
 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -150,6 +150,10 @@ sssdlibexec_PROGRAMS += selinux_child
 endif
 if HAVE_NSS
 sssdlibexec_PROGRAMS += p11_child
+if SSSD_USER
+polkit_rulesdir = $(datadir)/polkit-1/rules.d
+dist_polkit_rules_DATA = contrib/sssd-pcsc.rules
+endif
 endif
 
 if BUILD_PAC_RESPONDER
@@ -3566,10 +3570,6 @@ if BUILD_SEMANAGE
-chgrp $(SSSD_USER) $(DESTDIR)$(sssdlibexecdir)/selinux_child
chmod 4750 $(DESTDIR)$(sssdlibexecdir)/selinux_child
 endif
-if HAVE_NSS
-   -chgrp $(SSSD_USER) $(DESTDIR)$(sssdlibexecdir)/p11_child
-   chmod 4750 $(DESTDIR)$(sssdlibexecdir)/p11_child
-endif
 endif
 
 install-data-hook:
diff --git a/configure.ac b/configure.ac
index 
51c0b0da8a7cfb692eabf38a71e42fbb0e40b4b0..7f9b15979f608bef67a7813201b6d8b08b2c09d0
 100644
--- a/configure.ac
+++ b/configure.ac
@@ -406,6 +406,7 @@ my_srcdir=`readlink -f $srcdir`
 AC_DEFINE_UNQUOTED([ABS_SRC_DIR], ["$my_srcdir"], [Absolute path to the source 
directory])
 
 AC_CONFIG_FILES([Makefile contrib/sssd.spec src/examples/rwtab src/doxy.config
+ contrib/sssd-pcsc.rules
  src/sysv/sssd src/sysv/gentoo/sssd src/sysv/SUSE/sssd
  po/Makefile.in src/man/Makefile src/tests/cwrap/Makefile
  src/tests/intg/Makefile
diff --git a/contrib/sssd-pcsc.rules.in b/contrib/sssd-pcsc.rules.in
new file mode 100644
index 
..31d2dbe4f97bfb75071886a4d4ccb546a7674a80
--- /dev/null
+++ b/contrib/sssd-pcsc.rules.in
@@ -0,0 +1,15 @@
+// Please put this file in /usr/share/polkit-1/rules.d/ if SSSD is running as
+// unprivileged user '@SSSD_USER@' to allow access to the Smartcard via pcscd.
+polkit.addRule(function(action, subject) {
+if (action.id == "org.debian.pcsc-lite.access_card" &&
+subject.user == "@SSSD_USER@") {
+return polkit.Result.YES;
+}
+});
+
+polkit.addRule(function(action, subject) {
+if (action.id == "org.debian.pcsc-lite.access_pcsc" &&
+subject.user == "@SSSD_USER@") {
+return polkit.Result.YES;
+}
+});
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
index

[SSSD] [PATCH] BE: Add IFP to known clients

2015-11-10 Thread Pavel Březina 

This gets rid of confusing debug message:
[be_client_destructor] (0x0020): Unknown client removed ...
From 17b1d8216bab3770c58c79cf51c571cb184e8ab4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
Date: Tue, 10 Nov 2015 11:47:30 +0100
Subject: [PATCH] BE: Add IFP to known clients

This gets rid of confusing debug message:
[be_client_destructor] (0x0020): Unknown client removed ...
---
 src/providers/data_provider_be.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index effa185f905097e0be1e03596b5fc6da75a7e382..104016ca19c093dcbb4b0c1011f06e4bb4e2a49c 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -2107,6 +2107,9 @@ static int be_client_destructor(void *ctx)
 } else if (becli->bectx->pac_cli == becli) {
 DEBUG(SSSDBG_TRACE_FUNC, "Removed PAC client\n");
 becli->bectx->pac_cli = NULL;
+} else if (becli->bectx->ifp_cli == becli) {
+DEBUG(SSSDBG_TRACE_FUNC, "Removed IFP client\n");
+becli->bectx->ifp_cli = NULL;
 } else {
 DEBUG(SSSDBG_CRIT_FAILURE, "Unknown client removed ...\n");
 }
-- 
2.1.0

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

Re: [SSSD] [PATCHES] UTIL: Fix memory leak in switch_creds

2015-11-10 Thread Lukas Slebodnik 
On (11/11/15 07:58), Petr Cech wrote:
>On 11/09/2015 08:06 AM, Lukas Slebodnik wrote:
>>ehlo,
>>
>>You can see a leak in talloc report.
>>But it was ignored. So we didn't notice it for long time.
>>http://sssd-ci.duckdns.org/logs/job/29/90/rhel7/ci-build-debug/src/tests/cwrap/become_user-tests.log
>>
>>The first patch fixes the leak and the last one is prevention
>>for such mistakes in future.
>>
>>LS
>>
>Hi Lukáš,
>
>CI tests passed.
>
Could you send a link?

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

Re: [SSSD] [PATCHES] UTIL: Fix memory leak in switch_creds

2015-11-10 Thread Petr Cech 



On 11/11/2015 08:04 AM, Lukas Slebodnik wrote:

On (11/11/15 07:58), Petr Cech wrote:

>On 11/09/2015 08:06 AM, Lukas Slebodnik wrote:

>>ehlo,
>>
>>You can see a leak in talloc report.
>>But it was ignored. So we didn't notice it for long time.
>>http://sssd-ci.duckdns.org/logs/job/29/90/rhel7/ci-build-debug/src/tests/cwrap/become_user-tests.log
>>
>>The first patch fixes the leak and the last one is prevention
>>for such mistakes in future.
>>
>>LS
>>

>Hi Lukáš,
>
>CI tests passed.
>

Could you send a link?

LS

Yes, of course, I could.
And... there is it:
http://sssd-ci.duckdns.org/logs/job/32/54/summary.html
Petr
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] AD: Remove unused memory context from ad_user_conn_list

2015-11-10 Thread Lukas Slebodnik 
On (11/11/15 07:37), Petr Cech wrote:
>On 11/09/2015 07:44 AM, Lukas Slebodnik wrote:
>>ehlo,
>>
>>simple patch is attached.
>>
>>LS
>>
>>
>>0001-AD-Remove-unused-memory-context-from-ad_user_conn_li.patch
>>
>>
>> From cec2a8d6e72324d6a80a1df832230567f8b4c819 Mon Sep 17 00:00:00 2001
>>From: Lukas Slebodnik
>>Date: Fri, 23 Oct 2015 23:16:53 +0200
>>Subject: [PATCH] AD: Remove unused memory context from ad_user_conn_list
>>
>>---
>>  src/providers/ad/ad_common.c  | 3 +--
>>  src/providers/ad/ad_common.h  | 3 +--
>>  src/providers/ad/ad_id.c  | 2 +-
>>  src/tests/cmocka/test_ad_common.c | 6 ++
>>  4 files changed, 5 insertions(+), 9 deletions(-)
>>
>>diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
>>index 
>>ffc1351241e1874ebd37fb4a865ad5822cdea479..665e6ec4c50c22bca329a42309f7b465bc5ff5de
>> 100644
>>--- a/src/providers/ad/ad_common.c
>>+++ b/src/providers/ad/ad_common.c
>>@@ -1289,8 +1289,7 @@ ad_ldap_conn_list(TALLOC_CTX *mem_ctx,
>>  }
>>
>>  struct sdap_id_conn_ctx **
>>-ad_user_conn_list(TALLOC_CTX *mem_ctx,
>>-  struct ad_id_ctx *ad_ctx,
>>+ad_user_conn_list(struct ad_id_ctx *ad_ctx,
>>struct sss_domain_info *dom)
>>  {
>>  struct sdap_id_conn_ctx **clist;
>>diff --git a/src/providers/ad/ad_common.h b/src/providers/ad/ad_common.h
>>index 
>>0cefa1859aaa75731267917e66ab9a1905528e91..de6ffbff7d20f582c2689383f3d51e027e58277c
>> 100644
>>--- a/src/providers/ad/ad_common.h
>>+++ b/src/providers/ad/ad_common.h
>>@@ -154,8 +154,7 @@ ad_ldap_conn_list(TALLOC_CTX *mem_ctx,
>>struct sss_domain_info *dom);
>>
>>  struct sdap_id_conn_ctx **
>>-ad_user_conn_list(TALLOC_CTX *mem_ctx,
>>-  struct ad_id_ctx *ad_ctx,
>>+ad_user_conn_list(struct ad_id_ctx *ad_ctx,
>>struct sss_domain_info *dom);
>>
>>  struct sdap_id_conn_ctx *
>>diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
>>index 
>>45ea15a9abba417aa32a34768228b9f53b9ad71b..14a09c537461d8ef21a9c6f5155ce45e9e6cafcd
>> 100644
>>--- a/src/providers/ad/ad_id.c
>>+++ b/src/providers/ad/ad_id.c
>>@@ -247,7 +247,7 @@ get_conn_list(struct be_req *breq, struct ad_id_ctx 
>>*ad_ctx,
>>
>>  switch (ar->entry_type & BE_REQ_TYPE_MASK) {
>>  case BE_REQ_USER: /* user */
>>-clist = ad_user_conn_list(breq, ad_ctx, dom);
>>+clist = ad_user_conn_list(ad_ctx, dom);
>>  break;
>>  case BE_REQ_BY_SECID:   /* by SID */
>>  case BE_REQ_USER_AND_GROUP: /* get SID */
>>diff --git a/src/tests/cmocka/test_ad_common.c 
>>b/src/tests/cmocka/test_ad_common.c
>>index 
>>b0cf4b5e6b0559c2896273bfcfb1af99cad195a3..b1929ae35df6ed17c391772009b48dcbf33fa69e
>> 100644
>>--- a/src/tests/cmocka/test_ad_common.c
>>+++ b/src/tests/cmocka/test_ad_common.c
>>@@ -454,8 +454,7 @@ void test_user_conn_list(void **state)
>>   struct 
>> ad_common_test_ctx);
>>  assert_non_null(test_ctx);
>>
>>-conn_list = ad_user_conn_list(test_ctx,
>>-  test_ctx->ad_ctx,
>>+conn_list = ad_user_conn_list(test_ctx->ad_ctx,
>>test_ctx->dom);
>>  assert_non_null(conn_list);
>>
>>@@ -464,8 +463,7 @@ void test_user_conn_list(void **state)
>>  assert_null(conn_list[1]);
>>  talloc_free(conn_list);
>>
>>-conn_list = ad_user_conn_list(test_ctx,
>>-  test_ctx->ad_ctx,
>>+conn_list = ad_user_conn_list(test_ctx->ad_ctx,
>>test_ctx->subdom);
>>  assert_non_null(conn_list);
>>
>>-- 2.5.0
>
>Hi Lukáš,
>
>LGTM and CI tests passed:
>http://sssd-ci.duckdns.org/logs/job/32/53/summary.html
>
>=> ACK
>

master:
* a3ade2e98d397d000f224ae80c6512c959cca18e

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

Re: [SSSD] [PATCH] tools: Don't shadow 'exit'

2015-11-10 Thread Lukas Slebodnik 
On (10/11/15 09:00), Lukas Slebodnik wrote:
>On (10/11/15 08:59), Lukas Slebodnik wrote:
>>On (09/11/15 15:33), Jakub Hrozek wrote:
>>>On Mon, Nov 09, 2015 at 10:44:49AM +0100, Lukas Slebodnik wrote:
 Obvious ACK
>>>
>>>While reviewing your patches, I found one more place I forgot to fix,
>>>see the attached patch.
>>
>>>From b275e167c4c802c838d8b3d4d2d419e8af6fa99b Mon Sep 17 00:00:00 2001
>>>From: Jakub Hrozek 
>>>Date: Mon, 9 Nov 2015 10:27:41 +0100
>>>Subject: [PATCH] tools: Don't shadow 'exit'
>>>MIME-Version: 1.0
>>>Content-Type: text/plain; charset=UTF-8
>>>Content-Transfer-Encoding: 8bit
>>>
>>>Fixes:
>>>/sssd/src/tools/sss_override.c: In function ‘override_user_import’:
>>>/sssd/src/tools/sss_override.c:1471: warning: declaration of ‘exit’ 
>>> shadows a global declaration
>>>/usr/include/stdlib.h:544: warning: shadowed declaration is here
>>>/sssd/src/tools/sss_override.c: In function ‘override_group_import’:
>>>/sssd/src/tools/sss_override.c:1737: warning: declaration of ‘exit’ 
>>> shadows a global declaration
>>>/usr/include/stdlib.h:544: warning: shadowed declaration is here
>>>---
>>ACK
>>
>>http://sssd-ci.duckdns.org/logs/job/32/36/summary.html
>>
>master:
>
>* 3b9a62badec2478f978ac28d2e3b72a7dd16a6e5
>
Similar patches were pushed to stabel branch.
So

sssd-1-13:
* 4f8ed159b6fbaf7c1fe2e8a3643f28708d302c28

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

Re: [SSSD] [PATCHES] UTIL: Fix memory leak in switch_creds

2015-11-10 Thread Petr Cech 

On 11/09/2015 08:06 AM, Lukas Slebodnik wrote:

ehlo,

You can see a leak in talloc report.
But it was ignored. So we didn't notice it for long time.
http://sssd-ci.duckdns.org/logs/job/29/90/rhel7/ci-build-debug/src/tests/cwrap/become_user-tests.log

The first patch fixes the leak and the last one is prevention
for such mistakes in future.

LS


Hi Lukáš,

CI tests passed.


0001-UTIL-Fix-memory-leak-in-switch_creds.patch

 From 219d1bdd378f0a8c17a508e1f3e29a2d5435f4d0 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik
Date: Sat, 24 Oct 2015 14:19:11 +0200
Subject: [PATCH 1/5] UTIL: Fix memory leak in switch_creds

If we are already requested used then we needn't to call
setreeuid(), setresgid(). But we forgot to relase local
struct sss_creds *ssc, which is used for returnig saved credentials.
---

ACK


0002-TESTS-Initialize-leak-check.patch

 From 318b862f473daf9606bf7752283a63b36934908b Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik
Date: Sat, 24 Oct 2015 15:39:21 +0200
Subject: [PATCH 2/5] TESTS: Initialize leak check

If leak_check_setup is not called then global_talloc_context
was not initialized and check_leaks_pop(global_talloc_context) will fail.
---

ACK


0003-TESTS-Check-return-value-of-check_leaks_pop.patch

 From 21bf7449bb53209ad24c0ec4079a5810bb6f707b Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik
Date: Sat, 24 Oct 2015 15:15:39 +0200
Subject: [PATCH 3/5] TESTS: Check return value of check_leaks_pop
---

ACK


0004-TESTS-Make-check_leaks-static-function.patch

 From 7e95820146c58e68d9cdf356198d18a3f748ff81 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik
Date: Fri, 6 Nov 2015 15:13:29 +0100
Subject: [PATCH 4/5] TESTS: Make check_leaks static function
---

ACK


0005-TESTS-Add-warning-for-unused-result-of-leak-check-fu.patch

 From d9e428b18c3282fc877683b1a8228665c5f9d48a Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik
Date: Sat, 24 Oct 2015 15:48:26 +0200
Subject: [PATCH 5/5] TESTS: Add warning for unused result of leak check
  functions
---

ACK

Regards

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

Re: [SSSD] [PATCH] FAIL_OVER: Fix warning value computed is not used

2015-11-10 Thread Jakub Hrozek 
On Tue, Nov 10, 2015 at 02:39:03PM +0100, Jakub Hrozek wrote:
> On Tue, Nov 10, 2015 at 09:11:25AM +0100, Lukas Slebodnik wrote:
> > ehlo,
> > 
> > another warning on rhel6.
> 
> ACK

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

Re: [SSSD] [PATCH] tools: Don't shadow 'exit'

2015-11-10 Thread Jakub Hrozek 
On Tue, Nov 10, 2015 at 03:06:22PM +0100, Jakub Hrozek wrote:
> On Tue, Nov 10, 2015 at 09:03:55AM +0100, Lukas Slebodnik wrote:
> > On (09/11/15 20:53), Jakub Hrozek wrote:
> > >On Mon, Nov 09, 2015 at 06:35:05PM +0100, Lukas Slebodnik wrote:
> > >> BTW which version do you prefer?
> > >> a) signl
> > >> b) sig
> > >> c) a_signal
> > >
> > >I don't care :) Feel free to use a_signal since it came first.
> > 
> > Updated patches are attached.
> > 
> > LS
> 
> Thanks, ACK
> 
> CI: http://sssd-ci.duckdns.org/logs/job/32/40/summary.html

* master:
* df9e9a1f9b7dc255eb62c390163c25917b08f5a2
* 365fe7479c753f198430812337a7ba8cdb0baf7d
* 32dc4016585cbffc55a92a38e7a1e14c7e1e22ac
* sssd-1-13:
* 8539951ecba35b752850d2eabb1b4120c85005da
* 6eb4f6e8d87c31003b93ef32fff98ab14dccd1de
* 9dffda36139efc76e027c89d4e513ef0e1daf8c2

I also pushed the patches to sssd-1-13 because 1.13 will be supported on
RHEL-6 and it's better not to emit any compilation warnings..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] FAIL_OVER: Fix warning value computed is not used

2015-11-10 Thread Lukas Slebodnik 
On (10/11/15 14:55), Pavel Reichl wrote:
>
>
>On 11/10/2015 02:39 PM, Jakub Hrozek wrote:
>>On Tue, Nov 10, 2015 at 09:11:25AM +0100, Lukas Slebodnik wrote:
>>>ehlo,
>>>
>>>another warning on rhel6.
>>
>>ACK
>>
>>>
>>>BTW different solution would be to cast to void.
>>
>>I prefer this solution
>
>Why? I think that changing value of input parameter is generally a discouraged 
>practice, although in our code base it's common. (I'm just asking I'm *not* 
>nacking the patch.)
>
It is a macro which return itself.
Without changing anything.

It is basicly noop.
  int a = 1;
  a = a;

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

Re: [SSSD] [PATCH] FAIL_OVER: Fix warning value computed is not used

2015-11-10 Thread Jakub Hrozek 
On Tue, Nov 10, 2015 at 02:55:43PM +0100, Pavel Reichl wrote:
> 
> 
> On 11/10/2015 02:39 PM, Jakub Hrozek wrote:
> >On Tue, Nov 10, 2015 at 09:11:25AM +0100, Lukas Slebodnik wrote:
> >>ehlo,
> >>
> >>another warning on rhel6.
> >
> >ACK
> >
> >>
> >>BTW different solution would be to cast to void.
> >
> >I prefer this solution
> 
> Why? I think that changing value of input parameter is generally a 
> discouraged practice, although in our code base it's common. (I'm just asking 
> I'm *not* nacking the patch.)

It seems more readable that the parameter is changed. It's not really
input parameter per se, its refcount is increased.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] tools: Don't shadow 'exit'

2015-11-10 Thread Jakub Hrozek 
On Tue, Nov 10, 2015 at 09:03:55AM +0100, Lukas Slebodnik wrote:
> On (09/11/15 20:53), Jakub Hrozek wrote:
> >On Mon, Nov 09, 2015 at 06:35:05PM +0100, Lukas Slebodnik wrote:
> >> BTW which version do you prefer?
> >> a) signl
> >> b) sig
> >> c) a_signal
> >
> >I don't care :) Feel free to use a_signal since it came first.
> 
> Updated patches are attached.
> 
> LS

Thanks, ACK

CI: http://sssd-ci.duckdns.org/logs/job/32/40/summary.html
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] FAIL_OVER: Fix warning value computed is not used

2015-11-10 Thread Pavel Reichl 



On 11/10/2015 03:05 PM, Jakub Hrozek wrote:

On Tue, Nov 10, 2015 at 02:55:43PM +0100, Pavel Reichl wrote:



On 11/10/2015 02:39 PM, Jakub Hrozek wrote:

On Tue, Nov 10, 2015 at 09:11:25AM +0100, Lukas Slebodnik wrote:

ehlo,

another warning on rhel6.


ACK



BTW different solution would be to cast to void.


I prefer this solution


Why? I think that changing value of input parameter is generally a discouraged 
practice, although in our code base it's common. (I'm just asking I'm *not* 
nacking the patch.)


It seems more readable that the parameter is changed. It's not really
input parameter per se, its refcount is increased.


OK, thanks both.


___
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] [PATCH] FAIL_OVER: Fix warning value computed is not used

2015-11-10 Thread Pavel Reichl 



On 11/10/2015 02:39 PM, Jakub Hrozek wrote:

On Tue, Nov 10, 2015 at 09:11:25AM +0100, Lukas Slebodnik wrote:

ehlo,

another warning on rhel6.


ACK



BTW different solution would be to cast to void.


I prefer this solution


Why? I think that changing value of input parameter is generally a discouraged 
practice, although in our code base it's common. (I'm just asking I'm *not* 
nacking the patch.)



CI: http://sssd-ci.duckdns.org/logs/job/32/38/summary.html

(Fedora-20 failure is unrelated)
___
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] [PATCH] FAIL_OVER: Fix warning value computed is not used

2015-11-10 Thread Jakub Hrozek 
On Tue, Nov 10, 2015 at 09:11:25AM +0100, Lukas Slebodnik wrote:
> ehlo,
> 
> another warning on rhel6.

ACK

> 
> BTW different solution would be to cast to void.

I prefer this solution

CI: http://sssd-ci.duckdns.org/logs/job/32/38/summary.html

(Fedora-20 failure is unrelated)
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] BE: Add IFP to known clients

2015-11-10 Thread Lukas Slebodnik 
On (10/11/15 12:10), Pavel Březina wrote:
>This gets rid of confusing debug message:
>[be_client_destructor] (0x0020): Unknown client removed ...

>From 17b1d8216bab3770c58c79cf51c571cb184e8ab4 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
>Date: Tue, 10 Nov 2015 11:47:30 +0100
>Subject: [PATCH] BE: Add IFP to known clients
>
>This gets rid of confusing debug message:
>[be_client_destructor] (0x0020): Unknown client removed ...
>---
> src/providers/data_provider_be.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/src/providers/data_provider_be.c 
>b/src/providers/data_provider_be.c
>index 
>effa185f905097e0be1e03596b5fc6da75a7e382..104016ca19c093dcbb4b0c1011f06e4bb4e2a49c
> 100644
>--- a/src/providers/data_provider_be.c
>+++ b/src/providers/data_provider_be.c
>@@ -2107,6 +2107,9 @@ static int be_client_destructor(void *ctx)
> } else if (becli->bectx->pac_cli == becli) {
> DEBUG(SSSDBG_TRACE_FUNC, "Removed PAC client\n");
> becli->bectx->pac_cli = NULL;
>+} else if (becli->bectx->ifp_cli == becli) {
>+DEBUG(SSSDBG_TRACE_FUNC, "Removed IFP client\n");
>+becli->bectx->ifp_cli = NULL;
> } else {
> DEBUG(SSSDBG_CRIT_FAILURE, "Unknown client removed ...\n");
> }

ACK
http://sssd-ci.duckdns.org/logs/job/32/41/

The simples reproducer is
  pkill -9 sssD_ifp

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