[SSSD] [PATCHES] SUDO Integration

Jakub Hrozek jhrozek at redhat.com
Sun Jan 15 15:24:36 UTC 2012


On Fri, Jan 13, 2012 at 02:48:26PM +0100, Pavel Březina wrote:
> 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()



More information about the sssd-devel mailing list