On Fri, 2012-01-13 at 10:00 +0100, Pavel Březina wrote:
> 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 :-)
Sorry, I forgot to note in my original email that this addresses
https://fedorahosted.org/sssd/ticket/1138 and I attached patches for
both master and sssd-1-5 (they differ only by the debug level
specifications)
I have not directly tested these patches, but I have asked the original
reporter of the bug to do so and provided him with a scratch build. I
will include the results of those tests when they are available.
Please disregard. I seem to have replied to the wrong email...