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

Jakub Hrozek jhrozek at redhat.com
Thu Aug 20 20:39:06 UTC 2015


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!



More information about the sssd-devel mailing list