from:"Pavel Reichl"

[SSSD] Re: [PRELIMINARY] Data Provider changes

2016-04-21 Thread Pavel Reichl




On 04/21/2016 03:24 PM, Pavel Březina wrote:

Hi,
the data provider code is basically ready for someone to start looking into it. 
I'm in the process of converting old handlers to the new interface (sudo and 
hostid is finished) and at this moment I don't plan to do further changes to 
the dp interface
itself unless something shows up during the conversion.

I tried to keep the code separated for the moment so it can be split into more 
commits to simplify review and current functionality and even some future 
bisecting is not broken. For this the DP code itself is quite self-contained 
and changes in the
modules are done in separate files (marked as $orig_new.c). The last commit 
switches the old files for the new ones in the makefile so new code can be 
compiled, though at this moment is is only for development purpose.

There are too many patches to be sent to the list at this incomplete stage, you 
can check it out either from my fedorapeople repo [1] or github repo [2], which 
Pavel forced me to create (meh :-)).

Pavel Reichl is currently working on unit tests -- thank you!

[1] https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=backend
[2] https://github.com/pbrezina/sssd/tree/backend
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

Thanks, I added some more comments on the github.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Pavel Reichl

On 04/20/2016 02:16 PM, Jakub Hrozek wrote:

+for (num = 0, i = 0; i < res->count; i++) {
>+const struct ldb_val *val;
>+char *name;
>+int n;
>+int j;

Every time I see variables declared in a scope in C except loop control
variables I think "This should be a static function of its own":-)

Yes, and I think it's great that Simo does so, instead of hiding this fact by 
placing all variables at the beginning of the function. Limiting scope of 
variables as much as possible is a good think!
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Fix warning: Assigned value is garbage or undefined

2016-04-14 Thread Pavel Reichl




On 04/14/2016 10:28 AM, Lukas Slebodnik wrote:

ehlo,

@see commit message in attached trivial patch.

LS



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


Hello,

patch does not apply, code in patch doesn't look like recent master to me...?

Thanks
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] test_ad_common: Include missing header if building with NSS

2016-04-13 Thread Pavel Reichl




On 04/13/2016 03:04 PM, Lukas Slebodnik wrote:

ehlo,

attached patch fix a simple warning in unit test.

LS



LGTM, I'm just waiting for CI to finish.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] MAN: Remove duplicate description of the pam_account_locked_message option

2016-04-06 Thread Pavel Reichl




On 04/06/2016 10:39 AM, Pavel Reichl wrote:



On 04/06/2016 10:17 AM, Jakub Hrozek wrote:

Hi,

I found this minor man page bug when I was prepairing the 1.13.4 release
notes.



Obvious ACK.

I'm waiting for ci results but I don't expect any surprises there :-)
___


CI passed: http://sssd-ci.duckdns.org/logs/job/40/69/summary.html
ACK
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] MAN: Remove duplicate description of the pam_account_locked_message option

2016-04-06 Thread Pavel Reichl




On 04/06/2016 10:17 AM, Jakub Hrozek wrote:

Hi,

I found this minor man page bug when I was prepairing the 1.13.4 release
notes.



Obvious ACK.

I'm waiting for ci results but I don't expect any surprises there :-)
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] SDAP: Remove unused parameter talloc context

2016-04-04 Thread Pavel Reichl




On 04/04/2016 09:54 AM, Pavel Reichl wrote:

On 03/31/2016 08:14 AM, Lukas Slebodnik wrote:

ehlo,

yet another patch with unused parameter.
It has been found as part of unrelated work.

LS


LGTM, build passed. I'm just waiting for CI to finish so I can ACK.



CI passed: sssd-ci.duckdns.org/logs/job/40/46/summary.html

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

[SSSD] Re: [PATCH] IPA: Remove unused parameter from ipa_ext_group_member_check

2016-04-04 Thread Pavel Reichl


CI paseed: http://sssd-ci.duckdns.org/logs/job/40/42/summary.html

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

[SSSD] Re: [PATCH] IPA: Remove unused parameter from ipa_ext_group_member_check

2016-04-04 Thread Pavel Reichl




On 03/31/2016 08:13 AM, Lukas Slebodnik wrote:

ehlo,

simple patch is attached.

LS


LGTM, build passed. I'm just waiting for CI to finish so I can ACK.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] SDAP: Remove unused parameter talloc context

2016-04-04 Thread Pavel Reichl


On 03/31/2016 08:14 AM, Lukas Slebodnik wrote:

ehlo,

yet another patch with unused parameter.
It has been found as part of unrelated work.

LS


LGTM, build passed. I'm just waiting for CI to finish so I can ACK.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: Design document - sssctl

2016-03-23 Thread Pavel Reichl




On 03/23/2016 03:42 AM, Stephen Gallagher wrote:

On 03/22/2016 07:42 AM, Pavel Reichl wrote:

Hello,

Pavel Březina and I have prepared the 1st draft of design document. We mostly
focused on summing up its future functionality and its interface.

Please comment if you miss some essential functionality or if you would prefer
some different interface.

Thanks!

https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl




"This tool will communicate with InfoPipe responder through its public D-Bus
interface."

Presumably this means that the sssctl CLI will be a thin presentation-layer shim
over the D-BUS API (meaning that the CLI would contain no logic outside of
argument processing)? This would be the ideal case, since it would also allow
plugging in other front-ends for this (such as Cockpit).



Yes, this is current plan.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: Design document - sssctl

2016-03-22 Thread Pavel Reichl


Hello, I'm adding some people, who might be interested in this tool, to CC 
list. (Please feel free to extend).

On 03/22/2016 12:42 PM, Pavel Reichl wrote:

Hello,

Pavel Březina and I have prepared the 1st draft of design document. We mostly 
focused on summing up its future functionality and its interface.

Please comment if you miss some essential functionality or if you would prefer 
some different interface.

Thanks!

https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

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

[SSSD] Design document - sssctl

2016-03-22 Thread Pavel Reichl


Hello,

Pavel Březina and I have prepared the 1st draft of design document. We mostly 
focused on summing up its future functionality and its interface.

Please comment if you miss some essential functionality or if you would prefer 
some different interface.

Thanks!

https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] TOOL: Invalidation of sudo rules at sss_cache

2016-03-08 Thread Pavel Reichl


Hello Petr,

I just run through the code and I have some code style suggestions, feel free 
to disagree :-).

On 03/08/2016 01:11 PM, Petr Cech wrote:

0001-SYSDB-Add-new-funtions-into-sysdb_sudo.patch

...

+
+errno_t sysdb_search_sudo_rules(TALLOC_CTX *mem_ctx,
+struct sss_domain_info *domain,
+const char *sub_filter,
+const char **attrs,
+size_t *msgs_count,
+struct ldb_message ***msgs)


if count and msgs are output parameters then please add _ as a prefix.


+{
+TALLOC_CTX *tmp_ctx;
+struct ldb_dn *dn;
+char *filter;
+int ret;

errno_t might be better here.

+
+tmp_ctx = talloc_new(NULL);
+if (!tmp_ctx) {
+return ENOMEM;
+}


I think we prefere != NULL form - but this is not important.


+
+dn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb, SYSDB_TMPL_CUSTOM_SUBTREE,
+SUDORULE_SUBDIR, domain->name);
+if (!dn) {
+DEBUG(SSSDBG_OP_FAILURE, "Failed to build base dn\n");
+ret = ENOMEM;
+goto done;
+}
+
+if (sub_filter != NULL) {
+filter = talloc_asprintf(tmp_ctx, "(&%s%s)",
+ SUDO_ALL_FILTER, sub_filter);
+} else {
+filter = talloc_asprintf(tmp_ctx, "(&%s)", SUDO_ALL_FILTER);
+}
+if (!filter) {
+DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
+ret = ENOMEM;
+goto done;
+}
+
+DEBUG(SSSDBG_TRACE_INTERNAL,
+  "Search sudo rules with filter: %s\n", filter);
+
+ret = sysdb_search_entry(mem_ctx, domain->sysdb, dn,
+ LDB_SCOPE_SUBTREE, filter, attrs,
+ msgs_count, msgs);


You could use tmp_ctx context here, it would make code a bit longer because you 
would have to steal but also a bit more mem leak resistant.

+
+if (ret != EOK) {
+if (ret == ENOENT) {
+DEBUG(SSSDBG_TRACE_INTERNAL, "No such entry\n");
+}
+else if (ret) {
+DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret, 
strerror(ret));
+}
+}


   You can save indentation.

   if (ret == ENOENT) {
   ...
   } else if (ret != EOK) {
   ...
   }

  Please use sss_strerror instead of strerror().



+
+done:
+talloc_zfree(tmp_ctx);
+return ret;
+}
+





0002-TOOL-Invalidation-of-sudo-rules-at-sss_cache.patch


 From e0143502fce82d353955352d8717202346fed6b6 Mon Sep 17 00:00:00 2001
From: Petr Cech
Date: Tue, 23 Feb 2016 10:11:15 -0500
Subject: [PATCH 2/2] TOOL: Invalidation of sudo rules at sss_cache

This patch adds new functionality to sss_cach for invalidation of given
sudo rule or all sudo rules.

Resolves:
https://fedorahosted.org/sssd/ticket/2081
---

...

  }
@@ -577,6 +611,7 @@ errno_t init_context(int argc, const char *argv[], struct 
cache_tool_ctx **tctx)
  char *service = NULL;
  char *map = NULL;
  char *ssh_host = NULL;
+char *sudo_rule = NULL;
  char *domain = NULL;
  int debug = SSSDBG_DEFAULT;
  errno_t ret = EOK;
@@ -616,6 +651,12 @@ errno_t init_context(int argc, const char *argv[], struct 
cache_tool_ctx **tctx)
  { "ssh-hosts", 'H', POPT_ARG_NONE, NULL, 'h',
  _("Invalidate all SSH hosts"), NULL },
  #endif /* BUILD_SSH */
+#ifdef BUILD_SUDO
+{ "sudo-rule", 'r', POPT_ARG_STRING, _rule, 0,
+_("Invalidate particular sudo rule"), NULL },
+{ "sudo-rules", 'R', POPT_ARG_NONE, NULL, 'r',
+_("Invalidate all cached sudo rules"), NULL },
+#endif /* BUILD_SUDO */
  { "domain", 'd', POPT_ARG_STRING, , 0,
  _("Only invalidate entries from a particular domain"), NULL },
  POPT_TABLEEND
@@ -650,8 +691,12 @@ errno_t init_context(int argc, const char *argv[], struct 
cache_tool_ctx **tctx)
  case 'h':
  idb |= INVALIDATE_SSH_HOSTS;
  break;
+case 'r':
+idb |= INVALIDATE_SUDO_RULES;
+break;
  case 'e':
  idb = INVALIDATE_EVERYTHING;
+idb |= INVALIDATE_SUDO_RULES;
  break;
  }
  }
@@ -664,7 +709,7 @@ errno_t init_context(int argc, const char *argv[], struct 
cache_tool_ctx **tctx)
  }

  if (idb == INVALIDATE_NONE && !user && !group &&
-!netgroup && !service && !map && !ssh_host) {
+!netgroup && !service && !map && !ssh_host && !sudo_rule) {
  BAD_POPT_PARAMS(pc,
  _("Please select at least one object to invalidate\n"),
  ret, fini);
@@ -729,18 +774,28 @@ errno_t init_context(int argc, const char *argv[], struct 
cache_tool_ctx **tctx)
  ctx->update_ssh_host_filter = true;
  }

+if (idb & INVALIDATE_SUDO_RULES) {
+ctx->sudo_rule_filter = talloc_asprintf(ctx, "(%s=*)", SYSDB_NAME);
+

[SSSD] Re: [PATCH] NSS: Move a DEBUG message so that it's less confusing

2016-03-04 Thread Pavel Reichl




On 03/04/2016 11:46 AM, Jakub Hrozek wrote:

Hi,

the attached patch would hopefully make analyzing of NSS logs files in a
multi-domain scenario (typically a trust setup) less confusing.
Previously, we would print "Domain not found" even if the ID was
actually found..



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



Jakub, you might have attached by accident po/ca.po.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] PAM: Clarify man page for domains option

2016-03-04 Thread Pavel Reichl




On 03/02/2016 01:08 PM, Pavel Březina wrote:

On 02/09/2016 03:42 PM, Pavel Reichl wrote:



On 02/09/2016 08:17 AM, Jakub Hrozek wrote:

On Fri, Jan 29, 2016 at 02:30:36PM +0100, Pavel Reichl wrote:

Hello, please see trivial patch attached. Thanks.



 From 6d5f6b71c2d2f891470dc1c9f08ae67f5b6c02f5 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 29 Jan 2016 08:27:01 -0500
Subject: [PATCH] PAM: Clarify man page for domains option

Resolves:
https://fedorahosted.org/sssd/ticket/2946
---
  src/man/pam_sss.8.xml | 8 +---
  1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/man/pam_sss.8.xml b/src/man/pam_sss.8.xml
index
7794d3acfdfdbde491a3e1ada44481b73588e41f..278126c14d0a574a1e120762af264ef653deb0b0
100644
--- a/src/man/pam_sss.8.xml
+++ b/src/man/pam_sss.8.xml
@@ -145,9 +145,11 @@
  SSSD domain names, as specified in the
sssd.conf file.
  
  
-NOTE: Must be used in conjunction with the
-pam_trusted_users and
-pam_public_domains options.
+NOTE: If PAM service is being run by
untrusted user
+(pam_trusted_users option)
+then please make
+sure that restricted domains are public
+(pam_public_domains option).
  Please see the
  
  sssd.conf
--
2.4.3



I'm sorry, but this doesn't read any better to me. Especially I don't
understand "restricted domains are public", sounds like an oxymoron to
me.


Oh, sorry. By "restricted domain" I thought only the domains you are
restricting access to - like the only ones you can use. It's used in the
context of the first paragraph of domains option.

I'll try to rephrase.

"""
If PAM service is being run by untrusted
user(pam_trusted_users option) then please make sure that
domains entered into domains option are actually public
(pam_public_domains option). Otherwise access will be
denied because untrusted user would be trying to access non-public domain.
"""

Does it sound any better? Would you propose some other wording? Or we
can drop the note completely.

Thanks!


I think any description will be confusing without the knowledge of 
pam_trusted_users and pam_public_domains options. Since the default is that all 
users are considered to be trusted I don't think we need to mentioned it here. 
How about:

domains
Allows the administrator to restrict the domains a particular PAM
service is allowed to authenticate against. The format is a comma-
separated list of SSSD domain names, as specified in the sssd.conf
file.

See also: pam_public_domains, pam_trusted_users in sssd.conf(5)
manual page


It's fine by me. Shall you prepare a patch or do we want Jakub's or Aneta's 
approval first?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Make parameter name more descriptive

2016-03-02 Thread Pavel Reichl




On 03/02/2016 03:43 PM, Lukas Slebodnik wrote:

On (02/03/16 15:13), Pavel Reichl wrote:

On 03/02/2016 03:07 PM, Lukas Slebodnik wrote:

On (02/03/16 15:02), Pavel Reichl wrote:

On 03/02/2016 02:59 PM, Lukas Slebodnik wrote:

On (02/03/16 13:45), Pavel Reichl wrote:

On 03/02/2016 01:10 PM, Lukas Slebodnik wrote:

On (02/03/16 13:02), Pavel Reichl wrote:

On 03/02/2016 12:53 PM, Lukas Slebodnik wrote:

On (02/03/16 12:48), Pavel Březina wrote:

On 03/01/2016 03:54 PM, Pavel Reichl wrote:

I added one more similar patch.

sss_idmap_calculate_range can accept domain SID or range identifier on
its input. Previous parameter name was misleading.


Ack to both. They can sure be squashed before pushing but I don't care.

I miss a link here.

Anyway I would like to see a Sumit opinin about renaming variables.
Because name of variables is one of hard things problems in IT
http://martinfowler.com/bliki/TwoHardThings.html


Might we could do an all hands meeting,
because we really don't want to underestimate such important change.


If the patch is not important then it does not make a sense to push it.

BTW You introduced one of bad argument names in the recent
commit 8babbeee01e67893af4828ddfc922ecac0be4197


Sure, I did that and now I see that it would be nice to use a different name.


Sumit is fine with changes.
Would you be so kind and could you send squased patch which
we can push.


If you insist on squashing the patches then please do it while pushing it.


I would but could you help me with commit mesage?
I'm sorry but for me it's the same problem as name of variables
http://martinfowler.com/bliki/TwoHardThings.html
   "There are only two hard things in Computer Science: cache invalidation and
naming things."
   Phil Karlto

I would appreciate if autor of the patch could do it.

LS



Lukas can you just push the patches as they are?

I would like but there is issue with separating changes to patches.

In general it make sense to have small patches. And each patch
should do one thing e.g. do not mix unrelated coding style with change of
logic. However if TWO patches do the same it does not make a sense to separate
them. In samba, they separate this if the change is in different part of code
e.g. ldb, tdm, talloc, samba3 ... The reason is bacporting of such changes.
But in our case the change is in one module.


Sumit and Pavel ACKed them independently and neither of them expressed
the desire of having them squashed.

Neither of them expresed that it's the best idea ever to have changes in two
patches.

Please update patch and I will send a link to CI because neither of reviewers
did it.
/sssd-devel@lists.fedorahosted.org



Squashed patch attached. Do you want Sumit and Pavel to review the patch again?
>From 54e04f26494fa37b0caa10613e0455b0650ee0c8 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 1 Mar 2016 08:41:24 -0500
Subject: [PATCH] IDMAP: Make parameter names more descriptive

Domain SID (not name) is part of identification string for helper range
in generate_sec_slice_name().

Use more generic name for range identifier when calculating range for
new slice in sss_idmap_calculate_range().
---
 src/lib/idmap/sss_idmap.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index e3e9972b802748770a5f7440fa8ddc8ba75d3362..dbb152bb9afc3978f201c967ca31d6b67f7fc4b6 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -320,7 +320,7 @@ static bool check_dom_overlap(struct idmap_range_params *prim_range,
 }
 
 enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
-const char *dom_sid,
+const char *range_id,
 id_t *slice_num,
 struct sss_idmap_range *_range)
 {
@@ -362,8 +362,8 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
  */
 orig_slice = 0;
 } else {
-/* Hash the domain sid string */
-hash_val = murmurhash3(dom_sid, strlen(dom_sid), 0xdeadbeef);
+/* Hash the range identifier string */
+hash_val = murmurhash3(range_id, strlen(range_id), 0xdeadbeef);
 
 /* Now get take the modulus of the hash val and the max_slices
  * to determine its optimal position in the range.
@@ -536,13 +536,13 @@ idmap_error_code dom_check_collision(struct idmap_domain_info *dom_list,
 
 static char*
 generate_sec_slice_name(struct sss_idmap_ctx *ctx,
-const char *domain_name, uint32_t rid)
+const char *domain_sid, uint32_t rid)
 {
 const char *SEC_SLICE_NAME_FMT = "%s-%"PRIu32;
 char *slice_name;
 int len, len2;
 
-len = snprintf(NULL, 0, SEC_SLICE_NAME_FMT

[SSSD] Re: [PATCH] IDMAP: Make parameter name more descriptive

2016-03-02 Thread Pavel Reichl




On 03/02/2016 03:07 PM, Lukas Slebodnik wrote:

On (02/03/16 15:02), Pavel Reichl wrote:

On 03/02/2016 02:59 PM, Lukas Slebodnik wrote:

On (02/03/16 13:45), Pavel Reichl wrote:

On 03/02/2016 01:10 PM, Lukas Slebodnik wrote:

On (02/03/16 13:02), Pavel Reichl wrote:

On 03/02/2016 12:53 PM, Lukas Slebodnik wrote:

On (02/03/16 12:48), Pavel Březina wrote:

On 03/01/2016 03:54 PM, Pavel Reichl wrote:

I added one more similar patch.

sss_idmap_calculate_range can accept domain SID or range identifier on
its input. Previous parameter name was misleading.


Ack to both. They can sure be squashed before pushing but I don't care.

I miss a link here.

Anyway I would like to see a Sumit opinin about renaming variables.
Because name of variables is one of hard things problems in IT
http://martinfowler.com/bliki/TwoHardThings.html


Might we could do an all hands meeting,
because we really don't want to underestimate such important change.


If the patch is not important then it does not make a sense to push it.

BTW You introduced one of bad argument names in the recent
commit 8babbeee01e67893af4828ddfc922ecac0be4197


Sure, I did that and now I see that it would be nice to use a different name.


Sumit is fine with changes.
Would you be so kind and could you send squased patch which
we can push.


If you insist on squashing the patches then please do it while pushing it.


I would but could you help me with commit mesage?
I'm sorry but for me it's the same problem as name of variables
http://martinfowler.com/bliki/TwoHardThings.html
   "There are only two hard things in Computer Science: cache invalidation and
naming things."
   Phil Karlto

I would appreciate if autor of the patch could do it.

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



Lukas can you just push the patches as they are? Sumit and Pavel ACKed them 
independently and neither of them expressed the desire of having them squashed. 
I believe it would be the fastest way how to successfully finish this quest. 
Thanks!
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Make parameter name more descriptive

2016-03-02 Thread Pavel Reichl




On 03/02/2016 02:59 PM, Lukas Slebodnik wrote:

On (02/03/16 13:45), Pavel Reichl wrote:

On 03/02/2016 01:10 PM, Lukas Slebodnik wrote:

On (02/03/16 13:02), Pavel Reichl wrote:

On 03/02/2016 12:53 PM, Lukas Slebodnik wrote:

On (02/03/16 12:48), Pavel Březina wrote:

On 03/01/2016 03:54 PM, Pavel Reichl wrote:

I added one more similar patch.

sss_idmap_calculate_range can accept domain SID or range identifier on
its input. Previous parameter name was misleading.


Ack to both. They can sure be squashed before pushing but I don't care.

I miss a link here.

Anyway I would like to see a Sumit opinin about renaming variables.
Because name of variables is one of hard things problems in IT
http://martinfowler.com/bliki/TwoHardThings.html


Might we could do an all hands meeting,
because we really don't want to underestimate such important change.


If the patch is not important then it does not make a sense to push it.

BTW You introduced one of bad argument names in the recent
commit 8babbeee01e67893af4828ddfc922ecac0be4197


Sure, I did that and now I see that it would be nice to use a different name.


Sumit is fine with changes.
Would you be so kind and could you send squased patch which
we can push.


If you insist on squashing the patches then please do it while pushing it.



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


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

[SSSD] Re: [PATCH] IDMAP: Make parameter name more descriptive

2016-03-02 Thread Pavel Reichl




On 03/02/2016 01:10 PM, Lukas Slebodnik wrote:

On (02/03/16 13:02), Pavel Reichl wrote:

On 03/02/2016 12:53 PM, Lukas Slebodnik wrote:

On (02/03/16 12:48), Pavel Březina wrote:

On 03/01/2016 03:54 PM, Pavel Reichl wrote:

I added one more similar patch.

sss_idmap_calculate_range can accept domain SID or range identifier on
its input. Previous parameter name was misleading.


Ack to both. They can sure be squashed before pushing but I don't care.

I miss a link here.

Anyway I would like to see a Sumit opinin about renaming variables.
Because name of variables is one of hard things problems in IT
http://martinfowler.com/bliki/TwoHardThings.html


Might we could do an all hands meeting,
because we really don't want to underestimate such important change.


If the patch is not important then it does not make a sense to push it.

BTW You introduced one of bad argument names in the recent
commit 8babbeee01e67893af4828ddfc922ecac0be4197


Sure, I did that and now I see that it would be nice to use a different name.



Sumit ACKed this patch and he didn't ask to change the name.
It would good to know his opinion and maybe he will propose
better name. @see http://martinfowler.com/bliki/TwoHardThings.html


I just thought that it would be nice if 3 software engineers could decide if 
renaming variable is a good change without the need to ask senior colleagues.



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


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

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-03-02 Thread Pavel Reichl


Hello,

I updated the design document do reflect changes that were decided in this 
thread.

I also moved the document to more appropriate location.

https://fedorahosted.org/sssd/wiki/DesignDocs/IdmapAutoAssignNewSlices

Bye.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Make parameter name more descriptive

2016-03-02 Thread Pavel Reichl




On 03/02/2016 12:53 PM, Lukas Slebodnik wrote:

On (02/03/16 12:48), Pavel Březina wrote:

On 03/01/2016 03:54 PM, Pavel Reichl wrote:

I added one more similar patch.

sss_idmap_calculate_range can accept domain SID or range identifier on
its input. Previous parameter name was misleading.


Ack to both. They can sure be squashed before pushing but I don't care.

I miss a link here.

Anyway I would like to see a Sumit opinin about renaming variables.
Because name of variables is one of hard things problems in IT
http://martinfowler.com/bliki/TwoHardThings.html


Might we could do an all hands meeting, because we really don't want to 
underestimate such important change.



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


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

[SSSD] Re: [PATCH] IDMAP: Make parameter name more descriptive

2016-03-01 Thread Pavel Reichl




On 03/01/2016 04:54 PM, Lukas Slebodnik wrote:

On (01/03/16 15:54), Pavel Reichl wrote:

I added one more similar patch.

sss_idmap_calculate_range can accept domain SID or range identifier on its 
input. Previous parameter name was misleading.

Could you explain why it need to be in two separate patches?

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


2 separate changes? 2 different functions. I don't really care.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Make parameter name more descriptive

2016-03-01 Thread Pavel Reichl


I added one more similar patch.

sss_idmap_calculate_range can accept domain SID or range identifier on its 
input. Previous parameter name was misleading.
>From c17976f5c9053e09ea2033284dc3545d02ba0644 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 1 Mar 2016 08:41:24 -0500
Subject: [PATCH 1/2] IDMAP: Make parameter name more descriptive

Domain SID (not name) is part of identification string for helper range.
---
 src/lib/idmap/sss_idmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index e3e9972b802748770a5f7440fa8ddc8ba75d3362..905087b7510f2524adc94d4a845f9454ad760311 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -536,13 +536,13 @@ idmap_error_code dom_check_collision(struct idmap_domain_info *dom_list,
 
 static char*
 generate_sec_slice_name(struct sss_idmap_ctx *ctx,
-const char *domain_name, uint32_t rid)
+const char *domain_sid, uint32_t rid)
 {
 const char *SEC_SLICE_NAME_FMT = "%s-%"PRIu32;
 char *slice_name;
 int len, len2;
 
-len = snprintf(NULL, 0, SEC_SLICE_NAME_FMT, domain_name, rid);
+len = snprintf(NULL, 0, SEC_SLICE_NAME_FMT, domain_sid, rid);
 if (len <= 0) {
 return NULL;
 }
@@ -552,7 +552,7 @@ generate_sec_slice_name(struct sss_idmap_ctx *ctx,
 return NULL;
 }
 
-len2 = snprintf(slice_name, len + 1, SEC_SLICE_NAME_FMT, domain_name,
+len2 = snprintf(slice_name, len + 1, SEC_SLICE_NAME_FMT, domain_sid,
 rid);
 if (len != len2) {
 ctx->free_func(slice_name, ctx->alloc_pvt);
-- 
2.4.3

>From 579a6fc3f8b8a8ccd08b0ede51af2a44af789ee9 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 1 Mar 2016 09:46:26 -0500
Subject: [PATCH 2/2] IDMAP: Make parameter name more descriptive

Use more generic name for range identifier when calculating range for
new slice.
---
 src/lib/idmap/sss_idmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 905087b7510f2524adc94d4a845f9454ad760311..dbb152bb9afc3978f201c967ca31d6b67f7fc4b6 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -320,7 +320,7 @@ static bool check_dom_overlap(struct idmap_range_params *prim_range,
 }
 
 enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
-const char *dom_sid,
+const char *range_id,
 id_t *slice_num,
 struct sss_idmap_range *_range)
 {
@@ -362,8 +362,8 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
  */
 orig_slice = 0;
 } else {
-/* Hash the domain sid string */
-hash_val = murmurhash3(dom_sid, strlen(dom_sid), 0xdeadbeef);
+/* Hash the range identifier string */
+hash_val = murmurhash3(range_id, strlen(range_id), 0xdeadbeef);
 
 /* Now get take the modulus of the hash val and the max_slices
  * to determine its optimal position in the range.
-- 
2.4.3

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

[SSSD] [PATCH] IDMAP: Make parameter name more descriptive

2016-03-01 Thread Pavel Reichl


Hello,

Please see trivial patch attached, thanks

Bye.
>From 96e6f5cfe6d134748a4db248266fba774b32628b Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 1 Mar 2016 08:41:24 -0500
Subject: [PATCH] IDMAP: Make parameter name more descriptive

Domain SID (not name) is used for computaion of seed for ID mapping.
---
 src/lib/idmap/sss_idmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index e3e9972b802748770a5f7440fa8ddc8ba75d3362..905087b7510f2524adc94d4a845f9454ad760311 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -536,13 +536,13 @@ idmap_error_code dom_check_collision(struct idmap_domain_info *dom_list,
 
 static char*
 generate_sec_slice_name(struct sss_idmap_ctx *ctx,
-const char *domain_name, uint32_t rid)
+const char *domain_sid, uint32_t rid)
 {
 const char *SEC_SLICE_NAME_FMT = "%s-%"PRIu32;
 char *slice_name;
 int len, len2;
 
-len = snprintf(NULL, 0, SEC_SLICE_NAME_FMT, domain_name, rid);
+len = snprintf(NULL, 0, SEC_SLICE_NAME_FMT, domain_sid, rid);
 if (len <= 0) {
 return NULL;
 }
@@ -552,7 +552,7 @@ generate_sec_slice_name(struct sss_idmap_ctx *ctx,
 return NULL;
 }
 
-len2 = snprintf(slice_name, len + 1, SEC_SLICE_NAME_FMT, domain_name,
+len2 = snprintf(slice_name, len + 1, SEC_SLICE_NAME_FMT, domain_sid,
 rid);
 if (len != len2) {
 ctx->free_func(slice_name, ctx->alloc_pvt);
-- 
2.4.3

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

[SSSD] Re: [PATCH] PAM: Clarify man page for domains option

2016-02-29 Thread Pavel Reichl


Bump.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Add minor performance improvements

2016-02-29 Thread Pavel Reichl


Bump.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Add minor performance improvements

2016-02-17 Thread Pavel Reichl


On 02/15/2016 06:19 PM, Sumit Bose wrote:

On Tue, Jan 26, 2016 at 05:35:06PM +0100, Pavel Reichl wrote:

>Hello,
>
>please see simple patch attached.

Hi Pavel,

sorry for the delay. See comments below.

bye,
Sumit



Thanks for checking and for comments. Amended patch is attached.

Bye.
>From bf09421bcd4e5be5fad4d907ccdd8f99f7999b88 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 26 Jan 2016 11:28:22 -0500
Subject: [PATCH] IDMAP: Add minor performance improvements

Some ID ranges are precalculated when ID mapping is being initialized.
This patch utilizes these (helper) ranges when new domains are generated
if appropriate.
---
 src/lib/idmap/sss_idmap.c | 95 +--
 1 file changed, 84 insertions(+), 11 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index e3e9972b802748770a5f7440fa8ddc8ba75d3362..5c84f2a997e5134583ad8c5fea72b02c7e1e52a2 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -89,6 +89,49 @@ static char *idmap_strdup(struct sss_idmap_ctx *ctx, const char *str)
 return new;
 }
 
+static bool ranges_eq(const struct idmap_range_params *a,
+  const struct idmap_range_params *b)
+{
+if (a == NULL || b == NULL) {
+return false;
+}
+
+if (a->first_rid == b->first_rid
+&& a->min_id == b->min_id
+&& a->max_id == b->max_id) {
+return true;
+}
+
+return false;
+}
+
+static enum idmap_error_code
+construct_range(struct sss_idmap_ctx *ctx,
+const struct idmap_range_params *src,
+char *id,
+struct idmap_range_params **_dst)
+{
+struct idmap_range_params *dst;
+
+if (src == NULL || id == NULL || _dst == NULL) {
+return IDMAP_ERROR;
+}
+
+dst = ctx->alloc_func(sizeof(struct idmap_range_params), ctx->alloc_pvt);
+if (dst == NULL) {
+return IDMAP_OUT_OF_MEMORY;
+}
+
+dst->min_id = src->min_id;
+dst->max_id = src->max_id;
+dst->first_rid = src->first_rid;
+dst->next = NULL;
+dst->range_id = id;
+
+*_dst = dst;
+return IDMAP_SUCCESS;
+}
+
 static bool id_is_in_range(uint32_t id,
struct idmap_range_params *rp,
uint32_t *rid)
@@ -232,6 +275,20 @@ static void free_helpers(struct sss_idmap_ctx *ctx,
 }
 }
 
+static struct idmap_range_params*
+get_helper_by_id(struct idmap_range_params *helpers, const char *id)
+{
+struct idmap_range_params *it;
+
+for (it = helpers; it != NULL; it = it->next) {
+if (strcmp(it->range_id, id) == 0) {
+return it;
+}
+}
+
+return NULL;
+}
+
 static void sss_idmap_free_domain(struct sss_idmap_ctx *ctx,
   struct idmap_domain_info *dom)
 {
@@ -854,30 +911,44 @@ static bool comp_id(struct idmap_range_params *range_params, long long rid,
 
 static enum idmap_error_code
 get_range(struct sss_idmap_ctx *ctx,
+  struct idmap_range_params *helpers,
   const char *dom_sid,
   long long rid,
   struct idmap_range_params **_range)
 {
-char *secondary_name;
+char *secondary_name = NULL;;
 enum idmap_error_code err;
 int first_rid;
 struct idmap_range_params *range;
+struct idmap_range_params *helper;
 
 first_rid = (rid / ctx->idmap_opts.rangesize) * ctx->idmap_opts.rangesize;
 
 secondary_name = generate_sec_slice_name(ctx, dom_sid, first_rid);
 if (secondary_name == NULL) {
-return IDMAP_OUT_OF_MEMORY;
+err = IDMAP_OUT_OF_MEMORY;
+goto error;
 }
 
-err = generate_slice(ctx, secondary_name, first_rid, );
-if (err == IDMAP_OUT_OF_SLICES) {
-ctx->free_func(secondary_name, ctx->alloc_pvt);
-return err;
+helper = get_helper_by_id(helpers, secondary_name);
+if (helper != NULL) {
+/* Utilize helper's range. */
+err = construct_range(ctx, helper, secondary_name, );
+} else {
+/* Have to generate a whole new range. */
+err = generate_slice(ctx, secondary_name, first_rid, );
+}
+
+if (err != IDMAP_SUCCESS) {
+goto error;
 }
 
 *_range = range;
 return IDMAP_SUCCESS;
