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 :)
Ack
I've notified Dan that we renamed sss_sudo_get_result() to
sss_sudo_send_recv()