[SSSD] [PATCH] First Boot Seed Tool

Stephen Gallagher sgallagh at redhat.com
Wed Jul 18 21:08:06 UTC 2012


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.

Please also write a manpage for this new feature. See
src/man/sss_cache.8.xml for an example of how it should look.

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.

Typo on line 363: Only one 't' in "initialize"
Typo on line 295: slected -> selected

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.

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.

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.

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.

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).

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).

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).

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120718/38bbe8ed/attachment.sig>


More information about the sssd-devel mailing list