[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