[SSSD] [PATCH] First Boot Seed Tool

Pavel Březina pbrezina at redhat.com
Wed Jul 25 15:01:58 UTC 2012


On 07/20/2012 10:52 PM, Nick Guay wrote:
> On 18/07/12 17:08, Stephen Gallagher wrote:
>> On Wed, 2012-07-18 at 12:22 -0400, Nick Guay wrote:
>>> This tool is used to seed the cache with user information and a
>>> temporary password if the system does not have direct connectivity to a
>>> IPA/AD/LDAP provider at first boot. It requires a username and domain to
>>> be specified, and the corresponding domain must be configured in
>>> sssd.conf.
>>>
>>> https://fedorahosted.org/sssd/ticket/904
>> A good start, but Nack.
>>
>> First, there's no reason for this to be separate patches. Please squash
>> them together.
> One patch.
>> Please also write a manpage for this new feature. See
>> src/man/sss_cache.8.xml for an example of how it should look.
> Man page added. And spec file updated.
>> The main() function is a bit busier than I'd like to see. Please break
>> it up into logical functions so it's easier to read through.
> I broke up main() into a couple pieces, but left the main sysdb calls
> and argument parsing.
>> Typo on line 363: Only one 't' in "initialize"
>> Typo on line 295: slected -> selected
> Fixed.
>> Your password_input() function needs to prompt for the password twice
>> (when getting the password interactively) and compare the values. Typos
>> are a pain in the neck otherwise.
> Password is now prompted for twice.
>> Do not mix talloc pointers with non-talloc pointers. In
>> password_input(), you assign *password a talloc pointer if you're
>> reading from a file, or a pointer returned from getpass(). You need to
>> talloc_strdup() the return value from getpass().
>>
>> As soon as you talloc_strdup() the password, you need to call
>> password_destructor() directly on what you got from getpass().
>>
>> Make sure you set the password_destructor() as a talloc destructor on
>> the password you return from password_input() in all situations.
>>
>> password_input() needs to take a mem_ctx argument onto which the
>> password should be stolen from tmp_ctx once it's ready to be returned.
>>
>> Your use of fgets() is unsafe. (Well, almost all uses of fgets() are
>> unsafe). It doesn't handle interrupted communication (such as receiving
>> a signal while reading) cleanly. Take a look at get_krb5info() in
>> src/krb5_plugin/sssd_krb5_locator_plugin.c for an example of how to read
>> a file using open() and sss_atomic_read_s().
>>
>> You don't need to allocate temp at all in password_input(). This is a
>> good place to just use a stack variable and create temp[TEMP_LEN+1].
>> Also, if you were going to allocate it, it should have been allocated
>> inside "if (method == PASS_FILE)" so you don't waste an allocation on an
>> unused variable.
> Changed all the input functions to be safer.
>> You have a hard-coded limit of 10 characters for a password. This is
>> unacceptable. It should be much larger (I'd suggest 64 characters) and
>> documented in the as-yet missing manpage.
> Password is now expected to be of size PASS_MAX or 64 if PASS_MAX is not
> defined.
>> Don't use defines for PASS_PROMPT and PASS_FILE. Make it an enum
>> instead, so it would be a compile-time error if you passed the wrong
>> value somewhere.
> Changed to an enum.
>> Back in main(): After the initial sysdb_getpwnam() call, you check if
>> the cache has zero values, otherwise you call password_input() and cache
>> the password. You also need to check for > 1 values, because this would
>> indicate a corrupt cache (and you must report this to the user).
> Now errors out if more than one entry is found in the cache.
>> If sysdb_cache_password() fails during the non-interactive mode, you
>> MUST report a (translatable) error message to the user and immediately
>> fail. Right now you're falling down into the section of the code that
>> handles adding the user (which is wrong and may corrupt the cache).
> An error is now printed and then the program exits.
>> You're forcing interactive mode when the user wasn't found in the cache,
>> but this may not be the case if all of the necessary data was specified
>> at the command-line. (This means username, UID and primary GID. If
>> unspecified, the others should remain NULL).
> I wasn't sure what should be deemed the minimum amount of information in
> order to create an entry in the cache. Now it is expected to have
> username, domain, UID and primary GID in order to create or modify an
> entry.
>> There's no reason to do the check after committing the transaction. If
>> the transaction commit returned LDB_SUCCESS, you can trust that the data
>> is now there.
> Final cache check is now removed.

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.




More information about the sssd-devel mailing list