[SSSD] [PATCHES] SUDO: IPA provider

Lukas Slebodnik lslebodn at redhat.com
Fri May 3 16:52:57 UTC 2013


On (03/05/13 14:02), Jakub Hrozek wrote:
>On Mon, Apr 29, 2013 at 11:49:09AM +0200, Lukas Slebodnik wrote:
>> On (28/04/13 21:59), Jakub Hrozek wrote:
>> >On Fri, Apr 26, 2013 at 06:08:19PM +0200, Lukas Slebodnik wrote:
>> >> On (25/04/13 20:27), Jakub Hrozek wrote:
>> >> >On Thu, Apr 25, 2013 at 01:29:49PM +0200, Lukas Slebodnik wrote:
>> >> >> ehlo,
>> >> >> 
>> >> >> I am attaching two patches.
>> >> >> First patch make retrieving host information more reusable. It is a preparation
>> >> >> for easily reusing existing ldap code in SUDO IPA provider in second patch.
>> >> >> 
>> >> >> I am attaching two patches.
>> >> >> First patch make retrieving host information much more reusable. It is
>> >> >> a preparation for easily reusing existing ldap code in SUDO IPA provider
>> >> >> in the second patch.
>> >> >> 
>> >> >> LS
>> >> >
>> >> >Patch 0001: Ack
>> >> >
>> >> >Patch 0002: Could we reduce the code duplication between ipa_sudo_init()
>> >> >and ldap_sudo_init()? Maybe create a function like ldap_sudo_common_init
>> >> >or sdap_sudo_init that would do the common initialization work.
>> >> >
>> >> >The sssd-sudo manpage must be amended to include an IPA example
>> >> >according to this new provider.
>> >> >
>> >> >The ldap_sudo options must be added to the configAPI to the file
>> >> >src/config/etc/sssd.api.d/sssd-ipa.conf - there should be a section
>> >> >called [provider/ipa/sudo] that lists all the ldap_sudo_* options.
>> >> >
>> >> >I haven't tested the patches at all yet. Does the sudo_provider inherit
>> >> >its value from the id_provider? I think it should so that the
>> >> >configuration is as minimal as possible
>> >> 
>> >> Rewritten patch is attached.
>> >> 
>> >> - sdap_sudo_init is called from ipa_sudo_init
>> >>   - In this case, first refactoring patch isn't needed.
>> >> - man pages updated (I am expecting comments here)
>> >> - options added to src/config/etc/
>> >> 
>> >> LS
>> >
>> >Builds fine, the unit tests pass, basic functionality works OK as well
>> >(I only tested the basics, though, logged in as admin and performed sudo),
>> >the options are present in the configAPI. I still have a couple of comments,
>> >but in general, the patch is moving in the right direction.
>> >
>> >> --- a/src/config/SSSDConfigTest.py
>> >> +++ b/src/config/SSSDConfigTest.py
>> >> @@ -715,8 +715,8 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase):
>> >>          domain = SSSDConfig.SSSDDomain('sssd', self.schema)
>> >>  
>> >>          control_provider_dict = {
>> >> -            'ipa': ['id', 'auth', 'access', 'chpass', 'autofs', 'session',
>> >> -                    'hostid', 'subdomains'],
>> >> +            'ipa': ['id', 'auth', 'access', 'chpass', 'sudo', 'autofs',
>> >> +                    'session','hostid', 'subdomains'],
>> >                                ^^ 
>> >                            missing whitespace
>> >
>> 
>> Fixed
>> >>              'ad': ['id', 'auth', 'access', 'chpass'],
>> >>              'local': ['id', 'auth', 'chpass'],
>> >>              'ldap': ['id', 'auth', 'access', 'chpass', 'sudo', 'autofs'],
>> >> +            SSSD has native support of IPA provider for sudo compat tree.
>> >
>> >What about a new paragraph like this:
>> >When the SSSD is configured to use the IPA provider, the sudo provider
>> >is automatically enabled. The sudo search base is configured to use the
>> >compat tree (ou=sudoers,$DC).
>> >
>> 
>> Man pages updated.
>> >> +/*
>> >> + SSSD
>> >> +
>> >> + IPA Provider Initialization functions
>> >> +
>> >> + Authors:
>> >> + Lukas Slebodnik <lslebodn at redhat.com>
>> >> +
>> >> + Copyright (C) 2013 Red Hat
>> >> +
>> >> + This program is free software; you can redistribute it and/or modify
>> >> + it under the terms of the GNU General Public License as published by
>> >> + the Free Software Foundation; either version 3 of the License, or
>> >> + (at your option) any later version.
>> >> +
>> >> + This program is distributed in the hope that it will be useful,
>> >> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> >> + GNU General Public License for more details.
>> >> +
>> >> + You should have received a copy of the GNU General Public License
>> >> + along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> >> + */
>> >
>> >We tend to prefix multi-line comments with a leading "*":
>> >http://freeipa.org/page/Coding_Style#Comments
>> >
>> 
>> I copied src/providers/ipa/ipa_autofs.c to ipa_sudo.c
>> Every file starts with the same license comment in directory src/provides/ipa
>> 
>> Do you still think, that it should be fixed?
>> >> +
>> >> +#include "providers/ipa/ipa_common.h"
>> >> +#include "providers/ipa/ipa_id.h"
>> >> +#include "providers/ipa/ipa_auth.h"
>> >> +
>> >> +#include "providers/ldap/sdap_sudo.h"
>> >> +#include "db/sysdb_sudo.h"
>> >> +
>> >> +struct bet_ops ipa_sudo_ops = {
>> >> +    .handler = sdap_sudo_handler,
>> >> +    .finalize = NULL,
>> >> +    .check_online = sdap_check_online
>> >> +};
>> >> +
>> >
>> >The handlers seem wrong to me -- by default, when id_provider=ipa the ops
>> >structure would be initialized from sssm_ipa_id_init and would point to
>> >ipa_id_ops, but if it wasn't, then ipa_sudo init would initialize it to
>> >ipa_sudo_ops with completely different handlers.
>> >
>> >I haven't really been able to trigger the crash due to the talloc_move
>> >(see below), but I guess that if you set id_provider=ldap &&
>> >sudo_provider=ipa, then sssd_be would crash with SIGABRT as the
>> >sdap_sudo_handler would have received ipa_id_ctx but would expect
>> >sdap_id_ctx. I *think* that you'll have to create a wrapper similar to
>> >ipa_check_online that just passes the sdap_id_ctx to the correct
>> >function.
>> >
>> 
>> I tried id_provider=ldap and sudo_provider=ipa, but I was unable to
>> authenticate or receive informations with command getent.
>> 
>> >[snip]
>> >
>> >> +    *ops = &ipa_sudo_ops;
>> >> +
>> >> +    ipa_options = id_ctx->ipa_options;
>> >> +    ldap_options = id_ctx->sdap_id_ctx->opts;
>> >> +
>> >> +    ipa_options->id->sudorule_map = talloc_move(ipa_options->id,
>> >> +                                                &ldap_options->sudorule_map);
>> >
>> >I don't quite understand the talloc_move? It seems to break more
>> >esoteric configurations (id_provider=ldap, sudo_provider=ipa) and in
>> >general do you expect the ipa_id_ctx to outlive the ldap_options? In
>> >that case, let's do a deep-copy and not worry about performance, the
>> >startup is done just once and there are much more expensive operations
>> >than duplicating a memory array (like reading the keytab from disk and
>> >searching for a principal there).
>> 
>> I think that shallow copy is good enough.
>> 
>> Patch attached
>> 
>> LS
>
>We should remove ipa_sudo_ops and simply use sdap_ops now.
>
>sdap_check_online wouldn't work anyway because it expects sdap_id_ctx
>and would be passed in sdap_sudo_ctx.
>
>When reviewing the patch I realized that the .finalize handler is never
>called from back ends. We should investigate that:
>https://fedorahosted.org/sssd/ticket/1904
>
>There are also two whitespace issues in ipa_sudo_init.
>
>Functionally the patch works good.

Thank you for review.

Patch is attached.

LS
-------------- next part --------------
>From a2af3cbb59b46bb76abfd3878f7bdef76dada5be Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Wed, 24 Apr 2013 20:26:40 +0200
Subject: [PATCH] SUDO: IPA provider

This patch added auto configuration SUDO with ipa provider and compat tree.

https://fedorahosted.org/sssd/ticket/1733
---
 Makefile.am                             |  5 +++
 src/config/SSSDConfigTest.py            |  4 +--
 src/config/etc/sssd.api.d/sssd-ipa.conf | 21 +++++++++++++
 src/man/sssd-sudo.5.xml                 | 30 ++----------------
 src/providers/ipa/ipa_common.h          |  5 +++
 src/providers/ipa/ipa_init.c            | 24 ++++++++++++++
 src/providers/ipa/ipa_sudo.c            | 55 +++++++++++++++++++++++++++++++++
 7 files changed, 115 insertions(+), 29 deletions(-)
 create mode 100644 src/providers/ipa/ipa_sudo.c

diff --git a/Makefile.am b/Makefile.am
index c6dfcb2c75c9d03197f3d55ce135de05ab8feef8..a4f3ee9cac3f062665c7f00f3fb09fd42f7dd729 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1547,6 +1547,11 @@ libsss_ipa_la_SOURCES += \
     src/providers/ipa/ipa_autofs.c
 endif
 
+if BUILD_SUDO
+libsss_ipa_la_SOURCES += \
+    src/providers/ipa/ipa_sudo.c
+endif
+
 if BUILD_SSH
 libsss_ipa_la_SOURCES += src/providers/ipa/ipa_hostid.c
 endif
diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
index 6ae458b1a438ff0bb7b2e10765b1fff3f5494163..9c0e34e6482f0106156f24d164c8cf3f52fe3fa7 100755
--- a/src/config/SSSDConfigTest.py
+++ b/src/config/SSSDConfigTest.py
@@ -715,8 +715,8 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase):
         domain = SSSDConfig.SSSDDomain('sssd', self.schema)
 
         control_provider_dict = {
-            'ipa': ['id', 'auth', 'access', 'chpass', 'autofs', 'session',
-                    'hostid', 'subdomains'],
+            'ipa': ['id', 'auth', 'access', 'chpass', 'sudo', 'autofs',
+                    'session', 'hostid', 'subdomains'],
             'ad': ['id', 'auth', 'access', 'chpass'],
             'local': ['id', 'auth', 'chpass'],
             'ldap': ['id', 'auth', 'access', 'chpass', 'sudo', 'autofs'],
diff --git a/src/config/etc/sssd.api.d/sssd-ipa.conf b/src/config/etc/sssd.api.d/sssd-ipa.conf
index e88e32b1a2c522aac89559657ec2dc3609193bb5..e6f1bb0a88dd72a5f7bfcb269fa4736e7efd947c 100644
--- a/src/config/etc/sssd.api.d/sssd-ipa.conf
+++ b/src/config/etc/sssd.api.d/sssd-ipa.conf
@@ -194,3 +194,24 @@ ipa_selinux_usermap_uuid = str, None, false
 
 [provider/ipa/subdomains]
 ipa_subdomains_search_base = str, None, false
+
+[provider/ipa/sudo]
+ldap_sudo_search_base = str, None, false
+ldap_sudo_full_refresh_interval = int, None, false
+ldap_sudo_smart_refresh_interval = int, None, false
+ldap_sudo_use_host_filter = bool, None, false
+ldap_sudo_hostnames = str, None, false
+ldap_sudo_ip = str, None, false
+ldap_sudo_include_netgroups = bool, None, false
+ldap_sudo_include_regexp = bool, None, false
+ldap_sudorule_object_class = str, None, false
+ldap_sudorule_name = str, None, false
+ldap_sudorule_command = str, None, false
+ldap_sudorule_host = str, None, false
+ldap_sudorule_user = str, None, false
+ldap_sudorule_option = str, None, false
+ldap_sudorule_runasuser = str, None, false
+ldap_sudorule_runasgroup = str, None, false
+ldap_sudorule_notbefore = str, None, false
+ldap_sudorule_notafter = str, None, false
+ldap_sudorule_order = str, None, false
diff --git a/src/man/sssd-sudo.5.xml b/src/man/sssd-sudo.5.xml
index fec81533d9351c5ed8711c1649fe49a3939f8448..361fdb7b210df280cffce8c9147257bd0b3ecacb 100644
--- a/src/man/sssd-sudo.5.xml
+++ b/src/man/sssd-sudo.5.xml
@@ -89,33 +89,9 @@ ldap_sudo_search_base = ou=sudoers,dc=example,dc=com
 </programlisting>
         </para>
         <para>
-            The following example illustrates setting up SSSD to download
-            sudo rules from an IPA server. It is necessary to use the LDAP
-            provider and set appropriate connection parameters to authenticate
-            correctly against the IPA server, because SSSD does not have native
-            support of IPA provider for sudo yet.
-        </para>
-        <para>
-<programlisting>
-[sssd]
-config_file_version = 2
-services = nss, pam, sudo
-domains = EXAMPLE
-
-[domain/EXAMPLE]
-id_provider = ipa
-ipa_domain = example.com
-ipa_server = ipa.example.com
-ldap_tls_cacert = /etc/ipa/ca.crt
-
-sudo_provider = ldap
-ldap_uri = ldap://ipa.example.com
-ldap_sudo_search_base = ou=sudoers,dc=example,dc=com
-ldap_sasl_mech = GSSAPI
-ldap_sasl_authid = host/hostname.example.com
-ldap_sasl_realm = EXAMPLE.COM
-krb5_server = ipa.example.com
-</programlisting>
+            When the SSSD is configured to use the IPA provider, the sudo
+            provider is automatically enabled. The sudo search base
+            is configured to use the compat tree (ou=sudoers,$DC).
         </para>
     </refsect1>
 
diff --git a/src/providers/ipa/ipa_common.h b/src/providers/ipa/ipa_common.h
index ae1c91731e9637c06dad555d1a87ef2e0cf53184..d5c10a518a822060d1989b62f30544ee01c13dd0 100644
--- a/src/providers/ipa/ipa_common.h
+++ b/src/providers/ipa/ipa_common.h
@@ -180,4 +180,9 @@ int ipa_service_init(TALLOC_CTX *memctx, struct be_ctx *ctx,
                      struct ipa_options *options,
                      struct ipa_service **_service);
 
+int ipa_sudo_init(struct be_ctx *be_ctx,
+                  struct ipa_id_ctx *id_ctx,
+                  struct bet_ops **ops,
+                  void **pvt_data);
+
 #endif /* _IPA_COMMON_H_ */
diff --git a/src/providers/ipa/ipa_init.c b/src/providers/ipa/ipa_init.c
index b65a6cea1ddd05272a8ae522be5c8b070a93ac1e..0e9fe0dd185e7b237bf4cda077e872dfd0d07fe2 100644
--- a/src/providers/ipa/ipa_init.c
+++ b/src/providers/ipa/ipa_init.c
@@ -526,3 +526,27 @@ int sssm_ipa_subdomains_init(struct be_ctx *bectx,
 
     return EOK;
 }
+
+int sssm_ipa_sudo_init(struct be_ctx *bectx,
+                       struct bet_ops **ops,
+                       void **pvt_data)
+{
+#ifdef BUILD_SUDO
+    struct ipa_id_ctx *id_ctx;
+    int ret;
+
+    DEBUG(SSSDBG_TRACE_INTERNAL, ("Initializing IPA sudo handler\n"));
+
+    ret = sssm_ipa_id_init(bectx, ops, (void **) &id_ctx);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("sssm_ipa_id_init failed.\n"));
+        return ret;
+    }
+
+    return ipa_sudo_init(bectx, id_ctx, ops, pvt_data);
+#else
+    DEBUG(SSSDBG_MINOR_FAILURE, ("Sudo init handler called but SSSD is "
+                                 "built without sudo support, ignoring\n"));
+    return EOK;
+#endif
+}
diff --git a/src/providers/ipa/ipa_sudo.c b/src/providers/ipa/ipa_sudo.c
new file mode 100644
index 0000000000000000000000000000000000000000..726b1168583ff8e12009ad3dd25578ab85ced7d1
--- /dev/null
+++ b/src/providers/ipa/ipa_sudo.c
@@ -0,0 +1,55 @@
+/*
+    SSSD
+
+    IPA Provider Initialization functions
+
+    Authors:
+        Lukas Slebodnik <lslebodn at redhat.com>
+
+    Copyright (C) 2013 Red Hat
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 3 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include "providers/ipa/ipa_common.h"
+#include "providers/ldap/sdap_sudo.h"
+
+int ipa_sudo_init(struct be_ctx *be_ctx,
+                  struct ipa_id_ctx *id_ctx,
+                  struct bet_ops **ops,
+                  void **pvt_data)
+{
+    int ret;
+    struct ipa_options *ipa_options;
+    struct sdap_options *ldap_options;
+
+    DEBUG(SSSDBG_TRACE_INTERNAL, ("Initializing sudo IPA back end\n"));
+
+    /*
+     * SDAP_SUDO_SEARCH_BASE has already been initialized in
+     * function ipa_get_id_options
+     */
+    ret = sdap_sudo_init(be_ctx, id_ctx->sdap_id_ctx, ops, pvt_data);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("Cannot initialize LDAP SUDO [%d]: %s\n",
+                                  ret, strerror(ret)));
+        return ret;
+    }
+
+    ipa_options = id_ctx->ipa_options;
+    ldap_options = id_ctx->sdap_id_ctx->opts;
+
+    ipa_options->id->sudorule_map = ldap_options->sudorule_map;
+    return EOK;
+}
-- 
1.8.1.4



More information about the sssd-devel mailing list