[SSSD] [PATCHES] SUDO Integration

Pavel Březina pbrezina at redhat.com
Wed Jan 4 11:35:32 UTC 2012


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

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.

> Jan
>
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list