[SSSD] [PATCHES] IPA: add trusted domains with missing idrange

Jakub Hrozek jhrozek at redhat.com
Fri Oct 25 13:37:20 UTC 2013


On Fri, Oct 25, 2013 at 02:51:47PM +0200, Jakub Hrozek wrote:
> On Fri, Oct 25, 2013 at 12:06:25PM +0200, Sumit Bose wrote:
> > On Fri, Oct 25, 2013 at 11:21:28AM +0200, Jakub Hrozek wrote:
> > > On Thu, Oct 24, 2013 at 02:14:59PM +0200, Sumit Bose wrote:
> > > > Hi,
> > > > 
> > > > this patch set tries to fix https://fedorahosted.org/sssd/ticket/2101 .
> > > > Currently we rely on the fact that the external mapping is the default
> > > > in the case of an error. But this leads to a loot of useless CPU cycles
> > > > and maybe irritating error messages. With this patch set each trusted AD
> > > > member domain gets a proper range assigned if the forest root is
> > > > configured to manage the POSIX attributes.
> > > > 
> > > > bye,
> > > > Sumit
> > > 
> > > Hi,
> > > 
> > > I only have two minor comments, see below:
> > > 
> > > > From e1a86771137f67e0e8d24c4bbb014bfc21d85538 Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose <sbose at redhat.com>
> > > > Date: Wed, 23 Oct 2013 14:39:55 +0200
> > > > Subject: [PATCH 1/4] find_subdomain_by_sid: skip domains with missing
> > > >  domain_id
> > > 
> > > ACK. Thanks for the unit test.
> > > 
> > > > From dc0115802904c954c79d90cd846da9f5bd965019 Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose <sbose at redhat.com>
> > > > Date: Thu, 24 Oct 2013 11:44:11 +0200
> > > > Subject: [PATCH 2/4] idmap: add
> > > >  sss_idmap_domain_by_name_has_algorithmic_mapping()
> > > 
> > > The code itself is good, but we need to bump the version number. See the
> > > attached patch, can you squash it in if you agree?
> > 
> > There was no patch but I changed version-info to 2:0:2. Is this what you
> > would suggest?
> > 
> > > 
> > > > From dffdd7cc99025cb8790f34f79bdf294762f3468d Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose <sbose at redhat.com>
> > > > Date: Thu, 24 Oct 2013 11:45:57 +0200
> > > > Subject: [PATCH 3/4] sdap_idmap_domain_has_algorithmic_mapping: add domain
> > > >  name argument
> > > 
> > > ACK
> > > 
> > > > From 3e7283a39994ac24a9ff9dfede0d735cc89b4bb0 Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose <sbose at redhat.com>
> > > > Date: Wed, 23 Oct 2013 13:30:57 +0200
> > > > Subject: [PATCH 4/4] IPA: add trusted domains with missing idrange
> > > > 
> > > > If the forest root of a trusted forest is managing POSIX IDs for its
> > > > users and groups the same is assumed for all member domains in the
> > > > forest which do not have explicitly have an idrange set.
> > > > 
> > > > To reflect this SSSD will create the matching ranges automatically.
> > > 
> > > One nitpick:
> > > 
> > > > +    tmp_ctx = talloc_new(NULL);
> > > > +    if (tmp_ctx == NULL) {
> > > > +        DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n"));
> > > > +        return ENOMEM;
> > > > +    }
> > > > +
> > > > +    for (c = 0; c < range_count; c++) {
> > > > +        r = range_list[c];
> > > > +        if (r->trusted_dom_sid != NULL
> > > > +                && strcmp(r->trusted_dom_sid, forest_root->domain_id) == 0) {
> > > > +
> > > > +            if (r->range_type == NULL
> > > > +                    || strcmp(r->range_type, IPA_RANGE_AD_TRUST_POSIX) != 0) {
> > > > +                DEBUG(SSSDBG_MINOR_FAILURE,
> > > > +                      ("Forest root does not have range type [%s].\n",
> > > > +                       IPA_RANGE_AD_TRUST_POSIX));
> > > 
> > > we would leak tmp_ctx here, we should goto done instead.
> > 
> > good catch, fixed.
> > 
> > > 
> > > > +                return EINVAL;
> > > > +            }
> > > > +
> > > > +            range.min = r->base_id;
> > > > +            range.max = r->base_id + r->id_range_size -1;
> > > > +            range_id = talloc_asprintf(tmp_ctx, "%s-%s", dom_sid_str, r->name);
> > > > +            if (range_id == NULL) {
> > > > +                DEBUG(SSSDBG_OP_FAILURE, ("talloc_asprintf failed.\n"));
> > > > +                ret = ENOMEM;
> > > > +                goto done;
> > > > +            }
> > > > +
> > > > +            err = sss_idmap_add_domain_ex(idmap_ctx->map, dom_name, dom_sid_str,
> > > > +                                          &range, range_id, 0, true);
> > > > +            if (err != IDMAP_SUCCESS && err != IDMAP_COLLISION) {
> > > > +                DEBUG(SSSDBG_CRIT_FAILURE, ("Could not add range [%s] to ID map\n",
> > > > +                                            range_id));
> > > 
> > > If you're changing the patch, can you also reflow this line? It's past
> > > 80 characters and my editor highlights the line..
> > 
> > fixed.
> > 
> > Thank you for the review, new versions attached.
> > 
> > bye,
> > Sumit
> 
> ACK to these patches.

Pushed to master and sssd-1-11



More information about the sssd-devel mailing list