[SSSD] [PATCHES] SUDO Integration

Stephen Gallagher sgallagh at redhat.com
Fri Jan 13 15:04:12 UTC 2012


On Fri, 2012-01-13 at 09:54 -0500, Stephen Gallagher wrote:
> 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...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120113/0cc83f4f/attachment.sig>


More information about the sssd-devel mailing list