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.
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
