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 :)