[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