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.