[SSSD] [PATCH] SSSDConfigTest: Test real config without config_file_version

2015-10-15 Thread Lukas Slebodnik 
ehlo,

let's follow TDD :-)

attached are unit tests for regression in
https://fedorahosted.org/sssd/ticket/2837

LS
>From 2ba47cb1b132efd960cd771a9d78c8ba7aa59843 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Thu, 15 Oct 2015 10:32:09 +0200
Subject: [PATCH 1/2] SSSDConfigTest: Try load saved config

Python module SSSDConfig should be able to save configuration file
and later load the same configuration file without problem.

Unit test for:
https://fedorahosted.org/sssd/ticket/2837
---
 src/config/SSSDConfigTest.py | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
index 
5047bd7237188be4df83f76054afe6b43ceeec1d..620585eae5e233724a71e51de6f0ca67c40f7c3e
 100755
--- a/src/config/SSSDConfigTest.py
+++ b/src/config/SSSDConfigTest.py
@@ -157,6 +157,11 @@ class SSSDConfigTestValid(unittest.TestCase):
 #non-owners, and should not be executable by anyone
 self.assertFalse(S_IMODE(mode) & 0o177)
 
+# try to import saved configuration file
+config = SSSDConfig.SSSDConfig(srcdir + "/etc/sssd.api.conf",
+   srcdir + "/etc/sssd.api.d")
+config.import_config(configfile=of)
+
 #Remove the output file
 os.unlink(of)
 
@@ -191,6 +196,11 @@ class SSSDConfigTestValid(unittest.TestCase):
 #non-owners, and should not be executable by anyone
 self.assertFalse(S_IMODE(mode) & 0o177)
 
+# try to import saved configuration file
+config = SSSDConfig.SSSDConfig(srcdir + "/etc/sssd.api.conf",
+   srcdir + "/etc/sssd.api.d")
+config.import_config(configfile=of)
+
 #Remove the output file
 os.unlink(of)
 
-- 
2.5.0

>From 770cd4df22988fc1eb2d71e14b38f1e70670 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Thu, 15 Oct 2015 11:04:06 +0200
Subject: [PATCH 2/2] SSSDConfigTest: Test real config without
 config_file_version

src/config/testconfigs/sssd-valid.conf explicitly contains
config_file_version. Recently we changed the default value to 2
and therefore it needn't be listed in configuration file.
This patch add real ssds.conf without config_file_version
---
 src/config/SSSDConfigTest.py   | 56 ++
 .../testconfigs/sssd-implicit-config-version.conf  | 30 
 2 files changed, 86 insertions(+)
 create mode 100644 src/config/testconfigs/sssd-implicit-config-version.conf

diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
index 
620585eae5e233724a71e51de6f0ca67c40f7c3e..c0dcce3bc2a63afef825c4c4b626a9c3be78b7cc
 100755
--- a/src/config/SSSDConfigTest.py
+++ b/src/config/SSSDConfigTest.py
@@ -1248,6 +1248,62 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase):
 self.assertRaises(SSSDConfig.AlreadyInitializedError,
   sssdconfig.import_config, srcdir + 
"/testconfigs/sssd-valid.conf")
 
+def testImportConfigWithout_config_file_version(self):
+# Positive Test
+sssdconfig = SSSDConfig.SSSDConfig(srcdir + "/etc/sssd.api.conf",
+   srcdir + "/etc/sssd.api.d")
+sssdconfig.import_config(
+srcdir + "/testconfigs/sssd-implicit-config-version.conf"
+)
+
+# Verify that all sections were imported
+control_list = [
+'sssd',
+'nss',
+'pam',
+'domain/ad.example.com',
+]
+
+for section in control_list:
+self.assertTrue(sssdconfig.has_section(section),
+"Section [%s] missing" %
+section)
+for section in sssdconfig.sections():
+self.assertTrue(section['name'] in control_list)
+
+# Verify that all options were imported for a section
+control_list = [
+'services',
+'domains',
+]
+
+for option in control_list:
+self.assertTrue(sssdconfig.has_option('sssd', option),
+"Option [%s] missing from [sssd]" %
+option)
+for option in sssdconfig.options('sssd'):
+if option['type'] in ('empty', 'comment'):
+continue
+self.assertTrue(option['name'] in control_list,
+"Option [%s] unexpectedly found" %
+option)
+
+domain_control_list = [
+'cache_credentials',
+'id_provider',
+'auth_provider',
+'access_provider',
+'default_shell',
+'fallback_homedir',
+'cache_credentials',
+'use_fully_qualified_names',
+]
+
+domain = sssdconfig.get_domain("ad.example.com")
+
+for option in domain.get_all_options():
+self.assertTrue(option in domain_control_list)
+

