[SSSD] [PATCHES] SUDO Integration

Jan Zeleny jzeleny at redhat.com
Thu Jan 5 07:30:31 UTC 2012


Pavel Březina <pbrezina at 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



More information about the sssd-devel mailing list