[SSSD] [PATCH] IPA: check ranges for collisions before saving them

Jakub Hrozek jhrozek at redhat.com
Wed Feb 26 14:21:54 UTC 2014


On Wed, Feb 26, 2014 at 02:56:51PM +0100, Jakub Hrozek wrote:
> On Tue, Feb 25, 2014 at 05:29:53PM +0100, Jakub Hrozek wrote:
> > On Tue, Feb 25, 2014 at 05:15:18PM +0100, Sumit Bose wrote:
> > > On Mon, Feb 24, 2014 at 07:12:00PM +0100, Jakub Hrozek wrote:
> > > > On Thu, Feb 20, 2014 at 10:34:44AM +0100, Sumit Bose wrote:
> > > > > On Wed, Feb 19, 2014 at 04:23:55PM +0100, Jakub Hrozek wrote:
> > > > > > On Wed, Feb 12, 2014 at 03:43:51PM +0100, Sumit Bose wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > currently we trust the idrange data we get from the IPA server. But it
> > > > > > > turned out that some checks are missing on the server
> > > > > > > (https://fedorahosted.org/freeipa/ticket/4137) so SSSD should check the
> > > > > > > input before saving the data to the cache.
> > > > > > > 
> > > > > > > The first two patches make some code which already exists available for
> > > > > > > other callers. The third contains the actual functionality.
> > > > > > > 
> > > > > > > bye,
> > > > > > > Sumit
> > > > > > 
> > > > > > Hi, I rebased your patches on top of master, can you check if they look
> > > > > > OK to you and resend if they are fine? I'll continue testing with my
> > > > > > version in the meantime.
> > > > > 
> > > > > sorry I forgot to send a version for master myself. The patches are
> > > > > exactly the same as in my master tree.
> > > > > 
> > > > > Thanks
> > > > > 
> > > > > bye,
> > > > > Sumit
> > > > 
> > > 
> > > ...
> > > 
> > > > >          err = sss_idmap_add_domain_ex(idmap_ctx->map, name, sid, &range,
> > > > > -                                      r->name, rid, external_mapping);
> > > > > -        if (err != IDMAP_SUCCESS && err != IDMAP_COLLISION) {
> > > > > +                                      range_list[c]->name, rid,
> > > > > +                                      external_mapping);
> > > > > +        if (err != IDMAP_SUCCESS
> > > > > +                && allow_collisions && err != IDMAP_COLLISION) {
> > > > 
> > > > I don't think this condition is right, if allow_collisions is false,
> > > > then it never matches.
> > > 
> > > good catch, thank you, fixed.
> > > 
> > > > 
> > > > >              DEBUG(SSSDBG_CRIT_FAILURE, "Could not add range [%s] to ID map\n",
> > > > > -                                        r->name);
> > > > > +                                       range_list[c]->name);
> > > > >              ret = EIO;
> > > > >              goto done;
> > > > >          }
> > > > >      }
> > > > 
> > > > [snip]
> > > > 
> > > > > diff --git a/src/tests/cmocka/test_ipa_idmap.c b/src/tests/cmocka/test_ipa_idmap.c
> > > > 
> > > > [snip]
> > > > 
> > > > > +int main(int argc, const char *argv[])
> > > > > +{
> > > > > +    poptContext pc;
> > > > > +    int opt;
> > > > > +    struct poptOption long_options[] = {
> > > > > +        POPT_AUTOHELP
> > > > > +        SSSD_DEBUG_OPTS
> > > > > +        POPT_TABLEEND
> > > > > +    };
> > > > > +
> > > > > +    const UnitTest tests[] = {
> > > > > +        //unit_test_setup_teardown(test_add_domain,
> > > > > +        //                         test_sss_idmap_setup, test_sss_idmap_teardown),
> > > > 
> > > > It seems that no further patch includes test_add_domain either, did you
> > > > want to remove the commented out code?
> > > > 
> > > 
> > > yes, I removed it and added a new test to make sure the allow_collisions
> > > check above is working as expected.
> > > 
> > > New version for master and 1.11 are attached.
> > > 
> > > bye,
> > > Sumit
> > 
> > Thank you I will do some testing later today, so far I've submitted the
> > patches to Coverity.
> 
> The patches look good to me and work fine.
> 
> ACK to all three.

Pushed to master and sssd-1-11



More information about the sssd-devel mailing list