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(a)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(a)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(a)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(a)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@win.trust.test:sssdgroup123:
sssdgroup40004@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@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;
+ }
+