Pavel Březina <pbrez...@redhat.com> wrote: > Dne 3.1.2012 09:29, Jan Zelený napsal(a): > >> On Wed, Dec 21, 2011 at 04:20:09PM +0100, Jan Zelený wrote: > >>>> https://fedorahosted.org/sssd/ticket/1105 (review ticket) > >>>> https://fedorahosted.org/sssd/ticket/623 (sudo integration) > >>>> > >>>> Hello, > >>>> it is finally here. > >>>> > >>>> These patches *assume* that "Responders: Split getting domain by name > >>>> into separate function" patch has been applied, it can be found in > >>>> "Ability to set a domain as case sensitive or insensitive" thread. > >>>> > >>>> Design page: > >>>> https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOIntegrationNewApproa > >>>> c h > >>>> > >>>> What's left: > >>>> 1. implementing support of sssd in sudo sudoers plugin > >>>> 2. periodical update of all rules > >>>> 3. provide documentation > >>>> 4. in memory cache in responder > >>>> 5. refactor DP request using "RESPONDER: Refactor DP requests into > >>>> tevent_req style" patch > >>>> 6. provide few more configuration options > >>>> 7. support the IPA scheme > >>>> > >>>> How to configure it: > >>>> 1. configure --enable-all-experimental-features > >>>> > >>>> 2. sssd.conf: > >>>> services += sudo > >>>> domain options: sudo_provider, ldap_sudo_search_base > >>>> > >>>> How to test it: > >>>> > sss_sudo_cli username > >>>> > >>>> This will display rules for %username. It performs two lookups: > >>>> 1. using native sssd api to get and display raw date (it prints every > >>>> byte as char so don't be scared of silly characters that are shown > >>>> instead of numbers) > >>>> 2. using api that we provide to sudo and use it to display data in > >>>> user readable format > >>>> > >>>> Big thanks to Jakub, who has written whole sysdb api and a big part of > >>>> responder. > >>>> > >>>> Regards, > >>>> Pavel Březina. > >>> > >>> Ok, I've spent last couple days reviewing the code. My brief comments > >>> are below. Please let me know if any of those comments needs an > >>> explanation, I realize they are written in super-brief format. > >>> > >>> I'd also like to point out that the code is pretty complex, so it is > >>> very likely that I missed some issues. If anyone else is interested, > >>> feel free to do the review as well. > >>> > >>> If no file is specified, the line number means the line in the patch > >>> itself. > >>> > >>> Pavel, please note that I've not taken your second set of patches into > >>> account, I just didn't have the strength for that, perhaps next year > >>> ;-) > >>> > >>> #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. > > > >>> #0002: > >>> I'm not an autotools expert > >> > >> Me neither :) > >> > >>> , but don't lines 34-36 mean the same as line 33? > >> > >> AM_CONDITIONAL defines an automake conditional for use in Makefile.am, > >> but we also need AC_DEFINE to have a constant #defined in config.h > > > > Ok then, Ack for patch #0002. > > > >>> #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. > > > >>> line 408: shouldn't the SUDORULE_SUBDIR be something like cn=sudorules > >>> rather than just sudorules? > >> > >> No, the SYSDB_TMPL_CUSTOM filter hardcodes cn= > > > > Ok, thanks for clearing that up. > > > >>> #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' > >> > >>> line 270: is there a reason why we have one-member struct? Is there a > >>> plan for the future with this? > >> > >> Not sure, but wrapping the data in a structure provides encapsulation > >> for the future and does no harm. > > > > Ok. In general I have no problem with that, it just caught my eye. > > > >>> #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. > >> > >>> lines 148-168: I don't get this logic. Is it forbidden to set sudo > >>> search base if no ldap_search_base is specified? > >> > >> Yes I think that the logic is backwards. > > > > I was under the impression that ldap_search_base is only a fallback in > > case the more specific search base is not defined. But that impression > > came from IPA provider, now I see that the code in LDAP provider is > > different. Therefore I withdraw my comment. > > > >>> #0006: > >>> Ack > >>> > >>> #0007: > >>> line 534: is the TODO still valid? > >> > >> No. I love removing TODOs :) > >> > >>> 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. > >> > >>> responder patches: > >>> Is there a plan to implement negative cache like in NSS responder? At > >>> least for the sudosrv_get_user part. > >> > >> I was planning to reuse sss_ncache_check_user() > >> > >>> I also wonder if the original NSS responder > >>> routing nss_cmd_getpwnam_search could be utilized somehow instead of > >>> writing its copy. That would solve the negative cache issue. > >> > >> The same routing is used all over the place. With the number of > >> responders and supported maps growing the basic patterns of iterating > >> over domains is used very often. I was thinking about using a more > >> generic function (or perhaps a macro would be better..) to reuse the > >> commonly used code. Maybe something like NSS_GETENT() in nss-pam-ldapd > >> code. > > > > Yes, that was basically my idea. I filed a ticket to track this: > > > > https://fedorahosted.org/sssd/ticket/1126 > > > >>> 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. Thanks Jan _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
