[SSSD] [PATCHES] SUDO Integration

Jan Zelený jzeleny at redhat.com
Tue Jan 3 08:29:06 UTC 2012


> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120103/f6ef04fb/attachment.sig>


More information about the sssd-devel mailing list