+
+error:
+ctx->free_func(secondary_name, ctx->alloc_pvt);
+return err;
 }
 
 static enum idmap_error_code
@@ -904,9 +975,7 @@ spawn_dom(struct sss_idmap_ctx *ctx,
 it = ctx->idmap_domain_info;
 while (it != NULL) {
 /* Find the newly added domain. */
-if (it->range_params.first_rid == range->first_rid
-&& it->range_params.min_id == range->min_id
-&& it->range_params.max_id == range->max_id) {
+if (ranges_eq(>range_params, range)) {
 
 /* Share helpers. */

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-12 Thread Pavel Reichl

On 02/12/2016 02:17 PM, Lukas Slebodnik wrote:

On (12/02/16 12:54), Pavel Reichl wrote:

On 02/12/2016 09:00 AM, Lukas Slebodnik wrote:

On (11/02/16 15:30), Alexander Bokovoy wrote:

- Original Message -

On 02/10/2016 02:34 PM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

On 02/10/2016 11:46 AM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

On 02/10/2016 11:06 AM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

since getting those values requires to parse the string it would be
nice
to get some official details about the string.

Well, the string content after DSID- mark can be completely
missing while the hex of the code (80090308) will be there.

The presence of "DSID- ..." error message is regulated by
ulHideDSID character of the dsHeuristics attribute (MS-ADTS
6.1.1.2.4.1.2). So you can have Active Directory where DSID-
string is completely missing but Win32 code for the error is there.

Alexander thanks for looking into this, but what we need is to
distinguish between reasons for invalid credentials.

e.g.
Bind result: Invalid credentials(49), 80090308: LdapErr:
DSID-0C0903C5, comment: AcceptSecurityContext error, data 773, v23f0
Bind result: Invalid credentials(49), 80090308: LdapErr:
DSID-0C0903C5, comment: AcceptSecurityContext error, data 775, v23f0

As I said, you should not rely on the information being available to
you
as it might be disabled completely by the AD administrators in
ndsHeuristics attribute.

What are you going to do when ulHideDSID flag is set to 1?

The ticket is about providing extra info to user if account is locked.
If we can't decide, we return generic access denied and generic
message. Best afford attitude is fine here...IMO.

That's fine but please add documentation about the behavior into the
commit message so that we would have this discussion recorded somehow.

OK, would this:

"""
This patch is best effort only as decision whether account is actually
locked is based on parsing error message returned by AD. The format and
content of this error message might be subject of change in future
releases and also can be modified by
AD administrators.
"""

appended to the commit message of the first patch work for you?

Please refer to specific parts of MS-ADTS I've mentioned.

OK Alexander, would following work as commit message for you?

This patch is best effort only as decision whether account is actually locked
is based on parsing error message returned by AD. The format and content of
this error message might be subject of change in future releases and also
can be modified by AD
administrators.

If account is locked bind operation is expected to return following error
message:

-
Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5, comment:
AcceptSecurityContext error, data 775, v23f0
-

Where sub string 'data 775' implies that account is locked
(ERROR_ACCOUNT_LOCKED_OUT) [1]. However the 80090308 (error code 0x80090308,
SEC_E_INVALID_TOKEN) is the only guaranteed part of error string [2].

Error message is described in further detail as [3]:
-
When the server fails an LDAP operation with an error, and the server has
sufficient resources to compute a string value for the errorMessage field of
the LDAPResult, it includes a string in the errorMessage field of the
LDAPResult (see [RFC2251]
section 4.1.10). The string contains further information about the error.

The first eight characters of the errorMessage string are a 32-bit integer,
expressed in hexadecimal. Where protocol specifies the extended error code
"" there is no restriction on the value of the 32-bit integer.
It is recommended
that implementations use a Windows error code for the 32-bit integer in this
case in order to improve usability of the directory for clients. Where
protocol specifies an extended error code which is a Windows error code, the
32-bit integer is the
specified Windows error code. Any data after the eighth character is
strictly informational and used only for debugging. Conformant
implementations need not put any value beyond the eighth character of the
errorMessage field.
-

Pavel, this is great!

Happy to hear that, thanks for checking the MS-ADTS.

I like it too.

Good :-)

Please do not forget to use just 72 columns in commit message
http

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-12 Thread Pavel Reichl




On 02/09/2016 04:25 PM, Pavel Reichl wrote:



On 02/09/2016 08:09 AM, Jakub Hrozek wrote:

On Mon, Feb 08, 2016 at 01:56:07PM +0100, Pavel Reichl wrote:



diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index 
73a21bfa0049bc4d3cfacb49201707868c87e533..2dbc58a451686beda0faa9e9366bbc3b3b4c253e
 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -1040,6 +1040,27 @@ pam_account_expired_message = Account expired, please 
call help desk.
  
  
  
+pam_account_locked_message (string)
+
+
+   If user is authenticating and


Please ask someone for an English review (I know Dan started, but I
didn't see a fixed version yet). At the very least, this should read "a
user".


I attached Dan's patch. I took the liberty of adding note regarding pam 
verbosity. Hope it's fine by Dan.




+   account is locked then by default
+   'Permission denied' is output. This output will
+   be changed to content of this variable if it is
+   set.
+
+
+example:
+
+pam_account_locked_message = Account locked, please call help desk.
+
+
+
+Default: none
+
+
+
+
  p11_child_timeout (integer)
  
  


The rest of the patch looks good to me and seems to work as advertized.


Thanks.


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



Patch set with amended commit message. Commit message was acked by AB.
>From 0ac176a3e6f18969e47bbb1d30756f71f8708d9a Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 5 Feb 2016 07:27:38 -0500
Subject: [PATCH 1/3] SDAP: Add return code ERR_ACCOUNT_LOCKED

Add code to distinquish state when account is locked in Active
Directory server.

Tested against Windows Server 2012

This patch is best effort only as decision whether account is actually
locked is based on parsing error message returned by AD. The format and
content of this error message might be subject of change in future
releases and also can be modified by AD administrators.

If account is locked bind operation is expected to return following
error message:

-
Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5, comment:
AcceptSecurityContext error, data 775, v23f0
-

Where sub string 'data 775' implies that account is locked
(ERROR_ACCOUNT_LOCKED_OUT) [1]. However the 80090308 (error code
0x80090308, SEC_E_INVALID_TOKEN) is the only guaranteed part of error
string [2].

Error message is described in further detail as [3]:
-
When the server fails an LDAP operation with an error, and the server
has sufficient resources to compute a string value for the errorMessage
field of the LDAPResult, it includes a string in the errorMessage field
of the LDAPResult (see [RFC2251] section 4.1.10). The string contains
further information about the error.

The first eight characters of the errorMessage string are a 32-bit
integer, expressed in hexadecimal. Where protocol specifies the extended
error code "" there is no restriction on the value of the
32-bit integer.  It is recommended that implementations use a Windows
error code for the 32-bit integer in this case in order to improve
usability of the directory for clients.  Where protocol specifies an
extended error code which is a Windows error code, the 32-bit integer is
the specified Windows error code.  Any data after the eighth character
is strictly informational and used only for debugging. Conformant
implementations need not put any value beyond the eighth character of
the errorMessage field.
-

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms681386%28v=vs.85%29.aspx
[2] https://social.msdn.microsoft.com/Forums/en-US/e1d600c8-60b7-4ed0-94cb-20ddd6c1a1c6/msadts-user-locking-password-policies?forum=os_windowsprotocols
[3] MS-ADTS 3.1.1.3.1.9 https://msdn.microsoft.com/en-us/library/cc223253.aspx

Resolves:
https://fedorahosted.org/sssd/ticket/2839
---
 src/providers/data_provider.h  | 2 ++
 src/providers/ldap/ldap_auth.c | 4 
 src/providers/ldap/sdap_async_connection.c | 3 +++

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-12 Thread Pavel Reichl

On 02/12/2016 09:00 AM, Lukas Slebodnik wrote:

On (11/02/16 15:30), Alexander Bokovoy wrote:

- Original Message -

On 02/10/2016 02:34 PM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

On 02/10/2016 11:46 AM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

On 02/10/2016 11:06 AM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

since getting those values requires to parse the string it would be
nice
to get some official details about the string.

Well, the string content after DSID- mark can be completely
missing while the hex of the code (80090308) will be there.

Alexander thanks for looking into this, but what we need is to
distinguish between reasons for invalid credentials.

As I said, you should not rely on the information being available to
you
as it might be disabled completely by the AD administrators in
ndsHeuristics attribute.

What are you going to do when ulHideDSID flag is set to 1?

The ticket is about providing extra info to user if account is locked.
If we can't decide, we return generic access denied and generic
message. Best afford attitude is fine here...IMO.

That's fine but please add documentation about the behavior into the
commit message so that we would have this discussion recorded somehow.

OK, would this:

appended to the commit message of the first patch work for you?

Please refer to specific parts of MS-ADTS I've mentioned.

OK Alexander, would following work as commit message for you?

If account is locked bind operation is expected to return following error
message:

-
Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5, comment:
AcceptSecurityContext error, data 775, v23f0
-

Pavel, this is great!

Happy to hear that, thanks for checking the MS-ADTS.

I like it too.

Good :-)

Please do not forget to use just 72 columns in commit message
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

Sure.

It would be al

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-12 Thread Pavel Reichl




On 02/12/2016 02:19 PM, Pavel Reichl wrote:



On 02/09/2016 04:25 PM, Pavel Reichl wrote:



On 02/09/2016 08:09 AM, Jakub Hrozek wrote:

On Mon, Feb 08, 2016 at 01:56:07PM +0100, Pavel Reichl wrote:



diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index 
73a21bfa0049bc4d3cfacb49201707868c87e533..2dbc58a451686beda0faa9e9366bbc3b3b4c253e
 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -1040,6 +1040,27 @@ pam_account_expired_message = Account expired, please 
call help desk.
  
  
  
+pam_account_locked_message (string)
+
+
+   If user is authenticating and


Please ask someone for an English review (I know Dan started, but I
didn't see a fixed version yet). At the very least, this should read "a
user".


I attached Dan's patch. I took the liberty of adding note regarding pam 
verbosity. Hope it's fine by Dan.




+   account is locked then by default
+   'Permission denied' is output. This output will
+   be changed to content of this variable if it is
+   set.
+
+
+example:
+
+pam_account_locked_message = Account locked, please call help desk.
+
+
+
+Default: none
+
+
+
+
  p11_child_timeout (integer)
  
  


The rest of the patch looks good to me and seems to work as advertized.


Thanks.


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



Patch set with amended commit message. Commit message was acked by AB.


Lukas asked me for adding comment regarding documenting '775' string to the 
code. Please see updated patch set. Thanks.
>From 642eba8061f3e58f7885e030f4e30a3f59896142 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 5 Feb 2016 07:27:38 -0500
Subject: [PATCH 1/3] SDAP: Add return code ERR_ACCOUNT_LOCKED

Add code to distinquish state when account is locked in Active
Directory server.

Tested against Windows Server 2012

This patch is best effort only as decision whether account is actually
locked is based on parsing error message returned by AD. The format and
content of this error message might be subject of change in future
releases and also can be modified by AD administrators.

If account is locked bind operation is expected to return following
error message:

---
Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5, comment:
AcceptSecurityContext error, data 775, v23f0
---

Where sub string 'data 775' implies that account is locked
(ERROR_ACCOUNT_LOCKED_OUT) [1]. However the 80090308 (error code
0x80090308, SEC_E_INVALID_TOKEN) is the only guaranteed part of error
string [2].

Error message is described in further detail as [3]:
---
When the server fails an LDAP operation with an error, and the server
has sufficient resources to compute a string value for the errorMessage
field of the LDAPResult, it includes a string in the errorMessage field
of the LDAPResult (see [RFC2251] section 4.1.10). The string contains
further information about the error.

The first eight characters of the errorMessage string are a 32-bit
integer, expressed in hexadecimal. Where protocol specifies the extended
error code "" there is no restriction on the value of the
32-bit integer.  It is recommended that implementations use a Windows
error code for the 32-bit integer in this case in order to improve
usability of the directory for clients.  Where protocol specifies an
extended error code which is a Windows error code, the 32-bit integer is
the specified Windows error code.  Any data after the eighth character
is strictly informational and used only for debugging. Conformant
implementations need not put any value beyond the eighth character of
the errorMessage field.
---

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms681386%28v=vs.85%29.aspx
[2] https://social.msdn.microsoft.com/Forums/en-US/e1d600c8-60b7-4ed0-94cb-20ddd6c1a1c6/msadts-user-locking-password-policies?forum=os_windowsprotocols
[3] MS-ADTS 3.1.1.3.1.9
https://msdn.microsoft.com/en-us/library/cc223253.aspx

Resolves:
https://fedorahosted.org/sssd/t

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-11 Thread Pavel Reichl

On 02/10/2016 02:34 PM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

On 02/10/2016 11:46 AM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

On 02/10/2016 11:06 AM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

since getting those values requires to parse the string it would be nice
to get some official details about the string.

Well, the string content after DSID- mark can be completely
missing while the hex of the code (80090308) will be there.

Alexander thanks for looking into this, but what we need is to
distinguish between reasons for invalid credentials.

e.g.
Bind result: Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5,
comment: AcceptSecurityContext error, data 773, v23f0
Bind result: Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5,
comment: AcceptSecurityContext error, data 775, v23f0

As I said, you should not rely on the information being available to you
as it might be disabled completely by the AD administrators in
ndsHeuristics attribute.

What are you going to do when ulHideDSID flag is set to 1?

The ticket is about providing extra info to user if account is locked.
If we can't decide, we return generic access denied and generic
message. Best afford attitude is fine here...IMO.

That's fine but please add documentation about the behavior into the
commit message so that we would have this discussion recorded somehow.

OK, would this:

"""
This patch is best effort only as decision whether account is actually locked
is based on parsing error message returned by AD. The format and content of
this error message might be subject of change in future releases and also can
be modified by
AD administrators.
"""

appended to the commit message of the first patch work for you?

Please refer to specific parts of MS-ADTS I've mentioned.

OK Alexander, would following work as commit message for you?

This patch is best effort only as decision whether account is actually locked is based on parsing error message returned by AD. The format and content of this error message might be subject of change in future releases and also can be modified by AD
administrators.

If account is locked bind operation is expected to return following error
message:

-
Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5, comment:
AcceptSecurityContext error, data 775, v23f0
-

Error message is described in further detail as [3]:
-
When the server fails an LDAP operation with an error, and the server has sufficient resources to compute a string value for the errorMessage field of the LDAPResult, it includes a string in the errorMessage field of the LDAPResult (see [RFC2251]
section 4.1.10). The string contains further information about the error.

The first eight characters of the errorMessage string are a 32-bit integer, expressed in hexadecimal. Where protocol specifies the extended error code "" there is no restriction on the value of the 32-bit integer. It is recommended
that implementations use a Windows error code for the 32-bit integer in this case in order to improve usability of the directory for clients. Where protocol specifies an extended error code which is a Windows error code, the 32-bit integer is the
specified Windows error code. Any data after the eighth character is strictly informational and used only for debugging. Conformant implementations need not put any value beyond the eighth character of the errorMessage field.

[1]
https://msdn.microsoft.com/en-us/library/windows/desktop/ms681386%28v=vs.85%29.aspx
[2]
https://social.msdn.microsoft.com/Forums/en-US/e1d600c8-60b7-4ed0-94cb-20ddd6c1a1c6/msadts-user-locking-password-policies?forum=os_windowsprotocols
[3] MS-ADTS 3.1.1.3.1.9 https://msdn.microsoft.com/en-us/library/cc223253.aspx
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-10 Thread Pavel Reichl




On 02/10/2016 10:26 AM, Sumit Bose wrote:



Hi Alexander,

thank you for reaching out to MSFT for enhancing the docs. But I'm
afraid just checking for 80090308 is not sufficient as you can see from
http://www-01.ibm.com/support/docview.wss?uid=swg21290631 . The value
behind the 'data' string is really important to find out the real reason
for the denial. The values themselve like (0x)701, (0x)773 or (0x)775
are documented as well (although I do not have the link at hand). But


I suppose you mean 
https://msdn.microsoft.com/en-us/library/windows/desktop/ms681386%28v=vs.85%29.aspx
 (ERROR_ACCOUNT_LOCKED_OUT)



since getting those values requires to parse the string it would be nice
to get some official details about the string.

bye,
Sumit


--
/ Alexander Bokovoy

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


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

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-10 Thread Pavel Reichl




On 02/10/2016 10:38 AM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Sumit Bose wrote:

On Wed, Feb 10, 2016 at 10:45:39AM +0200, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Alexander Bokovoy wrote:
>On Mon, 08 Feb 2016, Sumit Bose wrote:
>>On Mon, Feb 08, 2016 at 10:34:16AM +0100, Pavel Reichl wrote:
>>>
>>>
>>>On 02/05/2016 03:16 PM, Lukas Slebodnik wrote:
>>>>>
>>>>The ticket is about "SSSD should be about to display message to the user 
when
>>>>the account in Active Directory is 'locked out'"
>>>>
>>>>If the string is not standardized among AD versions
>>>>than this ticket is NOT solved.
>>>
>>>So what do you propose? Rename ticket to contain version of tested
>>>AD? Or should we say user that although we have fix that would work
>>>for him it might not work for all AD versions so we won't provide it?
>>>
>>>Can we ask our QA to test on all AD version they can lay their hands on?
>>>
>>>>
>>>>>>Could you add link to msdn documentation where it is described that this 
string
>>>>>>is guaranted to be returned in such case?
>>>>>
>>>>>It's not MSDN, but:
>>>>>   http://www-01.ibm.com/support/docview.wss?uid=swg21290631
>>>>I would prefer official documantation (msdn) in code ar at least in
>>>>commit message.
>>>
>>>We don't have any and it's possible it's not publicly documented.
>>
>>The '755' in the message is a specific error code of some AD
>>calls which can be seen by the link Dan send. So I guess this will not
>>change.
>>
>>While I can make sense of some part of the remaining error string (yes,
>>it is just a string send with the LDAP bind response) I didn't found any
>>general description of the format and the components of the error string
>>on the MSFT sites.
>You should use error code 0x80090308, SEC_E_INVALID_TOKEN. The 80090308
>should be in the error string and this is what you have to compare
>against.
>
>https://social.msdn.microsoft.com/Forums/en-US/e1d600c8-60b7-4ed0-94cb-20ddd6c1a1c6/msadts-user-locking-password-policies?forum=os_windowsprotocols
>---
>Regarding the references in MS-ADTS on how account lockout policy is
>applied on Bind requests, I reported this as a document bug to the
>product team to request references being added.
>
>For LDAP errors returned by the server on a Simple Bind when the account
>is locked, the error is the same as if the Bind provided a wrong
>password. Windows Active Directory returns the LDAP error
>invalidCredentials.
>
>LDAP Bind Response error codes are documented in RFC2251 4.2.3..
>Examples of errors are operationsError , strongAuthRequired,
>inappropriateAuthentication, invalidCredentials, unavailable.
>
>RFC2251
>4.2.3. Bind Response
>- invalidCredentials: the wrong password was supplied or the SASL
> credentials could not be processed.
>The extended error information looks like this:
>---
>res = ldap_simple_bind_s(ld, 'contoso3\user1', ); // v.3
>Error <49>: ldap_simple_bind_s() failed: Invalid Credentials
>Server error: 80090308: LdapErr: DSID-0C0903A9, comment: AcceptSecurityContext 
error, data 775, v1db0
>Error 0x80090308 The token supplied to the function is invalid
>---
>
>MS-ERREF
>0x80090308 SEC_E_INVALID_TOKEN The token supplied to the function is
>invalid.
>
>The product team will be reflecting this in a future refresh of the
>MS-ADTS document.
>---
>
>>Alexander, do you think it would make sense to ask MSFT to enhance the
>>documentation of the LDAP error message format? And if yes, is sending
>>an email to doch...@microsoft.com sufficient or this there a more
>>elaborate process?
>I'll check with Edgar on why this piece is still missing from MS-ADTS
>five years after. ;)
So I re-read MS-ADTS and section 3.1.1.3.1.9 "Error Message Strings"
has all the information:
---
When the server fails an LDAP operation with an error, and the server
has sufficient resources to compute a string value for the errorMessage
field of the LDAPResult, it includes a string in the errorMessage field
of the LDAPResult (see [RFC2251] section 4.1.10). The string contains
further information about the error.

The first eight characters of the errorMessage string are a 32-bit
integer, expressed in hexadecimal. Where protocol specifies the extended
error code "" ther

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-10 Thread Pavel Reichl




On 02/10/2016 11:06 AM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

since getting those values requires to parse the string it would be nice
to get some official details about the string.

Well, the string content after DSID- mark can be completely
missing while the hex of the code (80090308) will be there.

The presence of "DSID- ..." error message is regulated by
ulHideDSID character of the dsHeuristics attribute (MS-ADTS
6.1.1.2.4.1.2). So you can have Active Directory where DSID-
string is completely missing but Win32 code for the error is there.



Alexander thanks for looking into this, but what we need is to
distinguish between reasons for invalid credentials.

e.g.
Bind result: Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5, 
comment: AcceptSecurityContext error, data 773, v23f0
Bind result: Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5, 
comment: AcceptSecurityContext error, data 775, v23f0

As I said, you should not rely on the information being available to you
as it might be disabled completely by the AD administrators in
ndsHeuristics attribute.

What are you going to do when ulHideDSID flag is set to 1?

The ticket is about providing extra info to user if account is locked. If we 
can't decide, we return generic access denied and generic message. Best afford 
attitude is fine here...IMO.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Add test to validate off by one bug

2016-02-10 Thread Pavel Reichl




On 02/10/2016 12:04 PM, Lukas Slebodnik wrote:

On (10/02/16 11:38), Pavel Březina wrote:

On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:

On (05/02/16 16:56), Pavel Reichl wrote:

Hopefully the last one.


>From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001


Hi,
I will chime in here since this mail remains clear...


+#define TEST_2922_MIN_ID 184260
+#define TEST_2922_MAX_ID 184279
+#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+
+#define TEST_2922_DFL_SLIDE 9212
+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0"
+/* Last SID = first SID + (default) rangesize -1 */
+#define TEST_2922_LAST_SID TEST_DOM_SID"-19"
+/* Last SID = first SID + rangesize */
+#define TEST_2922_LAST_SID_PLUS_ONE  TEST_DOM_SID"-20"
+

We should prefer to use constants instead of #defines.
Constants are more type safe.


I don't really care about type safety here I care about correct description
of magic values and no duplication of them. Given the #defines prefix those
can be used only for this test and some of the macros are used across the
test and fixture I'd prefer combination of both.

Keep MIN_ID and MAX_ID as macros. Drop _PLUS_ONE completely since "MAX_ID +
1" is descriptive enough. The rest should be as static const since they are
relevant only to one function in one test.

Is that good enough compromise?


struct test_ctx {
 TALLOC_CTX *mem_idmap;
 struct sss_idmap_ctx *idmap_ctx;
@@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool 
external_mapping,
 return 0;
}

+static int setup_ranges_2922(struct test_ctx *test_ctx)
+{
+struct sss_idmap_range range;
+enum idmap_error_code err;
+const char *name;
+const char *sid;
+/* Pick a new slice. */
+id_t slice_num = -1;
+
+if (test_ctx == NULL) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "test context is NULL\n");
+return 1;
+}
+
+name = TEST_DOM_NAME;
+sid = TEST_DOM_SID;
+
+err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, _num,
+);
+if (err != IDMAP_SUCCESS) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
+  IDMAP_SUCCESS, err);
+return 1;

Please use different numbers for different failures.

Persoanly, I do not like theusage of such error handling in tests.
But that was a  desigh decision of cmocka developers
which I do not consider as the right one.

Old versions of cmocka allowed to use assertions in setup and teradown
function. The same applies to check framework. Such decision add
unnecassary complexity to the review process. Reviewer need to check
that different error codes are returned or debug message is logged
with proper debug level. But that's the separete discusssion.


You can still use assertions in fixtures (I just checked with asn to be sure)
and that is the way we should lean to. So forget error codes and debug
messages here, use assertions to make it cleaner and shorter.


Thank you for another oninion. I prefer assertion as well but I was not
stricly against error codes.

You proposed more/less the same things as I wrote in my last mails.


Well no, asserts were out of question because they were banned by Jakub (2nd 
mail in this thread). If Jakub is fine with asserts I'm all for it.


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


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

[SSSD] Re: [PATCH] IDMAP: Add test to validate off by one bug

2016-02-10 Thread Pavel Reichl




On 02/10/2016 11:38 AM, Pavel Březina wrote:



You can still use assertions in fixtures (I just checked with asn to be sure) 
and that is the way we should lean to. So forget error codes and debug messages 
here, use assertions to make it cleaner and shorter.





Thanks Pavel, can you please check if first version of the patch would work for 
you?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Add test to validate off by one bug

2016-02-10 Thread Pavel Reichl




On 02/10/2016 12:52 PM, Jakub Hrozek wrote:

On Wed, Feb 10, 2016 at 12:43:18PM +0100, Pavel Reichl wrote:



On 02/10/2016 12:04 PM, Lukas Slebodnik wrote:

On (10/02/16 11:38), Pavel Březina wrote:

On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:

On (05/02/16 16:56), Pavel Reichl wrote:

Hopefully the last one.


>From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001


Hi,
I will chime in here since this mail remains clear...


+#define TEST_2922_MIN_ID 184260
+#define TEST_2922_MAX_ID 184279
+#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+
+#define TEST_2922_DFL_SLIDE 9212
+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0"
+/* Last SID = first SID + (default) rangesize -1 */
+#define TEST_2922_LAST_SID TEST_DOM_SID"-19"
+/* Last SID = first SID + rangesize */
+#define TEST_2922_LAST_SID_PLUS_ONE  TEST_DOM_SID"-20"
+

We should prefer to use constants instead of #defines.
Constants are more type safe.


I don't really care about type safety here I care about correct description
of magic values and no duplication of them. Given the #defines prefix those
can be used only for this test and some of the macros are used across the
test and fixture I'd prefer combination of both.

Keep MIN_ID and MAX_ID as macros. Drop _PLUS_ONE completely since "MAX_ID +
1" is descriptive enough. The rest should be as static const since they are
relevant only to one function in one test.

Is that good enough compromise?


struct test_ctx {
 TALLOC_CTX *mem_idmap;
 struct sss_idmap_ctx *idmap_ctx;
@@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool 
external_mapping,
 return 0;
}

+static int setup_ranges_2922(struct test_ctx *test_ctx)
+{
+struct sss_idmap_range range;
+enum idmap_error_code err;
+const char *name;
+const char *sid;
+/* Pick a new slice. */
+id_t slice_num = -1;
+
+if (test_ctx == NULL) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "test context is NULL\n");
+return 1;
+}
+
+name = TEST_DOM_NAME;
+sid = TEST_DOM_SID;
+
+err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, _num,
+);
+if (err != IDMAP_SUCCESS) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
+  IDMAP_SUCCESS, err);
+return 1;

