[SSSD] [PATCHES] support local overrides

Pavel Březina pbrezina at redhat.com
Mon Jul 27 10:04:33 UTC 2015


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.

> 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.

>
> 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.

> - 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...




More information about the sssd-devel mailing list