[SSSD] [PATCHES] SUDO Integration

Pavel Březina pbrezina at redhat.com
Thu Jan 12 11:42:57 UTC 2012


Dne 12.1.2012 12:17, Pavel Březina napsal(a):
> Dne 5.1.2012 08:30, Jan Zeleny napsal(a):
>>>>>> #0001:
>>>>>> line 105: talloc_array -> talloc
>>>>>
>>>>> talloc_array is better choice there...we want to allocate an array of
>>>>> "struct sysdb_attrs pointers"
>>>>
>>>> I know, I wrote a wrong line number. I meant line 40 (in the DEBUG
>>>> message). Sorry for the confusion.
>>>>
>>>>>> #0003:
>>>>>> I'm not sure about the NULL_CHECK macro. I don't condemn it but if I
>>>>>> choose to accept it, I'll want it to be used everywhere it's
>>>>>> possible.
>>>>>> At least in the scope if this patch.
>>>>>
>>>>> That was the intent but I forgot.
>>>>>
>>>>>> Nitpick: in the for on line 231 I'd add spaces around the = char
>>>>>
>>>>> OK
>>>>>
>>>>>> line 359: I don't think it is necessary to end purging the tree. I
>>>>>> think continuing is better action in this case.
>>>>>
>>>>> I was trying to be defensive and avoid ending up with rules that
>>>>> are no
>>>>> longer present on the server which might grant access to users that
>>>>> are
>>>>> not supposed to be in sudoers anymore..
>>>>
>>>> Exactly my point. If you get out of purging cycle on the first rule
>>>> that
>>>> doesn't have a name, you can potentially skip many more rules.
>>>>
>>>>>> #0004:
>>>>>> Nitpick: line 166: I'd move the condition to the following block
>>>>>> starting with if (!dbus_ret)
>>>>>
>>>>> Yes, it's supposed to be there.
>>>>>
>>>>>> line 199: really EOK?
>>>>>
>>>>> Thanks, it's supposed to say 'ret'
>
> Actually there should be EOK. At least the other be handlers returns EOK
> in any case.
>
> If it returns value different than EOK it will send some generic message
> back to the responder. This way I can specify format of the message.
>
>>>>>> #0005:
>>>>>> line 33: really SDAP_NETGROUP_SEARCH_BASE?
>>>>>
>>>>> That should say SDAP_SUDO_SEARCH_BASE. This code is commented out with
>>>>> #if 0, though, until we provide support for IPA sudo rules.
>>>>
>>>> I know, but for the sake of reducing possibility to overlook it in the
>>>> future, I'd prefer to fix it now.
>>>>
>>>>>> line 109: I find this misleading - the map is called native but
>>>>>> several
>>>>>> lines above, there is a statement that native sudo rules are not
>>>>>> supported
>>>>>
>>>>> Right, that's a mismatch between "native SUDO rules" and "native IPA
>>>>> SUDO rules".
>>>>>
>>>>>> line 131: for the comment to be true, you have to add
>>>>>> SDAP_SUDO_SEARCH_BASE to search_base_options
>>>>>
>>>>> Correct.
>
> The comment is invalid, we load sudo search base in
> ldap_get_sudo_options().
>
>>>>>> #0007:
>>>>>> line 534: is the TODO still valid?
>>>>>
>>>>> No. I love removing TODOs :)
>
> I used the first patch from "SUDO Integration - periodical update thread".
>
>>>>>> line 461: in the NSS responder, there is a check for NULL termination
>>>>>> of the string. Can something similar happen here as well?
>>>>>
>>>>> According to the current sudo sources, it can't, but better safe than
>>>>> sorry.
>
> If you mean line 461 in #0008 then I believe it is done in
> sudosrv_get_sudorules_parse_query().
>
>>>>>> sudosrv_dp.c: 92 - missing dbus_message_unref() in the error handling
>>>>>> block
>>>>>
>>>>> Right. We should fix it until we reuse the tevent_req DP requests in
>>>>> that code.
>>>>>
>>>>>> client patches:
>>>>>> First of all I'd like to have sss_sudo_get_result() renamed. The name
>>>>>> is quite confusing considering that it doesn't only fetch the result,
>>>>>> but it also makes the request. I suggest using something like
>>>>>> sss_sudo_get_rules(), that would make things much more readable.
>>>>>
>>>>> ok, let's rename it to sss_sudo_send_recv()
>>>>>
>>>>>> in sss_sudo_parse_rule: I don't think it's necessary to start
>>>>>> variables
>>>>>> with underscore if you don't have their local variable counterparts.
>>>>>> But that's just a readability suggestion.
>>>>>>
>>>>>> I'm not entirely sure about this newxt issue, but when you send
>>>>>> response to the client, you use SAFEALIGN_SET_UINT32 macro, but when
>>>>>> receiving the variable you don't use anything like that. I'm a bit
>>>>>> concerned that this might cause some issues on exotic architectures.
>>>>>
>>>>> sorry, but where in the code is the issue? Plain casting pointer
>>>>> dereference would be bad on architectures that don't align on word
>>>>> boundary.
>>>>
>>>> Well, it's not quite like that, memcpy is used. See
>>>> sss_sudo_parse_uint32(). I think that's the only place that hit me.
>>>
>>> We want the client API to be as small as possible because it will be
>>> moved to sudo. SAFEALIGN_* macros are defined in util.h so I decided to
>>> use memcpy() directly.
>>
>> Ok, please disregard my previous comment. I suppose that SAFEALIGN_*
>> macros
>> won't be subjects to change in the foreseeable future. Please consider at
>> least a comment in the code that this way is safe because it is the
>> same what
>> SAFEALIGN_* does.
>
> Thank you for the review.
>
> Patch addressing these issues is attached.
> It also includes:
> - sudo configuration options added to SSSDConfig.py
> - attrs[] in sysdb_sudo_purge_subdir() is now terminated with NULL
> - in sudosrv_get_sudorules_parse_query() I use strnlen() instead of
> strlen() to check validity of the input
>
> Ticket for this patch is https://fedorahosted.org/sssd/ticket/1125

I'm sending it one more time with the options included in 
config/etc/sssd.api.d/sssd-ldap.conf

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-SUDO-Integration-review-issues.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120112/2f2fa028/attachment.ksh>


More information about the sssd-devel mailing list