[SSSD] Time-Based Policies in FreeIPA

2015-07-14 Thread Stanislav Laznicka 

Hi,

If you're a member of the FreeIPA-devel mailing list, you may already 
know me. I am currently trying to re-introduce the time-based policies 
for FreeIPA HBAC Rules to both FreeIPA and SSSD. For some more 
information, see the design at 
http://www.freeipa.org/page/V4/Time-Based_Account_Policies or read the 
whole paper at http://www.fit.vutbr.cz/study/DP/DP.php.cs?id=17185file=t.


At the moment, I have a prototype solution which is able to decide 
whether an HBAC rule should or should not apply based on the time rules 
supplied with HBAC rule objects from FreeIPA. See the source code 
attached to this mail, if you will. The biggest problem of this solution 
is that it is not thread-safe.


It seems to be quite a problem to convert time from a certain time zone 
to a different time zone in C and keep it thread-safe. A very simple and 
also very ugly solution would be to have a mutex to guard each 
localtime() call as well as it should wrap the body of the 
time_to_timezone() function from the second patch. This seems rather 
unacceptable. The other solution would be to find another way to convert 
the time. Currently, there seems to be a C++ Boost solution based on a 
.csv file but it is not accepted well 
(https://github.com/boostorg/date_time/blob/master/data/date_time_zonespec.csv). 
I was also thinking on using the glibc tzfile parsers 
(http://code.woboq.org/userspace/glibc/time/tzfile.c.html#__tzfile_read) 
but they too seem rather thread-unsafe and trying to rework it in a 
thread-safe manner might be a painful thing to do.


I welcome any suggestions and ideas on the topic as I seem to be quite 
stuck here.


Cheers,
Stanislav Laznicka
From 428db72ca0a07c7dba1a6959696b3dc2df7d77ec Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka slazn...@redhat.com
Date: Mon, 13 Jul 2015 09:53:10 +0200
Subject: [PATCH 1/2] Added caching of time policies for IPA HBAC rules.

The time policies which are part of the FreeIPA HBAC Rule object
are now being cached along with other HBAC Rule objects' attributes.
Also, the cached time policies are transformed into hbac_time_rules
structure representation.

https://fedorahosted.org/freeipa/ticket/547
https://fedorahosted.org/freeipa/ticket/548
---
 Makefile.am  |   4 --
 src/providers/ipa/hbac_evaluator.c   |   7 --
 src/providers/ipa/ipa_access.c   |   3 +
 src/providers/ipa/ipa_hbac.h |   6 +-
 src/providers/ipa/ipa_hbac_common.c  | 120 +++
 src/providers/ipa/ipa_hbac_private.h |  10 +++
 src/providers/ipa/ipa_hbac_rules.c   |   7 +-
 src/tests/ipa_hbac-tests.c   |   6 ++
 8 files changed, 149 insertions(+), 14 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index b8cbc6df23ded1edb945a709b6dbe1c44eb54017..8665a0560cc3cd57f148325640c2e709a05f4c2a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -264,14 +264,10 @@ PYTHON_TESTS =
 
 if BUILD_PYTHON2_BINDINGS
 PYTHON_TESTS += src/config/SSSDConfigTest.py2.sh \
-src/tests/pyhbac-test.py2.sh \
-src/tests/pysss_murmur-test.py2.sh \
 $(NULL)
 endif
 if BUILD_PYTHON3_BINDINGS
 PYTHON_TESTS += src/config/SSSDConfigTest.py3.sh \
-src/tests/pyhbac-test.py3.sh \
-src/tests/pysss_murmur-test.py3.sh \
 $(NULL)
 endif
 
diff --git a/src/providers/ipa/hbac_evaluator.c b/src/providers/ipa/hbac_evaluator.c
index f40f9e0a7f16f5e012079c637b89c8e49ec5d15b..64af36aae2855d0fb5a737989cc3ee8959e1b5b6 100644
--- a/src/providers/ipa/hbac_evaluator.c
+++ b/src/providers/ipa/hbac_evaluator.c
@@ -38,13 +38,6 @@ typedef int errno_t;
 #define EOK 0
 #endif
 
-/* Placeholder structure for future HBAC time-based
- * evaluation rules
- */
-struct hbac_time_rules {
-int not_yet_implemented;
-};
-
 enum hbac_eval_result_int {
 HBAC_EVAL_MATCH_ERROR = -1,
 HBAC_EVAL_MATCHED,
diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
index 3198e2bd2a4c8355eeccc129c85ae3d7d67f61b0..c60775fd01f816c19b3617caa04088a8052d35ed 100644
--- a/src/providers/ipa/ipa_access.c
+++ b/src/providers/ipa/ipa_access.c
@@ -686,6 +686,9 @@ errno_t hbac_get_cached_rules(TALLOC_CTX *mem_ctx,
 IPA_EXTERNAL_HOST,
 IPA_MEMBER_HOST,
 IPA_HOST_CATEGORY,
+IPA_TIMEZONE,
+IPA_ACCESSTIME,
+IPA_ACCESSTIME_EXCLUDE,
 NULL };
 
 tmp_ctx = talloc_new(NULL);
diff --git a/src/providers/ipa/ipa_hbac.h b/src/providers/ipa/ipa_hbac.h
index f43611351c8a5dfb20ca3d075f0bcd7bb71798c9..7f8d4ecf78aced49c5523f0487269f03aa9e6569 100644
--- a/src/providers/ipa/ipa_hbac.h
+++ b/src/providers/ipa/ipa_hbac.h
@@ -73,7 +73,11 @@ enum hbac_eval_result {
 /**
  * Opaque type contained in hbac_evaluator.c
  */
-struct hbac_time_rules;
+ struct hbac_time_rules {
+ const char *timezone;
+ const char

Re: [SSSD] Time-Based Policies in FreeIPA

2015-07-14 Thread Stanislav Laznicka 

On 07/14/2015 09:32 AM, Lukas Slebodnik wrote:

On (14/07/15 09:11), Stanislav Laznicka wrote:

Hi,

If you're a member of the FreeIPA-devel mailing list, you may already know
me. I am currently trying to re-introduce the time-based policies for FreeIPA
HBAC Rules to both FreeIPA and SSSD. For some more information, see the
design athttp://www.freeipa.org/page/V4/Time-Based_Account_Policies  or read
the whole paper at
http://www.fit.vutbr.cz/study/DP/DP.php.cs?id=17185file=t.

At the moment, I have a prototype solution which is able to decide whether an
HBAC rule should or should not apply based on the time rules supplied with
HBAC rule objects from FreeIPA. See the source code attached to this mail, if
you will. The biggest problem of this solution is that it is not thread-safe.

It seems to be quite a problem to convert time from a certain time zone to a
different time zone in C and keep it thread-safe. A very simple and also very
ugly solution would be to have a mutex to guard each localtime() call as well
as it should wrap the body of the time_to_timezone() function from the second
patch. This seems rather unacceptable. The other solution would be to find
another way to convert the time. Currently, there seems to be a C++ Boost
solution based on a .csv file but it is not accepted well 
(https://github.com/boostorg/date_time/blob/master/data/date_time_zonespec.csv).
I was also thinking on using the glibc tzfile parsers
(http://code.woboq.org/userspace/glibc/time/tzfile.c.html#__tzfile_read) but
they too seem rather thread-unsafe and trying to rework it in a thread-safe
manner might be a painful thing to do.

I welcome any suggestions and ideas on the topic as I seem to be quite stuck
here.

Cheers,
Stanislav Laznicka

I did a very brief review of Makefile changes
I have a few questions.

From 428db72ca0a07c7dba1a6959696b3dc2df7d77ec Mon Sep 17 00:00:00 2001

From: Stanislav Laznickaslazn...@redhat.com
Date: Mon, 13 Jul 2015 09:53:10 +0200
Subject: [PATCH 1/2] Added caching of time policies for IPA HBAC rules.

The time policies which are part of the FreeIPA HBAC Rule object
are now being cached along with other HBAC Rule objects' attributes.
Also, the cached time policies are transformed into hbac_time_rules
structure representation.

https://fedorahosted.org/freeipa/ticket/547
https://fedorahosted.org/freeipa/ticket/548
---
Makefile.am  |   4 --
src/providers/ipa/hbac_evaluator.c   |   7 --
src/providers/ipa/ipa_access.c   |   3 +
src/providers/ipa/ipa_hbac.h |   6 +-
src/providers/ipa/ipa_hbac_common.c  | 120 +++
src/providers/ipa/ipa_hbac_private.h |  10 +++
src/providers/ipa/ipa_hbac_rules.c   |   7 +-
src/tests/ipa_hbac-tests.c   |   6 ++
8 files changed, 149 insertions(+), 14 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 
b8cbc6df23ded1edb945a709b6dbe1c44eb54017..8665a0560cc3cd57f148325640c2e709a05f4c2a
 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -264,14 +264,10 @@ PYTHON_TESTS =

if BUILD_PYTHON2_BINDINGS
PYTHON_TESTS += src/config/SSSDConfigTest.py2.sh \
-src/tests/pyhbac-test.py2.sh \
-src/tests/pysss_murmur-test.py2.sh \
 $(NULL)
endif
if BUILD_PYTHON3_BINDINGS
PYTHON_TESTS += src/config/SSSDConfigTest.py3.sh \
-src/tests/pyhbac-test.py3.sh \
-src/tests/pysss_murmur-test.py3.sh \
 $(NULL)

Could you explain why did you remove pysss_murmur-test?
How is it related to hbac?

It was probably just a mistake.

BTW I would prefer to temporary disable pyhbac-test test
or fix it rather that removing it.
I made a brief try to fix it but at the time I was writing the code, it 
was not my main goal so I decided just to remove it. I will fix it and 
add the python tests back once the issues with the solution itself are 
resolved.

From 856dc04c7bd9b4f46637c7c598dfff0bb8b95105 Mon Sep 17 00:00:00 2001

From: Stanislav Laznickaslazn...@redhat.com
Date: Mon, 13 Jul 2015 10:00:42 +0200
Subject: [PATCH 2/2] Added evaluation of time-policies in HBAC objects.

The time-policies in FreeIPA HBAC objects are now evaluated in
the libipa_hbac module.

FIXME: The evaluation is not thread-safe. See the time_to_timezone
function in ipa_timerules.c

https://fedorahosted.org/freeipa/ticket/547
https://fedorahosted.org/freeipa/ticket/548
---
Makefile.am|   2 +
src/providers/ipa/hbac_evaluator.c |  17 +-
src/providers/ipa/ipa_timerules.c  | 488 +
src/providers/ipa/ipa_timerules.h  |  40 +++
4 files changed, 545 insertions(+), 2 deletions(-)
create mode 100644 src/providers/ipa/ipa_timerules.c
create mode 100644 src/providers/ipa/ipa_timerules.h

diff --git a/Makefile.am b/Makefile.am
index 
8665a0560cc3cd57f148325640c2e709a05f4c2a..937cc99757b501f953c91aef40a13e69fb4c4f4e
 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -600,6 +600,7 @@ dist_noinst_HEADERS = \

Re: [SSSD] Time-Based Policies in FreeIPA

2015-07-14 Thread Lukas Slebodnik 
On (14/07/15 09:11), Stanislav Laznicka wrote:
Hi,

If you're a member of the FreeIPA-devel mailing list, you may already know
me. I am currently trying to re-introduce the time-based policies for FreeIPA
HBAC Rules to both FreeIPA and SSSD. For some more information, see the
design at http://www.freeipa.org/page/V4/Time-Based_Account_Policies or read
the whole paper at
http://www.fit.vutbr.cz/study/DP/DP.php.cs?id=17185file=t.

At the moment, I have a prototype solution which is able to decide whether an
HBAC rule should or should not apply based on the time rules supplied with
HBAC rule objects from FreeIPA. See the source code attached to this mail, if
you will. The biggest problem of this solution is that it is not thread-safe.

It seems to be quite a problem to convert time from a certain time zone to a
different time zone in C and keep it thread-safe. A very simple and also very
ugly solution would be to have a mutex to guard each localtime() call as well
as it should wrap the body of the time_to_timezone() function from the second
patch. This seems rather unacceptable. The other solution would be to find
another way to convert the time. Currently, there seems to be a C++ Boost
solution based on a .csv file but it is not accepted well 
(https://github.com/boostorg/date_time/blob/master/data/date_time_zonespec.csv).
I was also thinking on using the glibc tzfile parsers
(http://code.woboq.org/userspace/glibc/time/tzfile.c.html#__tzfile_read) but
they too seem rather thread-unsafe and trying to rework it in a thread-safe
manner might be a painful thing to do.

I welcome any suggestions and ideas on the topic as I seem to be quite stuck
here.

Cheers,
Stanislav Laznicka

I did a very brief review of Makefile changes
I have a few questions.

From 428db72ca0a07c7dba1a6959696b3dc2df7d77ec Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka slazn...@redhat.com
Date: Mon, 13 Jul 2015 09:53:10 +0200
Subject: [PATCH 1/2] Added caching of time policies for IPA HBAC rules.

The time policies which are part of the FreeIPA HBAC Rule object
are now being cached along with other HBAC Rule objects' attributes.
Also, the cached time policies are transformed into hbac_time_rules
structure representation.

https://fedorahosted.org/freeipa/ticket/547
https://fedorahosted.org/freeipa/ticket/548
---
 Makefile.am  |   4 --
 src/providers/ipa/hbac_evaluator.c   |   7 --
 src/providers/ipa/ipa_access.c   |   3 +
 src/providers/ipa/ipa_hbac.h |   6 +-
 src/providers/ipa/ipa_hbac_common.c  | 120 +++
 src/providers/ipa/ipa_hbac_private.h |  10 +++
 src/providers/ipa/ipa_hbac_rules.c   |   7 +-
 src/tests/ipa_hbac-tests.c   |   6 ++
 8 files changed, 149 insertions(+), 14 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 
b8cbc6df23ded1edb945a709b6dbe1c44eb54017..8665a0560cc3cd57f148325640c2e709a05f4c2a
 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -264,14 +264,10 @@ PYTHON_TESTS =
 
 if BUILD_PYTHON2_BINDINGS
 PYTHON_TESTS += src/config/SSSDConfigTest.py2.sh \
-src/tests/pyhbac-test.py2.sh \
-src/tests/pysss_murmur-test.py2.sh \
 $(NULL)
 endif
 if BUILD_PYTHON3_BINDINGS
 PYTHON_TESTS += src/config/SSSDConfigTest.py3.sh \
-src/tests/pyhbac-test.py3.sh \
-src/tests/pysss_murmur-test.py3.sh \
 $(NULL)
Could you explain why did you remove pysss_murmur-test?
How is it related to hbac?

BTW I would prefer to temporary disable pyhbac-test test
or fix it rather that removing it.

From 856dc04c7bd9b4f46637c7c598dfff0bb8b95105 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka slazn...@redhat.com
Date: Mon, 13 Jul 2015 10:00:42 +0200
Subject: [PATCH 2/2] Added evaluation of time-policies in HBAC objects.

The time-policies in FreeIPA HBAC objects are now evaluated in
the libipa_hbac module.

FIXME: The evaluation is not thread-safe. See the time_to_timezone
function in ipa_timerules.c

https://fedorahosted.org/freeipa/ticket/547
https://fedorahosted.org/freeipa/ticket/548
---
 Makefile.am|   2 +
 src/providers/ipa/hbac_evaluator.c |  17 +-
 src/providers/ipa/ipa_timerules.c  | 488 +
 src/providers/ipa/ipa_timerules.h  |  40 +++
 4 files changed, 545 insertions(+), 2 deletions(-)
 create mode 100644 src/providers/ipa/ipa_timerules.c
 create mode 100644 src/providers/ipa/ipa_timerules.h

diff --git a/Makefile.am b/Makefile.am
index 
8665a0560cc3cd57f148325640c2e709a05f4c2a..937cc99757b501f953c91aef40a13e69fb4c4f4e
 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -600,6 +600,7 @@ dist_noinst_HEADERS = \
 src/providers/ldap/sdap_dyndns.h \
 src/providers/ldap/sdap_async_enum.h \
 src/providers/ipa/ipa_common.h \
+  src/providers/ipa/ipa_timerules.h \
  ^^
  '\t' used instead of spaces.
 src/providers/ipa/ipa_config.h \
 src/providers/ipa/ipa_access.h \

Re: [SSSD] Time-Based Policies in FreeIPA

2015-07-14 Thread Sumit Bose 
On Tue, Jul 14, 2015 at 09:11:28AM +0200, Stanislav Laznicka wrote:
 Hi,
 
 If you're a member of the FreeIPA-devel mailing list, you may already know
 me. I am currently trying to re-introduce the time-based policies for
 FreeIPA HBAC Rules to both FreeIPA and SSSD. For some more information, see
 the design at http://www.freeipa.org/page/V4/Time-Based_Account_Policies or
 read the whole paper at
 http://www.fit.vutbr.cz/study/DP/DP.php.cs?id=17185file=t.
 
 At the moment, I have a prototype solution which is able to decide whether
 an HBAC rule should or should not apply based on the time rules supplied
 with HBAC rule objects from FreeIPA. See the source code attached to this
 mail, if you will. The biggest problem of this solution is that it is not
 thread-safe.

I have not look in detain at you patches but I think thread safety is
not an issue here. The HBAC evaluation is done in a single process
without using threads. To handle concurrent task we use the tevent
library and asynchronous request (see
https://tevent.samba.org/tevent_request.html for details). This means
whatever you do will change the state of the whole process but as long
as you do it in a single function and make sure to restore the original
state it might be safe. I say it might because tevent handles signals as
well so we have to make sure the state can be recovered even if a single
handler runs in between.

bye,
Sumit

 
 It seems to be quite a problem to convert time from a certain time zone to a
 different time zone in C and keep it thread-safe. A very simple and also
 very ugly solution would be to have a mutex to guard each localtime() call
 as well as it should wrap the body of the time_to_timezone() function from
 the second patch. This seems rather unacceptable. The other solution would
 be to find another way to convert the time. Currently, there seems to be a
 C++ Boost solution based on a .csv file but it is not accepted well 
 (https://github.com/boostorg/date_time/blob/master/data/date_time_zonespec.csv).
 I was also thinking on using the glibc tzfile parsers
 (http://code.woboq.org/userspace/glibc/time/tzfile.c.html#__tzfile_read) but
 they too seem rather thread-unsafe and trying to rework it in a thread-safe
 manner might be a painful thing to do.
 
 I welcome any suggestions and ideas on the topic as I seem to be quite stuck
 here.
 
 Cheers,
 Stanislav Laznicka

 From 428db72ca0a07c7dba1a6959696b3dc2df7d77ec Mon Sep 17 00:00:00 2001
 From: Stanislav Laznicka slazn...@redhat.com
 Date: Mon, 13 Jul 2015 09:53:10 +0200
 Subject: [PATCH 1/2] Added caching of time policies for IPA HBAC rules.
 
 The time policies which are part of the FreeIPA HBAC Rule object
 are now being cached along with other HBAC Rule objects' attributes.
 Also, the cached time policies are transformed into hbac_time_rules
 structure representation.
 
 https://fedorahosted.org/freeipa/ticket/547
 https://fedorahosted.org/freeipa/ticket/548
 ---
  Makefile.am  |   4 --
  src/providers/ipa/hbac_evaluator.c   |   7 --
  src/providers/ipa/ipa_access.c   |   3 +
  src/providers/ipa/ipa_hbac.h |   6 +-
  src/providers/ipa/ipa_hbac_common.c  | 120 
 +++
  src/providers/ipa/ipa_hbac_private.h |  10 +++
  src/providers/ipa/ipa_hbac_rules.c   |   7 +-
  src/tests/ipa_hbac-tests.c   |   6 ++
  8 files changed, 149 insertions(+), 14 deletions(-)
 
 diff --git a/Makefile.am b/Makefile.am
 index 
 b8cbc6df23ded1edb945a709b6dbe1c44eb54017..8665a0560cc3cd57f148325640c2e709a05f4c2a
  100644
 --- a/Makefile.am
 +++ b/Makefile.am
 @@ -264,14 +264,10 @@ PYTHON_TESTS =
  
  if BUILD_PYTHON2_BINDINGS
  PYTHON_TESTS += src/config/SSSDConfigTest.py2.sh \
 -src/tests/pyhbac-test.py2.sh \
 -src/tests/pysss_murmur-test.py2.sh \
  $(NULL)
  endif
  if BUILD_PYTHON3_BINDINGS
  PYTHON_TESTS += src/config/SSSDConfigTest.py3.sh \
 -src/tests/pyhbac-test.py3.sh \
 -src/tests/pysss_murmur-test.py3.sh \
  $(NULL)
  endif
  
 diff --git a/src/providers/ipa/hbac_evaluator.c 
 b/src/providers/ipa/hbac_evaluator.c
 index 
 f40f9e0a7f16f5e012079c637b89c8e49ec5d15b..64af36aae2855d0fb5a737989cc3ee8959e1b5b6
  100644
 --- a/src/providers/ipa/hbac_evaluator.c
 +++ b/src/providers/ipa/hbac_evaluator.c
 @@ -38,13 +38,6 @@ typedef int errno_t;
  #define EOK 0
  #endif
  
 -/* Placeholder structure for future HBAC time-based
 - * evaluation rules
 - */
 -struct hbac_time_rules {
 -int not_yet_implemented;
 -};
 -
  enum hbac_eval_result_int {
  HBAC_EVAL_MATCH_ERROR = -1,
  HBAC_EVAL_MATCHED,
 diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
 index 
 3198e2bd2a4c8355eeccc129c85ae3d7d67f61b0..c60775fd01f816c19b3617caa04088a8052d35ed
  100644
 --- a/src/providers/ipa/ipa_access.c
 +++ b/src/providers/ipa/ipa_access.c
 @@ -686,6 +686,9 @@ errno_t hbac_get_cached_rules(TALLOC_CTX *mem_ctx,

Re: [SSSD] [PATCH] UTIL: Function 2string for enum sss_cli_command

2015-07-14 Thread Petr Cech 

On 07/13/2015 07:13 PM, Lukas Slebodnik wrote:

On (13/07/15 10:57), Jakub Hrozek wrote:

On Mon, Jul 13, 2015 at 09:47:46AM +0200, Lukas Slebodnik wrote:

On (10/07/15 16:54), Jakub Hrozek wrote:

On Wed, Jul 08, 2015 at 03:26:52PM +0200, Sumit Bose wrote:

   I would suggest that you put sss_cli_command_2string() in a file on
   its own similar like atomic_io.c or authtok-utils.c. And add this file
   to pam_sss_la_SOURCES and libsss_debug_la_SOURCES in Makefile.am. I
   leave it up to you to decide what would be a good place for this file.
   The sss_client directory because the enum sss_cli_command is defined
   here as well or the util directory because the main usage for it is in
   the SSSD code and not in the pam_sss module.

This is really important, so much that I wonder if we should move all
the files that are used by both client code and daemon code to some new
directory in the SSSD tree (src/shared/ maybe) and use a different comment
header in these files.

We do not need to use sss_cmd2str in client code.
If you wan to see debug messages from pam_sss module then you
need to recompile source code with extra CFLAG to enable them.

Good point.


It very unlikely that debug messages in pam_sss code will used by users.
I would prefer do not touch client code or used just hexadecimal
represaentation (the same as in header file)

I agree, let's not touch the client unless needed.

Another reason for not using sss_cmd2str in client code is that
it depends on our debug_fn from internal library libsss_debug.

Even thought the function sss_cmd2str was not used in pam_sss.c
it was still linked with pam_sss.so and thus dlopen test failed.
Petr already noticed it; This mail is just summary of off the list
discussion.

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


Hi,

there is another repaired patch.
Changes are:
* hexadecimal numbers instead of cmd2str() in sss_client,
* added license preamble in headers of new files.

Andthere is a comment of Lukas Slebodnik for that I need more investigation.
 BTW It would be good to use new function also in backend code.
 src/providers/data_provider_be.c:1107:  Got request for 
[%#x][%d][%s]\n, type, attr_type, filter);

 I used to filter debug messages for be_get_account_info which print
 type as hexadecimal number. Maybe there are also other places.
 LS

Petr
From a93e36f11759cf9a748942e7632d4a07a088b098 Mon Sep 17 00:00:00 2001
From: Petr Cech pc...@redhat.com
Date: Wed, 8 Jul 2015 07:17:28 -0400
Subject: [PATCH] UTIL: Function 2string for enum sss_cli_command

Improvement of debug messages.
Instead of:(0x0400): Running command [17]...
We could see:(0x0400): Running command [17][SSS_NSS_GETPWNAM]...
(It's not used in sss_client. There are only hex numbers of commands.)

Resolves:
https://fedorahosted.org/sssd/ticket/2708
---
 Makefile.am  |   3 +-
 src/providers/dp_pam_data_util.c |  27 +
 src/responder/nss/nsssrv_cmd.c   |  30 ++---
 src/sss_client/pam_sss.c |   6 +-
 src/tools/tools_mc_util.c|   4 +-
 src/util/sss_cli_cmd.c   | 238 +++
 src/util/sss_cli_cmd.h   |  28 +
 7 files changed, 293 insertions(+), 43 deletions(-)
 create mode 100644 src/util/sss_cli_cmd.c
 create mode 100644 src/util/sss_cli_cmd.h

diff --git a/Makefile.am b/Makefile.am
index b8cbc6df23ded1edb945a709b6dbe1c44eb54017..430f2292a1be9e0f0b7cb56e8ecbf179e9978dcd 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -678,7 +678,8 @@ endif
 pkglib_LTLIBRARIES += libsss_debug.la
 libsss_debug_la_SOURCES = \
 src/util/debug.c \
-src/util/sss_log.c
+src/util/sss_log.c \
+src/util/sss_cli_cmd.c
 libsss_debug_la_LIBADD = \
 $(SYSLOG_LIBS)
 libsss_debug_la_LDFLAGS = \
diff --git a/src/providers/dp_pam_data_util.c b/src/providers/dp_pam_data_util.c
index 8724bf936f3f46fb8393c8a3da57215a73b4191a..10e91f5f7286db5e76ad98b6c7519f2482d006db 100644
--- a/src/providers/dp_pam_data_util.c
+++ b/src/providers/dp_pam_data_util.c
@@ -23,33 +23,10 @@
 */
 
 #include providers/data_provider.h
-
+#include util/sss_cli_cmd.h
 
 #define PAM_SAFE_ITEM(item) item ? item : not set
 
-static const char *pamcmd2str(int cmd) {
-switch (cmd) {
-case SSS_PAM_AUTHENTICATE:
-return PAM_AUTHENTICATE;
-case SSS_PAM_SETCRED:
-return PAM_SETCRED;
-case SSS_PAM_ACCT_MGMT:
-return PAM_ACCT_MGMT;
-case SSS_PAM_OPEN_SESSION:
-return PAM_OPEN_SESSION;
-case SSS_PAM_CLOSE_SESSION:
-return PAM_CLOSE_SESSION;
-case SSS_PAM_CHAUTHTOK:
-return PAM_CHAUTHTOK;
-case SSS_PAM_CHAUTHTOK_PRELIM:
-return PAM_CHAUTHTOK_PRELIM;
-case SSS_PAM_PREAUTH:
-return SSS_PAM_PREAUTH;
-default:
-return UNKNOWN;
-}
-}
-
 int pam_data_destructor(void *ptr)
 {
 struct pam_data *pd = talloc_get_type(ptr,

Re: [SSSD] [PATCH] Update few debug messages

2015-07-14 Thread Lukas Slebodnik 
On (19/06/15 12:53), Pavel Reichl wrote:
On 06/19/2015 10:46 AM, Lukas Slebodnik wrote:
On (19/06/15 10:37), Pavel Reichl wrote:
On 06/18/2015 06:16 PM, Lukas Slebodnik wrote:
ehlo,

I noticed the same noise in log files as user in ticker 2678.
It was a little bit related to the latest changes with setting
initgroups flag in the right time.

LS


Hello,
the patch won't apply on current master. Does it depend on some patch which
is being reviewed?
Yes,
It is related to [PATCH] SDAP: Remove user from cache for missing user in
LDAP. I didn't send them together because this patch does not fix regression.

LS
OK thanks, the only nitpick I found is usage of strerror, I know that Michal
has patch to change this globally but I think that we are in agreement to use
sss_strerror in changed/new code anyway, right?
+} else if (ret != ENOENT) {
 DEBUG(SSSDBG_OP_FAILURE,
-  sysdb_search_object_by_uuid did not return a 
\
-  single result.\n);
+  sysdb_search_object_by_uuid failed or returned

+  more than one result [%d][%s].\n,
+  ret, strerror(ret));

Otherwise code LGTM and CI passed, but I assume that I can't ack before
[PATCH] SDAP: Remove user from cache for missing user in LDAP gets pushed
to master anyway.


Patch could not be applied on top of master due to conflict.

Updated patch is attached.

LS
From 532b28d6af61f4137da5cbae3dff90265b1f4f1c Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik lsleb...@redhat.com
Date: Tue, 14 Jul 2015 13:16:39 +0200
Subject: [PATCH] Update few debug messages

It reduces a noise caused by canonicalization of non-existing user.

Resolves:
https://fedorahosted.org/sssd/ticket/2678
---
 src/db/sysdb_search.c| 7 ---
 src/providers/ldap/ldap_id.c | 5 +++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
index 
a8dcc9f8d6617be8e8fb82a1c6360c6df9726a37..c92474875a07059abff27e8d329a24187dde97a2
 100644
--- a/src/db/sysdb_search.c
+++ b/src/db/sysdb_search.c
@@ -1613,10 +1613,11 @@ errno_t sysdb_get_real_name(TALLOC_CTX *mem_ctx,
   res);
 if (ret == EOK  res-count == 1) {
 msg = res-msgs[0];
-} else {
+} else if (ret != ENOENT) {
 DEBUG(SSSDBG_OP_FAILURE,
-  sysdb_search_object_by_uuid did not return a  \
-  single result.\n);
+  sysdb_search_object_by_uuid failed or returned 
+  more than one result [%d][%s].\n,
+  ret, strerror(ret));
 ret = ENOENT;
 goto done;
 }
diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
index 
3245e1b12a69483f961f01210d13654b1c7c5345..89fc9aac64c2f1cba8f6bf793aa6fd262aefcf58
 100644
--- a/src/providers/ldap/ldap_id.c
+++ b/src/providers/ldap/ldap_id.c
@@ -1178,8 +1178,9 @@ static void groups_by_user_done(struct tevent_req *subreq)
 ret = sysdb_get_real_name(state, state-domain, state-name, cname);
 if (ret != EOK) {
 cname = state-name;
-DEBUG(SSSDBG_OP_FAILURE,
-  Failed to canonicalize name, using [%s].\n, cname);
+DEBUG(SSSDBG_TRACE_INTERNAL,
+  Failed to canonicalize name, using [%s] [%d]: %s.\n,
+  cname, ret, sss_strerror(ret));
 }
 }
 
-- 
2.4.3

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

Re: [SSSD] [PATCHES] Wildcard lookups for InfoPipe

2015-07-14 Thread Pavel Březina 

On 07/13/2015 06:25 PM, Jakub Hrozek wrote:

On Fri, Jul 10, 2015 at 01:30:07PM +0200, Pavel Březina wrote:

On 07/09/2015 12:17 PM, Jakub Hrozek wrote:

It may be also better
to move them into the sysdb module?


I would do that if we anticipate more listing intefaces using this
kind of filter.


I disagree since in my opinion it belongs into sysdb, but I will not insist.


If I read the code correctly, you always return only records that were
updated. It this sufficient? I think we need to return all records that
match the provided filter.


Please check my logic here, but I think the way it's coded now is
safer. On the back end side, we can't reasonably detect if an entry has
been removed if we only use the wildcard lookup. So the back end can't
reasonably decide which entries have been removed from the server.

The reason is the limit. There might be more entries on the server side
that those that match the filter, so what the back end sees is just a
subset. And the entries outside the limit can be cached by direct
lookups, so we simply shouldn't touch them..

What the code does now is only returns entries updated by the wildcard
lookup. The entries that were not updated are not returned to the
fronted -- they just sit in the cache until some application requests
them with a direct lookup. The direct lookup can detect removal of the
object on the server side and also remove the cached object.

Does that make sense?


Yes, thank you for explanation. I think the logic is sound.


if (state-result == NULL || state-result-count == 0 ||
state-input-type == CACHE_REQ_USER_BY_FILTER ||
state-input-type == CACHE_REQ_GROUP_BY_FILTER) {
ret = ENOENT;
} else {


I would like to avoid this type of code whenever possible. Can we replace it
with a macro e.g. always_check_cache(state-input) or function?


OK, done. Please feel free to suggest a better function name.



Is it a valid operation to use a filter that needs to be parse into name and
domain (user*@domain)?


I don't think so, we have a specialized function for that.


ListByDomainAndName


Ah, ok.





What function?

*Patch #01 tests: Move N_ELEMENTS definition to tests/common.h*
ACK

*Patch #02 SYSDB: Add functions to look up multiple entries including name
and custom filter*


+static char *enum_filter(TALLOC_CTX *mem_ctx,
+ const char *base_filter,
+ const char *name_filter,
+ const char *addtl_filter)


You are leaking memory here if any of the allocation fails. I know it will
be freed in the caller but it is not a good practice.


As discussed on IRC, I added a context.


Nack. You added a tmp_ctx but you still use mem_ctx :-)





*Patch #03 DP: Add DP_WILDCARD and
SSS_DP_WILDCARD_USER/SSS_DP_WILDCARD_GROUP*


+} else if (info-type == SSS_DP_WILDCARD_USER ||
+   info-type == SSS_DP_WILDCARD_GROUP) {
+if (info-extra) {
+filter = talloc_asprintf(info, %s=%s:%s, DP_WILDCARD,
+   info-opt_name, info-extra);
+} else {
+filter = talloc_asprintf(info, %s=%s, DP_WILDCARD,
+   info-opt_name);
+}


Do you think it is safe to use the same prefix for both users and groups
look up?


yes, because the target is defined with be_type.


Ok. Ack.





*Patch #04 cache_req: Extend cache_req with wildcard lookups*


case CACHE_REQ_USER_BY_FILTER:
case CACHE_REQ_GROUP_BY_FILTER:
/* Nothing to do, adding a wildcard request to ncache doesn't
 * make sense */



/* Return true if the request can be cached or false if the cache_req
* code needs to contact the DP every time
*/


Can you finish the sentence with dot so it is consistent with the rest of
the code?


Done.




static bool cache_req_cachable(struct cache_req_input *input)


How about cache_req_avoid_cache?


OK, renamed.



*Patch #05 UTIL: Add sss_filter_sanitize_ex*


+const char has_all_allow_asterisk_expected[] = \\5c\\28user\\29*name;


Can you add comment with unescaped characters so it is human-readable?


Done.



*Patch #06 LDAP: Fetch users and groups using wildcards*


@@ -953,6 +991,14 @@ static void groups_get_done(struct tevent_req *subreq)
  * group we have nothing to do here. */
 break;

+case BE_FILTER_WILDCARD:
+/* We can't know if all users are up-to-date, especially in a large
+ * environment. Do not delete any records, let the responder fetch
+ * the entries they are requested in
+ */
+break;


Copy and paste error... you meant groups here.


Yes, thanks. Fixed.



*Patch #07 LDAP: Add sdap_get_and_parse_generic_send*
ACK

*Patch #08 LDAP: Use sdap_get_and_parse_generic_/_recv*
ACK

*Patch #09 LDAP: Add sdap_lookup_type enum*
ACK

*Patch #10 LDAP: Add the wildcard_limit option*

Re: [SSSD] [PATCHES] Wildcard lookups for InfoPipe

2015-07-14 Thread Jakub Hrozek 
On Tue, Jul 14, 2015 at 02:39:13PM +0200, Pavel Březina wrote:
 *Patch #02 SYSDB: Add functions to look up multiple entries including name
 and custom filter*
 
 +static char *enum_filter(TALLOC_CTX *mem_ctx,
 + const char *base_filter,
 + const char *name_filter,
 + const char *addtl_filter)
 
 You are leaking memory here if any of the allocation fails. I know it will
 be freed in the caller but it is not a good practice.
 
 As discussed on IRC, I added a context.
 
 Nack. You added a tmp_ctx but you still use mem_ctx :-)

Let's try to again.

This is the only change in the set.
From 0758b5f22664ec45557cdcf62bd87a8ba2cb5e1c Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Wed, 25 Mar 2015 10:03:43 +0100
Subject: [PATCH 01/11] tests: Move N_ELEMENTS definition to tests/common.h

Avoids code duplication
---
 src/tests/cmocka/test_nested_groups.c | 3 ---
 src/tests/cmocka/test_nss_srv.c   | 3 ---
 src/tests/common.h| 2 ++
 src/tests/sbus_codegen_tests.c| 3 ---
 4 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/src/tests/cmocka/test_nested_groups.c 
b/src/tests/cmocka/test_nested_groups.c
index 
eef9df2dc25c231cb81243f48b759e2fced1a85d..8081ff26102e53b2e453838c3a18e4560ac5317e
 100644
--- a/src/tests/cmocka/test_nested_groups.c
+++ b/src/tests/cmocka/test_nested_groups.c
@@ -48,9 +48,6 @@
 #define GROUP_BASE_DN cn=groups, OBJECT_BASE_DN
 #define USER_BASE_DN cn=users, OBJECT_BASE_DN
 
-#define N_ELEMENTS(arr) \
-(sizeof(arr) / sizeof(arr[0]))
-
 struct nested_groups_test_ctx {
 struct sss_test_ctx *tctx;
 
diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
index 
d1a4c16851427a36b123f04cecee5fe5ae2d333d..3ab8d39c44a8bb8cacae20f534dcbeb6ca7dec08
 100644
--- a/src/tests/cmocka/test_nss_srv.c
+++ b/src/tests/cmocka/test_nss_srv.c
@@ -41,9 +41,6 @@
 #define TEST_ID_PROVIDER ldap
 #define TEST_DOM_SID S-1-5-21-444379608-1639770488-2995963434
 
-#define N_ELEMENTS(arr) \
-(sizeof(arr) / sizeof(arr[0]))
-
 struct nss_test_ctx {
 struct sss_test_ctx *tctx;
 struct sss_domain_info *subdom;
diff --git a/src/tests/common.h b/src/tests/common.h
index 
0b351f5d647a8f72bdbc399c6fe02579d4b4e1be..1c6de2c3d6eb924ba7306bd350f8546d61f30751
 100644
--- a/src/tests/common.h
+++ b/src/tests/common.h
@@ -30,6 +30,8 @@
 #include providers/data_provider.h
 #include providers/ldap/sdap.h
 
+#define N_ELEMENTS(arr) (sizeof(arr) / sizeof(arr[0]))
+
 extern TALLOC_CTX *global_talloc_context;
 
 #define check_leaks(ctx, bytes) _check_leaks((ctx), (bytes), __location__)
diff --git a/src/tests/sbus_codegen_tests.c b/src/tests/sbus_codegen_tests.c
index 
9e9be52e84672eb3ee3afa4e13d5f60047150e98..4637b92b84459041806bb5950e969e988716bec8
 100644
--- a/src/tests/sbus_codegen_tests.c
+++ b/src/tests/sbus_codegen_tests.c
@@ -33,9 +33,6 @@
 #include tests/sbus_codegen_tests_generated.h
 #include util/util_errors.h
 
-#define N_ELEMENTS(arr) \
-(sizeof(arr) / sizeof(arr[0]))
-
 /* The following 2 macros were taken from check's project source files (0.9.10)
  * http://check.sourceforge.net/
  */
-- 
2.4.3

From 4c83c5e2dfcedc6565367db1286e89daf6fcc801 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Tue, 24 Mar 2015 23:24:22 +0100
Subject: [PATCH 02/11] SYSDB: Add functions to look up multiple entries
 including name and custom filter

Related:
https://fedorahosted.org/sssd/ticket/2553

Adds new sysdb function:
- sysdb_enumpwent_filter
- sysdb_enumpwent_filter_with_views
- sysdb_enumgrent_filter
- sysdb_enumgrent_filter_with_views

These are similar to enumeration functions, but optionally allow to
specify a filter to be applied on user/group names. Also an additional
custom filter can be applied.
---
 src/db/sysdb.h  |  24 ++
 src/db/sysdb_search.c   | 252 --
 src/tests/cmocka/test_sysdb_views.c | 494 
 3 files changed, 691 insertions(+), 79 deletions(-)

diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index 
48dd26dd294333b265b69b28cd3b5d37f1293b43..0f745ccb1a646d77ba4ad3d714d5f4dce0a51211
 100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -601,10 +601,22 @@ int sysdb_enumpwent(TALLOC_CTX *mem_ctx,
 struct sss_domain_info *domain,
 struct ldb_result **res);
 
+int sysdb_enumpwent_filter(TALLOC_CTX *mem_ctx,
+   struct sss_domain_info *domain,
+   const char *name_filter,
+   const char *addtl_filter,
+   struct ldb_result **res);
+
 int sysdb_enumpwent_with_views(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
struct ldb_result **res);
 
+int sysdb_enumpwent_filter_with_views(TALLOC_CTX *mem_ctx,
+

[SSSD] [PATCH] nss_check_name_of_well_known_sid() improve name splitting

2015-07-14 Thread Sumit Bose 
Hi,

this patch should fix https://fedorahosted.org/sssd/ticket/2717 .

As you can see I added a new entry ipa_ad_default_names to the global
nss context so that the regular expression string is only evaluated
once. Since it is currently only used in
nss_check_name_of_well_known_sid() I do the initialization here to avoid
initialization when it is not needed. If you think this is too risky in
future I'm fine with moving the initialization to the general
initialization of the nss context.

bye,
Sumit
From 1ff1e1f5fb57dd7e1fa85eb758b8769dfb1260d0 Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Tue, 14 Jul 2015 14:41:34 +0200
Subject: [PATCH] nss_check_name_of_well_known_sid() improve name splitting

Currently in the default configuration
nss_check_name_of_well_known_sid() can only split fully-qualified names
in the u...@domain.name style. DOM\user style names will cause an error
and terminate the whole request.

With this patch both styles can be handled by default, additionally if
the name could not be split nss_check_name_of_well_known_sid() returns
ENOENT which can be handled more gracefully by the caller.

Resolves https://fedorahosted.org/sssd/ticket/2717
---
 src/responder/nss/nsssrv.h  |  1 +
 src/responder/nss/nsssrv_cmd.c  | 32 ++-
 src/tests/cmocka/test_nss_srv.c | 90 -
 src/util/usertools.c|  7 
 src/util/util.h |  3 ++
 5 files changed, 94 insertions(+), 39 deletions(-)

diff --git a/src/responder/nss/nsssrv.h b/src/responder/nss/nsssrv.h
index 
e293e3b4d03582abf6abf07cce61d3b6fdebfcae..6a77f9f1179e0bb2353a01edf14a27e576eba481
 100644
--- a/src/responder/nss/nsssrv.h
+++ b/src/responder/nss/nsssrv.h
@@ -76,6 +76,7 @@ struct nss_ctx {
 
 struct sss_idmap_ctx *idmap_ctx;
 struct sss_names_ctx *global_names;
+struct sss_names_ctx *ipa_ad_default_names;
 
 const char **extra_attributes;
 };
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 
0129467302f16af3180a5be47ff2e235da65..eb550cd007cfb6fac0271b7de211285b6cebfdef
 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -1248,13 +1248,43 @@ static int nss_check_name_of_well_known_sid(struct 
nss_cmd_ctx *cmdctx,
 size_t pctr = 0;
 
 nss_ctx = talloc_get_type(cmdctx-cctx-rctx-pvt_ctx, struct nss_ctx);
-ret = sss_parse_name(cmdctx, nss_ctx-global_names, full_name,
+
+if (nss_ctx-ipa_ad_default_names == NULL) {
+ret = get_default_ipa_ad_names(nss_ctx,
+   nss_ctx-ipa_ad_default_names);
+if (ret != EOK) {
+DEBUG(SSSDBG_OP_FAILURE,
+  get_default_ipa_ad_names failed, using global_names.\n);
+
+/* Instead of failing try the next best. */
+nss_ctx-ipa_ad_default_names = nss_ctx-global_names;
+}
+}
+
+ret = sss_parse_name(cmdctx, nss_ctx-ipa_ad_default_names, full_name,
  wk_dom_name, wk_name);
 if (ret != EOK) {
 DEBUG(SSSDBG_OP_FAILURE, sss_parse_name failed.\n);
 return ret;
 }
 
+if (wk_dom_name == NULL || wk_name == NULL) {
+ret = sss_parse_name(cmdctx, nss_ctx-global_names, full_name,
+ wk_dom_name, wk_name);
+if (ret != EOK) {
+DEBUG(SSSDBG_OP_FAILURE, sss_parse_name failed.\n);
+return ret;
+}
+
+if (wk_dom_name == NULL || wk_name == NULL) {
+DEBUG(SSSDBG_OP_FAILURE,
+  Unable to split [%s] in name and domain part.  \
+  Skipping check for well-known name.\n, full_name);
+
+return ENOENT;
+}
+}
+
 ret = name_to_well_known_sid(wk_dom_name, wk_name, wk_sid);
 talloc_free(wk_dom_name);
 talloc_free(wk_name);
diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
index 
d1a4c16851427a36b123f04cecee5fe5ae2d333d..effd1973b3c1e629fbb215c3fac750b3a34d3e40
 100644
--- a/src/tests/cmocka/test_nss_srv.c
+++ b/src/tests/cmocka/test_nss_srv.c
@@ -1737,63 +1737,77 @@ void test_nss_well_known_getidbysid_failure(void 
**state)
 void test_nss_well_known_getsidbyname(void **state)
 {
 errno_t ret;
+const char *names[] = { Cryptographic Operators@BUILTIN,
+BUILTIN\\Cryptographic Operators, NULL};
+size_t c;
 
-will_return(__wrap_sss_packet_get_body, WRAP_CALL_WRAPPER);
-will_return(__wrap_sss_packet_get_body, Cryptographic Operators@BUILTIN);
-will_return(__wrap_sss_packet_get_body, 0);
-will_return(__wrap_sss_packet_get_cmd, SSS_NSS_GETSIDBYNAME);
-will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);
-will_return(test_nss_well_known_sid_check, S-1-5-32-569);
+for (c = 0; names[c] != NULL; c++) {
+will_return(__wrap_sss_packet_get_body, WRAP_CALL_WRAPPER);
+will_return(__wrap_sss_packet_get_body, names[c]);
+

Re: [SSSD] [PATCH] nss_check_name_of_well_known_sid() improve name splitting

2015-07-14 Thread Lukas Slebodnik 
On (14/07/15 15:02), Sumit Bose wrote:
Hi,

this patch should fix https://fedorahosted.org/sssd/ticket/2717 .

As you can see I added a new entry ipa_ad_default_names to the global
nss context so that the regular expression string is only evaluated
once. Since it is currently only used in
nss_check_name_of_well_known_sid() I do the initialization here to avoid
initialization when it is not needed. If you think this is too risky in
future I'm fine with moving the initialization to the general
initialization of the nss context.

bye,
Sumit

From 1ff1e1f5fb57dd7e1fa85eb758b8769dfb1260d0 Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Tue, 14 Jul 2015 14:41:34 +0200
Subject: [PATCH] nss_check_name_of_well_known_sid() improve name splitting

Currently in the default configuration
nss_check_name_of_well_known_sid() can only split fully-qualified names
in the u...@domain.name style. DOM\user style names will cause an error
and terminate the whole request.

With this patch both styles can be handled by default, additionally if
the name could not be split nss_check_name_of_well_known_sid() returns
ENOENT which can be handled more gracefully by the caller.

Resolves https://fedorahosted.org/sssd/ticket/2717
---
 src/responder/nss/nsssrv.h  |  1 +
 src/responder/nss/nsssrv_cmd.c  | 32 ++-
 src/tests/cmocka/test_nss_srv.c | 90 -
 src/util/usertools.c|  7 
 src/util/util.h |  3 ++
 5 files changed, 94 insertions(+), 39 deletions(-)

diff --git a/src/responder/nss/nsssrv.h b/src/responder/nss/nsssrv.h
index 
e293e3b4d03582abf6abf07cce61d3b6fdebfcae..6a77f9f1179e0bb2353a01edf14a27e576eba481
 100644
--- a/src/responder/nss/nsssrv.h
+++ b/src/responder/nss/nsssrv.h
@@ -76,6 +76,7 @@ struct nss_ctx {
 
 struct sss_idmap_ctx *idmap_ctx;
 struct sss_names_ctx *global_names;
+struct sss_names_ctx *ipa_ad_default_names;
 
 const char **extra_attributes;
 };
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 
0129467302f16af3180a5be47ff2e235da65..eb550cd007cfb6fac0271b7de211285b6cebfdef
 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -1248,13 +1248,43 @@ static int nss_check_name_of_well_known_sid(struct 
nss_cmd_ctx *cmdctx,
 size_t pctr = 0;
 
 nss_ctx = talloc_get_type(cmdctx-cctx-rctx-pvt_ctx, struct nss_ctx);
-ret = sss_parse_name(cmdctx, nss_ctx-global_names, full_name,
nss_ctx-global_names is used just in two functions
  nss_check_name_of_well_known_sid
  nss_check_well_known_sid

All SID related functions should be used with AD.
I would prefer to directly change default to IPA/AD regex for global_names.

Or alternatively iterate over all domains and use their regexes.
In teory, someone can use alternative regex (?Pname[^@]+)(?Pdomain.+$)
and you will not be able to parse it with default ipa ad regex.

LS


+
+if (nss_ctx-ipa_ad_default_names == NULL) {
+ret = get_default_ipa_ad_names(nss_ctx,
+   nss_ctx-ipa_ad_default_names);
+if (ret != EOK) {
+DEBUG(SSSDBG_OP_FAILURE,
+  get_default_ipa_ad_names failed, using global_names.\n);
+
+/* Instead of failing try the next best. */
+nss_ctx-ipa_ad_default_names = nss_ctx-global_names;
+}
+}
+
+ret = sss_parse_name(cmdctx, nss_ctx-ipa_ad_default_names, full_name,
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] test common: sss_dp_get_account_recv() fix assignment

2015-07-14 Thread Jakub Hrozek 
On Fri, Jul 10, 2015 at 08:30:49PM +0200, Jakub Hrozek wrote:
 On Fri, Jul 10, 2015 at 05:48:57PM +0200, Sumit Bose wrote:
  Hi,
  
  this patch fixes a simple copy-and-paste issue in the common test code.
  Since we currently always call mock_account_recv() with the second and
  third argument set to 0 and NULL respectively it doesn't became an issue
  earlier.
  
  bye,
  Sumit
 
  From 5d2a8ee6d6d26d276b3be4c97fe0bf9e5cf7d099 Mon Sep 17 00:00:00 2001
  From: Sumit Bose sb...@redhat.com
  Date: Fri, 10 Jul 2015 12:37:43 +0200
  Subject: [PATCH] test common: sss_dp_get_account_recv() fix assignment
  
  ---
   src/tests/cmocka/common_mock_resp_dp.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
  
  diff --git a/src/tests/cmocka/common_mock_resp_dp.c 
  b/src/tests/cmocka/common_mock_resp_dp.c
  index 
  08d74179d8e2dc0561ea3e2d1e9b5580762ee633..a67ceee4a43184d9894c19a5c11161aca4e1caff
   100644
  --- a/src/tests/cmocka/common_mock_resp_dp.c
  +++ b/src/tests/cmocka/common_mock_resp_dp.c
  @@ -51,7 +51,7 @@ sss_dp_get_account_recv(TALLOC_CTX *mem_ctx,
   
   *dp_err = sss_mock_type(dbus_uint16_t);
   *dp_ret = sss_mock_type(dbus_uint32_t);
  -*dp_ret = sss_mock_type(dbus_uint32_t);
  +*err_msg = sss_mock_ptr_type(char *);
 
 (Spoiler alert: Sumit already showed me the bug on IRC)
 
 ACK.
 
 CI: http://sssd-ci.duckdns.org/logs/job/18/78/summary.html

Sorry for the delay in pushing (feel free to push the patch yourself
btw), pushed to master:
b1bea7c3d202eb3b53e219c9ccb83161ce47ca6a
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] Time-Based Policies in FreeIPA

2015-07-14 Thread Dmitri Pal 

On 07/14/2015 04:50 AM, Sumit Bose wrote:

On Tue, Jul 14, 2015 at 09:11:28AM +0200, Stanislav Laznicka wrote:

Hi,

If you're a member of the FreeIPA-devel mailing list, you may already know
me. I am currently trying to re-introduce the time-based policies for
FreeIPA HBAC Rules to both FreeIPA and SSSD. For some more information, see
the design at http://www.freeipa.org/page/V4/Time-Based_Account_Policies or
read the whole paper at
http://www.fit.vutbr.cz/study/DP/DP.php.cs?id=17185file=t.

At the moment, I have a prototype solution which is able to decide whether
an HBAC rule should or should not apply based on the time rules supplied
with HBAC rule objects from FreeIPA. See the source code attached to this
mail, if you will. The biggest problem of this solution is that it is not
thread-safe.

I have not look in detain at you patches but I think thread safety is
not an issue here. The HBAC evaluation is done in a single process
without using threads. To handle concurrent task we use the tevent
library and asynchronous request (see
https://tevent.samba.org/tevent_request.html for details). This means
whatever you do will change the state of the whole process but as long
as you do it in a single function and make sure to restore the original
state it might be safe. I say it might because tevent handles signals as
well so we have to make sure the state can be recovered even if a single
handler runs in between.

bye,
Sumit


It seems to be quite a problem to convert time from a certain time zone to a
different time zone in C and keep it thread-safe. A very simple and also
very ugly solution would be to have a mutex to guard each localtime() call
as well as it should wrap the body of the time_to_timezone() function from
the second patch. This seems rather unacceptable. The other solution would
be to find another way to convert the time. Currently, there seems to be a
C++ Boost solution based on a .csv file but it is not accepted well 
(https://github.com/boostorg/date_time/blob/master/data/date_time_zonespec.csv).
I was also thinking on using the glibc tzfile parsers
(http://code.woboq.org/userspace/glibc/time/tzfile.c.html#__tzfile_read) but
they too seem rather thread-unsafe and trying to rework it in a thread-safe
manner might be a painful thing to do.

I welcome any suggestions and ideas on the topic as I seem to be quite stuck
here.

Cheers,
Stanislav Laznicka
 From 428db72ca0a07c7dba1a6959696b3dc2df7d77ec Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka slazn...@redhat.com
Date: Mon, 13 Jul 2015 09:53:10 +0200
Subject: [PATCH 1/2] Added caching of time policies for IPA HBAC rules.

The time policies which are part of the FreeIPA HBAC Rule object
are now being cached along with other HBAC Rule objects' attributes.
Also, the cached time policies are transformed into hbac_time_rules
structure representation.

https://fedorahosted.org/freeipa/ticket/547
https://fedorahosted.org/freeipa/ticket/548
---
  Makefile.am  |   4 --
  src/providers/ipa/hbac_evaluator.c   |   7 --
  src/providers/ipa/ipa_access.c   |   3 +
  src/providers/ipa/ipa_hbac.h |   6 +-
  src/providers/ipa/ipa_hbac_common.c  | 120 +++
  src/providers/ipa/ipa_hbac_private.h |  10 +++
  src/providers/ipa/ipa_hbac_rules.c   |   7 +-
  src/tests/ipa_hbac-tests.c   |   6 ++
  8 files changed, 149 insertions(+), 14 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 
b8cbc6df23ded1edb945a709b6dbe1c44eb54017..8665a0560cc3cd57f148325640c2e709a05f4c2a
 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -264,14 +264,10 @@ PYTHON_TESTS =
  
  if BUILD_PYTHON2_BINDINGS

  PYTHON_TESTS += src/config/SSSDConfigTest.py2.sh \
-src/tests/pyhbac-test.py2.sh \
-src/tests/pysss_murmur-test.py2.sh \
  $(NULL)
  endif
  if BUILD_PYTHON3_BINDINGS
  PYTHON_TESTS += src/config/SSSDConfigTest.py3.sh \
-src/tests/pyhbac-test.py3.sh \
-src/tests/pysss_murmur-test.py3.sh \
  $(NULL)
  endif
  
diff --git a/src/providers/ipa/hbac_evaluator.c b/src/providers/ipa/hbac_evaluator.c

index 
f40f9e0a7f16f5e012079c637b89c8e49ec5d15b..64af36aae2855d0fb5a737989cc3ee8959e1b5b6
 100644
--- a/src/providers/ipa/hbac_evaluator.c
+++ b/src/providers/ipa/hbac_evaluator.c
@@ -38,13 +38,6 @@ typedef int errno_t;
  #define EOK 0
  #endif
  
-/* Placeholder structure for future HBAC time-based

- * evaluation rules
- */
-struct hbac_time_rules {
-int not_yet_implemented;
-};
-
  enum hbac_eval_result_int {
  HBAC_EVAL_MATCH_ERROR = -1,
  HBAC_EVAL_MATCHED,
diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
index 
3198e2bd2a4c8355eeccc129c85ae3d7d67f61b0..c60775fd01f816c19b3617caa04088a8052d35ed
 100644
--- a/src/providers/ipa/ipa_access.c
+++ b/src/providers/ipa/ipa_access.c
@@ -686,6 +686,9 @@ errno_t hbac_get_cached_rules(TALLOC_CTX *mem_ctx,

[SSSD] [PATCH] Use NSCD path in execl()

2015-07-14 Thread Jakub Hrozek 
Hi,

this is a small cleanup patch, see the commit message.
From caf8b6842309c3fb7891b44b71815a37ed4c833b Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Fri, 5 Jun 2015 17:10:21 +0200
Subject: [PATCH] Use NSCD path in execl()

man execl says:
The first argument, by convention, should point to the filename
associated with the file being executed.

We used just 'nscd' instead.
---
 src/util/nscd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/nscd.c b/src/util/nscd.c
index 
9f79a69cb5e0fe9674ace191fc90d448cfdf602a..f58aebcad69924bdd841a4bb51aedb0308237ac4
 100644
--- a/src/util/nscd.c
+++ b/src/util/nscd.c
@@ -59,7 +59,7 @@ int flush_nscd_cache(enum nscd_db flush_db)
 nscd_pid = fork();
 switch (nscd_pid) {
 case 0:
-execl(NSCD_PATH, nscd, NSCD_RELOAD_ARG, service, NULL);
+execl(NSCD_PATH, NSCD_PATH, NSCD_RELOAD_ARG, service, NULL);
 /* if this returns it is an error */
 DEBUG(SSSDBG_CRIT_FAILURE,
   execl(3) failed: %d(%s)\n, errno, strerror(errno));
-- 
2.4.3

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

Re: [SSSD] [PATCH] Use NSCD path in execl()

2015-07-14 Thread Lukas Slebodnik 
On (14/07/15 22:44), Jakub Hrozek wrote:
Hi,

this is a small cleanup patch, see the commit message.

From caf8b6842309c3fb7891b44b71815a37ed4c833b Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Fri, 5 Jun 2015 17:10:21 +0200
Subject: [PATCH] Use NSCD path in execl()

man execl says:
The first argument, by convention, should point to the filename
associated with the file being executed.

We used just 'nscd' instead.
---
 src/util/nscd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/nscd.c b/src/util/nscd.c
index 
9f79a69cb5e0fe9674ace191fc90d448cfdf602a..f58aebcad69924bdd841a4bb51aedb0308237ac4
 100644
--- a/src/util/nscd.c
+++ b/src/util/nscd.c
@@ -59,7 +59,7 @@ int flush_nscd_cache(enum nscd_db flush_db)
 nscd_pid = fork();
 switch (nscd_pid) {
 case 0:
-execl(NSCD_PATH, nscd, NSCD_RELOAD_ARG, service, NULL);
+execl(NSCD_PATH, NSCD_PATH, NSCD_RELOAD_ARG, service, NULL);
 /* if this returns it is an error */
 DEBUG(SSSDBG_CRIT_FAILURE,
   execl(3) failed: %d(%s)\n, errno, strerror(errno));

ACK

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