[SSSD] [PATCH] First Boot Seed Tool

Nick Guay nguay at redhat.com
Wed Aug 1 15:29:59 UTC 2012


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-First-boot-sss_seed-tool.patch
Type: text/x-patch
Size: 35992 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120801/caf01023/attachment.bin>


More information about the sssd-devel mailing list