[SSSD] [PATCHES] SUDO integration - first iteration

Pavel Březina pbrezina at redhat.com
Tue Nov 1 13:39:36 UTC 2011


Dne 31.10.2011 18:35, Jakub Hrozek napsal(a):
> On 10/10/2011 03:20 PM, Pavel Březina wrote:
>> https://fedorahosted.org/sssd/ticket/623
>>
>> Helpful links:
>>   - SUDO plugin API
>>     http://www.gratisoft.us/sudo/man/1.8.1/sudo_plugin.man.html
>>
>> What does it do currently?
>>   1. sudo plugin sends input data to responder
>>   2. responder sends back to plugin that user is allowed to run the
>>      command
>>
>> What does it not do?
>>   - Doesn't require any PAM authentication
>>   - Doesn't read anything from LDAP
>>   - Doesn't set any environment variables (and reset current environment)
>>     This actually can cause some troubles to applications, so far I've
>>     encountered an error message in VIM (no $HOME specified)
>>
>> How to test it?
>>   1. Install SUDO version 1.8 or greater
>>      I am running 1.8.2 built from source:
>>        ftp://ftp.sudo.ws/pub/sudo/sudo-1.8.2.tar.gz
>>
>>   2. Enable SUDO plugin in /etc/sudo.conf
>>      Plugin sss_sudo_policy /usr/lib/sudo/libsss_sudoplugin.so
>>
>>      Unfortunately, SUDO doesn't allow to have more than one policy
>>      plugin activated, so comment out standard sudoers plugin.
>>
>>   3. Enable it in sssd.conf ;-)
>>      services += sudo
>>
>>   4. Run sssd
>>
>>   5. Run sudo
>>
>> I left there some debug messages, so don't worry when you see:
>>    CMD Return code: 0
>>    errnop: 0
>>    Command exited with status 0
>>
>> in the output. But tell me if there is anything else than 0 :-)
>>
>
> In overall - good work! Pavel also has follow-up patches, so he wasn't
> idling in the meantime between him sending out this patchset and us
> getting it reviewed. Sorry for the delay, Pavel.

Thank you for your review.

>
> As sudo does not seem to have a bug tracker upstream and the Red Hat
> sudo maintainer is working with us, I'm going to open one generic "SSSD
> Sudo Feature tracker" bug in bugzilla.redhat.com for Fedora rawhide sudo
> and then one bug per feature we'd like to see in sudo blocking this
> tracker bug.
>
>> [PATCH 1/5]
>> Adds a client function that allows client to send data to responder.
>> And set sudo responder protocol version.
>>
>
> Ack but please consider squashing together with the responder part. Does
> this patch make sense standalone?

Most of the patches doesn't make sense to be standalone :)
It is actually used by plugin not the responder and I've put it in a 
separate patch so the review could be easier.

>
>> [PATCH 2/5]
>> SUDO plugin.
>>
>
> Please move private declarations (such as struct plugin_state) out of
> the header.

Done.

>
> The patch includes a fallback to sudoers plugin in case this a user is
> not found. Currently this is handled inside the sudoers plugin based on
> the order of "sudo" parameter in nsswitch.conf. The plugin order should
> be made more generic in sudo upstream, but short-term we may want to
> support some kind of fallback ourselves.
>
> There's a TODO in the code we discussed off-list:
> "TODO: Do we need to check major and minor version or major is enough?"
> We need to ask sudo upstream on the specifics of versioning.
>
> We couldn't agree on #defining macros such as APPEND_ELEMENT or
> APPEND_ZERO inside a function and #undefining them later. I know that it
> would be nice to use a nested function for the task if C allowed them,
> but I don't like neither defining macros inside a function nor macros
> silently changing local variables. Other opinions welcome.
>
> Casting pointers such as "result = *((int*)response);" should be changed
> to use our safe memory alignment macros to avoid reintroducing #390.

Done.

>
> get_qualified_command_path() should be enhanced in order to be
> compatible with sudo's find_path(). Long-term sudo should export a
> function to perform the path-lookup so plugins don't have to define
> their own function for the task.

Done.

>
> Nitpicks - sudo_printf_t in policy_open() should be renamed, we use
> underscore for "output" variables.

Done.

> - the harcoded fields[4] should be changed to use a constant. Same
> applies for looping over the fields.

Changed to NULL terminated array.

>
>> [PATCH 3/5]
>> SUDO responder.
>>
>
> Nested macros again. Same comment as for patch #2.
>
> If you want to keep functions that will be implemented in a future patch
> in this one (as opposed to simply adding them when they're available),
> please define a simple skeleton that would just return EOK.

I haven't decided yet if it is necessary (haven't think of it though).

>
>> [PATCH 4/5]
>> Configure and Makefile updates so we can build plugin and responder.
>>
>
> Ack, but this patch might be squashed together into the plugin and
> responder patches respectively, but I don't really have a strong opinion.
>
>> [PATCH 5/5]
>> Just a little update that adds sudo responder to known services of monitor.
>>
>>
>
> Ack, but this could be squashed into the responder itself.
>

Done.

I've attach some more patches currently indexed 5-8. It is a skeleton of 
data provider part. Their names should be self-explanatory.
The last one (SUDO responder - refresh SUDOers) is just a DP call for 
testing purpose, it will be changed of course :)
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-SUDO-integration-client-common-interface.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111101/7c3486dc/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-SUDO-integration-SUDO-plugin.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111101/7c3486dc/attachment-0001.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0003-SUDO-integration-SUDO-responder.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111101/7c3486dc/attachment-0002.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0004-SUDO-integration-configure-and-makefile-updated.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111101/7c3486dc/attachment-0003.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0005-SUDO-integration-Data-provider-backend-handler.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111101/7c3486dc/attachment-0004.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0006-SUDO-integration-Responder-Data-provider-communicati.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111101/7c3486dc/attachment-0005.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0007-SUDO-integration-SUDO-LDAP-provider.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111101/7c3486dc/attachment-0006.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0008-SUDO-Integration-SUDO-responder-refresh-SUDOers.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111101/7c3486dc/attachment-0007.ksh>


More information about the sssd-devel mailing list