Dne 31.10.2011 18:35, Jakub Hrozek napsal(a):
On 10/10/2011 03:20 PM, Pavel Březina wrote:
> Helpful links:
> - SUDO plugin API
> 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
> 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:
> 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
> [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 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.
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.
Nitpicks - sudo_printf_t in policy_open() should be renamed, we use
underscore for "output" variables.
- the harcoded fields 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.
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 :)