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

[Jakub Hrozek]

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?= 
> 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?= 
> 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?= 
> 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?= 
> 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 
[e-mail]:sssdgroup123:
[e-mail]: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>
>                    [e-mail]::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;
> }