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

Pavel Březina pbrezina at redhat.com
Thu Aug 20 10:54:47 UTC 2015


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 :)
>
>> +                    </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;
>> +    }
>> +
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sss_override-print-input-name-if-unable-to-parse-it.patch
Type: text/x-patch
Size: 933 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150820/96d07b67/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sss_override-support-domains-that-require-fqname.patch
Type: text/x-patch
Size: 3211 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150820/96d07b67/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-TOOLS-add-sss_colondb-API.patch
Type: text/x-patch
Size: 10852 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150820/96d07b67/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-sss_override-decompose-code-better.patch
Type: text/x-patch
Size: 15497 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150820/96d07b67/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-sss_override-support-import-and-export.patch
Type: text/x-patch
Size: 25464 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150820/96d07b67/attachment-0004.bin>


More information about the sssd-devel mailing list