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

Jakub Hrozek jhrozek at redhat.com
Wed Aug 19 21:02:36 UTC 2015


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.

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;

> 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..)

> +
> +    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.

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 :)

> +                    </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::

> +                    </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.

> +    if (tmp_ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> +        exit = EXIT_FAILURE;
> +        goto done;
> +    }
> +



More information about the sssd-devel mailing list