[SSSD] [PATCH] First Boot Seed Tool
Pavel Březina
pbrezina at redhat.com
Mon Jul 30 16:02:45 UTC 2012
On 07/27/2012 09:30 PM, Nick Guay wrote:
> On 25/07/12 11:01, Pavel Březina wrote:
>> Nack.
>>
>> Things to consider:
>>
>> - support reading password directly from command line
>> - -p 123 | -password 123
>> - -f file | --password-file file
>> - support reading everything from file that contains users in passwd
>> format
>>
>> Please include the manual page in src/man/Makefile.am and src/man
>> /po/po4a.cfg so it can be built and translated.
>>
>> A forward backslash is used to close a tag (ie </para> instead of
>> <\para>.
>>
>> sss_seed.c:
>>
>> Mark the functions as static.
>>
>> Don't use tools_ctx and octx and create a custom structure (say
>> seed_ctx) that will fit exactly to your needs.
>>
>> Our convention is to use tmp_ctx for temporary contexts.
>>
>> You don't use enum seed_input_type anywhere.
>>
>> 180: Missing NULL check.
>> 191: Typo "Passowrds"
>> 197: Create a local variable "char *password" and function parameter
>> "char **_password". Work with the "password" and then steal it at the
>> end:
>> *_password = talloc_steal(mem_ctx, password).
>>
>> seed_interactive_input(): Debug messages would be nice here when
>> something fails.
>>
>> 296: This check should be done at the beginning of the function.
>> 300: Use lower case in "domain".
>> 319, 320: The same thing as with the password variable.
>>
>> main(): Load every option into separate variable (pc_*) and
>> talloc_strdup() that into the structure (see sss_usermod) so you don't
>> mix malloc and talloc pointers.
>>
>> 490: Use CHECK_ROOT macro here.
>>
>> 500: You should use fully qualified name (user at domain) to be sure that
>> you get the correct user in multi domain environment. If UID is
>> specified you should test if it matches passwd->pw_uid.
>>
>> Use errno to get proper error code from getpwnam(). The good way would
>> be:
>>
>> errno = 0;
>> pc_passwd = getpwnam(...);
>> if (pc_passwd == NULL) {
>> ret = errno;
>> DEBUG(..., ("... [%d]: %s\n", ret, strerror(ret)));
>> goto done;
>> }
>>
>> You should copy (talloc_strdup()) passwd values you need instead of
>> simple assignment because it points to a static location which may be
>> overwritten by other calls.
>>
>> Also name pc_passwd doesn't make sense as it has nothing in common
>> with popt context.
>>
>> 518: You're missing space after comma in the comment.
>>
>> Put everything after line 579 into a separate function.
>>
>> Thanks,
>> Pavel.
>>
>
> Thanks, Pavel. I covered most of what you noted. I forgot to include one
> commit with the last patch I sent, which left the interactive input
> broken in the last patch. That should be fixed along with a shorter,
> more readable main function.
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
More information about the sssd-devel
mailing list