[SSSD] [PATCH] sdap_idmap: properly handle ranges for external mappings

Jakub Hrozek jhrozek at redhat.com
Thu Oct 17 10:47:25 UTC 2013


On Wed, Oct 09, 2013 at 04:08:16PM +0200, Sumit Bose wrote:
> Hi,
> 
> this patch set aims to fix https://fedorahosted.org/sssd/ticket/2105 .
> But since I had to touch the libsss_idmap code for this I found an issue
> here as well, which is fixed by the first two patches.
> 
> bye,
> Sumit

> From 5e9daa3af6b1d5ef178ad8737e98908543aa66eb Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Wed, 9 Oct 2013 15:20:38 +0200
> Subject: [PATCH 1/5] idmap: add internal function to free a domain struct

ACK

> 
> ---
>  src/lib/idmap/sss_idmap.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
> index c7ac0c7..45797f2 100644
> --- a/src/lib/idmap/sss_idmap.c
> +++ b/src/lib/idmap/sss_idmap.c
> @@ -213,6 +213,20 @@ enum idmap_error_code sss_idmap_init(idmap_alloc_func *alloc_func,
>      return IDMAP_SUCCESS;
>  }
>  
> +static void sss_idmap_free_domain(struct sss_idmap_ctx *ctx,
> +                                  struct idmap_domain_info *dom)
> +{
> +    if (ctx == NULL || dom == NULL) {
> +        return;
> +    }
> +
> +    ctx->free_func(dom->range_id, ctx->alloc_pvt);

This is a nice catch and could be a leak with other allocator than
talloc.

> +    ctx->free_func(dom->range, 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);
> +}
> +

> From e7263a273d6f0aa1d4ec83a9f9bcbd32d65e23c8 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Wed, 9 Oct 2013 15:21:48 +0200
> Subject: [PATCH 2/5] idmap: fix a memory leak if a collision is detected

I haven't tested the collision itself, but all paths that use the fail
label have the "err" variable set and cleanup function is called on
failure.

ACK.

> From c2af969630611dc1fc91d86e1d6141746c2cc82b Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Wed, 9 Oct 2013 15:22:53 +0200
> Subject: [PATCH 3/5] idmap: allow ranges with external mapping to overlap
> 
> If POSIX IDs are managed externally e.g. by AD it might be possible that
> the IDs are centrally manages for the whole forest. Hence there might
> not be a single ID range for each member domain in the forest but only a
> single ID range for the whole forest. This means that we have to allow
> collisions if ID ranges in this case.
> 
> Unit tests are added to make sure that the collisions are only allowed
> for external mappings.

ACK, thank you for the unit test.

> From af4ca9eec65e4bae6ed399bf2136c7a8542f2e59 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Wed, 9 Oct 2013 15:30:29 +0200
> Subject: [PATCH 4/5] sdap_idmap: add
>  sdap_idmap_get_configured_external_range()

ACK, looks good and was tested as part of the next patch.


> From 96387421488982ef67dd90e6eb7653c2c9cff04d Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Wed, 9 Oct 2013 15:31:33 +0200
> Subject: [PATCH 5/5] sdap_idmap: properly handle ranges for external mappings
> 
> Currently we relied on the fact that external ID mapping is used as
> default fallback in case of an error and did not properly add subdomains
> with external ID mapping to the idmap library. If debugging is enabled
> this leads to irritating debug messages for every user or group lookup.
> 
> With this patch this subdomains are added to the idmap library.
> 
> Fixes https://fedorahosted.org/sssd/ticket/2105
> ---

The warning is gone when using POSIX attributes. I tested both with and
without ID mapping and both cases still work fine.

There is just a typo (this patch this) that I will fix before pushing.

ACK.



More information about the sssd-devel mailing list