Please use different numbers for different failures.

Persoanly, I do not like theusage of such error handling in tests.
But that was a  desigh decision of cmocka developers
which I do not consider as the right one.

Old versions of cmocka allowed to use assertions in setup and teradown
function. The same applies to check framework. Such decision add
unnecassary complexity to the review process. Reviewer need to check
that different error codes are returned or debug message is logged
with proper debug level. But that's the separete discusssion.


You can still use assertions in fixtures (I just checked with asn to be sure)
and that is the way we should lean to. So forget error codes and debug
messages here, use assertions to make it cleaner and shorter.


Thank you for another oninion. I prefer assertion as well but I was not
stricly against error codes.

You proposed more/less the same things as I wrote in my last mails.


Well no, asserts were out of question because they were banned by Jakub (2nd 
mail in this thread). If Jakub is fine with asserts I'm all for it.


I didn't ban anything, I just don't like the way cmocka errors out with
asserts in fixtures (and I'm probably half-guilty for that..)


Sorry, I meant that I considered them as no way to go - I thought it was based 
on discussion you and Michal had.


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


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

[SSSD] Re: [PATCH] IDMAP: Add test to validate off by one bug

2016-02-10 Thread Pavel Reichl


On 02/10/2016 12:50 PM, Pavel Březina wrote:

On 02/10/2016 12:44 PM, Pavel Reichl wrote:



On 02/10/2016 11:38 AM, Pavel Březina wrote:



You can still use assertions in fixtures (I just checked with asn to
be sure) and that is the way we should lean to. So forget error codes
and debug messages here, use assertions to make it cleaner and shorter.





Thanks Pavel, can you please check if first version of the patch would
work for you?


Yes, it works for me.


Hello, I added types and made constants local to functions that use them as 
Lukas and Pavel wished for. I also fixed the trailing '{' it the function 
definition as Lukas pointed out.
>From 764032c532dd2f8119c42a877f017d9a2ffcce05 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 22 Jan 2016 08:34:14 -0500
Subject: [PATCH] IDMAP: Add test to validate off by one bug

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
 src/tests/cmocka/test_sss_idmap.c | 113 --
 1 file changed, 109 insertions(+), 4 deletions(-)

diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c
index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..f82e3dc51601850a480cf1daa6d5f6dbd940ddcb 100644
--- a/src/tests/cmocka/test_sss_idmap.c
+++ b/src/tests/cmocka/test_sss_idmap.c
@@ -43,6 +43,9 @@
 #define TEST_OFFSET 100
 #define TEST_OFFSET_STR "100"
 
+const int TEST_2922_MIN_ID = 184260;
+const int TEST_2922_MAX_ID = 184279;
+
 struct test_ctx {
 TALLOC_CTX *mem_idmap;
 struct sss_idmap_ctx *idmap_ctx;
@@ -128,7 +131,38 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping,
 return 0;
 }
 
-static int test_sss_idmap_setup_with_domains(void **state) {
+static int setup_ranges_2922(struct test_ctx *test_ctx)
+{
+const int TEST_2922_DFL_SLIDE = 9212;
+struct sss_idmap_range range;
+enum idmap_error_code err;
+const char *name;
+const char *sid;
+/* Pick a new slice. */
+id_t slice_num = -1;
+
+assert_non_null(test_ctx);
+
+name = TEST_DOM_NAME;
+sid = TEST_DOM_SID;
+
+err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, _num,
+);
+assert_int_equal(err, IDMAP_SUCCESS);
+/* Range computation should be deterministic. Lets validate that.  */
+assert_int_equal(range.min, TEST_2922_MIN_ID);
+assert_int_equal(range.max, TEST_2922_MAX_ID);
+assert_int_equal(slice_num, TEST_2922_DFL_SLIDE);
+
+err = sss_idmap_add_domain_ex(test_ctx->idmap_ctx, name, sid, ,
+  NULL, 0, false /* No external mapping */);
+assert_int_equal(err, IDMAP_SUCCESS);
+
+return 0;
+}
+
+static int test_sss_idmap_setup_with_domains(void **state)
+{
 struct test_ctx *test_ctx;
 
 test_sss_idmap_setup(state);
@@ -140,7 +174,21 @@ static int test_sss_idmap_setup_with_domains(void **state) {
 return 0;
 }
 
-static int test_sss_idmap_setup_with_domains_sec_slices(void **state) {
+static int test_sss_idmap_setup_with_domains_2922(void **state)
+{
+struct test_ctx *test_ctx;
+
+test_sss_idmap_setup(state);
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+assert_non_null(test_ctx);
+
+setup_ranges_2922(test_ctx);
+return 0;
+}
+
+static int test_sss_idmap_setup_with_domains_sec_slices(void **state)
+{
 struct test_ctx *test_ctx;
 
 test_sss_idmap_setup(state);
@@ -152,7 +200,8 @@ static int test_sss_idmap_setup_with_domains_sec_slices(void **state) {
 return 0;
 }
 
-static int test_sss_idmap_setup_with_external_mappings(void **state) {
+static int test_sss_idmap_setup_with_external_mappings(void **state)
+{
 struct test_ctx *test_ctx;
 
 test_sss_idmap_setup(state);
@@ -164,7 +213,8 @@ static int test_sss_idmap_setup_with_external_mappings(void **state) {
 return 0;
 }
 
-static int test_sss_idmap_setup_with_both(void **state) {
+static int test_sss_idmap_setup_with_both(void **state)
+{
 struct test_ctx *test_ctx;
 
 test_sss_idmap_setup(state);
@@ -298,6 +348,58 @@ void test_map_id(void **state)
 sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
 }
 
+/* https://fedorahosted.org/sssd/ticket/2922 */
+/* ID mapping - bug in computing max id for slice range */
+void test_map_id_2922(void **state)
+{
+const char* TEST_2922_FIRST_SID = TEST_DOM_SID"-0";
+/* Last SID = first SID + (default) rangesize -1 */
+const char* TEST_2922_LAST_SID = TEST_DOM_SID"-19";
+/* Last SID = first SID + rangesize */
+const char* TEST_2922_LAST_SID_PLUS_ONE = TEST_DOM_SID"-20";
+struct test_ctx *test_ctx;
+enum idmap_error_code err;
+uint32_t id;
+char *sid = NULL;
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+
+assert_non_null(test_ctx);
+
+/* Min UNIX ID to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MIN_ID, );
+asser

[SSSD] Re: [PATCH] krb5_child: Warn if user cannot read krb5.conf

2016-02-10 Thread Pavel Reichl




On 02/01/2016 11:10 AM, Sumit Bose wrote:

On Mon, Feb 01, 2016 at 10:45:56AM +0100, Pavel Reichl wrote:

I thought you were going to use 'fd' for return value of open(). I still think 
access() would be better function to use. We would not need to care about file 
descriptor at all.


It's a bit nit-picking but access() only checks if you are allowed to
access the file in the requested way not if you are really able to do
it. E.g. although the file-permission allows you to do so the SELinux
policy might prevent you from actually open the file.

Additionally from the access(3) man page "Warning:  Using these calls to
check if a user is authorized to, for example, open a file before
actually doing so using open(2) creates a security hole, because the
user might exploit the short time interval between checking and opening
the file to manipulate it.  For this reason, the use of this system call
should be avoided.  (In the example just described, a safer alternative
would be to temporarily switch the process's effective  user  ID  to the
real ID and then call open(2).)"


I think that this security hole is not relevant for our case, because we open the file to test if we have access and then we close it. File privileges can be changed before krb child actually access the file the very same way as if we tested by 
access(), right?


Anyway, I see the advantage of selinux policy being checked when the open() is 
performed so I no longer push for access().
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] krb5_child: Warn if user cannot read krb5.conf

2016-02-10 Thread Pavel Reichl




On 02/01/2016 02:39 PM, Pavel Reichl wrote:



On 02/01/2016 02:33 PM, Lukas Slebodnik wrote:

}

+void try_open_krb5_conf(void)
+{
+int fd;
+int ret;
+


Is there any reason for the function not to be static?
___


Bump, lets finish the review.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] krb5_child: Warn if user cannot read krb5.conf

2016-02-10 Thread Pavel Reichl


Just some nitpicks.

On 02/01/2016 02:33 PM, Lukas Slebodnik wrote:

+void try_open_krb5_conf(void)
+{
+int fd;
+int ret;
+
+fd = open("/etc/krb5.conf", O_RDONLY);
+if (fd != -1) {
+close(fd);
+} else {
+ret = errno;
+if (ret == EPERM) {
+DEBUG(SSSDBG_CRIT_FAILURE,
+  "User with uid:%"SPRIuid" gid:%"SPRIgid" cannot read "
+  "/etc/krb5.conf. It might cause problems.",


Missing '\n'


+  geteuid(), getegid());
+} else {
+DEBUG(SSSDBG_MINOR_FAILURE,
+  "Cannot open /etc/krb5.conf [%d]: %s\n",
+  ret, strerror(ret));


I see that we already use sss_strerror() in this module, so please use it as 
well.


+}
+}
+}

Thanks.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-10 Thread Pavel Reichl

On 02/10/2016 11:46 AM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

On 02/10/2016 11:06 AM, Alexander Bokovoy wrote:

On Wed, 10 Feb 2016, Pavel Reichl wrote:

since getting those values requires to parse the string it would be nice
to get some official details about the string.

Well, the string content after DSID- mark can be completely
missing while the hex of the code (80090308) will be there.

Alexander thanks for looking into this, but what we need is to
distinguish between reasons for invalid credentials.

e.g.
Bind result: Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5,
comment: AcceptSecurityContext error, data 773, v23f0
Bind result: Invalid credentials(49), 80090308: LdapErr: DSID-0C0903C5,
comment: AcceptSecurityContext error, data 775, v23f0

As I said, you should not rely on the information being available to you
as it might be disabled completely by the AD administrators in
ndsHeuristics attribute.

What are you going to do when ulHideDSID flag is set to 1?

The ticket is about providing extra info to user if account is locked.
If we can't decide, we return generic access denied and generic
message. Best afford attitude is fine here...IMO.

That's fine but please add documentation about the behavior into the
commit message so that we would have this discussion recorded somehow.

OK, would this:

"""
This patch is best effort only as decision whether account is actually locked is based on parsing error message returned by AD. The format and content of this error message might be subject of change in future releases and also can be modified by AD
administrators.

"""

appended to the commit message of the first patch work for you?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Add minor performance improvements

2016-02-09 Thread Pavel Reichl




On 01/26/2016 05:35 PM, Pavel Reichl wrote:

Hello,

please see simple patch attached.

Thanks.


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



Bump.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Add test to validate off by one bug

2016-02-09 Thread Pavel Reichl




On 02/09/2016 08:58 AM, Lukas Slebodnik wrote:

On (08/02/16 09:24), Pavel Reichl wrote:

On 02/05/2016 10:25 PM, Lukas Slebodnik wrote:

On (05/02/16 19:34), Michal Židek wrote:

On 02/05/2016 06:32 PM, Lukas Slebodnik wrote:

On (05/02/16 17:30), Michal Židek wrote:

On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:

On (05/02/16 16:56), Pavel Reichl wrote:

Hopefully the last one.


>From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001

From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 22 Jan 2016 08:34:14 -0500
Subject: [PATCH] IDMAP: Add test to validate off by one bug

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
src/tests/cmocka/test_sss_idmap.c | 125 ++
1 file changed, 125 insertions(+)

diff --git a/src/tests/cmocka/test_sss_idmap.c 
b/src/tests/cmocka/test_sss_idmap.c
index 
00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe
 100644
--- a/src/tests/cmocka/test_sss_idmap.c
+++ b/src/tests/cmocka/test_sss_idmap.c
@@ -43,6 +43,17 @@
#define TEST_OFFSET 100
#define TEST_OFFSET_STR "100"

+#define TEST_2922_MIN_ID 184260
+#define TEST_2922_MAX_ID 184279
+#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+
+#define TEST_2922_DFL_SLIDE 9212
+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0"
+/* Last SID = first SID + (default) rangesize -1 */
+#define TEST_2922_LAST_SID TEST_DOM_SID"-19"
+/* Last SID = first SID + rangesize */
+#define TEST_2922_LAST_SID_PLUS_ONE  TEST_DOM_SID"-20"
+

We should prefer to use constants instead of #defines.
Constants are more type safe.



Do you want Pavel to change it? If so, then I would prefer
at least the strings to remain as #defines. It is more
comfortable to concatenate string literals. But honestly
I am ok with the numbers to remain as #defines as well,
but I agree with your point in general.


Yes, please.

Define can still be used for simplification of string initialisation
e.g.
#define DOM_SID "S-1-2-3-4"
const char TEST_2922_LAST_SID[] = DOM_SID"-99";
const char TEST_2922_LAST_SID_PLUS_ONE[] = DOM_SID"-100";



Yes, they can. But do you require it? As I said I would
prefer the strings to remain #defines, but it is not strong
preference.


I'm sorry but your preference is not type safe.
It's much simpler to identify errors caused by wrong


Well, it's just your preference. It's not in out code style, right? Anyway it's 
often used in other cmocka tests so it's consistent with out code base.


It's not just my preference constants are already used in test
(module scope)

sh$ cd src/tests
sh$ git grep "^const" | wc -l
64


grep '#define' `find src/tests -name '*.c'`  | grep \" | wc -l
237



Could you give me any reason why macros are better from technical
point of view?


It doesn't matter. Both work and we are wasting time on fruitless discussion. You can always improve code. It's import to know when it's good enough. I say using defines in test is good enough and I don't see real benefit in changing it that would 
justify time spend on fixing.



We needn't support sssd on platforms with old C compiler
and we use c99(gnu99) anyway.

If no then I will appreciate usage of new-style constants.


usage of constants. If you like macros then you can be honored
in future to try find bugs in macro generated code (nss-pam-ldapd).

I think this is totally unrelated, please try to focus on purpose of this 
thread - test for id mapping.


It just an explanation of my reasoning.







struct test_ctx {
 TALLOC_CTX *mem_idmap;
 struct sss_idmap_ctx *idmap_ctx;
@@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool 
external_mapping,
 return 0;
}

+static int setup_ranges_2922(struct test_ctx *test_ctx)
+{
+struct sss_idmap_range range;
+enum idmap_error_code err;
+const char *name;
+const char *sid;
+/* Pick a new slice. */
+id_t slice_num = -1;
+
+if (test_ctx == NULL) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "test context is NULL\n");
+return 1;
+}
+
+name = TEST_DOM_NAME;
+sid = TEST_DOM_SID;
+
+err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, _num,
+);
+if (err != IDMAP_SUCCESS) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
+  IDMAP_SUCCESS, err);
+return 1;

Please use different numbers for different failures.


I think it is OK to use the same number in tests, as long as
the debug message is present. We can identify the error
based on stderr messages.


No it's not OK to have same return code.
The return codes have to be unique.


They do not need to be unique here. Compare:

[ RUN  ] test_map_id_2922
(Fri Feb  5 19:00:17:723118 2016) [sssd]
[test_sss_idmap_setup_with_domains_2922] (0x0010

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-09 Thread Pavel Reichl




On 02/09/2016 08:09 AM, Jakub Hrozek wrote:

On Mon, Feb 08, 2016 at 01:56:07PM +0100, Pavel Reichl wrote:



diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index 
73a21bfa0049bc4d3cfacb49201707868c87e533..2dbc58a451686beda0faa9e9366bbc3b3b4c253e
 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -1040,6 +1040,27 @@ pam_account_expired_message = Account expired, please 
call help desk.
  
  
  
+pam_account_locked_message (string)
+
+
+   If user is authenticating and


Please ask someone for an English review (I know Dan started, but I
didn't see a fixed version yet). At the very least, this should read "a
user".


I attached Dan's patch. I took the liberty of adding note regarding pam 
verbosity. Hope it's fine by Dan.




+   account is locked then by default
+   'Permission denied' is output. This output will
+   be changed to content of this variable if it is
+   set.
+
+
+example:
+
+pam_account_locked_message = Account locked, please call help desk.
+
+
+
+Default: none
+
+
+
+
  p11_child_timeout (integer)
  
  


The rest of the patch looks good to me and seems to work as advertized.


Thanks.
>From 2d634a7d72afc5116031803c3004e47884901c7a Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 5 Feb 2016 07:27:38 -0500
Subject: [PATCH 1/3] SDAP: Add return code ERR_ACCOUNT_LOCKED

Add code to distinquish state when account is locked in Active
Directory server.

Tested against Windows Server 2012

Resolves:
https://fedorahosted.org/sssd/ticket/2839
---
 src/providers/data_provider.h  | 2 ++
 src/providers/ldap/ldap_auth.c | 4 
 src/providers/ldap/sdap_async_connection.c | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h
index 39051b90c3aad96f62dcbb86a20bcfd8c954879b..7332b677d19f70f4736e4d0b68d55cdd3c67a4af 100644
--- a/src/providers/data_provider.h
+++ b/src/providers/data_provider.h
@@ -182,6 +182,8 @@ struct pam_data {
 bool offline_auth;
 bool last_auth_saved;
 int priv;
+int account_locked;
+
 #ifdef USE_KEYRING
 key_serial_t key_serial;
 #endif
diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c
index c94ba15bb17aa1641eb36781cc59ce158d48ca66..8d6a37b2ceb3347cb8092858889d07e5615e5c77 100644
--- a/src/providers/ldap/ldap_auth.c
+++ b/src/providers/ldap/ldap_auth.c
@@ -1302,6 +1302,10 @@ static void sdap_pam_auth_done(struct tevent_req *req)
 case ERR_PASSWORD_EXPIRED:
 state->pd->pam_status = PAM_NEW_AUTHTOK_REQD;
 break;
+case ERR_ACCOUNT_LOCKED:
+state->pd->account_locked = true;
+state->pd->pam_status = PAM_PERM_DENIED;
+break;
 default:
 state->pd->pam_status = PAM_SYSTEM_ERR;
 dp_err = DP_ERR_FATAL;
diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
index 2d9b1184f5d30b9df7f1d3e4b980a7e0107c6830..c1735513ff6dcc755daf06cb97da546eaded7eb9 100644
--- a/src/providers/ldap/sdap_async_connection.c
+++ b/src/providers/ldap/sdap_async_connection.c
@@ -754,6 +754,9 @@ static void simple_bind_done(struct sdap_op *op,
 
 if (result == LDAP_SUCCESS) {
 ret = EOK;
+} else if (result == LDAP_INVALID_CREDENTIALS
+   && errmsg != NULL && strstr(errmsg, "data 775,") != NULL) {
+ret = ERR_ACCOUNT_LOCKED;
 } else {
 ret = ERR_AUTH_FAILED;
 }
-- 
2.4.3

>From 9b11ae7485723742b1172bacb5062207e2361588 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 5 Feb 2016 07:31:45 -0500
Subject: [PATCH 2/3] PAM: Pass account lockout status and display message

Tested against Windows Server 2012.

Resolves:
https://fedorahosted.org/sssd/ticket/2839
---
 src/confdb/confdb.h  |  1 +
 src/config/SSSDConfig/__init__.py.in |  1 +
 src/config/etc/sssd.api.conf |  1 +
 src/man/sssd.conf.5.xml  | 21 +
 src/providers/dp_auth_util.c | 20 
 src/responder/pam/pamsrv_cmd.c   | 31 +++
 6 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index fcffcb5a6ff8b3f766ed9a693db874c7c6e3d9b9..e6789c8665cf677712d8e7fb72b6f0a41bca80

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-09 Thread Pavel Reichl




On 02/09/2016 08:12 AM, Lukas Slebodnik wrote:

On (08/02/16 17:18), Sumit Bose wrote:

[snip]

Agree.
I wrote in my previous mail that samab shoudl not be a blocker.
But we should not crash with samba in any case.



Why should the patch cause crash? I don't see it. Could you be more specific 
please?

If format of message would be different than access would be denied...no crash.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] PAM: Clarify man page for domains option

2016-02-09 Thread Pavel Reichl




On 02/09/2016 08:17 AM, Jakub Hrozek wrote:

On Fri, Jan 29, 2016 at 02:30:36PM +0100, Pavel Reichl wrote:

Hello, please see trivial patch attached. Thanks.



 From 6d5f6b71c2d2f891470dc1c9f08ae67f5b6c02f5 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 29 Jan 2016 08:27:01 -0500
Subject: [PATCH] PAM: Clarify man page for domains option

Resolves:
https://fedorahosted.org/sssd/ticket/2946
---
  src/man/pam_sss.8.xml | 8 +---
  1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/man/pam_sss.8.xml b/src/man/pam_sss.8.xml
index 
7794d3acfdfdbde491a3e1ada44481b73588e41f..278126c14d0a574a1e120762af264ef653deb0b0
 100644
--- a/src/man/pam_sss.8.xml
+++ b/src/man/pam_sss.8.xml
@@ -145,9 +145,11 @@
  SSSD domain names, as specified in the sssd.conf file.
  
  
-NOTE: Must be used in conjunction with the
-pam_trusted_users and
-pam_public_domains options.
+NOTE: If PAM service is being run by untrusted user
+(pam_trusted_users option)
+then please make
+sure that restricted domains are public
+(pam_public_domains option).
  Please see the
  
  sssd.conf
--
2.4.3



I'm sorry, but this doesn't read any better to me. Especially I don't
understand "restricted domains are public", sounds like an oxymoron to
me.


Oh, sorry. By "restricted domain" I thought only the domains you are 
restricting access to - like the only ones you can use. It's used in the context of the 
first paragraph of domains option.

I'll try to rephrase.

"""
If PAM service is being run by untrusted user(pam_trusted_users option) then please make sure that domains entered into domains option are actually public (pam_public_domains option). Otherwise access will be denied 
because untrusted user would be trying to access non-public domain.

"""

Does it sound any better? Would you propose some other wording? Or we can drop 
the note completely.

Thanks!

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

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-08 Thread Pavel Reichl




On 02/08/2016 10:48 AM, Jakub Hrozek wrote:

On Mon, Feb 08, 2016 at 10:34:16AM +0100, Pavel Reichl wrote:



On 02/05/2016 03:16 PM, Lukas Slebodnik wrote:



The ticket is about "SSSD should be about to display message to the user when
the account in Active Directory is 'locked out'"

If the string is not standardized among AD versions
than this ticket is NOT solved.


So what do you propose? Rename ticket to contain version of tested AD? Or 
should we say user that although we have fix that would work for him it might 
not work for all AD versions so we won't provide it?


It would be nice to mention what we tested with in the commit message.


OK, done.





Can we ask our QA to test on all AD version they can lay their hands on?


Yes, I think we can test 2012 and 2008. Probably not worth testing 2003
anymore.



I updated the relevant BZ.
>From 5a4ca73e16e4eec023108387cd8c572c34496e9b Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 5 Feb 2016 07:27:38 -0500
Subject: [PATCH 1/2] SDAP: Add return code ERR_ACCOUNT_LOCKED

Add code to distinquish state when account is locked in Active
Directory server.

Tested against Windows Server 2012

Resolves:
https://fedorahosted.org/sssd/ticket/2839
---
 src/providers/data_provider.h  | 2 ++
 src/providers/ldap/ldap_auth.c | 4 
 src/providers/ldap/sdap_async_connection.c | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h
index 39051b90c3aad96f62dcbb86a20bcfd8c954879b..7332b677d19f70f4736e4d0b68d55cdd3c67a4af 100644
--- a/src/providers/data_provider.h
+++ b/src/providers/data_provider.h
@@ -182,6 +182,8 @@ struct pam_data {
 bool offline_auth;
 bool last_auth_saved;
 int priv;
+int account_locked;
+
 #ifdef USE_KEYRING
 key_serial_t key_serial;
 #endif
diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c
index c94ba15bb17aa1641eb36781cc59ce158d48ca66..8d6a37b2ceb3347cb8092858889d07e5615e5c77 100644
--- a/src/providers/ldap/ldap_auth.c
+++ b/src/providers/ldap/ldap_auth.c
@@ -1302,6 +1302,10 @@ static void sdap_pam_auth_done(struct tevent_req *req)
 case ERR_PASSWORD_EXPIRED:
 state->pd->pam_status = PAM_NEW_AUTHTOK_REQD;
 break;
+case ERR_ACCOUNT_LOCKED:
+state->pd->account_locked = true;
+state->pd->pam_status = PAM_PERM_DENIED;
+break;
 default:
 state->pd->pam_status = PAM_SYSTEM_ERR;
 dp_err = DP_ERR_FATAL;
diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
index 2d9b1184f5d30b9df7f1d3e4b980a7e0107c6830..c1735513ff6dcc755daf06cb97da546eaded7eb9 100644
--- a/src/providers/ldap/sdap_async_connection.c
+++ b/src/providers/ldap/sdap_async_connection.c
@@ -754,6 +754,9 @@ static void simple_bind_done(struct sdap_op *op,
 
 if (result == LDAP_SUCCESS) {
 ret = EOK;
+} else if (result == LDAP_INVALID_CREDENTIALS
+   && errmsg != NULL && strstr(errmsg, "data 775,") != NULL) {
+ret = ERR_ACCOUNT_LOCKED;
 } else {
 ret = ERR_AUTH_FAILED;
 }
-- 
2.4.3

>From 637766eb543a54d4a96ae5c9692566a02522a742 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 5 Feb 2016 07:31:45 -0500
Subject: [PATCH 2/2] PAM: Pass account lockout status and display message

Tested against Windows Server 2012.

Resolves:
https://fedorahosted.org/sssd/ticket/2839
---
 src/confdb/confdb.h  |  1 +
 src/config/SSSDConfig/__init__.py.in |  1 +
 src/config/etc/sssd.api.conf |  1 +
 src/man/sssd.conf.5.xml  | 21 +
 src/providers/dp_auth_util.c | 20 
 src/responder/pam/pamsrv_cmd.c   | 31 +++
 6 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index fcffcb5a6ff8b3f766ed9a693db874c7c6e3d9b9..e6789c8665cf677712d8e7fb72b6f0a41bca80b1 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -118,6 +118,7 @@
 #define CONFDB_PAM_TRUSTED_USERS "pam_trusted_users"
 #define CONFDB_PAM_PUBLIC_DOMAINS "pam_public_domains"
 #define CONFDB_PAM_ACCOUNT_EXPIRED_MESSAGE "pam_account_expired_message"
+#define CONFDB_PAM_ACCOUNT_LOCKED_MESSAGE "pam_account_locked_message"
 #define CONFDB_PAM_CERT_AUTH "pam_cert_auth"
 #define CONFDB_PAM_CERT_DB_PATH "pam_cert_db_path"
 #define CONFDB_PAM_P11_CHILD_TIMEOUT "p11_child_timeout"
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index 1fdb907c5d010323c22b18b4c371c61e5928c40f..495cb650ee86e50031962a4fcf0c21aa79dc0142 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -91,6 +91,7 @@ option_strings = {

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-08 Thread Pavel Reichl




On 02/05/2016 03:16 PM, Lukas Slebodnik wrote:



The ticket is about "SSSD should be about to display message to the user when
the account in Active Directory is 'locked out'"

If the string is not standardized among AD versions
than this ticket is NOT solved.


So what do you propose? Rename ticket to contain version of tested AD? Or 
should we say user that although we have fix that would work for him it might 
not work for all AD versions so we won't provide it?

Can we ask our QA to test on all AD version they can lay their hands on?




Could you add link to msdn documentation where it is described that this string
is guaranted to be returned in such case?


It's not MSDN, but:
http://www-01.ibm.com/support/docview.wss?uid=swg21290631

I would prefer official documantation (msdn) in code ar at least in
commit message.


We don't have any and it's possible it's not publicly documented.



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


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

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-08 Thread Pavel Reichl




On 02/05/2016 03:42 PM, Dan Lavu wrote:

https://technet.microsoft.com/en-us/library/cc526636.aspx
https://support.microsoft.com/en-us/kb/218185

?



Dan, I might have missed it but I don't see there any mention of AD returning 
'data 775' in case that account is locked (which is a sub case of error invalid 
credentials).
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Add test to validate off by one bug

2016-02-08 Thread Pavel Reichl




On 02/05/2016 10:25 PM, Lukas Slebodnik wrote:

On (05/02/16 19:34), Michal Židek wrote:

On 02/05/2016 06:32 PM, Lukas Slebodnik wrote:

On (05/02/16 17:30), Michal Židek wrote:

On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:

On (05/02/16 16:56), Pavel Reichl wrote:

Hopefully the last one.


>From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001

From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 22 Jan 2016 08:34:14 -0500
Subject: [PATCH] IDMAP: Add test to validate off by one bug

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
src/tests/cmocka/test_sss_idmap.c | 125 ++
1 file changed, 125 insertions(+)

diff --git a/src/tests/cmocka/test_sss_idmap.c 
b/src/tests/cmocka/test_sss_idmap.c
index 
00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe
 100644
--- a/src/tests/cmocka/test_sss_idmap.c
+++ b/src/tests/cmocka/test_sss_idmap.c
@@ -43,6 +43,17 @@
#define TEST_OFFSET 100
#define TEST_OFFSET_STR "100"

+#define TEST_2922_MIN_ID 184260
+#define TEST_2922_MAX_ID 184279
+#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+
+#define TEST_2922_DFL_SLIDE 9212
+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0"
+/* Last SID = first SID + (default) rangesize -1 */
+#define TEST_2922_LAST_SID TEST_DOM_SID"-19"
+/* Last SID = first SID + rangesize */
+#define TEST_2922_LAST_SID_PLUS_ONE  TEST_DOM_SID"-20"
+

We should prefer to use constants instead of #defines.
Constants are more type safe.



Do you want Pavel to change it? If so, then I would prefer
at least the strings to remain as #defines. It is more
comfortable to concatenate string literals. But honestly
I am ok with the numbers to remain as #defines as well,
but I agree with your point in general.


Yes, please.

Define can still be used for simplification of string initialisation
e.g.
#define DOM_SID "S-1-2-3-4"
const char TEST_2922_LAST_SID[] = DOM_SID"-99";
const char TEST_2922_LAST_SID_PLUS_ONE[] = DOM_SID"-100";



Yes, they can. But do you require it? As I said I would
prefer the strings to remain #defines, but it is not strong
preference.


I'm sorry but your preference is not type safe.
It's much simpler to identify errors caused by wrong


Well, it's just your preference. It's not in out code style, right? Anyway it's 
often used in other cmocka tests so it's consistent with out code base.


usage of constants. If you like macros then you can be honored
in future to try find bugs in macro generated code (nss-pam-ldapd).

I think this is totally unrelated, please try to focus on purpose of this 
thread - test for id mapping.









struct test_ctx {
 TALLOC_CTX *mem_idmap;
 struct sss_idmap_ctx *idmap_ctx;
@@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool 
external_mapping,
 return 0;
}

+static int setup_ranges_2922(struct test_ctx *test_ctx)
+{
+struct sss_idmap_range range;
+enum idmap_error_code err;
+const char *name;
+const char *sid;
+/* Pick a new slice. */
+id_t slice_num = -1;
+
+if (test_ctx == NULL) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "test context is NULL\n");
+return 1;
+}
+
+name = TEST_DOM_NAME;
+sid = TEST_DOM_SID;
+
+err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, _num,
+);
+if (err != IDMAP_SUCCESS) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
+  IDMAP_SUCCESS, err);
+return 1;

Please use different numbers for different failures.


I think it is OK to use the same number in tests, as long as
the debug message is present. We can identify the error
based on stderr messages.


No it's not OK to have same return code.
The return codes have to be unique.


They do not need to be unique here. Compare:

[ RUN  ] test_map_id_2922
(Fri Feb  5 19:00:17:723118 2016) [sssd]
[test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL
Could not run the test - check test fixtures
[  ERROR   ] test_map_id_2922


With this:

[ RUN  ] test_map_id_2922
(Fri Feb  5 19:01:17:376602 2016) [sssd]
[test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL
Could not run the test - check test fixtures
[  ERROR   ] test_map_id_2922

I returned different error code in both cases but the result
is the same.


That's because of insufficient desing of libcmocka.
Unit test should not contain any if conditions
which are not tested. If you run code coverage of unit test
it should cover 100% of code path of unit test.

However, the "official way" of handling errors in setup
and teardown function does not allow to do this.
In other words, returning error code is useless.
As you pointed out there's no difference and we need to
use debug messages to fin

[SSSD] Re: [PATCH] IDMAP: Add test to validate off by one bug

2016-02-05 Thread Pavel Reichl


Michal told me off list about asserts I missed.


>From 8a5264f944dbe110b4d72a876cdc5ba2c112a73d Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 22 Jan 2016 08:34:14 -0500
Subject: [PATCH] IDMAP: Add test to validate off by one bug

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
 src/tests/cmocka/test_sss_idmap.c | 124 ++
 1 file changed, 124 insertions(+)

diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c
index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..b1d4f7b435d87c097c0e568b530a378c4071d3a7 100644
--- a/src/tests/cmocka/test_sss_idmap.c
+++ b/src/tests/cmocka/test_sss_idmap.c
@@ -43,6 +43,17 @@
 #define TEST_OFFSET 100
 #define TEST_OFFSET_STR "100"
 
+#define TEST_2922_MIN_ID 184260
+#define TEST_2922_MAX_ID 184279
+#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+
+#define TEST_2922_DFL_SLIDE 9212
+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0"
+/* Last SID = first SID + (default) rangesize -1 */
+#define TEST_2922_LAST_SID TEST_DOM_SID"-19"
+/* Last SID = first SID + rangesize */
+#define TEST_2922_LAST_SID_PLUS_ONE  TEST_DOM_SID"-20"
+
 struct test_ctx {
 TALLOC_CTX *mem_idmap;
 struct sss_idmap_ctx *idmap_ctx;
@@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping,
 return 0;
 }
 
+static int setup_ranges_2922(struct test_ctx *test_ctx)
+{
+struct sss_idmap_range range;
+enum idmap_error_code err;
+const char *name;
+const char *sid;
+/* Pick a new slice. */
+id_t slice_num = -1;
+
+if (test_ctx == NULL) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "test contex in NULL\n");
+return 1;
+}
+
+name = TEST_DOM_NAME;
+sid = TEST_DOM_SID;
+
+err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, _num,
+);
+if (err != IDMAP_SUCCESS) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
+  IDMAP_SUCCESS, err);
+return 1;
+}
+
+/* Range computation should be deterministic. Lets validate that.  */
+if (range.min != TEST_2922_MIN_ID
+|| range.max != TEST_2922_MAX_ID
+|| slice_num != TEST_2922_DFL_SLIDE) {
+DEBUG(SSSDBG_FATAL_FAILURE, "Failed to compute range correctly.\n");
+return 1;
+}
+
+err = sss_idmap_add_domain_ex(test_ctx->idmap_ctx, name, sid, ,
+  NULL, 0, false /* No external mapping */);
+if (err != IDMAP_SUCCESS) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "sss_idmap_add_domain_ex failed: exp: [%d] but got [%d]\n",
+  IDMAP_SUCCESS, err);
+return 1;
+}
+
+return 0;
+}
+
 static int test_sss_idmap_setup_with_domains(void **state) {
 struct test_ctx *test_ctx;
 
@@ -140,6 +198,22 @@ static int test_sss_idmap_setup_with_domains(void **state) {
 return 0;
 }
 
+static int test_sss_idmap_setup_with_domains_2922(void **state) {
+struct test_ctx *test_ctx;
+
+test_sss_idmap_setup(state);
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+if (test_ctx == NULL) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "test contex in NULL\n");
+return 1;
+}
+
+setup_ranges_2922(test_ctx);
+return 0;
+}
+
 static int test_sss_idmap_setup_with_domains_sec_slices(void **state) {
 struct test_ctx *test_ctx;
 
@@ -298,6 +372,53 @@ void test_map_id(void **state)
 sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
 }
 
+/* https://fedorahosted.org/sssd/ticket/2922 */
+/* ID mapping - bug in computing max id for slice range */
+void test_map_id_2922(void **state)
+{
+struct test_ctx *test_ctx;
+enum idmap_error_code err;
+uint32_t id;
+char *sid = NULL;
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+
+assert_non_null(test_ctx);
+
+/* Min UNIX ID to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MIN_ID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_string_equal(sid, TEST_2922_FIRST_SID);
+sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
+
+/* First SID to UNIX ID */
+err = sss_idmap_sid_to_unix(test_ctx->idmap_ctx, TEST_2922_FIRST_SID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_int_equal(id, TEST_2922_MIN_ID);
+
+/* Max UNIX ID to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MAX_ID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_string_equal(sid, TEST_2922_LAST_SID);
+sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
+
+/* Last SID to UNIX ID */
+err = sss_idmap_sid_to_unix(test_ctx->idmap_ctx, TEST_2922_LAST_SID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_int_equal(id, TEST_292

[SSSD] Re: [PATCH] IDMAP: Add test to validate off by one bug

2016-02-05 Thread Pavel Reichl


And now with correct patch. :-)
>From 549433ddba79916c54dd2d06342a34d67727c0be Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 22 Jan 2016 08:34:14 -0500
Subject: [PATCH] IDMAP: Add test to validate off by one bug

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
 src/tests/cmocka/test_sss_idmap.c | 117 ++
 1 file changed, 117 insertions(+)

diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c
index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..21531ed421d2c8460bc0004f08a135d7510d860b 100644
--- a/src/tests/cmocka/test_sss_idmap.c
+++ b/src/tests/cmocka/test_sss_idmap.c
@@ -43,6 +43,17 @@
 #define TEST_OFFSET 100
 #define TEST_OFFSET_STR "100"
 
+#define TEST_2922_MIN_ID 184260
+#define TEST_2922_MAX_ID 184279
+#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+
+#define TEST_2922_DFL_SLIDE 9212
+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0"
+/* Last SID = first SID + (default) rangesize -1 */
+#define TEST_2922_LAST_SID TEST_DOM_SID"-19"
+/* Last SID = first SID + rangesize */
+#define TEST_2922_LAST_SID_PLUS_ONE  TEST_DOM_SID"-20"
+
 struct test_ctx {
 TALLOC_CTX *mem_idmap;
 struct sss_idmap_ctx *idmap_ctx;
@@ -128,6 +139,50 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping,
 return 0;
 }
 
+static int setup_ranges_2922(struct test_ctx *test_ctx)
+{
+struct sss_idmap_range range;
+enum idmap_error_code err;
+const char *name;
+const char *sid;
+/* Pick a new slice. */
+id_t slice_num = -1;
+
+assert_non_null(test_ctx);
+
+name = TEST_DOM_NAME;
+sid = TEST_DOM_SID;
+
+err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, _num,
+);
+assert_int_equal(err, IDMAP_SUCCESS);
+if (err != IDMAP_SUCCESS) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
+  IDMAP_SUCCESS, err);
+return 1;
+}
+
+/* Range computation should be deterministic. Lets validate that.  */
+if (range.min != TEST_2922_MIN_ID
+|| range.max != TEST_2922_MAX_ID
+|| slice_num != TEST_2922_DFL_SLIDE) {
+DEBUG(SSSDBG_FATAL_FAILURE, "Failed to compute range correctly.\n");
+return 1;
+}
+
+err = sss_idmap_add_domain_ex(test_ctx->idmap_ctx, name, sid, ,
+  NULL, 0, false /* No external mapping */);
+if (err != IDMAP_SUCCESS) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "sss_idmap_add_domain_ex failed: exp: [%d] but got [%d]\n",
+  IDMAP_SUCCESS, err);
+return 1;
+}
+
+return 0;
+}
+
 static int test_sss_idmap_setup_with_domains(void **state) {
 struct test_ctx *test_ctx;
 
@@ -140,6 +195,18 @@ static int test_sss_idmap_setup_with_domains(void **state) {
 return 0;
 }
 
+static int test_sss_idmap_setup_with_domains_2922(void **state) {
+struct test_ctx *test_ctx;
+
+test_sss_idmap_setup(state);
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+assert_non_null(test_ctx);
+
+setup_ranges_2922(test_ctx);
+return 0;
+}
+
 static int test_sss_idmap_setup_with_domains_sec_slices(void **state) {
 struct test_ctx *test_ctx;
 
@@ -298,6 +365,53 @@ void test_map_id(void **state)
 sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
 }
 
+/* https://fedorahosted.org/sssd/ticket/2922 */
+/* ID mapping - bug in computing max id for slice range */
+void test_map_id_2922(void **state)
+{
+struct test_ctx *test_ctx;
+enum idmap_error_code err;
+uint32_t id;
+char *sid = NULL;
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+
+assert_non_null(test_ctx);
+
+/* Min UNIX ID to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MIN_ID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_string_equal(sid, TEST_2922_FIRST_SID);
+sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
+
+/* First SID to UNIX ID */
+err = sss_idmap_sid_to_unix(test_ctx->idmap_ctx, TEST_2922_FIRST_SID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_int_equal(id, TEST_2922_MIN_ID);
+
+/* Max UNIX ID to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MAX_ID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_string_equal(sid, TEST_2922_LAST_SID);
+sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
+
+/* Last SID to UNIX ID */
+err = sss_idmap_sid_to_unix(test_ctx->idmap_ctx, TEST_2922_LAST_SID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_int_equal(id, TEST_2922_MAX_ID);
+
+/* Max UNIX ID + 1 to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MAX_ID_PLUS_ONE,
+);
+assert_int_equal(err

[SSSD] Re: [PATCH] IDMAP: Add test to validate off by one bug

2016-02-05 Thread Pavel Reichl


Hopefully the last one.
>From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 22 Jan 2016 08:34:14 -0500
Subject: [PATCH] IDMAP: Add test to validate off by one bug

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
 src/tests/cmocka/test_sss_idmap.c | 125 ++
 1 file changed, 125 insertions(+)

diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c
index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644
--- a/src/tests/cmocka/test_sss_idmap.c
+++ b/src/tests/cmocka/test_sss_idmap.c
@@ -43,6 +43,17 @@
 #define TEST_OFFSET 100
 #define TEST_OFFSET_STR "100"
 
+#define TEST_2922_MIN_ID 184260
+#define TEST_2922_MAX_ID 184279
+#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+
+#define TEST_2922_DFL_SLIDE 9212
+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0"
+/* Last SID = first SID + (default) rangesize -1 */
+#define TEST_2922_LAST_SID TEST_DOM_SID"-19"
+/* Last SID = first SID + rangesize */
+#define TEST_2922_LAST_SID_PLUS_ONE  TEST_DOM_SID"-20"
+
 struct test_ctx {
 TALLOC_CTX *mem_idmap;
 struct sss_idmap_ctx *idmap_ctx;
@@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping,
 return 0;
 }
 
+static int setup_ranges_2922(struct test_ctx *test_ctx)
+{
+struct sss_idmap_range range;
+enum idmap_error_code err;
+const char *name;
+const char *sid;
+/* Pick a new slice. */
+id_t slice_num = -1;
+
+if (test_ctx == NULL) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "test context is NULL\n");
+return 1;
+}
+
+name = TEST_DOM_NAME;
+sid = TEST_DOM_SID;
+
+err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, _num,
+);
+if (err != IDMAP_SUCCESS) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
+  IDMAP_SUCCESS, err);
+return 1;
+}
+
+/* Range computation should be deterministic. Lets validate that.  */
+if (range.min != TEST_2922_MIN_ID
+|| range.max != TEST_2922_MAX_ID
+|| slice_num != TEST_2922_DFL_SLIDE) {
+DEBUG(SSSDBG_FATAL_FAILURE, "Failed to compute range correctly.\n");
+return 1;
+}
+
+err = sss_idmap_add_domain_ex(test_ctx->idmap_ctx, name, sid, ,
+  NULL, 0, false /* No external mapping */);
+if (err != IDMAP_SUCCESS) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "sss_idmap_add_domain_ex failed: exp: [%d] but got [%d]\n",
+  IDMAP_SUCCESS, err);
+return 1;
+}
+
+return 0;
+}
+
 static int test_sss_idmap_setup_with_domains(void **state) {
 struct test_ctx *test_ctx;
 
@@ -140,6 +198,23 @@ static int test_sss_idmap_setup_with_domains(void **state) {
 return 0;
 }
 
+static int test_sss_idmap_setup_with_domains_2922(void **state) {
+struct test_ctx *test_ctx;
+int ret;
+
+test_sss_idmap_setup(state);
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+if (test_ctx == NULL) {
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "test context is NULL\n");
+return 1;
+}
+
+ret = setup_ranges_2922(test_ctx);
+return ret;
+}
+
 static int test_sss_idmap_setup_with_domains_sec_slices(void **state) {
 struct test_ctx *test_ctx;
 
@@ -298,6 +373,53 @@ void test_map_id(void **state)
 sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
 }
 
