[SSSD] [PATCH] First Boot Seed Tool

Nick Guay nguay at redhat.com
Fri Jul 20 20:52:17 UTC 2012


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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-First-boot-sss_seed-tool.patch
Type: text/x-patch
Size: 27798 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120720/e21c1d0e/attachment.bin>


More information about the sssd-devel mailing list