Re: [SSSD] [PATCHES][IPA][RFE] Implement iCal based time managment in HBAC

2015-10-15 Thread Jakub Hrozek 
On Thu, Oct 15, 2015 at 09:50:03AM +0200, Lukas Slebodnik wrote:
> On (14/10/15 15:02), Jakub Hrozek wrote:
> >On Wed, Oct 07, 2015 at 10:20:03AM +0200, Stanislav Laznicka wrote:
> >> From 0a40a390afe56ec27c070ed743ba6f3d39b564aa Mon Sep 17 00:00:00 2001
> >> From: Stanislav Laznicka 
> >> Date: Mon, 13 Jul 2015 10:00:42 +0200
> >> Subject: [PATCH 2/6] IPA: Added evaluation of time policies in HBAC
> >> 
> >> The time policies in FreeIPA HBAC objects are now evaluated in
> >> the libipa_hbac module.
> >> 
> >> TODO: Had to disable the pyhbac tests until proper python
> >> bindings are implemented.
> >> 
> >> https://fedorahosted.org/freeipa/ticket/547
> >
> >It would be better to include the SSSD ticket in the commit message.
> >
> >It would also be nice to add a link to the design page into the commit
> >message.
> >
> >You added new functions to the API, so the version info of libipa_hbac
> >must be incremented:
> >
> > https://www.gnu.org/software/libtool/manual/libtool.html#Updating-version-info
> It's recomended to bump version-info once in a release.
> And we already did it for libhbac with changes in logging.

