[SSSD] [PATCHES] support local overrides

Jakub Hrozek jhrozek at redhat.com
Mon Jul 27 10:13:32 UTC 2015


On Mon, Jul 27, 2015 at 12:04:33PM +0200, Pavel Březina wrote:
> On 07/26/2015 09:40 PM, Jakub Hrozek wrote:
> >On Sun, Jul 26, 2015 at 08:14:22PM +0200, Jakub Hrozek wrote:
> >>On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> >>>https://fedorahosted.org/sssd/ticket/2584
> >>>
> >>>If you have any idea how to improve manual page, please, share it.
> >>
> >>Hi,
> >>
> >>please see my comments inline.
> >
> >btw the patches work more or less fine -- great work, thanks.
> >
> >The only functional error is the missing check for NULL res value if an
> >entry is found by getpwnam() but not sysdb_getpwnam():
> >
> >[root at client ~]# sss_override user-add root -n toor
> >ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la :
> >/usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header
> >Segmentation fault (core dumped)
> 
> I think it was problem of accessing dom rather than res. Check is there.

Thanks.

> 
> >After testing, I put the patches through Coverity. Here are the errors
> >(some were already mentioned in the review):
> 
> I fixed all of them, hopefully, except:
> 
> >Error: DEADCODE (CWE-561): [#def6]
> >sssd-1.13.1/src/tools/sss_override.c:302: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
> >sssd-1.13.1/src/tools/sss_override.c:331: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
> >sssd-1.13.1/src/tools/sss_override.c:296: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
> >sssd-1.13.1/src/tools/sss_override.c:325: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
> >sssd-1.13.1/src/tools/sss_override.c:324: between: When switching on "type", the value of "type" must be between 0 and 1.
> >sssd-1.13.1/src/tools/sss_override.c:324: dead_error_condition: The switch value "type" cannot reach the default case.
> >sssd-1.13.1/src/tools/sss_override.c:337: dead_error_begin: Execution cannot reach this statement: "default:".
> >#  335|               strtype = "group";
> >#  336|               break;
> >#  337|->         default:
> >#  338|               DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
> >#  339|               ret = ERR_INTERNAL;
> >
> 
> Yes, it is a dead code. But I think it's better to leave it in:
> 1) Without default a compiler will produce warning since not all
> SYSDB_MEMBER* are used.
> 2) Better to have it.
> 
> Do you think it may be better to use all SYSDB_MEMBER* instead of default
> branch? It will however also produce the coverity warning, I think.

I think this is OK and it's better to be paranoid and fail.

Just be prepared there will be a Coverity warning next Monday on the
internal list :-) and when it arrives, mark the Coverity error as a
false positive..

> 
> >
> >Also you forgot to package the new tool and the new man page in the specfile.
> >
> >And finally, some questions/proposals after testing:
> >
> >- Do you think it makes sense to also add -find and -show commands?
> 
> In the future, yes. We may want them.

Please file a ticket. I suspect we'll get a request, so it might be nice
to acknowledge the issue.

> 
> >- I wonder if we should explicitly barf if the user triying to override
> >   UID to 0 ? I did test it and it's not possible, sssd returns the original
> >   UID, which is good, I just wonder about a nicer error message.
> 
> Or a man page change? Or both? It is not in this patchset...

Both would be best, it's not like we're going to change that behaviour,
ever, so once we write these two, they're to stay :-) It doesn't have to
be part of this patchset either.



More information about the sssd-devel mailing list