[SSSD] [PATCHES] SUDO Integration

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


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

-------------- 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/3550f400/attachment.ksh>


More information about the sssd-devel mailing list