> 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/SUDOIntegrationNewApproac > > > 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. Jan
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
