[SSSD] [PATCH] sss_override: support import and export

Jakub Hrozek jhrozek at redhat.com
Thu Aug 20 20:44:32 UTC 2015


On Thu, Aug 20, 2015 at 10:39:06PM +0200, Jakub Hrozek wrote:
> On Thu, Aug 20, 2015 at 01:55:45PM +0200, Pavel Březina wrote:
> > On 08/20/2015 12:54 PM, Pavel Březina wrote:
> > >On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
> > >>On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
> > >>>On 08/18/2015 05:24 PM, Pavel Březina wrote:
> > >>>>On 08/16/2015 05:59 PM, Pavel Březina wrote:
> > >>>>>https://fedorahosted.org/sssd/ticket/2737
> > >>>>>
> > >>>>>Hi, it should work... :-) However, I wanted to make import as
> > >>>>>transaction so no changes are made if some error occurs, but I had
> > >>>>>some
> > >>>>>troubles with it so I gave up eventually.
> > >>>>>
> > >>>>>Of course, it would be quite difficult to make it safe across domains
> > >>>>>thus my intention was only to ensure that either all changes are
> > >>>>>done in
> > >>>>>a domain or none. But still leaving the possibility that changes are
> > >>>>>commited in one domain but cancelled in another.
> > >>>>>
> > >>>>>I tried to start sysdb transaction on all used domains and then
> > >>>>>commit/cancel it, writing some neat mechanism for it. However, I
> > >>>>>occasionally run into a problem when data provider hangs when
> > >>>>>trying to
> > >>>>>safe a user. It looked like some sort of race condition.
> > >>>>>
> > >>>>>Unfortunately I managed to delete the code so I can't show it to
> > >>>>>you, I
> > >>>>>think it would be a nice feature so if anyone familiar with ldb
> > >>>>>want to
> > >>>>>step in, he's welcome.
> > >>>>
> > >>>>New patches attached.
> > >>>
> > >>>I split it into more patches.
> > >>>
> > >>
> > >>There is a strange ordering between this patch and the FQDN -- these
> > >>patches can't be compiled without the FQDN one, but the FQDN doesn't
> > >>apply on master.
> > >
> > >Thanks, I fixed that. I'm sending all the patches here including the fqn
> > >one in correct order.
> > >
> > >>There are still some Coverity errors:
> > >>Error: COMPILER_WARNING:
> > >>sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
> > >>'sss_colondb_open'
> > >>sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may
> > >>be used uninitialized in this function [-Wmaybe-uninitialized]
> > >>#     if (fp == NULL && filename != NULL) {
> > >>#            ^
> > >>sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was
> > >>declared here
> > >>#     FILE *fp;
> > >>#           ^
> > >>#  271|       }
> > >>#  272|
> > >>#  273|->     if (fp == NULL && filename != NULL) {
> > >>#  274|           ret = errno;
> > >>#  275|           DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s
> > >>[%d]: %s\n",
> > >>
> > >>Error: NEGATIVE_RETURNS (CWE-394):
> > >>sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
> > >>"sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a
> > >>negative number.
> > >>sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant:
> > >>Explicitly returning negative value "-22".
> > >>sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning:
> > >>unsigned variable "fqlen" = "sss_fqname".
> > >>sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned
> > >>variable "fqlen" is incremented, which might cause an integer overflow.
> > >>#  340|       fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
> > >>#  341|       if (fqlen > 0) {
> > >>#  342|->         fqlen++; /* \0 */
> > >>#  343|       } else {
> > >>#  344|           return NULL;
> > >
> > >Hopefully fixed now.
> > >
> > >>
> > >>> From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
> > >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > >>>Date: Sat, 15 Aug 2015 13:53:25 +0200
> > >>>Subject: [PATCH 1/5] sss_override: print input name if unable to
> > >>>parse it
> > >>
> > >>ACK
> > >>
> > >>
> > >>> From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
> > >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > >>>Date: Wed, 19 Aug 2015 12:34:08 +0200
> > >>>Subject: [PATCH 2/5] TOOLS: add sss_colondb API
> > >>>
> > >>>To simplify import/export users and groups.
> > >>
> > >>More or less ack :-) see some questions inline
> > >>
> > >>>+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
> > >>>+                             struct sss_colondb *db,
> > >>>+                             struct sss_colondb_read_field *table)
> > >>>+{
> > >>>+    int readchars;
> > >>>+    size_t linelen = 0;
> > >>>+    char *line = NULL;
> > >>>+    char *tcline;
> > >>>+    char *rest;
> > >>>+    errno_t ret;
> > >>>+    int i;
> > >>>+
> > >>>+    if (db->mode != SSS_COLONDB_READ) {
> > >>>+        return ERR_INTERNAL;
> > >>>+    }
> > >>>+
> > >>>+    readchars = getline(&line, &linelen, db->file);
> > >>>+    if (readchars == -1) {
> > >>>+        /* Nothing was read. */
> > >>>+        if (errno != 0) {
> > >>>+            ret = errno;
> > >>>+            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
> > >>>+                  ret, sss_strerror(ret));
> > >>>+            return ret;
> > >>>+        }
> > >>>+
> > >>>+        return EOF;
> > >>>+    }
> > >>>+
> > >>>+    /* Copy line to mem_ctx. */
> > >>>+    tcline = talloc_strdup(mem_ctx, line);
> > >>
> > >>Do we need mem_ctx in this function? What about using db (or a special
> > >>memctx
> > >>instead of db) instead, then the caller could just talloc_free_children
> > >>(or we can talloc_free_children even in this function..)
> > >
> > >Yes, it is needed in my opinion. Let us see if we both understand the
> > >purpose here, if not I'll add a comment.
> > >
> > >We read line with getline() which returns a pointer that needs to be
> > >freed. But we read data into read_field table and then use it in the
> > >caller. The problem we need to solve is how to push line pointer to the
> > >caller so it can be freed when data is no longer needed. Is it clear?
> > >
> > >I can think of three solutions:
> > >1) Pass *line as output parameter so the caller can free it.
> > >2) Use talloc and talloc_strdup all strings to mem_ctx.
> > >3) Use talloc but strdup the whole line to mem_ctx.
> > >
> > >I chose 3) since it contains less allocations and it is quite simple and
> > >elegant.
> > >
> > >You can't free data in sss_colondb_readline() because you don't know
> > >when the caller stops using them. And calling talloc_free_children on
> > >encapsulated db context is not safe. Yes, it is doable now since it
> > >doesn't contain any children, but the context structure can be changed
> > >any time in the future.
> > >
> > >Maybe I don't understand what you mean by "special memctx" since that is
> > >what I use there.
> > >
> > >>
> > >>>+
> > >>>+    free(line);
> > >>>+    line = NULL;
> > >>
> > >>> From 0877b501f01dff26a312944a47a98cb88eb47d1e Mon Sep 17 00:00:00 2001
> > >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > >>>Date: Wed, 19 Aug 2015 12:43:15 +0200
> > >>>Subject: [PATCH 3/5] sss_override: decompose code better
> > >>
> > >>ACK
> > >>
> > >>thanks for splitting the patch, makes the review much easier.
> > >>
> > >>> From 1556d7f744302e8e7e3aa4e3459c5a4c7467bbd9 Mon Sep 17 00:00:00 2001
> > >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > >>>Date: Wed, 19 Aug 2015 12:35:12 +0200
> > >>>Subject: [PATCH 4/5] sss_override: support import and export
> > >>>
> > >>>Resolves:
> > >>>https://fedorahosted.org/sssd/ticket/2737
> > >>
> > >>Please ask Dan to add the import and export to his tests.
> > >>
> > >>Please also ask him to test UPN in the first column -- that's what's the
> > >>Dell tool supports.
> > >
> > >UPNs won't work yet, I think. I need to finally recreate my AD
> > >environment before I start working on those.
> > >
> > >>I'm having trouble overriding GIDs, do these work for you? For users,
> > >>all attributes work fine for me, also when I run "sss_override
> > >>group-add".
> > >>
> > >>My importfile looked like this:
> > >># cat importgroup
> > >>sssdgroup40003 at win.trust.test:sssdgroup123:
> > >>sssdgroup40004 at win.trust.test:sssdgroup124:124
> > >>
> > >>
> > >>>---
> > >>>  Makefile.am                |   2 +
> > >>>  src/man/sss_override.8.xml |  88 +++++++
> > >>>  src/tools/sss_override.c   | 596
> > >>>+++++++++++++++++++++++++++++++++++++++++++++
> > >>>  3 files changed, 686 insertions(+)
> > >>>
> > >>>diff --git a/Makefile.am b/Makefile.am
> > >>>index
> > >>>ed107fd5dc76b768176a3d7236b0bf1c75f212bf..f8c7c29df08c2ba9e74508c0f84ac57f6e6bac26
> > >>>100644
> > >>>--- a/Makefile.am
> > >>>+++ b/Makefile.am
> > >>>@@ -651,6 +651,7 @@ dist_noinst_HEADERS = \
> > >>>      src/lib/sifp/sss_sifp_private.h \
> > >>>      src/tests/cmocka/test_utils.h \
> > >>>      src/tools/common/sss_tools.h \
> > >>>+    src/tools/common/sss_colondb.h \
> > >>>      $(NULL)
> > >>>
> > >>>
> > >>>@@ -1331,6 +1332,7 @@ sss_signal_LDADD = \
> > >>>
> > >>>  sss_override_SOURCES = \
> > >>>      src/tools/sss_override.c \
> > >>>+    src/tools/common/sss_colondb.c \
> > >>>      $(SSSD_TOOLS_OBJ) \
> > >>>      $(NULL)
> > >>>  sss_override_LDADD = \
> > >>>diff --git a/src/man/sss_override.8.xml b/src/man/sss_override.8.xml
> > >>>index
> > >>>ec9a7bb75c13f4f18ece7f5f84baede14a8a1e2e..edb77d43f54b3f70d8fdb7260c4c8bc9e6c225d8
> > >>>100644
> > >>>--- a/src/man/sss_override.8.xml
> > >>>+++ b/src/man/sss_override.8.xml
> > >>>@@ -77,6 +77,50 @@
> > >>>              </varlistentry>
> > >>>              <varlistentry>
> > >>>                  <term>
> > >>>+                    <option>user-import</option>
> > >>>+                    <emphasis>FILE</emphasis>
> > >>>+                </term>
> > >>>+                <listitem>
> > >>>+                    <para>
> > >>>+                        Import user overrides from
> > >>><emphasis>FILE</emphasis>.
> > >>>+                        Data format is similar to standard passwd file.
> > >>>+                        The format is:
> > >>>+                    </para>
> > >>>+                    <para>
> > >>>+                        original_name:name:uid:gid:gecos:home:shell
> > >>>+                    </para>
> > >>>+                    <para>
> > >>>+                        where original_name is original name of the
> > >>>user whose
> > >>>+                        attributes should be overridden. The rest of
> > >>>fields
> > >>>+                        correspond to new values. You can omit a
> > >>>value simply
> > >>>+                        by leaving corresponding field empty.
> > >>>+                    </para>
> > >>>+                    <para>
> > >>>+                        Examples:
> > >>>+                    </para>
> > >>>+                    <para>
> > >>>+                        ckent:superman::::::
> > >>>+                    </para>
> > >>>+                    <para>
> > >>>+
> > >>>ckent at krypton.com::501:501:/home/earth:/bin/bash:Superman
> > >>
> > >>Unless having Superman as the shell is part of super-powers, I suspect
> > >>the order of the fields in the example doesn't match :)
> > 
> > Thanks :-)
> > 
> > >>
> > >>>+                    </para>
> > >>>+                </listitem>
> > >>>+            </varlistentry>
> > >>>+            <varlistentry>
> > >>>+                <term>
> > >>>+                    <option>user-export</option>
> > >>>+                    <emphasis>FILE</emphasis>
> > >>>+                </term>
> > >>>+                <listitem>
> > >>>+                    <para>
> > >>>+                        Export all overridden attributes and store
> > >>>them in
> > >>>+                        <emphasis>FILE</emphasis>. See
> > >>>+                        <emphasis>user-import</emphasis> for data
> > >>>format.
> > >>>+                    </para>
> > >>>+                </listitem>
> > >>>+            </varlistentry>
> > >>>+            <varlistentry>
> > >>>+                <term>
> > >>>                      <option>group-add</option>
> > >>>                      <emphasis>NAME</emphasis>
> > >>>                      <optional><option>-n,--name</option>
> > >>>NAME</optional>
> > >>>@@ -99,6 +143,50 @@
> > >>>                      </para>
> > >>>                  </listitem>
> > >>>              </varlistentry>
> > >>>+            <varlistentry>
> > >>>+                <term>
> > >>>+                    <option>group-import</option>
> > >>>+                    <emphasis>FILE</emphasis>
> > >>>+                </term>
> > >>>+                <listitem>
> > >>>+                    <para>
> > >>>+                        Import group overrides from
> > >>><emphasis>FILE</emphasis>.
> > >>>+                        Data format is similar to standard group file.
> > >>>+                        The format is:
> > >>>+                    </para>
> > >>>+                    <para>
> > >>>+                        original_name:name:gid
> > >>>+                    </para>
> > >>>+                    <para>
> > >>>+                        where original_name is original name of the
> > >>>group whose
> > >>>+                        attributes should be overridden. The rest of
> > >>>fields
> > >>>+                        correspond to new values. You can omit a
> > >>>value simply
> > >>>+                        by leaving corresponding field empty.
> > >>>+                    </para>
> > >>>+                    <para>
> > >>>+                        Examples:
> > >>>+                    </para>
> > >>>+                    <para>
> > >>>+                        admins:administrators:
> > >>
> > >>Isn't there supposed to be an empty field?
> > >>     admins:administrators::
> > 
> > No, the format is:
> > 
> > orig_name:new_name:new_gid
> > 
> > therefore there is no colon at the end.
> > 
> > >>
> > >>>+                    </para>
> > >>>+                    <para>
> > >>>+                        Domain Users:Users:501
> > >>>+                    </para>
> > >>>+                </listitem>
> > >>>+            </varlistentry>
> > >>>+            <varlistentry>
> > >>>+                <term>
> > >>>+                    <option>group-export</option>
> > >>>+                    <emphasis>FILE</emphasis>
> > >>>+                </term>
> > >>>+                <listitem>
> > >>>+                    <para>
> > >>>+                        Export all overridden attributes and store
> > >>>them in
> > >>>+                        <emphasis>FILE</emphasis>. See
> > >>>+                        <emphasis>group-import</emphasis> for data
> > >>>format.
> > >>>+                    </para>
> > >>>+                </listitem>
> > >>>+            </varlistentry>
> > >>>          </variablelist>
> > >>>      </refsect1>
> > >>
> > >>[...]
> > >>
> > >>>+static int override_user_import(struct sss_cmdline *cmdline,
> > >>>+                                struct sss_tool_ctx *tool_ctx,
> > >>>+                                void *pvt)
> > >>>+{
> > >>>+    TALLOC_CTX *tmp_ctx;
> > >>>+    struct sss_colondb *db;
> > >>>+    const char *filename;
> > >>>+    struct override_user obj;
> > >>>+    int linenum = 1;
> > >>>+    errno_t ret;
> > >>>+    int exit;
> > >>>+
> > >>>+    tmp_ctx = talloc_new(NULL);
> > >>>+    if (tmp_ctx == NULL) {
> > >>>+        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
> > >>>+        return EXIT_FAILURE;
> > >>>+    }
> > >>>+
> > >>>+    /**
> > >>>+     * Format: orig_name:name:uid:gid:gecos:home:shell
> > >>>+     */
> > >>>+    struct sss_colondb_read_field table[] = {
> > >>>+        {SSS_COLONDB_STRING, {.str = &obj.input_name}},
> > >>>+        {SSS_COLONDB_STRING, {.str = &obj.name}},
> > >>>+        {SSS_COLONDB_UINT32, {.uint32 = &obj.uid}},
> > >>>+        {SSS_COLONDB_UINT32, {.uint32 = &obj.gid}},
> > >>>+        {SSS_COLONDB_STRING, {.str = &obj.gecos}},
> > >>>+        {SSS_COLONDB_STRING, {.str = &obj.home}},
> > >>>+        {SSS_COLONDB_STRING, {.str = &obj.shell}},
> > >>>+        {SSS_COLONDB_SENTINEL, {0}}
> > >>>+    };
> > >>>+
> > >>>+    ret = parse_cmdline_import(cmdline, tool_ctx, &filename);
> > >>>+    if (ret != EOK) {
> > >>>+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
> > >>>+        exit = EXIT_FAILURE;
> > >>>+        goto done;
> > >>>+    }
> > >>>+
> > >>>+    db = sss_colondb_open(tool_ctx, SSS_COLONDB_READ, filename);
> > >>>+    if (db == NULL) {
> > >>>+        fprintf(stderr, _("Unable to open %s.\n"), filename);
> > >>>+        exit = EXIT_FAILURE;
> > >>>+        goto done;
> > >>>+    }
> > >>>+
> > >>>+    tmp_ctx = talloc_new(NULL);
> > >>
> > >>You assigned twice to tmp_ctx in this function. Also in
> > >>override_group_import.
> > 
> > Fixed.
> > 
> > Here are patches with rest of comments addressed.
> 
> Thank you.
> 
> I tried a couple of more tests like overriding UIDs or GIDs to zero and
> everything worked fine.
> 
> Coverity is clean, CI as well.
> 
> ACK
> 
> Thank you for the hard work on these patches!

Pushed to master:
    * 23fb01bf67a6058fb508da6d81515e8b18634beb
    * 5df5a6b852eccaafc8a3fb4eb31296d9587be483
    * a76f63544533f0404f7711a10c1a621c6045df17
    * 7eba58cfcf78e61af1c4ff98619aa97223eb7a5b
    * 4285cf181abd1d12dc144d5f86d73162bbd9cf05



More information about the sssd-devel mailing list