OK, good point (assuming the changes are valid, I haven't checked)

> 
> >and the src/providers/ipa/ipa_hbac.exports must be amended as well.
> >
> Yes it should be, otherwise it could not be used by other programs.
> because new function would not be visible. Or maybe they needn't be visible
> in public interface. (I didn't check changes in public header files)
> 
> LS
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

2015-10-15 Thread Petr Cech 

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

Re: [SSSD] [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

2015-10-15 Thread Sumit Bose 
On Wed, Oct 14, 2015 at 04:09:25PM +0200, Pavel Reichl wrote:
> 
> 
> On 10/14/2015 01:07 PM, Pavel Reichl wrote:
> >
> >
> >On 10/14/2015 01:01 PM, Jakub Hrozek wrote:
> >>On Wed, Oct 14, 2015 at 12:51:39PM +0200, Sumit Bose wrote:
> [snip]
> >>>So far I liked the flags attribute which controls the behaviour of
> >>>sdap_get_generic_ext_send() best (and I agree that allow_paging should
> >>>be include in the flags, but only for sdap_get_generic_ext_send() not
> >>>the "higher" level calls). Mainly because it scales, i.e. it can be used
> >>>in future to control sdap_get_generic_ext_send() even more.
> >>>
> [snip]
> >>>If you think this is all too much for the issue at hand (ignoring a
> >>>debug message) especially for versions that are already released what
> >>>about always ignoring the message (or only displaying with
> >>>SSSDBG_TRACE_ALL) if state->sizelimit != 0, i.e. explicitly set?
> >>
> >>No, I don't really mind, I think we already spent too much time
> >>discussing this simple patch.
> >>
> >>If you like the flags best, let's push the flags..
> >>___
> >
> >OK, I'll send updated patch reflecting comments relating 'flags' patch.
> >___
> 
> Please see updated patch set.
> 1st patch adds flag to control silencing of warning
> 2nd patch makes allow_paging part of flag
> 3rd patch removes unused parameter 'attrsonly' from function 
> sdap_get_and_parse_generic_send()
> 4rd patch makes attrsonly in sdap_get_generic_ext_state to be part of flag
> 
> If any of patches 2,3,4 is too controversial feel free to ignore it, I can 
> send it later in separate thread.
> 
> Thanks!

Thank you for the patches. 

I have two things I would like to discuss

> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> index 
> b81431f79f21755469bb9ff123d695a2a166e353..cbf199e6d524a3aa659ef040937b26571ab63735
>  100644
> --- a/src/providers/ldap/sdap_async.c
> +++ b/src/providers/ldap/sdap_async.c
> @@ -1164,6 +1164,7 @@ struct sdap_get_generic_ext_state {
>  void *cb_data;
>  
>  bool allow_paging;
> +bool sizelimit_silent;

What about putting the flags directly in the state and check them only
where they might apply. I think this will scale better because there is
no need to add a boolean for every new flag value and does not need the
(mostly useless?) conversion from flag to bool at the start of
sdap_get_generic_ext_send().

>  };
>  
>  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
> @@ -1172,6 +1173,9 @@ static void sdap_get_generic_op_finished(struct sdap_op 
> *op,
>   struct sdap_msg *reply,
>   int error, void *pvt);
>  
> +/* Be silent about exceeded size limit */
> +#define SDAP_SRCH_FLG_SIZELIMIT_SILENT0x01

I wonder if the (1 << 0) style notation used e.g. for flags in the
pam_sss module or for SBUS_PROPERTY_* would be less error-prone to
define flag values?

Otherwise the patches look good and I think it is a good idea to include
attrsonly here as well.

I didn't run any tests so far because I would like to hear your option
about the two suggestions from above first.

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

Re: [SSSD] [PATCHES][IPA][RFE] Implement iCal based time managment in HBAC

2015-10-15 Thread Lukas Slebodnik 
On (14/10/15 15:02), Jakub Hrozek wrote:
>On Wed, Oct 07, 2015 at 10:20:03AM +0200, Stanislav Laznicka wrote:
>> From 0a40a390afe56ec27c070ed743ba6f3d39b564aa Mon Sep 17 00:00:00 2001
>> From: Stanislav Laznicka 
>> Date: Mon, 13 Jul 2015 10:00:42 +0200
>> Subject: [PATCH 2/6] IPA: Added evaluation of time policies in HBAC
>> 
>> The time policies in FreeIPA HBAC objects are now evaluated in
>> the libipa_hbac module.
>> 
>> TODO: Had to disable the pyhbac tests until proper python
>> bindings are implemented.
>> 
>> https://fedorahosted.org/freeipa/ticket/547
>
>It would be better to include the SSSD ticket in the commit message.
>
>It would also be nice to add a link to the design page into the commit
>message.
>
>You added new functions to the API, so the version info of libipa_hbac
>must be incremented:
>
> https://www.gnu.org/software/libtool/manual/libtool.html#Updating-version-info
It's recomended to bump version-info once in a release.
And we already did it for libhbac with changes in logging.

>and the src/providers/ipa/ipa_hbac.exports must be amended as well.
>
Yes it should be, otherwise it could not be used by other programs.
because new function would not be visible. Or maybe they needn't be visible
in public interface. (I didn't check changes in public header files)

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

Re: [SSSD] [PATCH] util: Include disabled domains in link_forest_roots

2015-10-15 Thread Michal Židek 

On 10/13/2015 05:09 PM, Jakub Hrozek wrote:

On Tue, Oct 13, 2015 at 04:19:22PM +0200, Jakub Hrozek wrote:

On Tue, Oct 13, 2015 at 02:56:35PM +0200, Michal Židek wrote:

On 10/12/2015 10:31 AM, Jakub Hrozek wrote:

On Fri, Oct 09, 2015 at 03:14:52PM +0200, Michal Židek wrote:

On 10/09/2015 02:10 PM, Jakub Hrozek wrote:

On Fri, Oct 02, 2015 at 03:34:30PM +0200, Michal Židek wrote:

On 10/02/2015 07:44 AM, Lukas Slebodnik wrote:

On (01/10/15 15:41), Michal Židek wrote:

On 10/01/2015 03:08 PM, Lukas Slebodnik wrote:

On (01/10/15 13:56), Michal Židek wrote:

On 09/16/2015 03:56 PM, Michal Židek wrote:

On 09/15/2015 04:03 PM, Jakub Hrozek wrote:

On Tue, Sep 15, 2015 at 03:53:41PM +0200, Michal Židek wrote:

On 09/14/2015 05:43 PM, Jakub Hrozek wrote:

On Wed, Sep 09, 2015 at 02:43:59PM +0200, Michal Židek wrote:

Hi,

patch for ticket
https://fedorahosted.org/sssd/ticket/2673
is in the attachment.

Thanks.
Michal



 From 7c454bc2a737be05068418a5eef7fe9446bb5fa8 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= 
Date: Wed, 9 Sep 2015 14:37:48 +0200
Subject: [PATCH] util: Include disabled domains in link_forest_roots

Ticket:
https://fedorahosted.org/sssd/ticket/2673
---
  src/db/sysdb_subdomains.c |  6 +++---
  src/tests/cmocka/test_utils.c |  3 +++
  src/util/domain_info_utils.c  | 21 ++---
  src/util/util.h   |  3 +++
  4 files changed, 27 insertions(+), 6 deletions(-)


The patch looks good but whenever I see us adding more and more boolean
switches, I wonder if we should just use flags instead?

This:
 get_next_domain_ex(d, GND_USE_DISABLED | GND_DESCEND);
Reads quite a bit easier to me than:
 get_next_domain_ex(d, true, false);

Also, bonus point are acquired next time we add a new flag, because not
all callers of the function must be converted..

What do you think?


I think this is very good point. It will also give us implicit
boolean parameter value (if not set, it is automatically false)
which improves readability a lot. I wander if it is good to add
the _ex function in this case. Would you agree if I changed the
original get_next_domain to use flags? I know, it needs to be changed
on more places, but I think such small refactoring makes more
sence than adding _ex function with flags.

Or is this what you were saying? I am not sure because
you used the _ex function in your example of "nice"
usage.


If we have a test for different usages of the flag-based function, then
I guess it would be better than keeping the existing get_next_domain
function.

We can also split master from sssd-1-13 already..

btw the flag names were completely pulled out of thin air, feel free to
suggest better ones.


I split the refactoring of get_next_domain to separate patch.
The second patch is just one liner that switches on the
disabled domains in link_forest_roots.

Michal


    _   _ __  __ 
__/\_| __ )| | | |  \/  |  _ \__/\__
\/  _ \| | | | |\/| | |_) \/
/_  _\ |_) | |_| | |  | |  __//_  _\
  \/ |/ \___/|_|  |_|_| \/

(low priority, just keeping the thread alive :) )

