[SSSD] [PATCHES] SUDO Integration

Pavel Březina pbrezina at redhat.com
Fri Jan 13 13:48:26 UTC 2012


Dne 13.1.2012 10:00, Pavel Březina napsal(a):
> Dne 12.1.2012 12:42, Pavel Březina napsal(a):
>> 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
>
> I forgot to put sudo_provider option to sssd.api.conf... I hope this is
> finally everything :-)

While working on another ticket I noticed missing attribute in attr[] in 
sysdb_purge_sudorule_subtree(). New review patch is attached and I swear 
I won't send another one till this one is acked or nacked :)

-------------- 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/20120113/7f08e011/attachment.ksh>


More information about the sssd-devel mailing list