[SSSD] [PATCHES] SUDO Integration

Jan Zelený jzeleny at redhat.com
Wed Dec 21 15:20:09 UTC 2011


> 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/SUDOIntegrationNewApproach
> 
> 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

#0002:
I'm not an autotools expert, but don't lines 34-36 mean the same as line 33?

#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.

Nitpick: in the for on line 231 I'd add spaces around the = char

line 359: I don't think it is necessary to end purging the tree. I think 
continuing is better action in this case.

line 408: shouldn't the SUDORULE_SUBDIR be something like cn=sudorules rather 
than just sudorules?

#0004:
Nitpick: line 166: I'd move the condition to the following block starting with 
if (!dbus_ret)
line 199: really EOK?
line 270: is there a reason why we have one-member struct? Is there a plan for 
the future with this?

#0005:
line 33: really SDAP_NETGROUP_SEARCH_BASE?
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
line 131: for the comment to be true, you have to add SDAP_SUDO_SEARCH_BASE to 
search_base_options
lines 148-168: I don't get this logic. Is it forbidden to set sudo search base 
if no ldap_search_base is specified?

#0006:
Ack

#0007:
line 534: is the TODO still valid?
line 461: in the NSS responder, there is a check for NULL termination of the 
string. Can something similar happen here as well?

responder patches:
Is there a plan to implement negative cache like in NSS responder? At least 
for the sudosrv_get_user part. 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.

sudosrv_dp.c: 92 - missing dbus_message_unref() in the error handling block

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.

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.

That's all folks
Sorry if I was too strict or nit-picky
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/20111221/d34cbcff/attachment.sig>


More information about the sssd-devel mailing list