Would you mind to write unit tests for get_next_domain?

LS


It does have unit tests.


Let me rephrase your answer.
The tests has beed changed.

But new functionality is not test.
There is a change in test_utils.s

-dom = get_next_domain(test_ctx->dom_list, true);
+dom = get_next_domain(test_ctx->dom_list, SSS_GND_DESCEND);
 assert_null(dom);
+
+dom = get_next_domain(test_ctx->dom_list,
+  SSS_GND_DESCEND | SSS_GND_INCLUDE_DISABLED);
+assert_non_null(dom);

But assert_non_null cannot be considered as a valid test for new feature.

So would you mind to extend tests for SSS_GND_INCLUDE_DISABLED?
It ould be god to se what happed if SSS_GND_INCLUDE_DISABLED is used without
the flag SSS_GND_DESCEND. Or which domain will be returned in different cases.

You should consider a new unit test as an additional documentation.
So if new developer will not be sure hot to use it it will be obvious from
unit test. This is a reason why it might be better to write separate
test for new feature in separate patch. Because the current patch
mostly convert old code to the new style 's/true/SSS_GND_DESCEND/' 's/false/0/'
(In most cases, I prefer unit test to be part of new small features but
sometimes it's better to write unit test in separate patch.

LS


Ok. I removed the test changes from the first patch and added
enhanced tests in the new patch.

Michal



Can you rebase the patches atop master, please?

I will then review them. But from casual scrolling from the patches, the
changes look OK to me.


Thank you, rebased patches are attached.

Michal




 From 30af59fc71f09cbc613380df9d5c62b5c0f9a8b5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= 
Date: Wed, 9 Sep 2015 14:37:48 +0200
Subject: [PATCH 1/3] util: Update get_next_domain's interface

Update get next domain to be able to

[SSSD] [PATCH] SSSDConfig: Do not raise exception if config_file_version is missing

2015-10-15 Thread Michal Židek 

Hi,

attached patch fixes ticket:
https://fedorahosted.org/sssd/ticket/2837

Michal
>From 329db54ce4e0a930524271968457c930d6a62309 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= 
Date: Thu, 15 Oct 2015 18:53:37 +0200
Subject: [PATCH] SSSDConfig: Do not raise exception if config_file_version is
 missing

Ticket:
https://fedorahosted.org/sssd/ticket/2837
---
 src/config/SSSDConfig/__init__.py.in | 8 
 src/config/SSSDConfigTest.py | 5 -
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index 038de16..bf61c40 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -1405,10 +1405,10 @@ class SSSDConfig(SSSDChangeConf):
 try:
 if int(self.get('sssd', 'config_file_version')) != self.API_VERSION:
 raise ParsingError("Wrong config_file_version")
-except:
-# Either the 'sssd' section or the 'config_file_version' was not
-# present in the config file
-raise ParsingError("File contains no config_file_version")
+except TypeError:
+# This happens when config_file_version is missing. We
+# can assume it is the default version and continue.
+pass
 
 def new_config(self):
 """
diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
index 5047bd7..a850b8d 100755
--- a/src/config/SSSDConfigTest.py
+++ b/src/config/SSSDConfigTest.py
@@ -1226,11 +1226,6 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase):
srcdir + "/etc/sssd.api.d")
 self.assertRaises(SSSDConfig.ParsingError, sssdconfig.import_config, srcdir + "/testconfigs/sssd-badversion.conf")
 
-# Negative Test - No config file version
-sssdconfig = SSSDConfig.SSSDConfig(srcdir + "/etc/sssd.api.conf",
-   srcdir + "/etc/sssd.api.d")
-self.assertRaises(SSSDConfig.ParsingError, sssdconfig.import_config, srcdir + "/testconfigs/sssd-noversion.conf")
-
 # Negative Test - Already initialized
 sssdconfig = SSSDConfig.SSSDConfig(srcdir + "/etc/sssd.api.conf",
srcdir + "/etc/sssd.api.d")
-- 
2.1.0

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

Re: [SSSD] [PATCH] SSSDConfigTest: Test real config without config_file_version

2015-10-15 Thread Michal Židek 

On 10/15/2015 11:16 AM, Lukas Slebodnik wrote:

ehlo,

let's follow TDD :-)

attached are unit tests for regression in
https://fedorahosted.org/sssd/ticket/2837

LS



Thank you Lukas for the test.

Conditional ACK.

I will fully ack once the CI finishes
and the original issue is solved in master.

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

[SSSD] A security bug in SSSD 1.10 and later (CVE-2015-5292)

2015-10-15 Thread Jakub Hrozek 
=== A security bug in SSSD 1.10 and later ==
=
= Subject:  A memory leak was found in SSSD's PAC processing plugin
=
= CVE ID#:  CVE-2015-5292
=
= Summary:  When SSSD's PAC responder is configured and a user login
=   triggers parsing of the PAC blob (typically a GSSAPI
=   password-less login), a small amount of memory is leaked
=   in the context of the Kerberized  application. This can
=   eventually lead to memory exhaustion.
=
= Impact:   Low
=
= Acknowledgements: This bug was found by Thomas Oulevey from CERN
=
= Affects default
=  configuration:   Only for the IPA provider
=
= Introduced with:  1.10.0 beta2
=
===

 DESCRIPTION 
When SSSD's PAC responder is configured and a user login triggers parsing of
the PAC blob (typically a GSSAPI password-less login), a small amount of
memory is leaked in the context of the Kerberized application. This can
eventually lead to memory exhaustion.

The affected configration would include "pac" in the list of services in
the the "[sssd]" section of the /etc/sssd/sssd.conf config file. Please
note that SSSD automatically starts the PAC responder in case the provider
type is set to IPA.

Also note that the most widely deployed application with this configuration
is OpenSSH, where the bug is not noticeable because, the leak happens in
a short-lived child process, not the long-running deamon.

The fix was delivered as part of the 1.13.1 release. However, the security
implications of the bug were only determined later.

The bug is being tracked in the following Red Hat Bugzilla report:
https://bugzilla.redhat.com/show_bug.cgi?id=1267580

 PATCH AVAILABILITY 
The patch is available at:

https://git.fedorahosted.org/cgit/sssd.git/commit/?id=b4c44ebb8997d3debb33607c123ccfd9926e0cba
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel