On Fri, Sep 05, 2014 at 04:40:05PM +0200, Sumit Bose wrote:
On Fri, Sep 05, 2014 at 04:08:56PM +0200, Jakub Hrozek wrote:
> On Fri, Sep 05, 2014 at 04:01:06PM +0200, Sumit Bose wrote:
> > > +errno_t
> > > +ad_gpo_parse_map_options(struct ad_access_ctx *access_ctx)
> > > +{
> > > + char *gpo_default_right_config;
> > > + enum gpo_map_type gpo_default_right;
> > > + errno_t ret;
> > > + int i;
> > > +
> > > + for (i = 0; i < GPO_MAP_NUM_OPTS; i++) {
> > > +
> > > + struct gpo_map_option_entry entry = gpo_map_option_entries[i];
> > > +
> > > + char *entry_config =
dp_opt_get_string(access_ctx->ad_options,
> > > + entry.ad_basic_opt);
> > > +
> > > + ret = ad_gpo_parse_map_option(access_ctx, entry.gpo_map_type,
> > > +
access_ctx->gpo_map_options_table,
> > > + entry_config,
entry.gpo_map_defaults);
> >
> > ret is not checked here. As a result an invalid configuration where a
> > service is added to two different maps is not detected. I would suggest
> > to terminate SSSD here because the configuration is invalid and it is
> > not clear to the admin to which login right the service is added.
>
> Thank you for the review. Do you think we need to terminate the back
> end? Could we get away with denying all access so at least identity
> provider works?
That sounds like a good idea, but I'm afraid that this might be much
more work then just terminating. Maybe we can just terminate here and
add a ticket to add a more tolerant solution later?
Sure, this shouldn't block the patch review!