[SSSD] [PATCH] Add ldap_autofs_map_master_name option

Jakub Hrozek jhrozek at redhat.com
Tue Jun 18 09:15:12 UTC 2013


On Tue, Jun 18, 2013 at 10:29:36AM +0200, Lukas Slebodnik wrote:
> On (17/06/13 19:47), Jakub Hrozek wrote:
> >On Mon, Jun 17, 2013 at 05:28:17PM +0200, Lukas Slebodnik wrote:
> >> On (03/06/13 22:41), Jakub Hrozek wrote:
> >> >On Sat, Jun 01, 2013 at 12:30:47AM -0700, C. S. wrote:
> >> >> Hi Jakub,
> >> >> 
> >> >> Enclosed is the squashed patch.
> >> >> 
> >> >> No worries, re-reading the thread I think I missed a few things early on.
> >> >> You were suggesting adding an attribute to handle an alias on the master
> >> >> map, which is why rfc2307_autofs_mobject_map and rfc2307bis_autofs_entry_map
> >> >> would needed to have been updated(?).
> >> >> 
> >> >
> >> >Sorry, I can be very confusing sometimes. I was just pondering two
> >> >different options -- one was modifying the default_basic_opts[] array
> >> >which you did, the other was modifying the mobject and entry maps, which
> >> >I realized was the worse option.
> >> >
> >> >In the latest patch, there are still additions to
> >> >rfc2307_autofs_mobject_map and rfc2307bis_autofs_entry_map which should
> >> >go away. Can you test the attached patch? It is your patch with those
> >> >two extra lines removed. If it works for you, I'll run the patch by
> >> >another engineer to be sure (I won't be able to push patch with my
> >> >additions myself then) and push.
> >> >
> >> >The modified patch is attached.
> >> >
> >> >> Also, automounter works fine with different master map names, we use it in
> >> >> production without issue at least. It was my understanding that sssd needs
> >> >> to know the master map name so that it can expire it from the cache
> >> >> properly on updates, is that correct? So in sssd.conf you also have
> >> >> ldap_autofs_map_master_name = auto.slave, right?
> >> >> 
> >> >
> >> >Yes, correct. I modified both /etc/sysconfig/autofs and sssd.conf to
> >> >include the same master map called auto.slave.
> >> >
> >> >> Thanks again for your patience, much appreciated. ;-)
> >> >> 
> >> >
> >> >Thank you very much for the patch. Once the two additions to mobject and
> >> >mentry are removed, I'll simply push the patch. I didn't mean to confuse
> >> >you, sorry :)
> >> >
> >> >> 
> >> >> 
> >> >> 
> >> >> On Thu, May 30, 2013 at 9:44 AM, Jakub Hrozek <jhrozek at redhat.com> wrote:
> >> >> 
> >> >> > On Tue, May 28, 2013 at 04:32:00PM -0700, C. S. wrote:
> >> >> > > (Including once more as an attachment, it looks like something may have
> >> >> > > wrapped the lines when I had the patch in the body of the message.)
> >> >> > >
> >> >> > >
> >> >> > > On Tue, May 28, 2013 at 4:29 PM, C. S. <csleclish at gmail.com> wrote:
> >> >> > >
> >> >> > > > Hi Jakub,
> >> >> > > >
> >> >> > > > Thanks for the feedback! Very helpful. Looks like I dropped the LDAP
> >> >> > map
> >> >> > > > changes during a merge, that is now included. And I've removed the
> >> >> > > > strdup(), whoops! ;-)
> >> >> > > >
> >> >> > > > cs
> >> >> > > >
> >> >> >
> >> >> > Hi Cove,
> >> >> >
> >> >> > this patch is working for me with a couple of additonal changes that I
> >> >> > attach as a mini-patch. If you like, you can squash it into yours and
> >> >> > resend, then I think the patch would be good to go.
> >> >> >
> >> >> > Basically I think it's enough to have the master map name defined in the
> >> >> > LDAP options, not the map and entry option lists. Also the option lists
> >> >> > need the objectclass to be the first entry in the map.
> >> >> >
> >> >> > Sorry, I think I confused you with my previous e-mail.
> >> >> >
> >> >> > With the attached patch on top of yours, I was able to run automounter
> >> >> > with a custom master map (MASTER_MAP_NAME="auto.slave" in
> >> >> > /etc/sysconfig/autofs) just fine.
> >> >> >
> >> >> > Thanks again for the contribution.
> >> >> >
> >> >> > _______________________________________________
> >> >> > sssd-devel mailing list
> >> >> > sssd-devel at lists.fedorahosted.org
> >> >> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >> >> >
> >> >> >
> >> >
> >> >
> >> >> _______________________________________________
> >> >> sssd-devel mailing list
> >> >> sssd-devel at lists.fedorahosted.org
> >> >> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >> >
> >> 
> >> I would like to apploligize for late answer, but, I had few problems with
> >> autofs configuration.
> >> 
> >> >From e36d14827fed18e6a633c004996606f6f0b01509 Mon Sep 17 00:00:00 2001
> >> >From: Cove Schneider <cove at ilm.com>
> >> >Date: Fri, 31 May 2013 23:56:48 -0700
> >> >Subject: [PATCH] Add ldap_autofs_map_master_name option
> >> >
> >> >---
> >> > src/config/etc/sssd.api.d/sssd-ipa.conf  |  1 +
> >> > src/config/etc/sssd.api.d/sssd-ldap.conf |  1 +
> >> > src/db/sysdb_autofs.h                    |  5 +++--
> >> > src/man/sssd-ldap.5.xml                  | 13 +++++++++++++
> >> > src/providers/ad/ad_opts.h               |  1 +
> >> > src/providers/data_provider_be.c         |  7 -------
> >> > src/providers/ipa/ipa_opts.h             |  1 +
> >> > src/providers/ldap/ldap_common.c         | 15 +++++++++++++++
> >> > src/providers/ldap/ldap_common.h         |  2 ++
> >> > src/providers/ldap/ldap_opts.h           |  1 +
> >> > src/providers/ldap/sdap.h                |  1 +
> >> > src/providers/ldap/sdap_autofs.c         | 12 ++++++++++++
> >> > 12 files changed, 51 insertions(+), 9 deletions(-)
> >> >
> >> >diff --git a/src/config/etc/sssd.api.d/sssd-ipa.conf b/src/config/etc/sssd.api.d/sssd-ipa.conf
> >> >index e6f1bb0a88dd72a5f7bfcb269fa4736e7efd947c..5105a40a629d87a62a21cb6933a758943c1688cc 100644
> >> >--- a/src/config/etc/sssd.api.d/sssd-ipa.conf
> >> >+++ b/src/config/etc/sssd.api.d/sssd-ipa.conf
> >> >@@ -162,6 +162,7 @@ ipa_hostgroup_uuid = str, None, false
> >> > 
> >> > [provider/ipa/autofs]
> >> > ipa_automount_location = str, None, false
> >> >+ldap_autofs_map_master_name = str, None, false
> >> > ldap_autofs_map_object_class = str, None, false
> >> > ldap_autofs_map_name = str, None, false
> >> > ldap_autofs_entry_object_class = str, None, false
> >> >diff --git a/src/config/etc/sssd.api.d/sssd-ldap.conf b/src/config/etc/sssd.api.d/sssd-ldap.conf
> >> >index 14e979da3413fe575e83b9a508c6cd8029d2bfba..aa521567f3f657885ba8978fdd118174b0ff9b4f 100644
> >> >--- a/src/config/etc/sssd.api.d/sssd-ldap.conf
> >> >+++ b/src/config/etc/sssd.api.d/sssd-ldap.conf
> >> >@@ -153,6 +153,7 @@ ldap_sudorule_notafter = str, None, false
> >> > ldap_sudorule_order = str, None, false
> >> > 
> >> > [provider/ldap/autofs]
> >> >+ldap_autofs_map_master_name = str, None, false
> >> > ldap_autofs_map_object_class = str, None, false
> >> > ldap_autofs_map_name = str, None, false
> >> > ldap_autofs_entry_object_class = str, None, false
> >> >diff --git a/src/db/sysdb_autofs.h b/src/db/sysdb_autofs.h
> >> >index e3528ce4e6bd2c8b594c80828cfb02a9a7ce7923..97edaea03179d89d37073df41ca95cd544a53a29 100644
> >> >--- a/src/db/sysdb_autofs.h
> >> >+++ b/src/db/sysdb_autofs.h
> >> >@@ -28,8 +28,9 @@
> >> > #define AUTOFS_MAP_SUBDIR   "autofsmaps"
> >> > #define AUTOFS_ENTRY_SUBDIR "autofsentries"
> >> > 
> >> >-#define SYSDB_AUTOFS_MAP_OC     "automountMap"
> >> >-#define SYSDB_AUTOFS_MAP_NAME   "automountMapName"
> >> >+#define SYSDB_AUTOFS_MAP_MASTER_NAME   "automountMapMasterName"
> >> >+#define SYSDB_AUTOFS_MAP_OC            "automountMap"
> >> >+#define SYSDB_AUTOFS_MAP_NAME          "automountMapName"
> >> > 
> >> > #define SYSDB_AUTOFS_ENTRY_OC     "automount"
> >> > #define SYSDB_AUTOFS_ENTRY_KEY    "automountKey"
> >> >diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml
> >> >index 37df5ec1b2e0b150b407f5e3e39db21860055580..b06286742577b84c347701ab0cd56c4e661a44c8 100644
> >> >--- a/src/man/sssd-ldap.5.xml
> >> >+++ b/src/man/sssd-ldap.5.xml
> >> >@@ -2169,6 +2169,19 @@ ldap_access_filter = memberOf=cn=allowedusers,ou=Groups,dc=example,dc=com
> >> >         <para>
> >> >             <variablelist>
> >> >                 <varlistentry>
> >> >+                    <term>ldap_autofs_map_master_name (string)</term>
> >> >+                    <listitem>
> >> >+                        <para>
> >> >+                            The name of the automount master map in LDAP.
> >> >+                        </para>
> >> >+                        <para>
> >> >+                            Default: auto.master
> >> >+                        </para>
> >> >+                    </listitem>
> >> >+                </varlistentry>
> >> >+            </variablelist>
> >> >+            <variablelist>
> >> >+                <varlistentry>
> >> >                     <term>ldap_autofs_map_object_class (string)</term>
> >> >                     <listitem>
> >> >                         <para>
> >> >diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h
> >> >index 218614dca82ab1b5a2d54291ead3d9719a4c7e2a..8ce0c475265a8602edea72ecdff18f77f8ecab75 100644
> >> >--- a/src/providers/ad/ad_opts.h
> >> >+++ b/src/providers/ad/ad_opts.h
> >> >@@ -65,6 +65,7 @@ struct dp_option ad_def_ldap_opts[] = {
> >> >     { "ldap_sudo_include_netgroups", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
> >> >     { "ldap_sudo_include_regexp", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
> >> >     { "ldap_autofs_search_base", DP_OPT_STRING, NULL_STRING, NULL_STRING },
> >> >+    { "ldap_autofs_map_master_name", DP_OPT_STRING, NULL_STRING, NULL_STRING },
> >> >     { "ldap_schema", DP_OPT_STRING, { "ad" }, NULL_STRING },
> >> >     { "ldap_offline_timeout", DP_OPT_NUMBER, { .number = 60 }, NULL_NUMBER },
> >> >     { "ldap_force_upper_case_realm", DP_OPT_BOOL, BOOL_TRUE, BOOL_FALSE },
> >> >diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
> >> >index cd6715680eb4871806cc5148cecd2e118885a105..11e8979c2afe4bbcad19ef77b241843463e02c58 100644
> >> >--- a/src/providers/data_provider_be.c
> >> >+++ b/src/providers/data_provider_be.c
> >> >@@ -1643,13 +1643,6 @@ static int be_autofs_handler(DBusMessage *message, struct sbus_connection *conn)
> >> >         goto done;
> >> >     }
> >> > 
> >> >-    /* If a request for auto.master comes in, the automounter deamon
> >> >-     * has been reloaded. Expire all autofs maps to force reload
> >> >-     */
> >> >-    if (strcmp(be_autofs_req->mapname, "auto.master") == 0) {
> >> >-        be_autofs_req->invalidate = true;
> >> >-    }
> >> >-
> >> >     be_req->req_data = be_autofs_req;
> >> > 
> >> >     if (!be_cli->bectx->bet_info[BET_AUTOFS].bet_ops) {
> >> >diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h
> >> >index 4dfa72db430b1d3dfb528cc425c7156baf5ebe47..f8a3bdda0d8b2cc66d40b73dec2aab3b9e65b5d9 100644
> >> >--- a/src/providers/ipa/ipa_opts.h
> >> >+++ b/src/providers/ipa/ipa_opts.h
> >> >@@ -89,6 +89,7 @@ struct dp_option ipa_def_ldap_opts[] = {
> >> >     { "ldap_sudo_include_netgroups", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
> >> >     { "ldap_sudo_include_regexp", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
> >> >     { "ldap_autofs_search_base", DP_OPT_STRING, NULL_STRING, NULL_STRING },
> >> >+    { "ldap_autofs_map_master_name", DP_OPT_STRING, NULL_STRING, NULL_STRING },
> >> >     { "ldap_schema", DP_OPT_STRING, { "ipa_v1" }, NULL_STRING },
> >> >     { "ldap_offline_timeout", DP_OPT_NUMBER, { .number = 60 }, NULL_NUMBER },
> >> >     { "ldap_force_upper_case_realm", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
> >> >diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
> >> >index acb24b190157efe06967963ad05014cf9017db7f..33faf232bdd13c79c90db76b90fea4770a458a6b 100644
> >> >--- a/src/providers/ldap/ldap_common.c
> >> >+++ b/src/providers/ldap/ldap_common.c
> >> >@@ -407,10 +407,25 @@ int ldap_get_autofs_options(TALLOC_CTX *memctx,
> >> >                             struct sdap_options *opts)
> >> > {
> >> >     const char *search_base;
> >> >+    const char *master_map;
> >> >     struct sdap_attr_map *default_entry_map;
> >> >     struct sdap_attr_map *default_mobject_map;
> >> >     int ret;
> >> > 
> >> >+    /* automount master map name */
> >> >+    master_map = dp_opt_get_string(opts->basic, SDAP_AUTOFS_MAP_MASTER_NAME);
> >> >+    if (master_map == NULL) {
> >> >+        ret = dp_opt_set_string(opts->basic, SDAP_AUTOFS_MAP_MASTER_NAME,
> >> >+            AUTOFS_MAP_MASTER_NAME);
> >> >+        if (ret != EOK) {
> >> >+            DEBUG(SSSDBG_OP_FAILURE, ("Could not set autofs master map name"
> >> >+                  " to default value\n"));
> >> >+            return ret;
> >> >+        }
> >> >+        master_map = dp_opt_get_string(opts->basic, SDAP_AUTOFS_MAP_MASTER_NAME);
> >> >+    }
> >> >+    DEBUG(SSSDBG_FUNC_DATA, ("Option ldap_autofs_map_master_name set to %s\n", master_map));
> >> >+
> >> >     /* search base */
> >> >     search_base = dp_opt_get_string(opts->basic, SDAP_SEARCH_BASE);
> >> >     if (search_base != NULL) {
> >> >diff --git a/src/providers/ldap/ldap_common.h b/src/providers/ldap/ldap_common.h
> >> >index 2d17b755808e58831b458bcc86a50eb74b1f1057..ec48db68a457b9cf4e490b649eaefd0573ad8e71 100644
> >> >--- a/src/providers/ldap/ldap_common.h
> >> >+++ b/src/providers/ldap/ldap_common.h
> >> >@@ -39,6 +39,8 @@
> >> > #define LDAP_SSL_URI "ldaps://"
> >> > #define LDAP_LDAPI_URI "ldapi://"
> >> > 
> >> >+#define AUTOFS_MAP_MASTER_NAME "auto.master"
> >> >+
> >> > /* a fd the child process would log into */
> >> > extern int ldap_child_debug_fd;
> >> > 
> >> >diff --git a/src/providers/ldap/ldap_opts.h b/src/providers/ldap/ldap_opts.h
> >> >index 807716c18244ae70759d9b2eff7e40fd3c634fb2..8d0a5a647f068a01c5170f30a7f6865f7ca5e0c7 100644
> >> >--- a/src/providers/ldap/ldap_opts.h
> >> >+++ b/src/providers/ldap/ldap_opts.h
> >> >@@ -56,6 +56,7 @@ struct dp_option default_basic_opts[] = {
> >> >     { "ldap_sudo_include_netgroups", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
> >> >     { "ldap_sudo_include_regexp", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
> >> >     { "ldap_autofs_search_base", DP_OPT_STRING, NULL_STRING, NULL_STRING },
> >> >+    { "ldap_autofs_map_master_name", DP_OPT_STRING, NULL_STRING, NULL_STRING },
> >> >     { "ldap_schema", DP_OPT_STRING, { "rfc2307" }, NULL_STRING },
> >> >     { "ldap_offline_timeout", DP_OPT_NUMBER, { .number = 60 }, NULL_NUMBER },
> >> >     { "ldap_force_upper_case_realm", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
> >> >diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
> >> >index 162250fff76295807cce0779bebdf88577ed755c..7d2d3f534c04f4a21a920286fa17f199277999e9 100644
> >> >--- a/src/providers/ldap/sdap.h
> >> >+++ b/src/providers/ldap/sdap.h
> >> >@@ -164,6 +164,7 @@ enum sdap_basic_opt {
> >> >     SDAP_SUDO_INCLUDE_NETGROUPS,
> >> >     SDAP_SUDO_INCLUDE_REGEXP,
> >> >     SDAP_AUTOFS_SEARCH_BASE,
> >> >+    SDAP_AUTOFS_MAP_MASTER_NAME,
> >> >     SDAP_SCHEMA,
> >> >     SDAP_OFFLINE_TIMEOUT,
> >> >     SDAP_FORCE_UPPER_CASE_REALM,
> >> >diff --git a/src/providers/ldap/sdap_autofs.c b/src/providers/ldap/sdap_autofs.c
> >> >index 0bb211aa3e3ae9ec822e117f8febf8378bb09f79..e4ef7257bd251fdf7cc672c7f96bb67e3bc4ff32 100644
> >> >--- a/src/providers/ldap/sdap_autofs.c
> >> >+++ b/src/providers/ldap/sdap_autofs.c
> >> >@@ -30,6 +30,7 @@
> >> > #include "providers/ldap/sdap.h"
> >> > #include "providers/ldap/sdap_async.h"
> >> > #include "providers/dp_backend.h"
> >> >+#include "providers/data_provider.h"
> >> > #include "db/sysdb_autofs.h"
> >> > #include "util/util.h"
> >> > 
> >> >@@ -82,6 +83,7 @@ void sdap_autofs_handler(struct be_req *be_req)
> >> >     struct sdap_id_ctx *id_ctx;
> >> >     struct be_autofs_req *autofs_req;
> >> >     struct tevent_req *req;
> >> >+    const char *master_map;
> >> >     int ret = EOK;
> >> > 
> >> >     DEBUG(SSSDBG_TRACE_INTERNAL, ("sdap autofs handler called\n"));
> >> >@@ -98,6 +100,16 @@ void sdap_autofs_handler(struct be_req *be_req)
> >> >     DEBUG(SSSDBG_FUNC_DATA, ("Requested refresh for: %s\n",
> >> >           autofs_req->mapname ? autofs_req->mapname : "<ALL>\n"));
> >> > 
> >> >+    if (autofs_req->mapname != NULL) {
> >> >+        master_map = dp_opt_get_string(id_ctx->opts->basic,
> >> >+                                       SDAP_AUTOFS_MAP_MASTER_NAME);
> >>           ^^^^^^^^^^
> >> I think, that function dp_opt_get_string can return NULL.
> >> It would be not good idea to compare NULL with autofs_req->mapname.
> >
> >It shouldn't happen but yeah, famous last words :-)
> >
> >I agree with Lukas, let's be extra paranoid and also add a NULL check
> >here. If we ever get a NULL string, we'll just use stale data, nothing
> >terrible.
> It is not about to be extra paranoid. I hit this bug, but
> I forgot to attach back_trace in my previous mail.
> 
> >
> >Thank you for testing!
> >
> >> 
> >> >+        if (strcmp(master_map, autofs_req->mapname) == 0) {
> >> >+            autofs_req->invalidate = true;
> >> >+            DEBUG(SSSDBG_FUNC_DATA, ("Refresh of automount master map triggered: %s\n",
> >> >+                  autofs_req->mapname));
> >> >+        }
> >> >+    }
> >> >+
> >> >     if (autofs_req->invalidate) {
> >> >         ret = sysdb_invalidate_autofs_maps(id_ctx->be->domain->sysdb,
> >> >                                            id_ctx->be->domain);
> >> >-- 
> >> >1.8.2.1
> 
> I attach back_trace for record.
> LS

How did you get into this situation? I thought that the option defaulted
to "auto.master" even in the map. If not, then I think it should.



More information about the sssd-devel mailing list