On Thu, Aug 20, 2015 at 10:39:06PM +0200, Jakub Hrozek wrote: > 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?= <pbrez...@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?= <pbrez...@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?= <pbrez...@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?= <pbrez...@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 > > >>sssdgroup40...@win.trust.test:sssdgroup123: > > >>sssdgroup40...@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> > > >>>+ > > >>>ck...@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!
Pushed to master: * 23fb01bf67a6058fb508da6d81515e8b18634beb * 5df5a6b852eccaafc8a3fb4eb31296d9587be483 * a76f63544533f0404f7711a10c1a621c6045df17 * 7eba58cfcf78e61af1c4ff98619aa97223eb7a5b * 4285cf181abd1d12dc144d5f86d73162bbd9cf05 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel