[SSSD] [PATCH] First Boot Seed Tool

Pavel Březina pbrezina at redhat.com
Wed Aug 1 16:00:25 UTC 2012


On 1.8.2012 17:29, Nick Guay wrote:
> On 01/08/12 07:56, Pavel Březina wrote:
>> On 07/31/2012 06:08 PM, Nick Guay wrote:
>>> On 30/07/12 12:02, Pavel Březina wrote:
>>>> Nack.
>>>>
>>>> The code is way better now. Especially the main function. Hovewer,
>>>> there occurred several more problems, mostly about talloc contexts. I
>>>> highly suggest you reading talloc tutorial
>>>> (http://talloc.samba.org/talloc/doc/html/libtalloc__tutorial.html),
>>>> especially the links that are at the bottom of this mail.
>>>>
>>>> Avoid stealing talloc context on NULL. You should basically add
>>>> mem_ctx parameter to the function and use it instead of NULL. You can
>>>> find more information about usage of temporary contexts at [1]
>>>>
>>>> Create and obey good talloc context hierarchy. How to do that can be
>>>> found at [3][4].
>>>>
>>>> sss_seed.c:304 Trailing whitespace.
>>>>
>>>> struct seed_ctx:
>>>>  - remove member 'transaction_done', it is used only in one function
>>>> and does not describe state of the tool, so it should be a local
>>>> variable. The same applies to snctx.
>>>>  - remove member 'error', it is not used at all
>>>>  - remove member 'domain', you initialize it but don't use it
>>>>
>>>> main():
>>>>  - don't mix errno codes (like ENOMEM) with stdlib EXIT_FAILURE, use
>>>> errno codes and convert them to EXIT_FAILURE/SUCCESS after 'done' label
>>>>  - when -i option is specified, you want to print an error if user is
>>>> cached or allow replacing sysdb entry with provided values
>>>>  - line 869: this check should be done even when user_cached == true
>>>>  - provide uctx as a memory context in seed_password_input() and
>>>> seed_check_groups() calls
>>>>  - this is not necessary if you will create a good context hierarchy
>>>> (you need just talloc_free(sctx)
>>>>     if (sctx != NULL) {
>>>>         talloc_zfree(sctx->uctx);
>>>>     }
>>>>     talloc_free(sctx);
>>>>
>>>> seed_init():
>>>>  - don't mix errno codes (like ENOMEM) with stdlib EXIT_FAILURE,
>>>> return only errno codes and transfer them in the caller if necessary
>>>>  - allow specifying username either via option or as a left over
>>>> argument, not both
>>>>
>>>> seed_dom_user_info():
>>>>  - getpwnam() as I wrote before, you should use a fully qualified name
>>>> here, you may get wrong user info otherwise (it may be a local user
>>>> with the same name, or user from different domain)
>>>>  - you need to talloc_zfree() uctx members before setting a new value,
>>>> you are leaking memory otherwise. But you don't actually need to fill
>>>> uctx here, because once the user is cached, you are changing only
>>>> password.
>>>>  - this is very wrong usage of talloc_steal() because you are
>>>> destroying talloc context hierarchy, more information at [2][3].
>>>> Simply remove it and it will be ok.
>>>>
>>>> seed_dom_user_info() should also contain sysdb_getpwnam(). Calling
>>>> getpwnam() is used to force sssd to load user information into the
>>>> cache. So what you should do is:
>>>>
>>>> static int seed_cached_user_info(TALLOC_CTX *mem_ctx,
>>>>                                  const char *username,
>>>>                                  const char *domainname)
>>>>
>>>>     passwd = getpwnam(user at domain);
>>>>     sysdb_getpwnam(...);
>>>>     if (cached) {
>>>>         return EOK;
>>>>     } else {
>>>>         return ENOENT;
>>>>     }
>>>>
>>>> In main():
>>>>    ret = seed_cached_user_info(sctx, ..., &new_uctx);
>>>>    if (ret == EOK) {
>>>>        DEBUG(user is cached, only the password will be set);
>>>>    } else if (ret == ENOENT) {
>>>>        using provided values
>>>>    } else {
>>>>        error
>>>>    }
>>>>
>>>> seed_init_db():
>>>>  - use const char *domain_name
>>>>  - use **_confdb(...) in the parameter list and *confdb(...) as a
>>>> local variable, steal it to mem_ctx only at the end when everything
>>>> goes fine:
>>>>
>>>>     *_confdb = talloc_steal(mem_ctx, confdb);
>>>>     *_sysdb = talloc_steal(mem_ctx, sysdb);
>>>>     *_domain = talloc_steal(mem_ctx, domain);
>>>>
>>>> done:
>>>>     talloc_free(tmp_ctx);
>>>>     return ret;
>>>>
>>>> seed_interactive_input():
>>>>  - provide tmp_ctx as a memory context and the steal the values to uctx
>>>>
>>>> seed_prompt(): no need for tmp_ctx here, use:
>>>>     prompt = talloc_asprintf(NULL, _("Enter %s:"), req);
>>>>     ...
>>>>     talloc_free(prompt);
>>>>
>>>> seed_str_input(): no need for tmp_ctx here, use:
>>>>     *_input = talloc_strdup(mem_ctx, buf);
>>>>
>>>> line 202: can you put the PASS_MAX macro at the beginning please?
>>>>
>>>> seed_password_input(): split this function into two (one for reading
>>>> from file and one for reading from stdin)
>>>>
>>>> seed_check_groups():
>>>>  - you are leaking memory on errors, use tmp_ctx pattern here as well
>>>>  - check context hierarchy - addgroups[i] should be allocated on
>>>> addgroups which should be allocated on uctx
>>>>  - **_var, *var, talloc_steal() pattern would be nice here too
>>>>
>>>> More info about stealing at [4]
>>>>
>>>> [1]
>>>> http://talloc.samba.org/talloc/doc/html/libtalloc__bestpractices.html#bp-tmpctx-2
>>>>
>>>>
>>>> [2]
>>>> http://talloc.samba.org/talloc/doc/html/libtalloc__context.html#context-hierarchy
>>>>
>>>>
>>>> [3]
>>>> http://talloc.samba.org/talloc/doc/html/libtalloc__bestpractices.html
>>>> [4] http://talloc.samba.org/talloc/doc/html/libtalloc__stealing.html
>>>>
>>>
>>> Fixed the talloc problems noted, and also fixed the mismatched error
>>> codes. The username option is no longer a standalone option. getpwnam()
>>> and initgroups() now use the fully qualified name. If the user is
>>> cached, only the password is updated for the cached entry. An error is
>>> now produced and interactive input is skipped if the -i option is
>>> provided for a cached user.
>>
>> Nack.
>>
>> Good job. We are almost there. There are just few talloc context
>> hierarchy violations remaining, few nitpicks. I created a separate
>> ticket to handle these so we can get the code to beta 6 in time.
>>
>> https://fedorahosted.org/sssd/ticket/1448
>>
>> These needs to be fixed ASAP:
>>
>> seed_init():
>> Use SSSDBG_DEFAULT as the default debug level. (otherwise it's really
>> noisy)
>>
>> seed_init_db():
>> When you are not able to connect to the confdb, print error to stderr
>> so it can be seen even when no debug level is set (ERROR macro).
>>
>> seed_password_input_prompt():
>> When passwords don't match, return EINVAL or something (otherwise we
>> are facing SIGSEGV).
>>
>> sss_seed.c:667 - return ret here, not ENOENT
>>
>> sss_seed.c:788: use goto done instead of break
>>
>> seed_check_groups():
>> 1. When user is in cache, you are performing initgroups so all his
>> groups are stored in the cache, you don't need to check it
>> 2. When user is *not* in the cache then this function will fail on all
>> groups that are not in the cache.
>> 3. You actually don't store/modify groups anywhere.
>> The main point of this tool is to allow the user to log in when the
>> provider is offline. It is not necessary to store all his groups in
>> sysdb as they will be downloaded once the provider is back online.
>> Thus remove groups related code entirely.
>>
>> sss_seed.c:894 - exit with EEXIST after the error instead of
>> continuing (user obviously wants the interactive input, so just
>> ignoring the -i option is bad)
>>
>> When it ends successfully, print a message about it to stdout.
>>
>> And last one: when compiling with:
>> -m64 -mtune=generic -O0 -ggdb3 -D_FORTIFY_SOURCE=2
>> -fstack-protector-all -Werror -Wall -Wextra -Wno-sign-compare
>> -Wno-unused-parameter
>> I get the error:
>> src/tools/sss_seed.c:67:12: error: declaration of ‘index’ shadows a
>> global declaration [-Werror=shadow]
>>
>> Thanks,
>> Pavel.
>>
>
> Fixed what was in the ticket and removed the groups check, which was not
> necessary for the online provisioning case, but does not cover offline
> case in which groups would have to be added.

Great! One final nack though, but that would be a quick fix.

283: move those #endif to the macros they belong to:
#ifndef BUFSIZE
#define BUFSIZE 1024
#endif

#ifndef PASS_MAX
#define PASS_MAX 64
#endif

seed_interactive_input(): you don't test for NULL when strdup

And remove groups and addgroups from uctx structure, popt, interactive 
input.

Thanks,
Pavel.



More information about the sssd-devel mailing list