+/* https://fedorahosted.org/sssd/ticket/2922 */
+/* ID mapping - bug in computing max id for slice range */
+void test_map_id_2922(void **state)
+{
+struct test_ctx *test_ctx;
+enum idmap_error_code err;
+uint32_t id;
+char *sid = NULL;
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+
+assert_non_null(test_ctx);
+
+/* Min UNIX ID to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MIN_ID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_string_equal(sid, TEST_2922_FIRST_SID);
+sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
+
+/* First SID to UNIX ID */
+err = sss_idmap_sid_to_unix(test_ctx->idmap_ctx, TEST_2922_FIRST_SID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_int_equal(id, TEST_2922_MIN_ID);
+
+/* Max UNIX ID to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MAX_ID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_string_equal(sid, TEST_2922_LAST_SID);
+sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
+
+/* Last SID to UNIX ID */
+err = sss_idmap_sid_to_unix(test_ctx->idmap_ctx, TEST_2922_LAST_SID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_int_equal(id, TEST_292

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-05 Thread Pavel Reichl




On 02/05/2016 03:37 PM, Dan Lavu wrote:

Pavel,

I found the wording to be strange, this is shorter and more concise description 
of the parameter. I also took the liberty of editing the description of 
'pam_account_expired_message' as well. I don't think the comment about ssh keys 
is necessary,
since it applies to all pam auth phase not just keys.

Dan


Thanks Dan, ssh keys were used because for default value of pam_verbosity, for 
other services then ssh would nothing be outputted (as a security measure).
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] IDMAP: Add test to validate off by one bug

2016-02-05 Thread Pavel Reichl




On 02/05/2016 03:22 PM, Michal Židek wrote:

On 01/22/2016 02:55 PM, Pavel Reichl wrote:

Hello,

please see simple test adding test for
https://fedorahosted.org/sssd/ticket/2922.
Sumit proposed to test if mapping of UNIX MAX_ID + 1 fails to be mapped
to SID.

Without patch for #2922 test fails otherwise test passes.

Thanks.



Hi,

Please define TEST_2922_MAX_ID_PLUS_ONE macro like this:
#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)

I asked Jakub about the asserts in setup functions and he
confirmed that we should not use them in test setup functions.

So please do not use asserts in the tests but return
an error code. It is OK if you only change the functions
that you added, no need to remove the asserts from other
setup functions that already exist in the file.

I think it is OK to use DEBUG macro with SSSDBG_FATAL_FAILURE
level in the setup fuctions as well.

Michal


OK, thanks for comments. Please see update patch set.



___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
>From 915c12f8c868c43eed6667215abb584f265dfef2 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 22 Jan 2016 08:34:14 -0500
Subject: [PATCH] IDMAP: Add test to validate off by one bug

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
 src/tests/cmocka/test_sss_idmap.c | 102 ++
 1 file changed, 102 insertions(+)

diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c
index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..618e61900fb2ffa78d1322702f15e22eef01340b 100644
--- a/src/tests/cmocka/test_sss_idmap.c
+++ b/src/tests/cmocka/test_sss_idmap.c
@@ -43,6 +43,17 @@
 #define TEST_OFFSET 100
 #define TEST_OFFSET_STR "100"
 
+#define TEST_2922_MIN_ID 184260
+#define TEST_2922_MAX_ID 184279
+#define TEST_2922_MAX_ID_PLUS_ONE 184280
+
+#define TEST_2922_DFL_SLIDE 9212
+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0"
+/* Last SID = first SID + (default) rangesize -1 */
+#define TEST_2922_LAST_SID TEST_DOM_SID"-19"
+/* Last SID = first SID + rangesize */
+#define TEST_2922_LAST_SID_PLUS_ONE  TEST_DOM_SID"-20"
+
 struct test_ctx {
 TALLOC_CTX *mem_idmap;
 struct sss_idmap_ctx *idmap_ctx;
@@ -128,6 +139,35 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping,
 return 0;
 }
 
+static int setup_ranges_2922(struct test_ctx *test_ctx)
+{
+struct sss_idmap_range range;
+enum idmap_error_code err;
+const char *name;
+const char *sid;
+/* Pick a new slice. */
+id_t slice_num = -1;
+
+assert_non_null(test_ctx);
+
+name = TEST_DOM_NAME;
+sid = TEST_DOM_SID;
+
+err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, _num,
+);
+assert_int_equal(err, IDMAP_SUCCESS);
+/* Range computation should be deterministic. Lets validate that.  */
+assert_int_equal(range.min, TEST_2922_MIN_ID);
+assert_int_equal(range.max, TEST_2922_MAX_ID);
+assert_int_equal(slice_num, TEST_2922_DFL_SLIDE);
+
+err = sss_idmap_add_domain_ex(test_ctx->idmap_ctx, name, sid, ,
+  NULL, 0, false /* No external mapping */);
+assert_int_equal(err, IDMAP_SUCCESS);
+
+return 0;
+}
+
 static int test_sss_idmap_setup_with_domains(void **state) {
 struct test_ctx *test_ctx;
 
@@ -140,6 +180,18 @@ static int test_sss_idmap_setup_with_domains(void **state) {
 return 0;
 }
 
+static int test_sss_idmap_setup_with_domains_2922(void **state) {
+struct test_ctx *test_ctx;
+
+test_sss_idmap_setup(state);
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+assert_non_null(test_ctx);
+
+setup_ranges_2922(test_ctx);
+return 0;
+}
+
 static int test_sss_idmap_setup_with_domains_sec_slices(void **state) {
 struct test_ctx *test_ctx;
 
@@ -298,6 +350,53 @@ void test_map_id(void **state)
 sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
 }
 
+/* https://fedorahosted.org/sssd/ticket/2922 */
+/* ID mapping - bug in computing max id for slice range */
+void test_map_id_2922(void **state)
+{
+struct test_ctx *test_ctx;
+enum idmap_error_code err;
+uint32_t id;
+char *sid = NULL;
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+
+assert_non_null(test_ctx);
+
+/* Min UNIX ID to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MIN_ID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_string_equal(sid, TEST_2922_FIRST_SID);
+sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
+
+/* First SID to UNIX ID */
+err = sss_idmap_sid_to_unix(test_ctx->idmap_ctx, TEST_2922_FIRST_SID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_int_equal(id, TEST_2922_MIN_ID);
+
+/* Max UNIX ID to SID */
+

[SSSD] Re: [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-05 Thread Pavel Reichl




On 02/05/2016 11:01 AM, Jakub Hrozek wrote:

On Tue, Feb 02, 2016 at 08:48:43PM +0100, Pavel Reichl wrote:

...


I would prefer to split this patch into two, one that patches the LDAP
code to return ERR_ACCOUNT_LOCKED and one that passes on and displays
the message.


Done.



 From 511ef599902827d76193a1e634ace193df15dead Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 2 Feb 2016 14:35:15 -0500
Subject: [PATCH] PAM: Notify user of denial due to AD account lockout

Resolves:
https://fedorahosted.org/sssd/ticket/2839
---
index 
2d9b1184f5d30b9df7f1d3e4b980a7e0107c6830..763c5ed050bd482d334ad617349938dfc89f79da
 100644
--- a/src/providers/ldap/sdap_async_connection.c
+++ b/src/providers/ldap/sdap_async_connection.c
@@ -754,6 +754,9 @@ static void simple_bind_done(struct sdap_op *op,

  if (result == LDAP_SUCCESS) {
  ret = EOK;
+} else if (result == LDAP_INVALID_CREDENTIALS
+   && strstr(errmsg, "data 775,") != NULL) {

 ~~
I don't think this is safe, strstr() doesn't handle NULL input well.
Please add a check for "&& errmgs != NULL" before calling strstr.

Sure.



Otherwise the patch looks good, we just need to also ask some Native
speaker for manpage comments..


I pinged Dan about that.


Please see updated patch set, thanks!
>From 4adb25b045be46dc0a178748e2b0aeb5ea09ed1d Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 5 Feb 2016 07:27:38 -0500
Subject: [PATCH 1/2] SDAP: Add return code ERR_ACCOUNT_LOCKED

Add code to distinquish state when account is locked in Active
Directory server.

Resolves:
https://fedorahosted.org/sssd/ticket/2839
---
 src/providers/data_provider.h  | 2 ++
 src/providers/ldap/ldap_auth.c | 4 
 src/providers/ldap/sdap_async_connection.c | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h
index 39051b90c3aad96f62dcbb86a20bcfd8c954879b..7332b677d19f70f4736e4d0b68d55cdd3c67a4af 100644
--- a/src/providers/data_provider.h
+++ b/src/providers/data_provider.h
@@ -182,6 +182,8 @@ struct pam_data {
 bool offline_auth;
 bool last_auth_saved;
 int priv;
+int account_locked;
+
 #ifdef USE_KEYRING
 key_serial_t key_serial;
 #endif
diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c
index c94ba15bb17aa1641eb36781cc59ce158d48ca66..8d6a37b2ceb3347cb8092858889d07e5615e5c77 100644
--- a/src/providers/ldap/ldap_auth.c
+++ b/src/providers/ldap/ldap_auth.c
@@ -1302,6 +1302,10 @@ static void sdap_pam_auth_done(struct tevent_req *req)
 case ERR_PASSWORD_EXPIRED:
 state->pd->pam_status = PAM_NEW_AUTHTOK_REQD;
 break;
+case ERR_ACCOUNT_LOCKED:
+state->pd->account_locked = true;
+state->pd->pam_status = PAM_PERM_DENIED;
+break;
 default:
 state->pd->pam_status = PAM_SYSTEM_ERR;
 dp_err = DP_ERR_FATAL;
diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
index 2d9b1184f5d30b9df7f1d3e4b980a7e0107c6830..c1735513ff6dcc755daf06cb97da546eaded7eb9 100644
--- a/src/providers/ldap/sdap_async_connection.c
+++ b/src/providers/ldap/sdap_async_connection.c
@@ -754,6 +754,9 @@ static void simple_bind_done(struct sdap_op *op,
 
 if (result == LDAP_SUCCESS) {
 ret = EOK;
+} else if (result == LDAP_INVALID_CREDENTIALS
+   && errmsg != NULL && strstr(errmsg, "data 775,") != NULL) {
+ret = ERR_ACCOUNT_LOCKED;
 } else {
 ret = ERR_AUTH_FAILED;
 }
-- 
2.4.3

>From 62d50e027a157b8743b644167c954c949de06aea Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 5 Feb 2016 07:31:45 -0500
Subject: [PATCH 2/2] PAM: Pass account lockout status and display message

Resolves:
https://fedorahosted.org/sssd/ticket/2839
---
 src/confdb/confdb.h  |  1 +
 src/config/SSSDConfig/__init__.py.in |  1 +
 src/config/etc/sssd.api.conf |  1 +
 src/man/sssd.conf.5.xml  | 21 +
 src/providers/dp_auth_util.c | 20 
 src/responder/pam/pamsrv_cmd.c   | 31 +++
 6 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index fcffcb5a6ff8b3f766ed9a693db874c7c6e3d9b9..e6789c8665cf677712d8e7fb72b6f0a41bca80b1 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -118,6 +118,7 @@
 #define CONFDB_PAM_TRUSTED_USERS "pam_trusted_users"
 #define CONFDB_PAM_PUBLIC_DOMAINS "pam_public_domains"
 #define CONFDB_PAM_ACCOUNT_EXPIRED_MESSAGE "pam_account_expired_message"
+#define CONFDB_PAM_ACCOUNT_LOCKED_MESSAGE "pam_account_locked_message"
 #define CONFDB_PAM_CERT_AUTH "pam_cert_auth"
 #def

[SSSD] [PATCH] PAM: Notify user of denial due to AD account lockout

2016-02-02 Thread Pavel Reichl


Hello,

please see attached patch.

To test connect to AD using ldap provider (for both id and auth). Lock account of AD user by entering invalid password repeatedly. In pam section of sssd.conf set pam_account_locked_message option. After failing to su as locked user you should see 
message containing this information.


Thanks!
>From 511ef599902827d76193a1e634ace193df15dead Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 2 Feb 2016 14:35:15 -0500
Subject: [PATCH] PAM: Notify user of denial due to AD account lockout

Resolves:
https://fedorahosted.org/sssd/ticket/2839
---
 src/confdb/confdb.h|  1 +
 src/config/SSSDConfig/__init__.py.in   |  1 +
 src/config/etc/sssd.api.conf   |  1 +
 src/man/sssd.conf.5.xml| 21 
 src/providers/data_provider.h  |  2 ++
 src/providers/dp_auth_util.c   | 20 +++
 src/providers/ldap/ldap_auth.c |  4 
 src/providers/ldap/sdap_async_connection.c |  3 +++
 src/responder/pam/pamsrv_cmd.c | 31 ++
 9 files changed, 76 insertions(+), 8 deletions(-)

diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index fcffcb5a6ff8b3f766ed9a693db874c7c6e3d9b9..e6789c8665cf677712d8e7fb72b6f0a41bca80b1 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -118,6 +118,7 @@
 #define CONFDB_PAM_TRUSTED_USERS "pam_trusted_users"
 #define CONFDB_PAM_PUBLIC_DOMAINS "pam_public_domains"
 #define CONFDB_PAM_ACCOUNT_EXPIRED_MESSAGE "pam_account_expired_message"
+#define CONFDB_PAM_ACCOUNT_LOCKED_MESSAGE "pam_account_locked_message"
 #define CONFDB_PAM_CERT_AUTH "pam_cert_auth"
 #define CONFDB_PAM_CERT_DB_PATH "pam_cert_db_path"
 #define CONFDB_PAM_P11_CHILD_TIMEOUT "p11_child_timeout"
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index 1fdb907c5d010323c22b18b4c371c61e5928c40f..495cb650ee86e50031962a4fcf0c21aa79dc0142 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -91,6 +91,7 @@ option_strings = {
 'pam_trusted_users' : _('List of trusted uids or user\'s name'),
 'pam_public_domains' : _('List of domains accessible even for untrusted users.'),
 'pam_account_expired_message' : _('Message printed when user account is expired.'),
+'pam_account_locked_message' : _('Message printed when user account is locked.'),
 'p11_child_timeout' : _('How many seconds will pam_sss wait for p11_child to finish'),
 
 # [sudo]
diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
index 89cf8634ffb8115d9e65cf66dc9b1ed630415c15..baa15539cbb5a925b19bac0452cde43ca9f71033 100644
--- a/src/config/etc/sssd.api.conf
+++ b/src/config/etc/sssd.api.conf
@@ -61,6 +61,7 @@ get_domains_timeout = int, None, false
 pam_trusted_users = str, None, false
 pam_public_domains = str, None, false
 pam_account_expired_message = str, None, false
+pam_account_locked_message = str, None, false
 p11_child_timeout = int, None, false
 
 [sudo]
diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index 73a21bfa0049bc4d3cfacb49201707868c87e533..2dbc58a451686beda0faa9e9366bbc3b3b4c253e 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -1040,6 +1040,27 @@ pam_account_expired_message = Account expired, please call help desk.
 
 
 
+pam_account_locked_message (string)
+
+
+   If user is authenticating and
+   account is locked then by default
+   'Permission denied' is output. This output will
+   be changed to content of this variable if it is
+   set.
+
+
+example:
+
+pam_account_locked_message = Account locked, please call help desk.
+
+
+
+Default: none
+
+
+
+
 p11_child_timeout (integer)
 
 
diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h
index 39051b90c3aad96f62dcbb86a20bcfd8c954879b..7332b677d19f70f4736e4d0b68d55cdd3c67a4af 100644
--- a/src/providers/data_provider.h
+++ b/src/providers/data_provider.h
@@ -182,6 +182,8 @@ struct pam_data {
 bool offline_auth;
 bool last_auth_saved;
 int priv;
+int account_locked;
+
 #ifdef USE_KEYRING
 key_serial_t key_serial;
 #endif
diff --git a/src/providers/dp_auth_util.c b/src/providers/dp_auth_util.c
index f8a30c5d4e6da7ce6ac28723032241

[SSSD] Re: [PATCH] NSS: Fix memory leak netgroup

2016-02-02 Thread Pavel Reichl


On 01/25/2016 08:54 PM, Jakub Hrozek wrote:

On Mon, Jan 25, 2016 at 05:16:37PM +0100, Pavel Reichl wrote:

Hello,

attached patch does not seem to suffer from these errors any more.

Shall I ask user who reported the bug If he is willing to test this new version 
of the patch? IIRC he needs more then a week for a testing to be conclusive...

Thanks.




Yes, please. We can do the review in the meantime.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org



User just confirmed that packages seem to be running fine on the test host, no 
memory usage growth.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] krb5_child: Warn if user cannot read krb5.conf

2016-02-01 Thread Pavel Reichl


I thought you were going to use 'fd' for return value of open(). I still think 
access() would be better function to use. We would not need to care about file 
descriptor at all.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] krb5_child: Warn if user cannot read krb5.conf

2016-02-01 Thread Pavel Reichl




On 02/01/2016 02:33 PM, Lukas Slebodnik wrote:

}

+void try_open_krb5_conf(void)
+{
+int fd;
+int ret;
+


Is there any reason for the function not to be static?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] [PATCH] PAM: Clarify man page for domains option

2016-01-29 Thread Pavel Reichl


Hello, please see trivial patch attached. Thanks.
>From 6d5f6b71c2d2f891470dc1c9f08ae67f5b6c02f5 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 29 Jan 2016 08:27:01 -0500
Subject: [PATCH] PAM: Clarify man page for domains option

Resolves:
https://fedorahosted.org/sssd/ticket/2946
---
 src/man/pam_sss.8.xml | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/man/pam_sss.8.xml b/src/man/pam_sss.8.xml
index 7794d3acfdfdbde491a3e1ada44481b73588e41f..278126c14d0a574a1e120762af264ef653deb0b0 100644
--- a/src/man/pam_sss.8.xml
+++ b/src/man/pam_sss.8.xml
@@ -145,9 +145,11 @@
 SSSD domain names, as specified in the sssd.conf file.
 
 
-NOTE: Must be used in conjunction with the
-pam_trusted_users and
-pam_public_domains options.
+NOTE: If PAM service is being run by untrusted user
+(pam_trusted_users option)
+then please make
+sure that restricted domains are public
+(pam_public_domains option).
 Please see the
 
 sssd.conf
-- 
2.4.3

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

[SSSD] Re: [PATCH] NSS: Fix memory leak netgroup

2016-01-29 Thread Pavel Reichl

On 01/28/2016 12:28 PM, Jakub Hrozek wrote:

On Mon, Jan 25, 2016 at 08:54:53PM +0100, Jakub Hrozek wrote:

On Mon, Jan 25, 2016 at 05:16:37PM +0100, Pavel Reichl wrote:

Hello,

attached patch does not seem to suffer from these errors any more.

Shall I ask user who reported the bug If he is willing to test this new version
of the patch? IIRC he needs more then a week for a testing to be conclusive...

Thanks.

Yes, please. We can do the review in the meantime.

The new code looks good to me and there is definitely improvement in
memory consumption. Before the patch, after requesting 10 netgroups
the memory usage went from ~30 to ~250MBs. After the patch, the memory
went from ~30 to ~50 MBs.

Did you take a look in your testing what is the extra 20MBs or should I?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

How exactly did you measure the memory use? In my testing I used talloc_full_report (the patch was attached in 1st version of patch). I rerun query for 10 netgroups, right after the test talloc reported 'full talloc report on 'null_context'
(total 1738774 bytes in 48682 blocks)', a few minutes after the test finished I got: 'full talloc report on 'null_context' (total 16258 bytes in 417 blocks)'.

I'm not seeing any such leaks as you observed.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] krb5_child: Warn if user cannot read krb5.conf

2016-01-29 Thread Pavel Reichl




On 01/29/2016 01:41 PM, Lukas Slebodnik wrote:

https://fedorahosted.org/sssd/ticket/2931
---
  src/providers/krb5/krb5_child.c | 17 +
  1 file changed, 17 insertions(+)

diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 
12eb9e2093d2bdd7d67e8d029fec1455488aa67c..88bcaddc419c1e6dc5d9a0c69b50c45a45c95efc
 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -2675,6 +2675,23 @@ int main(int argc, const char *argv[])
  goto done;
  }

+ret = open("/etc/krb5.conf", O_RDONLY);
+if (ret == EOK)


I thought that open() returns file descriptor on success and and -1 in case of 
error. Was I wrong?

 {

+close(ret);
+} else {
+ret = errno;
+if (ret == EPERM) {
+DEBUG(SSSDBG_CRIT_FAILURE,
+  "User with uid:%"SPRIuid" gid:%"SPRIgid" cannot read "
+  "/etc/krb5.conf. It might cause problems.",
+  geteuid(), getegid());
+} else {
+DEBUG(SSSDBG_MINOR_FAILURE,
+  "Cannot open /etc/krb5.conf [%d]: %s\n",
+  ret, strerror(ret));
+}
+}
+
  DEBUG(SSSDBG_TRACE_INTERNAL,
"Running as [%"SPRIuid"][%"SPRIgid"].\n", geteuid(), getegid());

-- 2.5.0

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

[SSSD] Re: [PATCH] krb5_child: Warn if user cannot read krb5.conf

2016-01-29 Thread Pavel Reichl


Could we use access() instead of open()?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] [PATCH] SDAP: Add error code to debug message

2016-01-28 Thread Pavel Reichl


Hello, please see trivial patch attached. Thanks
>From c106248c20c8afa829bea0f2fcad2506ef6cbea9 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Thu, 28 Jan 2016 10:47:35 -0500
Subject: [PATCH] SDAP: Add error code to debug message

---
 src/providers/ldap/sdap_async_users.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c
index 69396aeed23c44bc2cf878410d357984d875b6d8..480bbc2031249a66abf61737f6c86a7afdd3885f 100644
--- a/src/providers/ldap/sdap_async_users.c
+++ b/src/providers/ldap/sdap_async_users.c
@@ -901,7 +901,8 @@ static void sdap_get_users_done(struct tevent_req *subreq)
 ret = sdap_search_user_recv(state, subreq, >higher_usn,
 >users, >count);
 if (ret) {
-DEBUG(SSSDBG_OP_FAILURE, "Failed to retrieve users\n");
+DEBUG(SSSDBG_OP_FAILURE, "Failed to retrieve users [%d][%s].\n",
+  ret, sss_strerror(ret));
 tevent_req_error(req, ret);
 return;
 }
@@ -911,7 +912,8 @@ static void sdap_get_users_done(struct tevent_req *subreq)
   state->users, state->count,
   >higher_usn);
 if (ret) {
-DEBUG(SSSDBG_OP_FAILURE, "Failed to store users.\n");
+DEBUG(SSSDBG_OP_FAILURE, "Failed to store users [%d][%s].\n",
+  ret, sss_strerror(ret));
 tevent_req_error(req, ret);
 return;
 }
-- 
2.4.3

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

[SSSD] Re: [PATCH] NSS: Fix memory leak netgroup

2016-01-28 Thread Pavel Reichl




On 01/28/2016 12:28 PM, Jakub Hrozek wrote:

On Mon, Jan 25, 2016 at 08:54:53PM +0100, Jakub Hrozek wrote:

On Mon, Jan 25, 2016 at 05:16:37PM +0100, Pavel Reichl wrote:

Hello,

attached patch does not seem to suffer from these errors any more.

Shall I ask user who reported the bug If he is willing to test this new version 
of the patch? IIRC he needs more then a week for a testing to be conclusive...

Thanks.




Yes, please. We can do the review in the meantime.


The new code looks good to me and there is definitely improvement in
memory consumption. Before the patch, after requesting 10 netgroups
the memory usage went from ~30 to ~250MBs. After the patch, the memory
went from ~30 to ~50 MBs.

Did you take a look in your testing what is the extra 20MBs or should I?


I would expect that this memory is occupied by dummy records to speed up 
negative queries...but these records should be freed in final time...how long 
have you been waiting after your 'massive' query?


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


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

[SSSD] [PATCH] IDMAP: Man change for ldap_idmap_range_size option

2016-01-28 Thread Pavel Reichl


Hello,

please see simple patch attached.

Thanks.
>From 6dd7fc03013547c8f4ebe199c0d1501b97231f5a Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Thu, 28 Jan 2016 05:03:40 -0500
Subject: [PATCH] IDMAP: Man change for ldap_idmap_range_size option

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
 src/man/include/ldap_id_mapping.xml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/man/include/ldap_id_mapping.xml b/src/man/include/ldap_id_mapping.xml
index a088c4e81d81c5670edea8ae8081abe80927446a..9252b1caa56b086b640ab0b2a79069616cef6443 100644
--- a/src/man/include/ldap_id_mapping.xml
+++ b/src/man/include/ldap_id_mapping.xml
@@ -178,7 +178,9 @@ ldap_schema = ad
 
 For example, if your most recently-added Active Directory user has
 objectSid=S-1-5-21-215332-2176343378-3404031434-1107,
-ldap_idmap_range_size must be at least 1107.
+ldap_idmap_range_size must be at least 1108 as
+range size is equal to maximal SID minus minimal SID plus one
+(e.g. 1108 = 1107 - 0 + 1).
 
 
 It is important to plan ahead for future expansion, as changing this
-- 
2.4.3

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

[SSSD] Re: IDMAP: Fix minor memory leak

2016-01-27 Thread Pavel Reichl




On 01/27/2016 02:58 PM, Michal Židek wrote:

On 01/22/2016 06:38 PM, Pavel Reichl wrote:

[snip]

  first_rid += ctx->idmap_opts.rangesize;
@@ -631,6 +631,14 @@ get_helpers(struct sss_idmap_ctx *ctx,

  *_sec_slices = sec_slices;
  return IDMAP_SUCCESS;
+
+done:


You use this goto target only case of failure. Could you
change it's name to 'fail'? Or alternatively you can refactor
the function to have single exit point and keep the done label.
Choice is yours.



OK, done.
>From 2e422126bfe73cd3f57a965ba95f1ac15c229806 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 22 Jan 2016 12:30:23 -0500
Subject: [PATCH] IDMAP: Fix minor memory leak

---
 src/lib/idmap/sss_idmap.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 269ef0132ff3b9ffbfbe65006361fac6d4f88cf9..e3e9972b802748770a5f7440fa8ddc8ba75d3362 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -607,13 +607,13 @@ get_helpers(struct sss_idmap_ctx *ctx,
 for (int i = 0; i < ctx->idmap_opts.extra_slice_init; i++) {
 secondary_name = generate_sec_slice_name(ctx, domain_sid, first_rid);
 if (secondary_name == NULL) {
-return IDMAP_OUT_OF_MEMORY;
+err = IDMAP_OUT_OF_MEMORY;
+goto fail;
 }
 
 err = generate_slice(ctx, secondary_name, first_rid, );
 if (err != IDMAP_SUCCESS) {
-ctx->free_func(secondary_name, ctx->alloc_pvt);
-return err;
+goto fail;
 }
 
 first_rid += ctx->idmap_opts.rangesize;
@@ -631,6 +631,14 @@ get_helpers(struct sss_idmap_ctx *ctx,
 
 *_sec_slices = sec_slices;
 return IDMAP_SUCCESS;
+
+fail:
+ctx->free_func(secondary_name, ctx->alloc_pvt);
+
+/* Free already generated helpers. */
+free_helpers(ctx, sec_slices, true);
+
+return err;
 }
 
 enum idmap_error_code sss_idmap_add_domain_ex(struct sss_idmap_ctx *ctx,
-- 
2.4.3

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

[SSSD] [PATCH] IDMAP: Add minor performance improvements

2016-01-26 Thread Pavel Reichl


Hello,

please see simple patch attached.

Thanks.
>From 8ea41341650498aade9ce91ac04501327d89a558 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 26 Jan 2016 11:28:22 -0500
Subject: [PATCH] IDMAP: Add minor performance improvements

Some ID ranges are precalculated when ID mapping is being initialized.
This patch utilizes these (helper) ranges when new domains are generated
if appropriate.
---
 src/lib/idmap/sss_idmap.c | 96 +--
 1 file changed, 85 insertions(+), 11 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 269ef0132ff3b9ffbfbe65006361fac6d4f88cf9..394f9dcee69f7340e3af907fabd612b9ab205df8 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -89,6 +89,48 @@ static char *idmap_strdup(struct sss_idmap_ctx *ctx, const char *str)
 return new;
 }
 
+static bool ranges_eq(const struct idmap_range_params *a,
+  const struct idmap_range_params *b)
+{
+if (a == NULL || b == NULL) {
+return false;
+}
+
+if (a->first_rid == b->first_rid
+&& a->min_id == b->min_id
+&& a->max_id == b->max_id) {
+return true;
+}
+
+return false;
+}
+
+static struct idmap_range_params*
+construct_range(struct sss_idmap_ctx *ctx,
+const struct idmap_range_params *src,
+char *id)
+{
+struct idmap_range_params *dst;
+
+if (src == NULL || id == NULL) {
+return NULL;
+}
+
+dst = ctx->alloc_func(sizeof(struct idmap_range_params), ctx->alloc_pvt);
+if (dst == NULL) {
+return NULL;
+}
+
+/* Copy */
+dst->min_id = src->min_id;
+dst->max_id = src->max_id;
+dst->first_rid = src->first_rid;
+dst->next = src->next;
+
+dst->range_id = id;
+return dst;
+}
+
 static bool id_is_in_range(uint32_t id,
struct idmap_range_params *rp,
uint32_t *rid)
@@ -232,6 +274,18 @@ static void free_helpers(struct sss_idmap_ctx *ctx,
 }
 }
 
+static struct idmap_range_params*
+get_helper_by_id(struct idmap_range_params *helpers, const char *id)
+{
+for (struct idmap_range_params *it = helpers; it != NULL; it = it->next) {
+if (strcmp(it->range_id, id) == 0) {
+return it;
+}
+}
+
+return NULL;
+}
+
 static void sss_idmap_free_domain(struct sss_idmap_ctx *ctx,
   struct idmap_domain_info *dom)
 {
@@ -846,30 +900,48 @@ static bool comp_id(struct idmap_range_params *range_params, long long rid,
 
 static enum idmap_error_code
 get_range(struct sss_idmap_ctx *ctx,
+  struct idmap_range_params *helpers,
   const char *dom_sid,
   long long rid,
   struct idmap_range_params **_range)
 {
-char *secondary_name;
+char *secondary_name = NULL;;
 enum idmap_error_code err;
 int first_rid;
 struct idmap_range_params *range;
+struct idmap_range_params *helper;
 
 first_rid = (rid / ctx->idmap_opts.rangesize) * ctx->idmap_opts.rangesize;
 
 secondary_name = generate_sec_slice_name(ctx, dom_sid, first_rid);
 if (secondary_name == NULL) {
-return IDMAP_OUT_OF_MEMORY;
+err = IDMAP_OUT_OF_MEMORY;
+goto error;
 }
 
-err = generate_slice(ctx, secondary_name, first_rid, );
-if (err == IDMAP_OUT_OF_SLICES) {
-ctx->free_func(secondary_name, ctx->alloc_pvt);
-return err;
+helper = get_helper_by_id(helpers, secondary_name);
+if (helper != NULL) {
+/* Utilize helper's range. */
+range = construct_range(ctx, helper, secondary_name);
+if (range == NULL) {
+err = IDMAP_OUT_OF_MEMORY;
+goto error;
+}
+range->next = NULL;
+} else {
+/* Have to generate a whole new range. */
+err = generate_slice(ctx, secondary_name, first_rid, );
+if (err == IDMAP_OUT_OF_SLICES) {
+goto error;
+}
 }
 
 *_range = range;
 return IDMAP_SUCCESS;
+
+error:
+ctx->free_func(secondary_name, ctx->alloc_pvt);
+return err;
 }
 
 static enum idmap_error_code
@@ -896,9 +968,7 @@ spawn_dom(struct sss_idmap_ctx *ctx,
 it = ctx->idmap_domain_info;
 while (it != NULL) {
 /* Find the newly added domain. */
-if (it->range_params.first_rid == range->first_rid
-&& it->range_params.min_id == range->min_id
-&& it->range_params.max_id == range->max_id) {
+if (ranges_eq(>range_params, range)) {
 
 /* Share helpers. */
 it->helpers = parent->helpers;
@@ -950,8 +1020,7 @@ add_dom_for_sid(struct sss_idmap_ctx *ctx,
 goto done;
 }
 
-/* todo optimize */
-err = get_range(ctx, matched_dom->sid,

[SSSD] Re: [PATCH] NSS: Fix memory leak netgroup

2016-01-25 Thread Pavel Reichl




On 12/07/2015 03:59 PM, Jakub Hrozek wrote:

On Fri, Dec 04, 2015 at 05:18:53PM +0100, Pavel Reichl wrote:



On 12/04/2015 03:37 PM, Jakub Hrozek wrote:

On Thu, Dec 03, 2015 at 03:29:22PM +0100, Pavel Reichl wrote:


On 12/03/2015 02:06 PM, Jakub Hrozek wrote:

On Mon, Nov 30, 2015 at 06:45:00PM +0100, Pavel Reichl wrote:



On 11/30/2015 06:02 PM, Jakub Hrozek wrote:


Wouldn't it then be better to see if another same object is already in
the hashtable and free it before replacing?


I agree it would be best. I tried that before and failed because I could not 
decipher out the relation of talloc contexts.

I tried that again. Seems that leaks are gone. Segfaults were not happening 
during my testing.



Code got even messier :-(


I think the code would look nice if it was placed in the else branch of
create_negcache_netgr() :-)


Done, thanks for hint.



I also don't think we need the tmp_ctx change..


I dare to disagree here. With prev. versions of patch I was experiencing
segfaults. IIRC step_context is being allocated on context of the netgroup
that is freed. I also think that it's not a good idea to allocate local data
on context that does not hold any reference to that data. But it might be
matter of personal taste.


What kind of segfaults, what was the backtrace? I think it might be good to
add talloc_zfree() instead of talloc_free() or explicitly NULL the pointer,
that should solve the issue without adding a new context...

Since it's per-domain, I wonder if step_ctx->dctx might be better
candidate than step_ctx either way.



Hello Jakub, I amended the patch as you proposed. You can now experience the 
segfault yourself - just query a non-existing netgroup.
I attached the backtrace as well.

I can investiage and use talloc reporting if you wish. But still using tmp_ctx 
seems as way of the least effort...


I think you are right, the step_ctx hierarchy is tricky, so the
temporary context looks like the easiest solution. Please do one more
change in the patch:


OK, please see attached patch.




 From 0160ee92c4d9a9542abfd3e918686526a52e367d Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 27 Nov 2015 07:53:00 -0500
Subject: [PATCH] NSS: Fix memory leak netgroup


[...]


@@ -461,6 +469,11 @@ static errno_t create_negcache_netgr(struct 
setent_step_ctx *step_ctx)
  }

  done:
+/* Free netgroup after step_ctx is not needed. */
+if (netgr_to_be_freed) {
+talloc_zfree(netgr_to_be_freed);
+}


Since talloc_free(NULL) is a noop, we should drop the if completely.


OK, done.



While testing the patches I found that sometimes (not always, thought) I
see this valgrind error when a non-existing netgroup is requested:
Mon Dec  7 14:53:15 2015) [sssd[nss]] [accept_fd_handler] (0x0400): Client 
connected!
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_cmd_get_version] (0x0200): Received 
client version [1].
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_cmd_get_version] (0x0200): Offered 
version [1].
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_parse_name_for_domains] (0x0200): 
name 'ngr' matched without domain, user is ngr
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [setnetgrent_send] (0x0100): Requesting info 
for netgroup [ngr] from []
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [lookup_netgr_step] (0x0100): Requesting 
info for [n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [lookup_netgr_step] (0x0040): No results 
for netgroup ngr (domain ipa.test)
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [get_dp_name_and_id] (0x0400): Not a 
LOCAL view, continuing with provided values.
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_dp_issue_request] (0x0400): Issuing 
request for [0x428c3b:4:n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_dp_get_account_msg] (0x0400): 
Creating request for [ipa.test][0x1004][FAST BE_REQ_NETGROUP][1][name=ngr]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_dp_internal_get_send] (0x0400): 
Entering request [0x428c3b:4:n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_dp_get_reply] (0x1000): Got reply 
from Data Provider - DP error code: 0 errno: 0 error message: Success
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [lookup_netgr_step] (0x0100): Requesting 
info for [n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [lookup_netgr_step] (0x0400): Returning 
info for netgroup [n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_dp_req_destructor] (0x0400): 
Deleting request: [0x428c3b:4:n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [nss_cmd_getnetgrent] (0x0100): 
Requesting netgroup data
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [nss_cmd_getnetgrent] (0x0400): 
Returning results for [ngr]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [nss_cmd_getnetgrent_process] (0x0200): 
No entries found
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [client_recv] (0x0200): Client 
disconnected!
(Mon Dec  7 14:53:20 2015) [sssd[nss]] [accept_fd_handler] (0x0400): Client 
connected!
(Mon Dec  7 14:53:20 2015) [sssd[nss]] [sss_cmd_ge

[SSSD] [PATCH] IDMAP: Add test to validate off by one bug

2016-01-22 Thread Pavel Reichl


Hello,

please see simple test adding test for 
https://fedorahosted.org/sssd/ticket/2922.
Sumit proposed to test if mapping of UNIX MAX_ID + 1 fails to be mapped to SID.

Without patch for #2922 test fails otherwise test passes.

Thanks.
>From 915c12f8c868c43eed6667215abb584f265dfef2 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 22 Jan 2016 08:34:14 -0500
Subject: [PATCH] IDMAP: Add test to validate off by one bug

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
 src/tests/cmocka/test_sss_idmap.c | 102 ++
 1 file changed, 102 insertions(+)

diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c
index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..618e61900fb2ffa78d1322702f15e22eef01340b 100644
--- a/src/tests/cmocka/test_sss_idmap.c
+++ b/src/tests/cmocka/test_sss_idmap.c
@@ -43,6 +43,17 @@
 #define TEST_OFFSET 100
 #define TEST_OFFSET_STR "100"
 
+#define TEST_2922_MIN_ID 184260
+#define TEST_2922_MAX_ID 184279
+#define TEST_2922_MAX_ID_PLUS_ONE 184280
+
+#define TEST_2922_DFL_SLIDE 9212
+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0"
+/* Last SID = first SID + (default) rangesize -1 */
+#define TEST_2922_LAST_SID TEST_DOM_SID"-19"
+/* Last SID = first SID + rangesize */
+#define TEST_2922_LAST_SID_PLUS_ONE  TEST_DOM_SID"-20"
+
 struct test_ctx {
 TALLOC_CTX *mem_idmap;
 struct sss_idmap_ctx *idmap_ctx;
@@ -128,6 +139,35 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping,
 return 0;
 }
 
+static int setup_ranges_2922(struct test_ctx *test_ctx)
+{
+struct sss_idmap_range range;
+enum idmap_error_code err;
+const char *name;
+const char *sid;
+/* Pick a new slice. */
+id_t slice_num = -1;
+
+assert_non_null(test_ctx);
+
+name = TEST_DOM_NAME;
+sid = TEST_DOM_SID;
+
+err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, _num,
+);
+assert_int_equal(err, IDMAP_SUCCESS);
+/* Range computation should be deterministic. Lets validate that.  */
+assert_int_equal(range.min, TEST_2922_MIN_ID);
+assert_int_equal(range.max, TEST_2922_MAX_ID);
+assert_int_equal(slice_num, TEST_2922_DFL_SLIDE);
+
+err = sss_idmap_add_domain_ex(test_ctx->idmap_ctx, name, sid, ,
+  NULL, 0, false /* No external mapping */);
+assert_int_equal(err, IDMAP_SUCCESS);
+
+return 0;
+}
+
 static int test_sss_idmap_setup_with_domains(void **state) {
 struct test_ctx *test_ctx;
 
@@ -140,6 +180,18 @@ static int test_sss_idmap_setup_with_domains(void **state) {
 return 0;
 }
 
+static int test_sss_idmap_setup_with_domains_2922(void **state) {
+struct test_ctx *test_ctx;
+
+test_sss_idmap_setup(state);
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+assert_non_null(test_ctx);
+
+setup_ranges_2922(test_ctx);
+return 0;
+}
+
 static int test_sss_idmap_setup_with_domains_sec_slices(void **state) {
 struct test_ctx *test_ctx;
 
@@ -298,6 +350,53 @@ void test_map_id(void **state)
 sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
 }
 
+/* https://fedorahosted.org/sssd/ticket/2922 */
+/* ID mapping - bug in computing max id for slice range */
+void test_map_id_2922(void **state)
+{
+struct test_ctx *test_ctx;
+enum idmap_error_code err;
+uint32_t id;
+char *sid = NULL;
+
+test_ctx = talloc_get_type(*state, struct test_ctx);
+
+assert_non_null(test_ctx);
+
+/* Min UNIX ID to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MIN_ID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_string_equal(sid, TEST_2922_FIRST_SID);
+sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
+
+/* First SID to UNIX ID */
+err = sss_idmap_sid_to_unix(test_ctx->idmap_ctx, TEST_2922_FIRST_SID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_int_equal(id, TEST_2922_MIN_ID);
+
+/* Max UNIX ID to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MAX_ID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_string_equal(sid, TEST_2922_LAST_SID);
+sss_idmap_free_sid(test_ctx->idmap_ctx, sid);
+
+/* Last SID to UNIX ID */
+err = sss_idmap_sid_to_unix(test_ctx->idmap_ctx, TEST_2922_LAST_SID, );
+assert_int_equal(err, IDMAP_SUCCESS);
+assert_int_equal(id, TEST_2922_MAX_ID);
+
+/* Max UNIX ID + 1 to SID */
+err = sss_idmap_unix_to_sid(test_ctx->idmap_ctx, TEST_2922_MAX_ID_PLUS_ONE,
+);
+assert_int_equal(err, IDMAP_NO_DOMAIN);
+
+/* Last SID + 1 to UNIX ID */
+err = sss_idmap_sid_to_unix(test_ctx->idmap_ctx,
+TEST_2922_LAST_SID_PLUS_ONE, );
+/* Auto adding new ranges is disable in this test.  */
+assert_int_equal(err, IDMAP_NO_RANGE);
+}
+
 void test_m

[SSSD] IDMAP: Fix minor memory leak

2016-01-22 Thread Pavel Reichl


Hello,

please see simple patch attached.

Memory leak is a minor problem as code is called just once for a new domain and 
it would occur only if run out of memory or all slices.

Thanks
>From 0368027c4b8eb0b4ffb4256332ace9a51133cfde Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 22 Jan 2016 12:30:23 -0500
Subject: [PATCH] IDMAP: Fix minor memory leak

---
 src/lib/idmap/sss_idmap.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 269ef0132ff3b9ffbfbe65006361fac6d4f88cf9..41a9f92724913c2311a83668f2f56dfe292ed8fe 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -607,13 +607,13 @@ get_helpers(struct sss_idmap_ctx *ctx,
 for (int i = 0; i < ctx->idmap_opts.extra_slice_init; i++) {
 secondary_name = generate_sec_slice_name(ctx, domain_sid, first_rid);
 if (secondary_name == NULL) {
-return IDMAP_OUT_OF_MEMORY;
+err = IDMAP_OUT_OF_MEMORY;
+goto done;
 }
 
 err = generate_slice(ctx, secondary_name, first_rid, );
 if (err != IDMAP_SUCCESS) {
-ctx->free_func(secondary_name, ctx->alloc_pvt);
-return err;
+goto done;
 }
 
 first_rid += ctx->idmap_opts.rangesize;
@@ -631,6 +631,14 @@ get_helpers(struct sss_idmap_ctx *ctx,
 
 *_sec_slices = sec_slices;
 return IDMAP_SUCCESS;
+
+done:
+ctx->free_func(secondary_name, ctx->alloc_pvt);
+
+/* Free already generated helpers. */
+free_helpers(ctx, sec_slices, true);
+
+return err;
 }
 
 enum idmap_error_code sss_idmap_add_domain_ex(struct sss_idmap_ctx *ctx,
-- 
2.4.3

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

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-20 Thread Pavel Reichl


Hello,

Updated patch set is attached.

Among other thinks I have renamed ldap_idmap_extra_slice_init to 
ldap_idmap_extra_slice_min and changed desc. in man page. Feel free to propose 
better name or desc.

I'm now going to have a look at test for the 1st patch.

Bye.
>From bfb1c99ccdfa6b65949912ef61f36137e2706b5f Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Wed, 13 Jan 2016 09:07:39 -0500
Subject: [PATCH 1/3] IDMAP: Fix computing max id for slice range

Max value of id mapping range was 1 unit too high.

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
 src/lib/idmap/sss_idmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 4c453120539a549807e9b6bb4db2dc396c1b3152..b5457f92dbb91ac5109ad17258920549e8808d26 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -336,7 +336,7 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
 }
 
 min = (rangesize * new_slice) + idmap_lower;
-max = min + rangesize;
+max = min + rangesize - 1;
 /* Verify that this slice is not already in use */
 do {
 for (dom = ctx->idmap_domain_info; dom != NULL; dom = dom->next) {
@@ -353,7 +353,7 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
 }
 
 min = (rangesize * new_slice) + idmap_lower;
-max = min + rangesize;
+max = min + rangesize - 1;
 break;
 }
 }
@@ -371,7 +371,7 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
 }
 
 _range->min = (rangesize * new_slice) + idmap_lower;
-_range->max = _range->min + rangesize;
+_range->max = _range->min + rangesize - 1;
 
 if (slice_num) {
 *slice_num = new_slice;
-- 
2.4.3

>From a729f65da6de71be84e4343dca1c0ac49cb7b91e Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Thu, 26 Nov 2015 10:46:34 -0500
Subject: [PATCH 2/3] IDMAP: New structure for domain range params

Create new internal structure idmap_range_params by merging ID mapping
range relevant fields from idmap_domain_info and remove corrsponding
fields.

Resolves:
https://fedorahosted.org/sssd/ticket/2188
---
 src/lib/idmap/sss_idmap.c | 117 --
 1 file changed, 60 insertions(+), 57 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index b5457f92dbb91ac5109ad17258920549e8808d26..23ed46a583547a3f2f0bca5ab62824bd045e56b9 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -33,13 +33,21 @@
 #define SID_FMT "%s-%d"
 #define SID_STR_MAX_LEN 1024
 
+/* Hold all parameters for unix<->sid mapping relevant for
+ * given slice. */
+struct idmap_range_params {
+uint32_t min_id;
+uint32_t max_id;
+char *range_id;
+
+uint32_t first_rid;
+};
+
 struct idmap_domain_info {
 char *name;
 char *sid;
-struct sss_idmap_range *range;
+struct idmap_range_params range_params;
 struct idmap_domain_info *next;
-uint32_t first_rid;
-char *range_id;
 bool external_mapping;
 };
 
@@ -72,37 +80,17 @@ static char *idmap_strdup(struct sss_idmap_ctx *ctx, const char *str)
 return new;
 }
 
-static struct sss_idmap_range *idmap_range_dup(struct sss_idmap_ctx *ctx,
-   struct sss_idmap_range *range)
-{
-struct sss_idmap_range *new = NULL;
-
-CHECK_IDMAP_CTX(ctx, NULL);
-
-
-new = ctx->alloc_func(sizeof(struct sss_idmap_range), ctx->alloc_pvt);
-if (new == NULL) {
-return NULL;
-}
-
-memset(new, 0, sizeof(struct sss_idmap_range));
-
-new->min = range->min;
-new->max = range->max;
-
-return new;
-}
-
-static bool id_is_in_range(uint32_t id, struct idmap_domain_info *dom,
+static bool id_is_in_range(uint32_t id,
+   struct idmap_range_params *rp,
uint32_t *rid)
 {
-if (id == 0 || dom == NULL || dom->range == NULL) {
+if (id == 0 || rp == NULL) {
 return false;
 }
 
-if (id >= dom->range->min && id <= dom->range->max) {
+if (id >= rp->min_id && id <= rp->max_id) {
 if (rid != NULL) {
-*rid = dom->first_rid + (id - dom->range->min);
+*rid = rp->first_rid + (id - rp->min_id);
 }
 
 return true;
@@ -220,8 +208,7 @@ static void sss_idmap_free_domain(struct sss_idmap_ctx *ctx,
 return;
 }
 
-ctx->free_func(dom->range_id, ctx->alloc_pvt);
-ctx->free_func(dom->range, ctx->alloc_pvt);
+ctx->free_func(dom->range_params.range_id, ctx->alloc_pvt);
 ctx->fr

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-20 Thread Pavel Reichl


Thank you Sumit and Lukas for comments updated patch set attached. Bye.

PS: diff is here

http://paste.fedoraproject.org/312853/45330609/
>From bfb1c99ccdfa6b65949912ef61f36137e2706b5f Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Wed, 13 Jan 2016 09:07:39 -0500
Subject: [PATCH 1/3] IDMAP: Fix computing max id for slice range

Max value of id mapping range was 1 unit too high.

Resolves:
https://fedorahosted.org/sssd/ticket/2922
---
 src/lib/idmap/sss_idmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 4c453120539a549807e9b6bb4db2dc396c1b3152..b5457f92dbb91ac5109ad17258920549e8808d26 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -336,7 +336,7 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
 }
 
 min = (rangesize * new_slice) + idmap_lower;
-max = min + rangesize;
+max = min + rangesize - 1;
 /* Verify that this slice is not already in use */
 do {
 for (dom = ctx->idmap_domain_info; dom != NULL; dom = dom->next) {
@@ -353,7 +353,7 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
 }
 
 min = (rangesize * new_slice) + idmap_lower;
-max = min + rangesize;
+max = min + rangesize - 1;
 break;
 }
 }
@@ -371,7 +371,7 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
 }
 
 _range->min = (rangesize * new_slice) + idmap_lower;
-_range->max = _range->min + rangesize;
+_range->max = _range->min + rangesize - 1;
 
 if (slice_num) {
 *slice_num = new_slice;
-- 
2.4.3

>From a729f65da6de71be84e4343dca1c0ac49cb7b91e Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Thu, 26 Nov 2015 10:46:34 -0500
Subject: [PATCH 2/3] IDMAP: New structure for domain range params

Create new internal structure idmap_range_params by merging ID mapping
range relevant fields from idmap_domain_info and remove corrsponding
fields.

Resolves:
https://fedorahosted.org/sssd/ticket/2188
---
 src/lib/idmap/sss_idmap.c | 117 --
 1 file changed, 60 insertions(+), 57 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index b5457f92dbb91ac5109ad17258920549e8808d26..23ed46a583547a3f2f0bca5ab62824bd045e56b9 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -33,13 +33,21 @@
 #define SID_FMT "%s-%d"
 #define SID_STR_MAX_LEN 1024
 
+/* Hold all parameters for unix<->sid mapping relevant for
+ * given slice. */
+struct idmap_range_params {
+uint32_t min_id;
+uint32_t max_id;
+char *range_id;
+
+uint32_t first_rid;
+};
+
 struct idmap_domain_info {
 char *name;
 char *sid;
-struct sss_idmap_range *range;
+struct idmap_range_params range_params;
 struct idmap_domain_info *next;
-uint32_t first_rid;
-char *range_id;
 bool external_mapping;
 };
 
@@ -72,37 +80,17 @@ static char *idmap_strdup(struct sss_idmap_ctx *ctx, const char *str)
 return new;
 }
 
-static struct sss_idmap_range *idmap_range_dup(struct sss_idmap_ctx *ctx,
-   struct sss_idmap_range *range)
-{
-struct sss_idmap_range *new = NULL;
-
-CHECK_IDMAP_CTX(ctx, NULL);
-
-
-new = ctx->alloc_func(sizeof(struct sss_idmap_range), ctx->alloc_pvt);
-if (new == NULL) {
-return NULL;
-}
-
-memset(new, 0, sizeof(struct sss_idmap_range));
-
-new->min = range->min;
-new->max = range->max;
-
-return new;
-}
-
-static bool id_is_in_range(uint32_t id, struct idmap_domain_info *dom,
+static bool id_is_in_range(uint32_t id,
+   struct idmap_range_params *rp,
uint32_t *rid)
 {
-if (id == 0 || dom == NULL || dom->range == NULL) {
+if (id == 0 || rp == NULL) {
 return false;
 }
 
-if (id >= dom->range->min && id <= dom->range->max) {
+if (id >= rp->min_id && id <= rp->max_id) {
 if (rid != NULL) {
-*rid = dom->first_rid + (id - dom->range->min);
+*rid = rp->first_rid + (id - rp->min_id);
 }
 
 return true;
@@ -220,8 +208,7 @@ static void sss_idmap_free_domain(struct sss_idmap_ctx *ctx,
 return;
 }
 
-ctx->free_func(dom->range_id, ctx->alloc_pvt);
-ctx->free_func(dom->range, ctx->alloc_pvt);
+ctx->free_func(dom->range_params.range_id, ctx->alloc_pvt);
 ctx->free_func(dom->name, ctx->alloc_pvt);
 ctx->free_func(dom->sid, ctx->alloc_pvt);
 ctx->free_func(dom, ctx->alloc_pvt);
@@

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-19 Thread Pavel Reichl




On 01/18/2016 10:12 AM, Sumit Bose wrote:

On Wed, Jan 13, 2016 at 12:11:48PM +0100, Pavel Reichl wrote:



On 01/12/2016 04:46 PM, Pavel Reichl wrote:

...


I would add a test where you set the idmap parameters to some smaller
values than the default, e.g. range size to 10 and the number of initial
slices to 5. Then initialize the id-mapping for some domain and run a
loop with rid values from 0 to 80, call sss_idmap_sid_to_unix() and save
the POSIX IDs in an array. Then run sss_idmap_unix_to_sid() with the
values from the array and check if the expected SID is returned. This
will exercise adding more slices than initialized at startup and make
sure that there are no gaps between the slices or off-by-one errors at
the boundary of the slices.


OK, I will work on this now.


Done in attached patch (only change in the patch set).


Thank you. First, the patches work very well in my testing (after adding
the fix for the off-by-one error you found). Nevertheless I have two
comments which hopefully make the patches more consistent with the
current usage and maybe even a bit more robust.

The first was suggested by Simo. For the initial idrange we use the
domain SID as input for murmurhash. For the secondary slices currently
the domain name with a #{rangenumber} suffix is used. Simo suggested to
use "domain-SID"-"first-RID-of-the-new-idrange" as input. Both formats
are equivalent because the domain-SID maps 1:1 to the domain name and
the first RID is rangenumber*rangesize. Nevertheless I think the scheme
Simo suggested is better, because it continues to use the domain-SID as
we already do for the initial idrange (I'm afraid using the domain name
here was a suggestion by me, I'm sorry I didn't thought about this more
carefully). Additionally using the first RID instead of the rangenumber
make sense because the first RID is a property of the idrange and
independent of the SSSD configuration parameters.


Last patch implements changes described in prev. paragraph. In final patch set 
I will merge it with second patch. I just want do make the change obvious.



While the first suggestion one is a minor change the second is a bit
larger. I checked how the idranges are used by the IPA provider a
realized that here SSSD already supports multiple ranges for the same
domain as well. If on the IPA server multiple ranges are configured for
the same domain for each a new struct idmap_domain_info is created and
added to the list. To keep track of the different ranges of the domain
the range_id field is filled with the server side name of the idrange. I
think it would make the usage more consistent, might make the plan to
support manually configured range more easy and might even make the
given patches a bit more easy if this can be adopted here as well. So
instead of using or adding a secondary range if a RID outside of the
existing ranges is found a new suitable struct idmap_domain_info is
created and added to the list. The sec_ranges and use_sec_ranges
elements can still be used but maybe should get different names.
use_sec_ranges should still indicate if automatically adding ranges is
allowed for the domain or not. sec_ranges should only be used as helpers
in the sss_idmap_unix_to_sid() request if no matching range is found for
the given POSIX ID. This is still needed to have a cut-off to not search
the full POSIX ID space and to avoid the recalculation of the murmur
hash in case SSSD is spammed with useless getpwuid() or getgrgid()
requests.


I'm working on this right now.



I hope I was able to explain and justify the suggestions properly,
please ping you if you have questions.

bye,
Sumit






Additionally I think it would be more reliable if the secondary slices
can be saved to the cache and can be read at startup as well. I'm
thinking of the case when there is a collision in the secondary slices
of different domains. Currently we try to find a unused slice if a
collision is detected. But if after a restart the slices are initialized
in a different order the colliding slices might get swapped. Maybe
callbacks can be added to sss_idmap_add_auto_domain_ex() which can be
set to a function which can reads and writes the data from the cache?


OK, I will work on this after the test is done.


Working on this now.




Thanks.



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




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




 From ef2ff4c1ee7e1c76fc55eb223f15f396e1af885f Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Thu, 26 Nov 2015 10:46:34 -0500
Subject: [PATCH 1/2] IDMAP: New structure for domain range params

Create new internal structure idmap_range_params by merging ID mapping

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-19 Thread Pavel Reichl




On 01/19/2016 04:27 PM, Lukas Slebodnik wrote:

On (19/01/16 12:51), Pavel Reichl wrote:

//snip
@see main comment in ldap_id_mapping.xml


From a832b47f9ce4f288831b9c0e214493bdac7a9b7a Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 27 Nov 2015 04:15:00 -0500
Subject: [PATCH 2/4] IDMAP: introduce secondary slices

Resolves:
https://fedorahosted.org/sssd/ticket/2188
---
src/config/SSSDConfig/__init__.py.in |   1 +
src/config/etc/sssd.api.d/sssd-ad.conf   |   1 +
src/config/etc/sssd.api.d/sssd-ipa.conf  |   1 +
src/config/etc/sssd.api.d/sssd-ldap.conf |   1 +
src/lib/idmap/sss_idmap.c| 470 +++
src/lib/idmap/sss_idmap.exports  |   5 +-
src/lib/idmap/sss_idmap.h|  59 
src/lib/idmap/sss_idmap_private.h|   4 +
src/man/include/ldap_id_mapping.xml  |  20 ++
src/providers/ad/ad_opts.c   |   1 +
src/providers/ipa/ipa_opts.c |   1 +
src/providers/ldap/ldap_opts.c   |   1 +
src/providers/ldap/sdap.h|   1 +
src/providers/ldap/sdap_idmap.c  |  15 +-
src/tests/cmocka/test_sss_idmap.c|  86 +-
src/tests/sss_idmap-tests.c  | 191 +
16 files changed, 795 insertions(+), 63 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.exports b/src/lib/idmap/sss_idmap.exports
index 
52115636d5a6b936f18b4392e9d12adc26c85f53..ad1818e10506857b15927d95523b104fcf18f890
 100644
--- a/src/lib/idmap/sss_idmap.exports
+++ b/src/lib/idmap/sss_idmap.exports
@@ -1,4 +1,4 @@
-SSS_IDMAP_0.4 {
+SSS_IDMAP_0.5 {

 # public functions
 global:
@@ -8,13 +8,16 @@ SSS_IDMAP_0.4 {
 sss_idmap_ctx_set_lower;
 sss_idmap_ctx_set_upper;
 sss_idmap_ctx_set_rangesize;
+sss_idmap_ctx_set_extra_slice_init;
 sss_idmap_ctx_get_autorid;
 sss_idmap_ctx_get_lower;
 sss_idmap_ctx_get_upper;
 sss_idmap_ctx_get_rangesize;
+sss_idmap_ctx_get_extra_slice_init;
 sss_idmap_calculate_range;
 sss_idmap_add_domain;
 sss_idmap_add_domain_ex;
+sss_idmap_add_auto_domain_ex;
 sss_idmap_check_collision;
 sss_idmap_check_collision_ex;
 sss_idmap_sid_to_unix;

The last change in this file was in sssd-1.12.0. There were many releases
therefore we cannot touch version SSS_IDMAP_0.4. We can only extend it.


I'm afraid I don't understand and given the tight time schedule I would like to 
ask you if you could be so kind and prepare the proper patch for this. It would 
be really helpful.



@see git log for other files
./src/sss_client/idmap/sss_nss_idmap.exports
./src/providers/ipa/ipa_hbac.exports

You also forgot to bump version-info in Makefile.am for this library.







diff --git a/src/lib/idmap/sss_idmap.h b/src/lib/idmap/sss_idmap.h
index 
0797083293f7e010962828ddcd72709b290859b9..c19f6ad7bd4c4628d08bf5d90888d89bf1c79d5d
 100644
--- a/src/lib/idmap/sss_idmap.h
+++ b/src/lib/idmap/sss_idmap.h
@@ -175,6 +175,17 @@ enum idmap_error_code
sss_idmap_ctx_set_rangesize(struct sss_idmap_ctx *ctx, id_t rangesize);

/**
+ * @brief Set the number of secondary slices available for domain
+ *
+ * @param[in] ctx  idmap context
+ * @param[in] extra_slice_init number of secondary slices to be generated
+ * at startup
+ */
+enum idmap_error_code
+sss_idmap_ctx_set_extra_slice_init(struct sss_idmap_ctx *ctx,
+  int extra_slice_init);
+
+/**
  * @brief Check if autorid compatibility mode is set
  *
  * @param[in] ctx   idmap context
@@ -211,6 +222,16 @@ enum idmap_error_code
sss_idmap_ctx_get_rangesize(struct sss_idmap_ctx *ctx, id_t *rangesize);

/**
+ * @brief Get the maximal number of secondary slices available for domain
+ *
+ * @param[in] ctx  idmap context
+ * @param[out] _extra_slice_initmaximal number of secondary slices
+ */
+enum idmap_error_code
+sss_idmap_ctx_get_extra_slide_max(struct sss_idmap_ctx *ctx,
+  int *_extra_slide_max);
+
+/**
  * @brief Calculate new range of available POSIX IDs
  *
  * @param[in] ctx   Idmap context
@@ -291,6 +312,44 @@ enum idmap_error_code sss_idmap_add_domain_ex(struct 
sss_idmap_ctx *ctx,
   bool external_mapping);

/**
+ * @brief Add a domain with the first mappable RID to the idmap context and
+ * generate automatically secondary slices
+ *
+ * @param[in] ctx Idmap context
+ * @param[in] domain_name Zero-terminated string with the domain name
+ * @param[in] domain_sid  Zero-terminated string representation of the domain
+ *SID (S-1-15-.)
+ * @param[in] range   TBD Some information about the id ranges of this
+ *domain
+ * @param[in] range_idoptional unique identifier of a range, it is needed
+ *to allow updates at runtime
+ *

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-13 Thread Pavel Reichl




On 01/12/2016 04:46 PM, Pavel Reichl wrote:

...


I would add a test where you set the idmap parameters to some smaller
values than the default, e.g. range size to 10 and the number of initial
slices to 5. Then initialize the id-mapping for some domain and run a
loop with rid values from 0 to 80, call sss_idmap_sid_to_unix() and save
the POSIX IDs in an array. Then run sss_idmap_unix_to_sid() with the
values from the array and check if the expected SID is returned. This
will exercise adding more slices than initialized at startup and make
sure that there are no gaps between the slices or off-by-one errors at
the boundary of the slices.


OK, I will work on this now.


Done in attached patch (only change in the patch set).



Additionally I think it would be more reliable if the secondary slices
can be saved to the cache and can be read at startup as well. I'm
thinking of the case when there is a collision in the secondary slices
of different domains. Currently we try to find a unused slice if a
collision is detected. But if after a restart the slices are initialized
in a different order the colliding slices might get swapped. Maybe
callbacks can be added to sss_idmap_add_auto_domain_ex() which can be
set to a function which can reads and writes the data from the cache?


OK, I will work on this after the test is done.


Working on this now.




Thanks.



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




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

>From ef2ff4c1ee7e1c76fc55eb223f15f396e1af885f Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Thu, 26 Nov 2015 10:46:34 -0500
Subject: [PATCH 1/2] IDMAP: New structure for domain range params

Create new internal structure idmap_range_params by merging ID mapping
range relevant fields from idmap_domain_info and remove corrsponding
fields.

Resolves:
https://fedorahosted.org/sssd/ticket/2188
---
 src/lib/idmap/sss_idmap.c | 117 --
 1 file changed, 60 insertions(+), 57 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 4c453120539a549807e9b6bb4db2dc396c1b3152..fce3010d8e840fa86bc8225ef000e6e789072108 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -33,13 +33,21 @@
 #define SID_FMT "%s-%d"
 #define SID_STR_MAX_LEN 1024
 
+/* Hold all parameters for unix<->sid mapping relevant for
+ * given slice. */
+struct idmap_range_params {
+uint32_t min_id;
+uint32_t max_id;
+char *range_id;
+
+uint32_t first_rid;
+};
+
 struct idmap_domain_info {
 char *name;
 char *sid;
-struct sss_idmap_range *range;
+struct idmap_range_params range_params;
 struct idmap_domain_info *next;
-uint32_t first_rid;
-char *range_id;
 bool external_mapping;
 };
 
@@ -72,37 +80,17 @@ static char *idmap_strdup(struct sss_idmap_ctx *ctx, const char *str)
 return new;
 }
 
-static struct sss_idmap_range *idmap_range_dup(struct sss_idmap_ctx *ctx,
-   struct sss_idmap_range *range)
-{
-struct sss_idmap_range *new = NULL;
-
-CHECK_IDMAP_CTX(ctx, NULL);
-
-
-new = ctx->alloc_func(sizeof(struct sss_idmap_range), ctx->alloc_pvt);
-if (new == NULL) {
-return NULL;
-}
-
-memset(new, 0, sizeof(struct sss_idmap_range));
-
-new->min = range->min;
-new->max = range->max;
-
-return new;
-}
-
-static bool id_is_in_range(uint32_t id, struct idmap_domain_info *dom,
+static bool id_is_in_range(uint32_t id,
+   struct idmap_range_params *rp,
uint32_t *rid)
 {
-if (id == 0 || dom == NULL || dom->range == NULL) {
+if (id == 0 || rp == NULL) {
 return false;
 }
 
-if (id >= dom->range->min && id <= dom->range->max) {
+if (id >= rp->min_id && id <= rp->max_id) {
 if (rid != NULL) {
-*rid = dom->first_rid + (id - dom->range->min);
+*rid = rp->first_rid + (id - rp->min_id);
 }
 
 return true;
@@ -220,8 +208,7 @@ static void sss_idmap_free_domain(struct sss_idmap_ctx *ctx,
 return;
 }
 
-ctx->free_func(dom->range_id, ctx->alloc_pvt);
-ctx->free_func(dom->range, ctx->alloc_pvt);
+ctx->free_func(dom->range_params.range_id, ctx->alloc_pvt);
 ctx->free_func(dom->name, ctx->alloc_pvt);
 ctx->free_func(dom->sid, ctx->alloc_pvt);
 ctx->free_func(dom, ctx->alloc_pvt);
@@ -340,9 +327,12 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ct

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-12 Thread Pavel Reichl




On 01/06/2016 04:34 PM, Sumit Bose wrote:

On Mon, Jan 04, 2016 at 03:40:05PM +0100, Pavel Reichl wrote:

Hello Sumit, thanks for comments. I attached rebased patch with fixes as you 
proposed.

On 12/10/2015 01:23 PM, Sumit Bose wrote:


Some unit-test where failing for me because of an issue in cleaning up
the secondary slices. 'it = it->next' does not work because it was just
freed in the body of the loop. I fixed it with the change below, but
maybe a do-while loop would be even nicer here?


Sorry to hear that. I fixed that in attached patch set.
Which test failed for you? make check is always passing in my environment.


This is what I see:

$ ./sss_idmap-tests
Running suite(s): IDMAP
88%: Checks: 25, Failures: 0, Errors: 3
(null):-1:S:IDMAP mapping tests:idmap_test_sid2uid_ss:0: (after this point) 
Received signal 11 (Segmentation fault)
(null):-1:S:IDMAP mapping tests:idmap_test_uid2sid_ss:0: (after this point) 
Received signal 11 (Segmentation fault)
(null):-1:S:IDMAP mapping tests:idmap_test_sid2uid_ext_sec_slices:0: (after 
this point) Received signal 11 (Segmentation fault)

and here is what valgrind has to say:

$ valgrind .libs/lt-sss_idmap-tests
==17919== Memcheck, a memory error detector
==17919== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==17919== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==17919== Command: .libs/lt-sss_idmap-tests
==17919==
Running suite(s): IDMAP
==17919== Invalid read of size 4
==17919==at 0x407C595: sss_idmap_free_domain.part.1 (sss_idmap.c:220)
==17919==by 0x407C859: sss_idmap_free_domain (sss_idmap.c:240)
==17919==by 0x407C859: sss_idmap_free (sss_idmap.c:241)
==17919==by 0x804B60D: idmap_ctx_teardown (sss_idmap-tests.c:75)
==17919==by 0x4050271: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x405080D: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x4050BED: srunner_run (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x40511D3: srunner_run_all (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x80492D1: main (sss_idmap-tests.c:747)
==17919==  Address 0x4437d68 is 64 bytes inside a block of size 68 free'd
==17919==at 0x402C26D: free (in 
/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==17919==by 0x4068C9E: _talloc_free_internal (talloc.c:1057)
==17919==by 0x4068C9E: _talloc_free (talloc.c:1581)
==17919==by 0x407C594: sss_idmap_free_domain.part.1 (sss_idmap.c:222)
==17919==by 0x407C859: sss_idmap_free_domain (sss_idmap.c:240)
==17919==by 0x407C859: sss_idmap_free (sss_idmap.c:241)
==17919==by 0x804B60D: idmap_ctx_teardown (sss_idmap-tests.c:75)
==17919==by 0x4050271: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x405080D: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x4050BED: srunner_run (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x40511D3: srunner_run_all (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x80492D1: main (sss_idmap-tests.c:747)
==17919==
100%: Checks: 25, Failures: 0, Errors: 0
==17919==
==17919== HEAP SUMMARY:
==17919== in use at exit: 0 bytes in 0 blocks
==17919==   total heap usage: 1,918 allocs, 1,918 frees, 749,697 bytes allocated
==17919==
==17919== All heap blocks were freed -- no leaks are possible
==17919==
==17919== For counts of detected and suppressed errors, rerun with: -v
==17919== ERROR SUMMARY: 32 errors from 1 contexts (suppressed: 0 from 0)






diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index c4ae811..7ab5c66 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -210,6 +210,7 @@ static void sss_idmap_free_domain(struct
sss_idmap_ctx *ctx,
struct idmap_domain_info *dom)
  {
  struct idmap_range_params *it;
+struct idmap_range_params *next;

  if (ctx == NULL || dom == NULL) {
  return;
@@ -217,7 +218,8 @@ static void sss_idmap_free_domain(struct
sss_idmap_ctx *ctx,

  ctx->free_func(dom->range_params.range_id, ctx->alloc_pvt);
  /* Free list of secondary ranges */
-for (it = dom->sec_ranges; it != NULL; it = it->next) {
+for (it = dom->sec_ranges; it != NULL; it = next) {
+next = it->next;
  ctx->free_func(it->range_id, ctx->alloc_pvt);
  ctx->free_func(it, ctx->alloc_pvt);
  }


While checking and playing with the test I came across

+/* Todo - Add tests */
+/* Add size of primary slice for first_rid of secondary slices. */
+rid += ctx->idmap_opts.rangesize;

in sss_idmap_add_auto_domain_ex(). Does the 'Todo' only refer to 'Add
tests' or to 'Add size of primary slice for first_rid of secondary
slices.' as well? I'm asking because you really have to use the size of
the primary slice and not ctx->idmap_opts.rangesize here because the
primary range is given as a parameter to sss_idmap_add_auto_domain_ex()
and the size might be different then ctx->idmap_opts.rangesize. E.g. in
the unit-test the

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-11 Thread Pavel Reichl


Hello Sumit, thanks for comments and sorry for my delayed response, I'm 
addressing the issues right now, in this mail I just want quickly discuss the 
concern you have about
in loop declaration.

I generally I prefer this kind of declaration as it limits scope of variables 
to minimum and thus makes it easier to read and comprehend (It makes it obvious 
that variable is
used just in this single loop and it's insignificant for the rest of the code).

In my experience there's not a performance hit at all some even claim that by 
limiting scope of variables you give hints to compiler and thus generated code 
is faster.

This topic was discussed for example here:

http://stackoverflow.com/questions/407255/difference-between-declaring-variables-before-or-in-loop
http://stackoverflow.com/questions/1956353/efficiency-of-c-variable-declaration

I think that the conclusion of these threads complies with what I have stated 
above. But I suppose it would be possible to find threads claiming opposite so 
I'm aware it's
not definite source of truth :-)

Anyway, If you prefer If I kept all declaration at the begging of the function 
for legacy purposes or other I will do it (without any hard feelings).

Thanks for patience.

On 01/06/2016 04:34 PM, Sumit Bose wrote:


...


@@ -447,8 +437,13 @@ enum idmap_error_code sss_idmap_check_collision(struct 
sss_idmap_ctx *ctx,
  enum idmap_error_code err;

  for (dom = ctx->idmap_domain_info; dom != NULL; dom = dom->next) {
-err = sss_idmap_check_collision_ex(dom->name, dom->sid, dom->range,
-   dom->first_rid, dom->range_id,
+struct sss_idmap_range range = { dom->range_params.min_id,
+ dom->range_params.max_id };


In general I agree with the refactoring in this patch, but I don't like
the declaration of variables in the code. Especially in the case here
and below where variables are declared inside a loop. Mainly because I
do not know if compilers are able handle this smart or if this might be
more expensive than declaring the variables outside of the loop.


+
+err = sss_idmap_check_collision_ex(dom->name, dom->sid,
+   ,
+   dom->range_params.first_rid,
+   dom->range_params.range_id,
 dom->external_mapping,
 n_name, n_sid, n_range, 
n_first_rid,
 n_range_id, n_external_mapping);
@@ -467,12 +462,22 @@ static enum idmap_error_code dom_check_collision(
  enum idmap_error_code err;

  for (dom = dom_list; dom != NULL; dom = dom->next) {
-err = sss_idmap_check_collision_ex(dom->name, dom->sid, dom->range,
-   dom->first_rid, dom->range_id,
+struct sss_idmap_range range = { dom->range_params.min_id,
+ dom->range_params.max_id };
+struct sss_idmap_range new_dom_range = {
+new_dom->range_params.min_id,
+new_dom->range_params.max_id };
+
+
+err = sss_idmap_check_collision_ex(dom->name, dom->sid,
+   ,
+   dom->range_params.first_rid,
+   dom->range_params.range_id,
 dom->external_mapping,
 new_dom->name, new_dom->sid,
-   new_dom->range, new_dom->first_rid,
-   new_dom->range_id,
+   _dom_range,
+   new_dom->range_params.first_rid,
+   new_dom->range_params.range_id,
 new_dom->external_mapping);
  if (err != IDMAP_SUCCESS) {
  return err;


...

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

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-11 Thread Pavel Reichl

On 01/11/2016 02:21 PM, Jakub Hrozek wrote:

On Mon, Jan 11, 2016 at 02:09:48PM +0100, Sumit Bose wrote:

On Mon, Jan 11, 2016 at 01:03:33PM +0100, Pavel Reichl wrote:

Hello Sumit, thanks for comments and sorry for my delayed response, I'm
addressing the issues right now, in this mail I just want quickly discuss the
concern you have about
in loop declaration.

I generally I prefer this kind of declaration as it limits scope of variables
to minimum and thus makes it easier to read and comprehend (It makes it obvious
that variable is
used just in this single loop and it's insignificant for the rest of the code).

In my experience there's not a performance hit at all some even claim that by
limiting scope of variables you give hints to compiler and thus generated code
is faster.

This topic was discussed for example here:

http://stackoverflow.com/questions/407255/difference-between-declaring-variables-before-or-in-loop
http://stackoverflow.com/questions/1956353/efficiency-of-c-variable-declaration

I think that the conclusion of these threads complies with what I have stated
above. But I suppose it would be possible to find threads claiming opposite so
I'm aware it's
not definite source of truth :-)

Anyway, If you prefer If I kept all declaration at the begging of the function
for legacy purposes or other I will do it (without any hard feelings).

Thank you for the link, so it looks compilers can handle this well. I'm
fine with this scheme, iirc we had this discussion for the loop index
before and agreed that it makes sense to reduce the scope where possilbe
to the loop. But I don't remember if there was an agreement about
variabled inside the loop as well? But if we can agree on this I would
like to ask you to update the paragraph in the coding style which
currently reads "HIGHLY RECOMMENDED: Always declare all variables at the
top of the function, normally try to avoid declaring local variables in
internal loops.".

I really don't want to turn this threat into code style discussion, so I'll do
as you decide.

I personally think that as soon as you start mixing variable
declarations into code,

I always declare vars at the beginning of the block and separate it with new
line from the rest of the code so IMO I don't mix code declaration and code.

you should maybe think about splitting the code

That's certainly true, but I think we can agree that there are many, many functions that declare long list of vars that are just used in single loops and single ifs. But you have to find that out by carefully reading the code. I just make this
obvious and thus i do provide hints that maybe we should decompose function into more functions.

into more function..

IMO with the exception of indexes mixing code and declaration actually
/decreases/ readability.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

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

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-11 Thread Pavel Reichl




On 01/11/2016 03:14 PM, Sumit Bose wrote:

On Mon, Jan 11, 2016 at 03:04:03PM +0100, Pavel Reichl wrote:



On 01/11/2016 02:21 PM, Jakub Hrozek wrote:

On Mon, Jan 11, 2016 at 02:09:48PM +0100, Sumit Bose wrote:

On Mon, Jan 11, 2016 at 01:03:33PM +0100, Pavel Reichl wrote:

Hello Sumit, thanks for comments and sorry for my delayed response, I'm 
addressing the issues right now, in this mail I just want quickly discuss the 
concern you have about
in loop declaration.

I generally I prefer this kind of declaration as it limits scope of variables 
to minimum and thus makes it easier to read and comprehend (It makes it obvious 
that variable is
used just in this single loop and it's insignificant for the rest of the code).

In my experience there's not a performance hit at all some even claim that by 
limiting scope of variables you give hints to compiler and thus generated code 
is faster.

This topic was discussed for example here:

http://stackoverflow.com/questions/407255/difference-between-declaring-variables-before-or-in-loop
http://stackoverflow.com/questions/1956353/efficiency-of-c-variable-declaration

I think that the conclusion of these threads complies with what I have stated 
above. But I suppose it would be possible to find threads claiming opposite so 
I'm aware it's
not definite source of truth :-)

Anyway, If you prefer If I kept all declaration at the begging of the function 
for legacy purposes or other I will do it (without any hard feelings).


Thank you for the link, so it looks compilers can handle this well. I'm
fine with this scheme, iirc we had this discussion for the loop index
before and agreed that it makes sense to reduce the scope where possilbe
to the loop. But I don't remember if there was an agreement about
variabled inside the loop as well? But if we can agree on this I would
like to ask you to update the paragraph in the coding style which
currently reads "HIGHLY RECOMMENDED: Always declare all variables at the
top of the function, normally try to avoid declaring local variables in
internal loops.".


I really don't want to turn this threat into code style discussion, so I'll do 
as you decide.



Given the current recommended coding style and the other comments in the
thread I would like to ask you to move the declaration out of the loop
to the beginning of the function.

bye,
Sumit


OK, I'll do that.


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


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

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-11 Thread Pavel Reichl

On 01/11/2016 02:32 PM, Pavel Březina wrote:

On 01/11/2016 02:21 PM, Jakub Hrozek wrote:

On Mon, Jan 11, 2016 at 02:09:48PM +0100, Sumit Bose wrote:

On Mon, Jan 11, 2016 at 01:03:33PM +0100, Pavel Reichl wrote:

Hello Sumit, thanks for comments and sorry for my delayed response, I'm
addressing the issues right now, in this mail I just want quickly discuss the
concern you have about
in loop declaration.

In my experience there's not a performance hit at all some even claim that by
limiting scope of variables you give hints to compiler and thus generated code
is faster.

This topic was discussed for example here:

http://stackoverflow.com/questions/407255/difference-between-declaring-variables-before-or-in-loop
http://stackoverflow.com/questions/1956353/efficiency-of-c-variable-declaration

Anyway, If you prefer If I kept all declaration at the begging of the function
for legacy purposes or other I will do it (without any hard feelings).

I personally think that as soon as you start mixing variable
declarations into code, you should maybe think about splitting the code
into more function..

IMO with the exception of indexes mixing code and declaration actually
/decreases/ readability.

I must agree with Jakub on this one. Such scenario is a perfect hint that you
should decompose the code into functions. In case of get_sec_slices() it just
prolongs the for loop and makes the code harder to understand since you need to
keep the scope
in mind.

I think that compiler keeps scope in mind so I should make things easier.

BTW I haven't read whole Sumit's review so he might have already mentioned it
but prev seems to be pretty much useless.

Why do you think so?

I checked the code and I think it's used to link members of list...I suppose
there's better way but I can't see it right now. Do you?

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

[SSSD] [PATCH] intg - test regr 2849

2016-01-06 Thread Pavel Reichl


Hello,

I've been working on a integration test for 
https://fedorahosted.org/sssd/ticket/2849.

This patch is WIP as I have troubles with infopipe. Unless test is run as root 
it fails to connect to system bus:


Traceback (most recent call last):
  File "/home/preichl/sssd/src/tests/intg/ldap_local_override_test.py", line 
1038, in test_regr_2849_override
bus = dbus.SystemBus()
  File "/usr/lib64/python2.7/site-packages/dbus/_dbus.py", line 194, in __new__
private=private)
  File "/usr/lib64/python2.7/site-packages/dbus/_dbus.py", line 100, in __new__
bus = BusConnection.__new__(subclass, bus_type, mainloop=mainloop)
  File "/usr/lib64/python2.7/site-packages/dbus/bus.py", line 122, in __new__
bus = cls._new_for_bus(address_or_type, mainloop=mainloop)
DBusException: org.freedesktop.DBus.Error.NoReply: Did not receive a reply. 
Possible causes include: the remote application did not send a reply, the 
message bus security policy blocked the reply, the reply timeout expired, or 
the network connection was broken.


If I run test as root then I have to make sure that there are no other 
instances of SSSD running.

Any ideas how to work around this troubles?

Thanks!

>From a347a9e7e7eb9eb2195d3301cf7e8cbc5ac42b4e Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Wed, 6 Jan 2016 13:12:29 -0500
Subject: [PATCH] intg - test regr 2849

---
 src/tests/intg/ldap_local_override_test.py | 94 ++
 1 file changed, 94 insertions(+)

diff --git a/src/tests/intg/ldap_local_override_test.py b/src/tests/intg/ldap_local_override_test.py
index d5f58fc7a5207413df70b2f103887c0a72a47aa7..27714dfd0e5d38c17f8676abdc0848d19b7351aa 100644
--- a/src/tests/intg/ldap_local_override_test.py
+++ b/src/tests/intg/ldap_local_override_test.py
@@ -29,6 +29,7 @@ import pytest
 import ds_openldap
 import ldap_ent
 import sssd_id
+import dbus
 from util import unindent
 
 try:
@@ -171,6 +172,38 @@ def prepare_sssd(request, ldap_conn, use_fully_qualified_names=False):
 request.addfinalizer(teardown)
 
 
+def prepare_sssd_with_ifp(request, ldap_conn, use_fully_qualified_names=False):
+"""Prepare SSSD with defaults"""
+conf = unindent("""\
+[sssd]
+domains = LDAP
+services= nss, ifp
+
+[nss]
+memcache_timeout = 1
+
+[domain/LDAP]
+ldap_auth_disable_tls_never_use_in_production = true
+ldap_schema = rfc2307
+id_provider = ldap
+auth_provider   = ldap
+sudo_provider   = ldap
+ldap_uri= {ldap_conn.ds_inst.ldap_url}
+ldap_search_base= {ldap_conn.ds_inst.base_dn}
+use_fully_qualified_names = {use_fully_qualified_names}
+""").format(**locals())
+create_conf_fixture(request, conf)
+create_sssd_fixture(request)
+
+def teardown():
+# remove user export file
+try:
+os.unlink(OVERRIDE_FILENAME)
+except:
+pass
+request.addfinalizer(teardown)
+
+
 #
 # Common asserts for users
 #
@@ -951,3 +984,64 @@ def test_regr_2790_override(ldap_conn, env_regr_2790_override):
 assert res == sssd_id.NssReturnCode.SUCCESS, \
 "Could not find groups for user2 %d" % errno
 assert sorted(grp_list) == sorted(["group1", "group2"])
+
+
+# Regression test for bug #2849
+# cache_req: don't search override values in LDAP when using LOCAL view
+
+@pytest.fixture
+def env_regr_2849_override(request, ldap_conn):
+
+prepare_sssd_with_ifp(request, ldap_conn)
+
+# Add entries
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+ent_list.add_user("user1", 10001, 20001)
+ent_list.add_user("user2", 10002, 20002)
+ent_list.add_group("group1", 2001,
+   ["user1", "user2"])
+ent_list.add_group("group2", 2002,
+   ["user2"])
+
+create_ldap_fixture(request, ldap_conn, ent_list)
+
+# Assert entries are not overridden
+with pytest.raises(KeyError):
+pwd.getpwnam('alias1')
+with pytest.raises(KeyError):
+pwd.getpwnam('alias1@LDAP')
+with pytest.raises(KeyError):
+pwd.getpwnam('alias2')
+with pytest.raises(KeyError):
+pwd.getpwnam('alias2@LDAP')
+
+# Override
+subprocess.check_call(["sss_override", "user-add", "user1",
+   "-n", "alias1"])
+subprocess.check_call(["sss_override", "user-add", "user2",
+   "-n", "alias2"])
+
+restart_sssd()
+
+
+def getResults(results):
+l = []
+
+for r in results:
+l.append(str(r))
+
+return l
+
+
+def test_regr_2849_overri

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2016-01-04 Thread Pavel Reichl


Hello Sumit, thanks for comments. I attached rebased patch with fixes as you 
proposed.

On 12/10/2015 01:23 PM, Sumit Bose wrote:


Some unit-test where failing for me because of an issue in cleaning up
the secondary slices. 'it = it->next' does not work because it was just
freed in the body of the loop. I fixed it with the change below, but
maybe a do-while loop would be even nicer here?


Sorry to hear that. I fixed that in attached patch set.
Which test failed for you? make check is always passing in my environment.



diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index c4ae811..7ab5c66 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -210,6 +210,7 @@ static void sss_idmap_free_domain(struct
sss_idmap_ctx *ctx,
struct idmap_domain_info *dom)
  {
  struct idmap_range_params *it;
+struct idmap_range_params *next;

  if (ctx == NULL || dom == NULL) {
  return;
@@ -217,7 +218,8 @@ static void sss_idmap_free_domain(struct
sss_idmap_ctx *ctx,

  ctx->free_func(dom->range_params.range_id, ctx->alloc_pvt);
  /* Free list of secondary ranges */
-for (it = dom->sec_ranges; it != NULL; it = it->next) {
+for (it = dom->sec_ranges; it != NULL; it = next) {
+next = it->next;
  ctx->free_func(it->range_id, ctx->alloc_pvt);
  ctx->free_func(it, ctx->alloc_pvt);
  }


While checking and playing with the test I came across

+/* Todo - Add tests */
+/* Add size of primary slice for first_rid of secondary slices. */
+rid += ctx->idmap_opts.rangesize;

in sss_idmap_add_auto_domain_ex(). Does the 'Todo' only refer to 'Add
tests' or to 'Add size of primary slice for first_rid of secondary
slices.' as well? I'm asking because you really have to use the size of
the primary slice and not ctx->idmap_opts.rangesize here because the
primary range is given as a parameter to sss_idmap_add_auto_domain_ex()
and the size might be different then ctx->idmap_opts.rangesize. E.g. in
the unit-test the range is [1234,9876].


I removed the misleading comment about adding tests. I have no intention of 
changing how 'rid' is set currently.



About sss_idmap_add_auto_domain_ex(), I think having auto-generated
secondary ranges with external_mapping==true make no sense. I would
suggest to just create the primary range in this case.


OK, done.



I'll continue reviewing and testing ...


Thanks!
>From e0aa542e1f1329583eb89d166df87185cffd6362 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Thu, 26 Nov 2015 10:46:34 -0500
Subject: [PATCH 1/2] IDMAP: New structure for domain range params

Create new internal structure idmap_range_params by merging ID mapping
range relevant fields from idmap_domain_info and remove corrsponding
fields.

Resolves:
https://fedorahosted.org/sssd/ticket/2188
---
 src/lib/idmap/sss_idmap.c | 110 +++---
 1 file changed, 56 insertions(+), 54 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 4c453120539a549807e9b6bb4db2dc396c1b3152..7a91e419983d59bed6a8ae06f7cfd448e8e18f63 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -33,13 +33,21 @@
 #define SID_FMT "%s-%d"
 #define SID_STR_MAX_LEN 1024
 
+/* Hold all parameters for unix<->sid mapping relevant for
+ * given slice. */
+struct idmap_range_params {
+uint32_t min_id;
+uint32_t max_id;
+char *range_id;
+
+uint32_t first_rid;
+};
+
 struct idmap_domain_info {
 char *name;
 char *sid;
-struct sss_idmap_range *range;
+struct idmap_range_params range_params;
 struct idmap_domain_info *next;
-uint32_t first_rid;
-char *range_id;
 bool external_mapping;
 };
 
@@ -72,37 +80,17 @@ static char *idmap_strdup(struct sss_idmap_ctx *ctx, const char *str)
 return new;
 }
 
-static struct sss_idmap_range *idmap_range_dup(struct sss_idmap_ctx *ctx,
-   struct sss_idmap_range *range)
-{
-struct sss_idmap_range *new = NULL;
-
-CHECK_IDMAP_CTX(ctx, NULL);
-
-
-new = ctx->alloc_func(sizeof(struct sss_idmap_range), ctx->alloc_pvt);
-if (new == NULL) {
-return NULL;
-}
-
-memset(new, 0, sizeof(struct sss_idmap_range));
-
-new->min = range->min;
-new->max = range->max;
-
-return new;
-}
-
-static bool id_is_in_range(uint32_t id, struct idmap_domain_info *dom,
+static bool id_is_in_range(uint32_t id,
+   struct idmap_range_params *rp,
uint32_t *rid)
 {
-if (id == 0 || dom == NULL || dom->range == NULL) {
+if (id == 0 || rp == NULL) {
 return false;
 }
 
-if (id >= dom->range->min && id <= dom->range->max) {
+if (id >= rp->min_id && id <= rp->max_id) {

[SSSD]Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2015-12-09 Thread Pavel Reichl

On 12/08/2015 05:27 PM, Pavel Reichl wrote:

On 12/02/2015 02:18 PM, Pavel Reichl wrote:

Hello,

I decided to share this design document although it still a work in progress. 
Attached patches are just prove of concept and are very much work in progress. 
So far patches also defers from design in order in which secondary slices are 
generated.

Thanks for feedback on this early state of effort.

Bye.

https://fedorahosted.org/sssd/wiki/IdmapAutoAssignNewSlices

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

Hello, attached patches are still work in progress - I mainly addressed Sumit's 
comments focused on:
1) Changing secondary slices from arrays to link list (to make adding new 
slices easy in sid_to_unix()).
2) Added new function sss_idmap_add_auto_domain_ex() which adds secondary 
slices - it's not a good idea to add secondary slices in 
sss_idmap_add_domain_ex() because it's called by IPA which won't use them.
3) Changing sss_idmap_sid_to_unix() to always generate new secondary slice if 
RID is out of scope of currently allocated secondary slices.
4) And other smaller fixes.

Also, option ldap_idmap_extra_slice_max was added and patch was refactored to 
remove code duplication.

I still need to increase code coverage and do more testing.

Bye.

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

I did some testing and amended the patch a little.

You can see diff here:

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index a92066b..c4ae811 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -562,6 +562,7 @@ generate_slice(struct sss_idmap_ctx *ctx, char *slice_name, 
uint32_t first_rid,

 err = sss_idmap_calculate_range(ctx, slice_name, NULL, _range);
 if (err != IDMAP_SUCCESS) {
+ctx->free_func(slice, ctx->alloc_pvt);
 return err;
 }

@@ -718,17 +719,18 @@ sss_idmap_add_auto_domain_ex(struct sss_idmap_ctx *ctx,
 rid += ctx->idmap_opts.rangesize;
 err = get_sec_slices(ctx, domain_name, rid,
  >idmap_domain_info->sec_ranges);
-if (err != IDMAP_SUCCESS) {
+if (err == IDMAP_SUCCESS) {
+ctx->idmap_domain_info->use_sec_ranges = true;
+} else {
 /* Running out of slices for secondary mapping is a non-fatal
  * problem. */
-if (err != IDMAP_OUT_OF_SLICES) {
-return err;
+if (err == IDMAP_OUT_OF_SLICES) {
+err = IDMAP_SUCCESS;
 }
+ctx->idmap_domain_info->use_sec_ranges = false;
 }

-ctx->idmap_domain_info->use_sec_ranges = true;
-
-return IDMAP_SUCCESS;
+return err;
 }

 enum idmap_error_code sss_idmap_add_domain(struct sss_idmap_ctx *ctx,
@@ -874,7 +876,7 @@ enum idmap_error_code sss_idmap_sid_to_unix(struct 
sss_idmap_ctx *ctx,

 /* Try secondary slices */
 if (matched_dom != NULL && matched_dom->use_sec_ranges) {
-struct idmap_range_params *last_slide;
+struct idmap_range_params *last_slide = NULL;
 struct idmap_range_params *new_slice;
 enum idmap_error_code err;

@@ -898,7 +900,11 @@ enum idmap_error_code sss_idmap_sid_to_unix(struct 
sss_idmap_ctx *ctx,
 return err;
 }

-last_slide->next = new_slice;
+if (last_slide == NULL) {
+matched_dom->sec_ranges = new_slice;
+} else {
+last_slide->next = new_slice;
+}

 if (comp_id(new_slice, rid, _id)) {
 return IDMAP_SUCCESS;

>From caf11d143460c14a37a7f112f7e2240789d1b611 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Thu, 26 Nov 2015 10:46:34 -0500
Subject: [PATCH 1/2] IDMAP: New structure for domain range params

Create new internal structure idmap_range_params by merging ID mapping
range relevant fields from idmap_domain_info and remove corrsponding
fields.

Resolves:
https://fedorahosted.org/sssd/ticket/2188
---
 src/lib/idmap/sss_idmap.c | 110 +++---
 1 file changed, 56 insertions(+), 54 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 4c453120539a549807e9b6bb4db2dc396c1b3152..7a91e419983d59bed6a8ae06f7cfd448e8e18f63 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -33,13 +33,21 @@
 #define SID_FMT "%s-%d"
 #define SID_STR_MAX_LEN 1024

+/* Hold all parameters for unix<->sid mapping relevant for
+ * given slice. */
+struct idmap_range_params {
+uint32_t min_id;
+uint32_t max_id;
+char *range_id;
+
+uint32_t first_rid;
+};
+
 struct idmap_domain_info {
 char *name;
 char *sid;
-struct sss_idmap_range *range;
+

[SSSD]Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

2015-12-08 Thread Pavel Reichl


On 12/02/2015 02:18 PM, Pavel Reichl wrote:

Hello,

I decided to share this design document although it still a work in progress. 
Attached patches are just prove of concept and are very much work in progress. 
So far patches also defers from design in order in which secondary slices are 
generated.

Thanks for feedback on this early state of effort.

Bye.

https://fedorahosted.org/sssd/wiki/IdmapAutoAssignNewSlices


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


Hello, attached patches are still work in progress - I mainly addressed Sumit's 
comments focused on:
1) Changing secondary slices from arrays to link list (to make adding new 
slices easy in sid_to_unix()).
2) Added new function sss_idmap_add_auto_domain_ex() which adds secondary 
slices - it's not a good idea to add secondary slices in 
sss_idmap_add_domain_ex() because it's called by IPA which won't use them.
3) Changing sss_idmap_sid_to_unix() to always generate new secondary slice if 
RID is out of scope of currently allocated secondary slices.
4) And other smaller fixes.

Also, option ldap_idmap_extra_slice_max was added and patch was refactored to 
remove code duplication.

I still need to increase code coverage and do more testing.

Bye.
>From caf11d143460c14a37a7f112f7e2240789d1b611 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Thu, 26 Nov 2015 10:46:34 -0500
Subject: [PATCH 1/2] IDMAP: New structure for domain range params

Create new internal structure idmap_range_params by merging ID mapping
range relevant fields from idmap_domain_info and remove corrsponding
fields.

Resolves:
https://fedorahosted.org/sssd/ticket/2188
---
 src/lib/idmap/sss_idmap.c | 110 +++---
 1 file changed, 56 insertions(+), 54 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 4c453120539a549807e9b6bb4db2dc396c1b3152..7a91e419983d59bed6a8ae06f7cfd448e8e18f63 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -33,13 +33,21 @@
 #define SID_FMT "%s-%d"
 #define SID_STR_MAX_LEN 1024
 
+/* Hold all parameters for unix<->sid mapping relevant for
+ * given slice. */
+struct idmap_range_params {
+uint32_t min_id;
+uint32_t max_id;
+char *range_id;
+
+uint32_t first_rid;
+};
+
 struct idmap_domain_info {
 char *name;
 char *sid;
-struct sss_idmap_range *range;
+struct idmap_range_params range_params;
 struct idmap_domain_info *next;
-uint32_t first_rid;
-char *range_id;
 bool external_mapping;
 };
 
@@ -72,37 +80,17 @@ static char *idmap_strdup(struct sss_idmap_ctx *ctx, const char *str)
 return new;
 }
 
-static struct sss_idmap_range *idmap_range_dup(struct sss_idmap_ctx *ctx,
-   struct sss_idmap_range *range)
-{
-struct sss_idmap_range *new = NULL;
-
-CHECK_IDMAP_CTX(ctx, NULL);
-
-
-new = ctx->alloc_func(sizeof(struct sss_idmap_range), ctx->alloc_pvt);
-if (new == NULL) {
-return NULL;
-}
-
-memset(new, 0, sizeof(struct sss_idmap_range));
-
-new->min = range->min;
-new->max = range->max;
-
-return new;
-}
-
-static bool id_is_in_range(uint32_t id, struct idmap_domain_info *dom,
+static bool id_is_in_range(uint32_t id,
+   struct idmap_range_params *rp,
uint32_t *rid)
 {
-if (id == 0 || dom == NULL || dom->range == NULL) {
+if (id == 0 || rp == NULL) {
 return false;
 }
 
-if (id >= dom->range->min && id <= dom->range->max) {
+if (id >= rp->min_id && id <= rp->max_id) {
 if (rid != NULL) {
-*rid = dom->first_rid + (id - dom->range->min);
+*rid = rp->first_rid + (id - rp->min_id);
 }
 
 return true;
@@ -220,8 +208,7 @@ static void sss_idmap_free_domain(struct sss_idmap_ctx *ctx,
 return;
 }
 
-ctx->free_func(dom->range_id, ctx->alloc_pvt);
-ctx->free_func(dom->range, ctx->alloc_pvt);
+ctx->free_func(dom->range_params.range_id, ctx->alloc_pvt);
 ctx->free_func(dom->name, ctx->alloc_pvt);
 ctx->free_func(dom->sid, ctx->alloc_pvt);
 ctx->free_func(dom, ctx->alloc_pvt);
@@ -340,9 +327,12 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
 /* Verify that this slice is not already in use */
 do {
 for (dom = ctx->idmap_domain_info; dom != NULL; dom = dom->next) {
-if ((dom->range->min <= min && dom->range->max >= max) ||
-(dom->range->min >= min && dom->range->min <= max) ||
-(dom-&

[SSSD]Re: [PATCH] NSS: Fix memory leak netgroup

2015-12-07 Thread Pavel Reichl




On 12/07/2015 03:59 PM, Jakub Hrozek wrote:

On Fri, Dec 04, 2015 at 05:18:53PM +0100, Pavel Reichl wrote:



On 12/04/2015 03:37 PM, Jakub Hrozek wrote:

On Thu, Dec 03, 2015 at 03:29:22PM +0100, Pavel Reichl wrote:


On 12/03/2015 02:06 PM, Jakub Hrozek wrote:

On Mon, Nov 30, 2015 at 06:45:00PM +0100, Pavel Reichl wrote:



On 11/30/2015 06:02 PM, Jakub Hrozek wrote:


Wouldn't it then be better to see if another same object is already in
the hashtable and free it before replacing?


I agree it would be best. I tried that before and failed because I could not 
decipher out the relation of talloc contexts.

I tried that again. Seems that leaks are gone. Segfaults were not happening 
during my testing.



Code got even messier :-(


I think the code would look nice if it was placed in the else branch of
create_negcache_netgr() :-)


Done, thanks for hint.



I also don't think we need the tmp_ctx change..


I dare to disagree here. With prev. versions of patch I was experiencing
segfaults. IIRC step_context is being allocated on context of the netgroup
that is freed. I also think that it's not a good idea to allocate local data
on context that does not hold any reference to that data. But it might be
matter of personal taste.


What kind of segfaults, what was the backtrace? I think it might be good to
add talloc_zfree() instead of talloc_free() or explicitly NULL the pointer,
that should solve the issue without adding a new context...

Since it's per-domain, I wonder if step_ctx->dctx might be better
candidate than step_ctx either way.



Hello Jakub, I amended the patch as you proposed. You can now experience the 
segfault yourself - just query a non-existing netgroup.
I attached the backtrace as well.

I can investiage and use talloc reporting if you wish. But still using tmp_ctx 
seems as way of the least effort...


I think you are right, the step_ctx hierarchy is tricky, so the
temporary context looks like the easiest solution. Please do one more
change in the patch:


OK, please see attached patch.




 From 0160ee92c4d9a9542abfd3e918686526a52e367d Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 27 Nov 2015 07:53:00 -0500
Subject: [PATCH] NSS: Fix memory leak netgroup


[...]


@@ -461,6 +469,11 @@ static errno_t create_negcache_netgr(struct 
setent_step_ctx *step_ctx)
  }

  done:
+/* Free netgroup after step_ctx is not needed. */
+if (netgr_to_be_freed) {
+talloc_zfree(netgr_to_be_freed);
+}


Since talloc_free(NULL) is a noop, we should drop the if completely.


OK, done.



While testing the patches I found that sometimes (not always, thought) I
see this valgrind error when a non-existing netgroup is requested:
Mon Dec  7 14:53:15 2015) [sssd[nss]] [accept_fd_handler] (0x0400): Client 
connected!
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_cmd_get_version] (0x0200): Received 
client version [1].
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_cmd_get_version] (0x0200): Offered 
version [1].
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_parse_name_for_domains] (0x0200): 
name 'ngr' matched without domain, user is ngr
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [setnetgrent_send] (0x0100): Requesting info 
for netgroup [ngr] from []
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [lookup_netgr_step] (0x0100): Requesting 
info for [n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [lookup_netgr_step] (0x0040): No results 
for netgroup ngr (domain ipa.test)
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [get_dp_name_and_id] (0x0400): Not a 
LOCAL view, continuing with provided values.
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_dp_issue_request] (0x0400): Issuing 
request for [0x428c3b:4:n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_dp_get_account_msg] (0x0400): 
Creating request for [ipa.test][0x1004][FAST BE_REQ_NETGROUP][1][name=ngr]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_dp_internal_get_send] (0x0400): 
Entering request [0x428c3b:4:n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_dp_get_reply] (0x1000): Got reply 
from Data Provider - DP error code: 0 errno: 0 error message: Success
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [lookup_netgr_step] (0x0100): Requesting 
info for [n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [lookup_netgr_step] (0x0400): Returning 
info for netgroup [n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [sss_dp_req_destructor] (0x0400): 
Deleting request: [0x428c3b:4:n...@ipa.test]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [nss_cmd_getnetgrent] (0x0100): 
Requesting netgroup data
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [nss_cmd_getnetgrent] (0x0400): 
Returning results for [ngr]
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [nss_cmd_getnetgrent_process] (0x0200): 
No entries found
(Mon Dec  7 14:53:15 2015) [sssd[nss]] [client_recv] (0x0200): Client 
disconnected!
(Mon Dec  7 14:53:20 2015) [sssd[nss]] [accept_fd_handler] (0x0400): Client 
connected!
(Mon Dec  7 14:53:20 2015) [sssd[nss]] [sss_cmd_ge

[SSSD]Re: [PATCH] NSS: Fix memory leak netgroup

2015-12-04 Thread Pavel Reichl




On 12/04/2015 03:37 PM, Jakub Hrozek wrote:

On Thu, Dec 03, 2015 at 03:29:22PM +0100, Pavel Reichl wrote:


On 12/03/2015 02:06 PM, Jakub Hrozek wrote:

On Mon, Nov 30, 2015 at 06:45:00PM +0100, Pavel Reichl wrote:



On 11/30/2015 06:02 PM, Jakub Hrozek wrote:


Wouldn't it then be better to see if another same object is already in
the hashtable and free it before replacing?


I agree it would be best. I tried that before and failed because I could not 
decipher out the relation of talloc contexts.

I tried that again. Seems that leaks are gone. Segfaults were not happening 
during my testing.



Code got even messier :-(


I think the code would look nice if it was placed in the else branch of
create_negcache_netgr() :-)


Done, thanks for hint.



I also don't think we need the tmp_ctx change..


I dare to disagree here. With prev. versions of patch I was experiencing
segfaults. IIRC step_context is being allocated on context of the netgroup
that is freed. I also think that it's not a good idea to allocate local data
on context that does not hold any reference to that data. But it might be
matter of personal taste.


What kind of segfaults, what was the backtrace? I think it might be good to
add talloc_zfree() instead of talloc_free() or explicitly NULL the pointer,
that should solve the issue without adding a new context...

Since it's per-domain, I wonder if step_ctx->dctx might be better
candidate than step_ctx either way.



Hello Jakub, I amended the patch as you proposed. You can now experience the 
segfault yourself - just query a non-existing netgroup.
I attached the backtrace as well.

I can investiage and use talloc reporting if you wish. But still using tmp_ctx 
seems as way of the least effort...


I think you are right, the step_ctx hierarchy is tricky, so the
temporary context looks like the easiest solution. Please do one more
change in the patch:


OK, please see attached patch.




 From 0160ee92c4d9a9542abfd3e918686526a52e367d Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 27 Nov 2015 07:53:00 -0500
Subject: [PATCH] NSS: Fix memory leak netgroup


[...]


@@ -461,6 +469,11 @@ static errno_t create_negcache_netgr(struct 
setent_step_ctx *step_ctx)
  }

  done:
+/* Free netgroup after step_ctx is not needed. */
+if (netgr_to_be_freed) {
+talloc_zfree(netgr_to_be_freed);
+}


Since talloc_free(NULL) is a noop, we should drop the if completely.


OK, done.


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

>From 47e73fdd381240b67eba1d7e1de44a589388c503 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 27 Nov 2015 07:53:00 -0500
Subject: [PATCH] NSS: Fix memory leak netgroup

If netgroup cannot be found in setnetgrent_retry() function
set_netgroup_entry() is called which steals getent_ctx directly to
nss_ctx->netgroups.
Subsequently function lookup_netgr_step() is called that (in case of
nenexisting group) will call create_negcache_netgr() which creates
a new dummy object to serve as negative cache. While doing so it calls
again set_netgroup_entry() for the same netgroup and it calls
hash_enter.

hash_enter will remove previously hashed entry for netgroup (created in
setnetgrent_retry()) from hash table but it won't be freed and thus it
leaks. This patch marks such netgroup and freed it after the call of
create_negcache_netgr().

Resolves:
https://fedorahosted.org/sssd/ticket/2865
---
 src/responder/nss/nsssrv_netgroup.c | 22 +++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c
index 9a78c1119c2f4e06e43ebec29ace775adc997e08..4647a20af6b0f8e00c120839bc7e880e89156de6 100644
--- a/src/responder/nss/nsssrv_netgroup.c
+++ b/src/responder/nss/nsssrv_netgroup.c
@@ -434,6 +434,7 @@ static errno_t create_negcache_netgr(struct setent_step_ctx *step_ctx)
 {
 errno_t ret;
 struct getent_ctx *netgr;
+struct getent_ctx *netgr_to_be_freed = NULL;
 
 netgr = talloc_zero(step_ctx->nctx, struct getent_ctx);
 if (netgr == NULL) {
@@ -441,6 +442,13 @@ static errno_t create_negcache_netgr(struct setent_step_ctx *step_ctx)
 ret = ENOMEM;
 goto done;
 } else {
+/* Is there already netgroup with such name? */
+ret = get_netgroup_entry(step_ctx->nctx, step_ctx->name,
+ _to_be_freed);
+if (ret != EOK) {
+netgr_to_be_freed = NULL;
+}
+
 netgr->ready = true;
 netgr->found = false;
 netgr->entries = NULL;
@@ -461,6 +469,8 @@ static errno_t create_negcache_netgr(struct setent_step_ctx *step_ctx)
 }
 
 done:
+talloc_zfree(netgr_to_be_freed);
+
 if (ret != EOK) {
 talloc_free(netgr);
 }
@@ -474,6 +484

[SSSD]Re: [PATCH] NSS: Fix memory leak netgroup

2015-12-03 Thread Pavel Reichl



On 12/03/2015 02:06 PM, Jakub Hrozek wrote:

On Mon, Nov 30, 2015 at 06:45:00PM +0100, Pavel Reichl wrote:



On 11/30/2015 06:02 PM, Jakub Hrozek wrote:


Wouldn't it then be better to see if another same object is already in
the hashtable and free it before replacing?


I agree it would be best. I tried that before and failed because I could not 
decipher out the relation of talloc contexts.

I tried that again. Seems that leaks are gone. Segfaults were not happening 
during my testing.



Code got even messier :-(


I think the code would look nice if it was placed in the else branch of
create_negcache_netgr() :-)


Done, thanks for hint.



I also don't think we need the tmp_ctx change..


I dare to disagree here. With prev. versions of patch I was experiencing
segfaults. IIRC step_context is being allocated on context of the netgroup
that is freed. I also think that it's not a good idea to allocate local data
on context that does not hold any reference to that data. But it might be
matter of personal taste.


What kind of segfaults, what was the backtrace? I think it might be good to
add talloc_zfree() instead of talloc_free() or explicitly NULL the pointer,
that should solve the issue without adding a new context...

Since it's per-domain, I wonder if step_ctx->dctx might be better
candidate than step_ctx either way.



Hello Jakub, I amended the patch as you proposed. You can now experience the 
segfault yourself - just query a non-existing netgroup.
I attached the backtrace as well.

I can investiage and use talloc reporting if you wish. But still using tmp_ctx 
seems as way of the least effort...
>From 0160ee92c4d9a9542abfd3e918686526a52e367d Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 27 Nov 2015 07:53:00 -0500
Subject: [PATCH] NSS: Fix memory leak netgroup

If netgroup cannot be found in setnetgrent_retry() function
set_netgroup_entry() is called which steals getent_ctx directly to
nss_ctx->netgroups.
Subsequently function lookup_netgr_step() is called that (in case of
nenexisting group) will call create_negcache_netgr() which creates
a new dummy object to serve as negative cache. While doing so it calls
again set_netgroup_entry() for the same netgroup and it calls
hash_enter.

hash_enter will remove previously hashed entry for netgroup (created in
setnetgrent_retry()) from hash table but it won't be freed and thus it
leaks. This patch marks such netgroup and freed it after the call of
create_negcache_netgr().

Resolves:
https://fedorahosted.org/sssd/ticket/2865
---
 src/responder/nss/nsssrv_netgroup.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c
index 9a78c1119c2f4e06e43ebec29ace775adc997e08..8b1f807615ee352d60740e8852c4b0f0a6c91b73 100644
--- a/src/responder/nss/nsssrv_netgroup.c
+++ b/src/responder/nss/nsssrv_netgroup.c
@@ -434,6 +434,7 @@ static errno_t create_negcache_netgr(struct setent_step_ctx *step_ctx)
 {
 errno_t ret;
 struct getent_ctx *netgr;
+struct getent_ctx *netgr_to_be_freed = NULL;
 
 netgr = talloc_zero(step_ctx->nctx, struct getent_ctx);
 if (netgr == NULL) {
@@ -441,6 +442,13 @@ static errno_t create_negcache_netgr(struct setent_step_ctx *step_ctx)
 ret = ENOMEM;
 goto done;
 } else {
+/* Is there already netgroup with such name? */
+ret = get_netgroup_entry(step_ctx->nctx, step_ctx->name,
+ _to_be_freed);
+if (ret != EOK) {
+netgr_to_be_freed = NULL;
+}
+
 netgr->ready = true;
 netgr->found = false;
 netgr->entries = NULL;
@@ -461,6 +469,11 @@ static errno_t create_negcache_netgr(struct setent_step_ctx *step_ctx)
 }
 
 done:
+/* Free netgroup after step_ctx is not needed. */
+if (netgr_to_be_freed) {
+talloc_zfree(netgr_to_be_freed);
+}
+
 if (ret != EOK) {
 talloc_free(netgr);
 }
@@ -494,7 +507,8 @@ static errno_t lookup_netgr_step(struct setent_step_ctx *step_ctx)
 /* make sure to update the dctx if we changed domain */
 step_ctx->dctx->domain = dom;
 
-talloc_free(name);
+talloc_zfree(name);
+
 name = sss_get_cased_name(step_ctx, step_ctx->name,
   dom->case_sensitive);
 if (!name) {
@@ -623,10 +637,11 @@ static errno_t lookup_netgr_step(struct setent_step_ctx *step_ctx)
   "create_negcache_netgr failed with: %d:[%s], ignored.\n",
   ret, sss_strerror(ret));
 }
+
 ret = ENOENT;
 
 done:
-talloc_free(name);
+talloc_zfree(name);
 return ret;
 }
 
-- 
2.4.3

Continuing.

Program received signal SIGABRT, Aborted.
0x7fa8b469e9c8 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:55
#0  0x7fa8b469e9c8 in __GI_raise (sig=sig@entry=6) at 
../s

[SSSD][DESIGN] ID mapping - Automatically assign new slices for any AD domain

2015-12-02 Thread Pavel Reichl


Hello,

I decided to share this design document although it still a work in progress. 
Attached patches are just prove of concept and are very much work in progress. 
So far patches also defers from design in order in which secondary slices are 
generated.

Thanks for feedback on this early state of effort.

Bye.

https://fedorahosted.org/sssd/wiki/IdmapAutoAssignNewSlices
>From caf11d143460c14a37a7f112f7e2240789d1b611 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Thu, 26 Nov 2015 10:46:34 -0500
Subject: [PATCH 1/2] IDMAP: New structure for domain range params

Create new internal structure idmap_range_params by merging ID mapping
range relevant fields from idmap_domain_info and remove corrsponding
fields.

Resolves:
https://fedorahosted.org/sssd/ticket/2188
---
 src/lib/idmap/sss_idmap.c | 110 +++---
 1 file changed, 56 insertions(+), 54 deletions(-)

diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 4c453120539a549807e9b6bb4db2dc396c1b3152..7a91e419983d59bed6a8ae06f7cfd448e8e18f63 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -33,13 +33,21 @@
 #define SID_FMT "%s-%d"
 #define SID_STR_MAX_LEN 1024
 
+/* Hold all parameters for unix<->sid mapping relevant for
+ * given slice. */
+struct idmap_range_params {
+uint32_t min_id;
+uint32_t max_id;
+char *range_id;
+
+uint32_t first_rid;
+};
+
 struct idmap_domain_info {
 char *name;
 char *sid;
-struct sss_idmap_range *range;
+struct idmap_range_params range_params;
 struct idmap_domain_info *next;
-uint32_t first_rid;
-char *range_id;
 bool external_mapping;
 };
 
@@ -72,37 +80,17 @@ static char *idmap_strdup(struct sss_idmap_ctx *ctx, const char *str)
 return new;
 }
 
-static struct sss_idmap_range *idmap_range_dup(struct sss_idmap_ctx *ctx,
-   struct sss_idmap_range *range)
-{
-struct sss_idmap_range *new = NULL;
-
-CHECK_IDMAP_CTX(ctx, NULL);
-
-
-new = ctx->alloc_func(sizeof(struct sss_idmap_range), ctx->alloc_pvt);
-if (new == NULL) {
-return NULL;
-}
-
-memset(new, 0, sizeof(struct sss_idmap_range));
-
-new->min = range->min;
-new->max = range->max;
-
-return new;
-}
-
-static bool id_is_in_range(uint32_t id, struct idmap_domain_info *dom,
+static bool id_is_in_range(uint32_t id,
+   struct idmap_range_params *rp,
uint32_t *rid)
 {
-if (id == 0 || dom == NULL || dom->range == NULL) {
+if (id == 0 || rp == NULL) {
 return false;
 }
 
-if (id >= dom->range->min && id <= dom->range->max) {
+if (id >= rp->min_id && id <= rp->max_id) {
 if (rid != NULL) {
-*rid = dom->first_rid + (id - dom->range->min);
+*rid = rp->first_rid + (id - rp->min_id);
 }
 
 return true;
@@ -220,8 +208,7 @@ static void sss_idmap_free_domain(struct sss_idmap_ctx *ctx,
 return;
 }
 
-ctx->free_func(dom->range_id, ctx->alloc_pvt);
-ctx->free_func(dom->range, ctx->alloc_pvt);
+ctx->free_func(dom->range_params.range_id, ctx->alloc_pvt);
 ctx->free_func(dom->name, ctx->alloc_pvt);
 ctx->free_func(dom->sid, ctx->alloc_pvt);
 ctx->free_func(dom, ctx->alloc_pvt);
@@ -340,9 +327,12 @@ enum idmap_error_code sss_idmap_calculate_range(struct sss_idmap_ctx *ctx,
 /* Verify that this slice is not already in use */
 do {
 for (dom = ctx->idmap_domain_info; dom != NULL; dom = dom->next) {
-if ((dom->range->min <= min && dom->range->max >= max) ||
-(dom->range->min >= min && dom->range->min <= max) ||
-(dom->range->max >= min && dom->range->max <= max)) {
+uint32_t dmin = dom->range_params.min_id;
+uint32_t dmax = dom->range_params.max_id;
+
+if ((dmin <= min && dmax >= max) ||
+(dmin >= min && dmin <= max) ||
+(dmax >= min && dmax <= max)) {
 /* This range overlaps one already registered
  * We'll try the next available slot
  */
@@ -447,8 +437,13 @@ enum idmap_error_code sss_idmap_check_collision(struct sss_idmap_ctx *ctx,
 enum idmap_error_code err;
 
 for (dom = ctx->idmap_domain_info; dom != NULL; dom = dom->next) {
-err = sss_idmap_check_collision_ex(dom->name, dom->sid, dom->range,
-   dom->first_rid, dom->range_id,
+struct sss_idmap_range range = { dom->range_para

[SSSD]Re: [PATCH] NSS: Fix memory leak netgroup

2015-11-30 Thread Pavel Reichl




On 11/30/2015 06:02 PM, Jakub Hrozek wrote:


Wouldn't it then be better to see if another same object is already in
the hashtable and free it before replacing?


I agree it would be best. I tried that before and failed because I could not 
decipher out the relation of talloc contexts.

I tried that again. Seems that leaks are gone. Segfaults were not happening 
during my testing.



Code got even messier :-(


I think the code would look nice if it was placed in the else branch of
create_negcache_netgr() :-)


Done, thanks for hint.



I also don't think we need the tmp_ctx change..


I dare to disagree here. With prev. versions of patch I was experiencing segfaults. IIRC step_context is being allocated on context of the netgroup that is freed. I also think that it's not a good idea to allocate local data on context that does not 
hold any reference to that data. But it might be matter of personal taste.



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

>From 8f2d4cdac0f5bee1847d7b13e904e84e7d706841 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 27 Nov 2015 07:53:00 -0500
Subject: [PATCH] NSS: Fix memory leak netgroup

If netgroup cannot be found in setnetgrent_retry() function
set_netgroup_entry() is called which steals getent_ctx directly to
nss_ctx->netgroups.
Subsequently function lookup_netgr_step() is called that (in case of
nenexisting group) will call create_negcache_netgr() which creates
a new dummy object to serve as negative cache. While doing so it calls
again set_netgroup_entry() for the same netgroup and it calls
hash_enter.

hash_enter will remove previously hashed entry for netgroup (created in
setnetgrent_retry()) from hash table but it won't be freed and thus it
leaks. This patch marks such netgroup and freed it after the call of
create_negcache_netgr().

Resolves:
https://fedorahosted.org/sssd/ticket/2865
---
 src/responder/nss/nsssrv_netgroup.c | 21 ++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c
index 9a78c1119c2f4e06e43ebec29ace775adc997e08..27ae1474a70858efb8040b864dd0e21a8bb674de 100644
--- a/src/responder/nss/nsssrv_netgroup.c
+++ b/src/responder/nss/nsssrv_netgroup.c
@@ -434,6 +434,7 @@ static errno_t create_negcache_netgr(struct setent_step_ctx *step_ctx)
 {
 errno_t ret;
 struct getent_ctx *netgr;
+struct getent_ctx *netgr_to_be_freed = NULL;
 
 netgr = talloc_zero(step_ctx->nctx, struct getent_ctx);
 if (netgr == NULL) {
@@ -441,6 +442,11 @@ static errno_t create_negcache_netgr(struct setent_step_ctx *step_ctx)
 ret = ENOMEM;
 goto done;
 } else {
+/* Is there already netgroup with such name? */
+ret = get_netgroup_entry(step_ctx->nctx, step_ctx->name,
+ _to_be_freed);
+if (ret != EOK) netgr_to_be_freed = NULL;
+
 netgr->ready = true;
 netgr->found = false;
 netgr->entries = NULL;
@@ -461,6 +467,9 @@ static errno_t create_negcache_netgr(struct setent_step_ctx *step_ctx)
 }
 
 done:
+/* Free netgroup after step_ctx is not needed. */
+if (netgr_to_be_freed) talloc_zfree(netgr_to_be_freed);
+
 if (ret != EOK) {
 talloc_free(netgr);
 }
@@ -474,6 +483,12 @@ static errno_t lookup_netgr_step(struct setent_step_ctx *step_ctx)
 struct getent_ctx *netgr;
 char *name = NULL;
 uint32_t lifetime;
+TALLOC_CTX *tmp_ctx;
+
+tmp_ctx = talloc_new(NULL);
+if (tmp_ctx == NULL) {
+return ENOMEM;
+}
 
 /* Check each domain for this netgroup name */
 while (dom) {
@@ -494,8 +509,7 @@ static errno_t lookup_netgr_step(struct setent_step_ctx *step_ctx)
 /* make sure to update the dctx if we changed domain */
 step_ctx->dctx->domain = dom;
 
-talloc_free(name);
-name = sss_get_cased_name(step_ctx, step_ctx->name,
+name = sss_get_cased_name(tmp_ctx, step_ctx->name,
   dom->case_sensitive);
 if (!name) {
 DEBUG(SSSDBG_CRIT_FAILURE, "sss_get_cased_name failed\n");
@@ -623,10 +637,11 @@ static errno_t lookup_netgr_step(struct setent_step_ctx *step_ctx)
   "create_negcache_netgr failed with: %d:[%s], ignored.\n",
   ret, sss_strerror(ret));
 }
+
 ret = ENOENT;
 
 done:
-talloc_free(name);
+talloc_free(tmp_ctx);
 return ret;
 }
 
-- 
2.4.3

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

[SSSD]Re: [PATCH] NSS: Fix memory leak netgroup

2015-11-27 Thread Pavel Reichl




On 11/27/2015 10:06 AM, Jakub Hrozek wrote:

On Wed, Nov 18, 2015 at 03:06:03PM +0100, Pavel Reichl wrote:

Hello,

attached patch proposes solution for leaking memory when non-existing netgroup 
is looked up.

1st patch is just for testing - just call 'pkill -SIGUSR1 sssd_nss' and talloc 
report will be generated in /tmp/sssd_nss_talloc_report_full.

For details about the bug please see commit message in 2nd patch.

User who reported the bug confirmed that so far it seems that memory leak has 
been fixed and he didn't report any side effects.

Thanks!



 From abbf720b832beb6d04f909f598e7114a19f72a03 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Wed, 30 Sep 2015 07:54:31 -0400
Subject: [PATCH 1/2] talloc_report for nss responder

---
  src/responder/nss/nsssrv.c | 26 ++
  1 file changed, 26 insertions(+)

diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c
index 
d8eff7968c4929663412aa56d08414689b921a22..79ca9ba89d49a5fd4ee3389e0f881b1a01f9c7a9
 100644
--- a/src/responder/nss/nsssrv.c
+++ b/src/responder/nss/nsssrv.c
@@ -403,6 +403,20 @@ static void nss_dp_reconnect_init(struct sbus_connection 
*conn,
  /* nss_shutdown(rctx); */
  }

+static void signal_nss_talloc_report(struct tevent_context *ev,
+ struct tevent_signal *se,
+ int signum,
+ int count,
+ void *siginfo,
+ void *private_data)
+{
+FILE *f = fopen("/tmp/sssd_nss_talloc_report_full", "w");
+if (f != NULL) {
+talloc_report_full(NULL, f);
+fclose(f);
+}
+}
+
  int nss_process_init(TALLOC_CTX *mem_ctx,
   struct tevent_context *ev,
   struct confdb_ctx *cdb)
@@ -417,6 +431,8 @@ int nss_process_init(TALLOC_CTX *mem_ctx,
  int hret;
  int fd_limit;

+talloc_enable_leak_report_full();
+
  nss_cmds = get_nss_cmds();

  ret = sss_process_init(mem_ctx, ev, cdb,
@@ -558,6 +574,16 @@ int nss_process_init(TALLOC_CTX *mem_ctx,
  goto fail;
  }

+/* Handle SIGUSR1 to force offline behavior */
+BlockSignals(false, SIGUSR1);
+struct tevent_signal *tes;
+tes = tevent_add_signal(rctx->ev, rctx, SIGUSR1, 0,
+signal_nss_talloc_report, rctx);
+if (tes == NULL) {
+ret = EIO;
+goto fail;
+}
+
  DEBUG(SSSDBG_TRACE_FUNC, "NSS Initialization complete\n");

  return EOK;
--
2.4.3




 From 0c05cef940b8a8650537056db4ade09b0b6a6fa6 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 10 Nov 2015 10:56:56 -0500
Subject: [PATCH 2/2] NSS: Fix memory leak netgroup

If netgroup cannot be found in setnetgrent_retry() function
set_netgroup_entry() is called which steals getent_ctx directly to
nss_ctx->netgroups.
Subsequently function lookup_netgr_step() is called that (in case of
nenexisting group) will call create_negcache_netgr() which creates
a new dummy object to serve as negative cache. While doing so it calls
again set_netgroup_entry() for the same netgroup and it calls
hash_enter.

hash_enter will remove previously hashed entry for netgroup (created in
setnetgrent_retry()) from hash table but it won't be freed and thus it
leaks.

This patch sets netgroup lifetime for netgroups allocated in
setnetgrent_retry().


We already set the netgroup lifetime in both the found and notfound case
in lookup_netgr_step(). If I understand the code correctly, the issue is
that we create a new negative result in create_negcache_netgr(),
overwriting the one that we created in setnetgrent_retry(), correct?


Agree.



Wouldn't it then be better to see if another same object is already in
the hashtable and free it before replacing?


I agree it would be best. I tried that before and failed because I could not 
decipher out the relation of talloc contexts.

I tried that again. Seems that leaks are gone. Segfaults were not happening 
during my testing. Code got even messier :-(


>From ec1906631352beb1a9024b4d8d0a5cd53c641418 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 27 Nov 2015 07:53:00 -0500
Subject: [PATCH] NSS: Fix memory leak netgroup

If netgroup cannot be found in setnetgrent_retry() function
set_netgroup_entry() is called which steals getent_ctx directly to
nss_ctx->netgroups.
Subsequently function lookup_netgr_step() is called that (in case of
nenexisting group) will call create_negcache_netgr() which creates
a new dummy object to serve as negative cache. While doing so it calls
again set_netgroup_entry() for the same netgroup and it calls
hash_enter.

hash_enter will remove previously hashed entry for netgroup (created in
setnetgrent_retry()) from hash table but it won't be freed and thus it
leaks. This patch marks such netgroup and freed it

[SSSD]Re: [PATCHES] ldap: skip sdap_save_grpmem() if ignore_group_members is set

2015-11-27 Thread Pavel Reichl




On 11/25/2015 01:16 PM, Sumit Bose wrote:



Hi Pavel,

thank you for the suggestions. I did both changes, although I did the
first a bit differently. I didn't like the separate return as well but
originally was too lazy to rename 'fail' to 'done'.

New version attached.

bye,
Sumit



Thanks, ACK.


 http://sssd-ci.duckdns.org/logs/job/34/05/summary.html
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD]Re: [PATCH] NSS: Fix memory leak netgroup

2015-11-26 Thread Pavel Reichl


Bump.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD]Re: [PATCHES] ldap: skip sdap_save_grpmem() if ignore_group_members is set

2015-11-24 Thread Pavel Reichl




On 11/23/2015 05:35 PM, Pavel Reichl wrote:

On 11/17/2015 11:51 AM, Sumit Bose wrote:


 From 14ad252d2715e5eb1eed139d4497fc0155c51f85 Mon Sep 17 00:00:00 2001
From: Sumit Bose<sb...@redhat.com>
Date: Fri, 13 Nov 2015 12:15:52 +0100
Subject: [PATCH 2/2] initgr: only search for primary group if it is not
  already cached

Related tohttps://fedorahosted.org/sssd/ticket/2868
---
  src/providers/ldap/sdap_async_initgroups.c | 26 +++---
  1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/providers/ldap/sdap_async_initgroups.c 
b/src/providers/ldap/sdap_async_initgroups.c
index 
e9fcb272d3e906555c63d51e0aab90d1f575f3f8..2c25505753c0705649a70bb7ee3ab0dd42d2c944
 100644
--- a/src/providers/ldap/sdap_async_initgroups.c
+++ b/src/providers/ldap/sdap_async_initgroups.c
@@ -3065,6 +3065,7 @@ static void sdap_get_initgr_done(struct tevent_req 
*subreq)
  char *dom_sid_str;
  char *group_sid_str;
  struct sdap_options *opts = state->opts;
+struct ldb_message *msg;

  DEBUG(SSSDBG_TRACE_ALL, "Initgroups done\n");

@@ -3182,14 +3183,25 @@ static void sdap_get_initgr_done(struct tevent_req 
*subreq)
  goto fail;
  }

-subreq = groups_get_send(req, state->ev, state->id_ctx,
- state->id_ctx->opts->sdom, state->conn,
- gid, BE_FILTER_IDNUM, BE_ATTR_ALL, false, false);
-if (!subreq) {
-ret = ENOMEM;
-goto fail;
+ret = sysdb_search_group_by_gid(tmp_ctx, state->dom, primary_gid, NULL,
+);
+if (ret == EOK) {
+DEBUG(SSSDBG_TRACE_FUNC,
+  "Primary group already cached, nothing to do.\n");
+talloc_free(tmp_ctx);

Do you think it would be bad idea to remove this line

+tevent_req_done(req);
+return;

and this?

I think we can reuse 'free' and 'return' that is executed right before 'fail' 
namespace. It would be nice to avoid adding extra return.


+} else {


would you also consider moving


gid = talloc_asprintf(state, "%lu", (unsigned long)primary_gid);
if (gid == NULL) {
ret = ENOMEM;
goto fail;
}

into this branch only?


+subreq = groups_get_send(req, state->ev, state->id_ctx,
+ state->id_ctx->opts->sdom, state->conn,
+ gid, BE_FILTER_IDNUM, BE_ATTR_ALL, false,
+ false);
+if (!subreq) {
+ret = ENOMEM;
+goto fail;
+}
+tevent_req_set_callback(subreq, sdap_get_initgr_pgid, req);
  }
-tevent_req_set_callback(subreq, sdap_get_initgr_pgid, req);

  talloc_free(tmp_ctx);
  return;
-- 2.1.0



Hello I'm just starting to review the patch. I haven't tested the code yet but 
it looks OK to me and CI passed. I have just a little proposal mentioned above.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

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

[SSSD]Re: Replace hexadecimal debug level code in log messages with a text tag

2015-11-24 Thread Pavel Reichl




On 11/24/2015 09:50 AM, Jakub Hrozek wrote:

On Mon, Nov 23, 2015 at 03:00:05PM +0100, Pavel Reichl wrote:



On 11/23/2015 02:53 PM, Lukas Slebodnik wrote:

On (23/11/15 14:37), Pavel Reichl wrote:



On 11/23/2015 10:26 AM, Lukas Slebodnik wrote:
[snip]


(And this is different than the 80-chars limit which still makes sense
for developers who often like to vertically split their editors)


It's not about 80 character limit. We already does not meet this criteria.
Small example:
(Mon Nov 23 10:12:24 2015) [sssd[be[ipa.corp.example.com]]] [server_setup] 
(0x0400): CONFDB: /var/lib/sss/db/config.ldb

Even the 190 columns is not enough for sme messages.
(Mon Nov 23 10:12:24 2015) [sssd[be[ipa.corp.example.com]]] 
[sbus_message_handler] (0x2000): Received SBUS method 
org.freedesktop.sssd.dataprovider.getDomains on path 
/org/freedesktop/sssd/dataprovider


But with string representations it will be extra 20 columns.


Yes, let's drop the number completely.





c) the size of logs will be increased.
"

So... if INFO, ERROR, FATAL and so on are wrong, well. Is there any idea how
to improve readability of logs?


Not the simple one.

Firstly we need to fix point a) ; otherwise we will just confuse users with
any string representation of debug level.


I think it would be enough to make sure that during the most common
operations, there are no logs with failure levels. The difference
between FUNC_DATA and TRACE_FUNC is small even to us, much less to an
admin.

So it might make sense to set clear difference between FUNC_DATA and TRACE_FUNC
(or other debug_levels) and check it as part of review.

Otherwise it does not make a sense to use such variety of debug levels.
Because if difference is small to us (developers) then we can merge them
and simplify things.


Maybe, but neither of this helps admins distinguish what messages to
look for in the logs.


String representation of debug_level will not help either.


I don't agree. For random user the difference between 0x0010 and 0x4000 is
virtually non existing.

But they can use old numbers. Which are mentioned in sssd.conf (1..9).
The higher number means more verbose output. It still
does not solve the problems with maping new debug levels to the semantic
of that debug level. It can only be used just for increasing/decreasing
a verbosity of log files.


But on contrary every code contributor is aware of
significant difference between fatal failure or trace all or previously
between 1 and 10. I support conversion of hexa numbers to strings as a mean
for easier debugging by users.


One more time, The string representation will not help becuase @see point a)
It will just confuse users. So it's better to not confuse users and rather
print hexadecimal number which does not say anything. (but can be efficiently
used for filtering)


It seems that you are only one to think so.


Well, I agree with one Lukas' point that we should convert at least the
errors to something less verbose. Otherwise, check what messages we would
mark as error during a single failed and successfull auth:

# sssd -i -d 3
(Tue Nov 24 08:45:28 2015) [sssd[ifp]] [sbus_signal_handler_got_caller_id] 
(0x0080): Received signal org.freedesktop.DBus.NameAcquired that we are not 
listening to.
(Tue Nov 24 08:45:28 2015) [sssd[ifp]] [sbus_signal_handler_got_caller_id] 
(0x0080): Received signal org.freedesktop.DBus.NameAcquired that we are not 
listening to.
(Tue Nov 24 08:45:29 2015) [sssd[be[ipa.test]]] [be_run_online_cb] (0x0080): 
Going online. Running callbacks.
(Tue Nov 24 08:45:38 2015) [[sssd[krb5_child[6555 [sss_krb5_prompter] 
(0x0020): Cannot handle password prompts.
(Tue Nov 24 08:45:38 2015) [sssd[be[ipa.test]]] [krb5_auth_store_creds] 
(0x0010): unsupported PAM command [249].
(Tue Nov 24 08:45:38 2015) [sssd[be[ipa.test]]] [krb5_auth_store_creds] 
(0x0010): password not available, offline auth may not work.
(Tue Nov 24 08:45:39 2015) [[sssd[krb5_child[6556 [get_and_save_tgt] 
(0x0020): 1229: [-1765328360][Preauthentication failed]
(Tue Nov 24 08:45:39 2015) [[sssd[krb5_child[6556 [map_krb5_error] 
(0x0020): 1298: [-1765328360][Preauthentication failed]
(Tue Nov 24 08:45:46 2015) [[sssd[krb5_child[6561 [sss_krb5_prompter] 
(0x0020): Cannot handle password prompts.
(Tue Nov 24 08:45:46 2015) [sssd[be[ipa.test]]] [krb5_auth_store_creds] 
(0x0010): unsupported PAM command [249].
(Tue Nov 24 08:45:46 2015) [sssd[be[ipa.test]]] [krb5_auth_store_creds] 
(0x0010): password not available, offline auth may not work.
(Tue Nov 24 08:45:48 2015) [sssd[be[ipa.test]]] [ipa_hbac_evaluate_rules] 
(0x0080): Access granted by HBAC rule [allow_all]
(Tue Nov 24 08:45:48 2015) [[sssd[selinux_child[6563 [get_seuser] (0x0040): 
SELinux user for admin: unconfined_u
(Tue Nov 24 08:45:48 2015) [[sssd[selinux_child[6563 [get_seuser] (0x0040): 
SELinux range for admin: s0-s0:c0.c1023
(Tue Nov 24 08:45:48 2015) [sssd[nss]] [nss_cmd_getgrgid_search] (0x0080): No 
matching domain

[SSSD]Re: DDNS: Use nsupdate keywords for all transactions

2015-11-23 Thread Pavel Reichl




On 11/23/2015 09:20 AM, Lukas Slebodnik wrote:

On (20/11/15 15:57), Pavel Reichl wrote:

Hello,

please see attached patch.

Thanks!



From 5af54ded9173d336113c65ab1e83dec5d1ca77b5 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Fri, 20 Nov 2015 09:25:11 -0500
Subject: [PATCH] DDNS: Use nsupdate keywords for all transactions

Keywords realm and server might be used in fallback attempt to update
DDNS.

This patch puts them explicitely to the nsupdate transaction.

Resolves:
https://fedorahosted.org/sssd/ticket/2872

The bug in ticket #2872 is cause by bad design document for ticket #2495
https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate


Could you please explain why the design is bad?

Let me quote Petr Spacek's comment from ticket #2872

Let me clarify this: server and realm commands affect nsupdate's behavior. The 
workaround which executes nsupdate binary again for each DNS update transaction 
forces us to start each transaction without hacks (explicit keyword usage) and 
add the keywords again in second attempt for each transaction.


The workaround Petr mentions is https://fedorahosted.org/sssd/ticket/2783 and 
was filled several months after the design was finished.



So it would be good to keep design document in sync.
I could not find review of that desing document on sssd-devel.


https://lists.fedorahosted.org/archives/list/sssd-devel%40lists.fedorahosted.org/thread/WWLJEXNQDGXBZWR6RYEHTHVUFNTBGJHY/#WWLJEXNQDGXBZWR6RYEHTHVUFNTBGJHY


It might be root of the problem. We might also ask bind maintainer
for yet another review.

Fortunatelly, this RFE(and also bug)  is not in rhel7.2.
The patches were pushed only to sssd-1.13.1 but they might be in another rhel.
So it would be good to extend section with how to test.

After the review of desing document we can tcontinue with review of patches.


I agree we could update the document, but I don't agree it's a prerequisite for 
reviewing patches.



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


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

1 2 3 4 5 6 7 8 9 10 >

1 - 100 of 923 matches

Mail list logo