[SSSD] [PATCH] User home directories management

Jakub Hrozek jhrozek at redhat.com
Wed Oct 21 18:30:31 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/21/2009 09:05 AM, Martin Nagy wrote:
> Hi, sorry, I accidentally sent the email too early. I'll continue where
> I left, see below.
> 
> On Wed, 2009-10-21 at 06:39 +0200, Martin Nagy wrote:
>> On Tue, 2009-10-20 at 13:33 +0200, Jakub Hrozek wrote:
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> On 10/19/2009 11:32 AM, Jakub Hrozek wrote:
>>>> On 10/15/2009 04:18 PM, Jakub Hrozek wrote:
>>>>>> User home directories management
>>>>>>
>>>>>> Create and populate user directories on useradd, delete them on userdel
>>>>>>
>>>>>> Fixes: #212
>>>> Rebased on top of 02bf83eaf9eb9733536c6c56afcbc70e312b3cd6
>>>>
>>>> 	Jakub
>>>
>>> ...and now with the correct patch. Sorry for any confusion.
>>
>> +               <option>-M</option>,<option>--not-create-home</option>
>> +        { "not-create-home", 'M', POPT_ARG_NONE, NULL, 'M', _("Never
>>  create user's directory, overrides config"), NULL },
>>
>> Can we make this --no-create-home instead? It somehow sounds better for
>> me and I think it's something of a custom to do it that way.
>>
>> +                    <para>
>> +                        Do not create the user's home directory, even
>> +                        if the system wide setting from is set to TRUE.
>> +                    </para>
>> Sounds like the "from" doesn't belong there. Anyway, maybe saying
>> something like "Overrides system wide setting(s?)." instead?
>>
>> +                    <option>-R</option>,<option>--not-remove</option>
>> Same as above.
>>
>> +                    Files in the user's home directory will NOT be
>> +                    removed along with the home directory itself and
>> +                    the user's mail spool. Overrides the configuration.
>> "along with the home directory itself and the user's mail spool" seems
>> to be a little bit redundant.
>>
>> +                            and directories to be copied in the userĀ“s
>> Bad apostrophe, should be "'".
>>
>> In server/python/pysss.c:
>> +    int remove_home;
>> This should be initialized to 0.
>>
>> In files-tests.c:
>> Being a PITA: You have some symbols here (both functions and variables)
>> that should be static. Also, tpl_dir should be const and tpl_filename is
>> not used anywhere.
>>
>> +static int create_simple_file(const char *name, const char *content)
>> +{
>> +    int fd = -1;
>> The initialization is not needed. BTW, you can safely make this function
>> return void and eliminate the last return. Remember that fail_if()
>> terminates the process. This would save you couple of check later after
>> calling create_simple_file().
>>

All the above issues should be fixed.

>> You could also make very simple wrappers around standard functions you
>> use a lot like mkdir(), chdir(), access(), etc. For example, something
>> like this:
>>
>> static void xchdir(const char *path)
>> {
>>     int ret;
>>     const char *err_str;
>>     ret = chdir(path);
>>     if (ret != 0) {
>>         err_str = strerror(errno);
>>         fail("Failed to chdir to %s: %s", path, err_str);
>>     }
>> }
>>
>> This is just a suggestion, I won't NACK because of it, but IMHO the
>> tests would look nicer and be easier to read since you don't have to put
>> so much fail_if's after every function call.
>>

Umm I'd like to avoid rewriting the code, but it sure is doable if required.

>> +    int debug;
>> Should be initialized to 0.
>>

done

>> In sss_sync_ops.c, userdel_defaults():
>> Don't forget to free conf_path.
>>

done

>> In useradd_defaults():
>> +    DEBUG(7, ("Mail dir: %s\n", data->home));
>> Should be data->maildir. Jakub pointed out this one himself while
>> looking over my shoulder :) Btw, this function looks a bit messy when it
>> comes to cleaning up after failure, please fix this. Could you also try
>> to reduce the number of variables in it? For example, you could remove
>> maildir by passing &data->maildir to confdb_get_string() directly. Same
>> goes for dfl_shell, dfl_create_home and umask. Other variables could
>> probably be made more local. Reducing the number of variables will make
>> the function a lot more easier to read (less things to keep track on).
>>

the function was tidied up

>> In sss_useradd.c:
>> +        { "create-home", 'm', POPT_ARG_NONE, NULL, 'm', _("Crate user's
>> directory if does not exist"), NULL },
>>
>> s/Crate/Create/
>>

done

>> In main():
>> +        switch (ret) {
>> +            case 'G':
>> +                groups = poptGetOptArg(pc);
>> +                if (!groups) {
>> +                    ret = -1;
>> +                    break;
>> +                }
>> +
>> +            case 'm':
>> +                pc_create_home = DO_CREATE_HOME;
>> +                break;
>> The first break shouldn't be inside of the if, but it wouldn't be able
>> to break out of the while loop anyway, so you should add a goto instead.
>>

It would (that's what ret=-1 was for) but a goto is much more convenient
here - fixed.

>> +        if (ret != EOK) {
>> +            switch (ret) {
>> +                case EEXIST:
>> +                    ERROR("User's home directory already exists, not
>> copying"
>> +                           "data from skeldir\n");
>> +                    break;
>> +
>> +                default:
>> +                    ERROR("Cannot create user's home directory\n");
>> +                    ret = EXIT_FAILURE;
>> +                    goto fini;
>> +            }
>> +        }
>> Suggestion: Use the more idiomatic and easier to read:
>> if (ret == EEXIST) {
>>     ERROR("User's home directory already exists, not copying"
>>           "data from skeldir\n");
>> } else if (ret != EOK) {
>>     ERROR("Cannot create user's home directory\n");
>>     ret = EXIT_FAILURE;
>>     goto fini;
>> }
>>

Good suggestion, thanks.

>> +            ERROR("Cannot create user's mail spool\n");
>> +            DEBUG(1, ("Cannot create user's mail spool: [%d][%s].\n",
>> +                        ret, strerror(ret)));
>> Why don't you use only ERROR and include the error string there? I think
>> that strerror() will translate the message according to the locale, so
>> that shouldn't be an issue.
>>

Because I generally think that the user should not be bothered with
strerror messages, they can be cryptic (think ENOENT returned from a
database search - strerror is "No such file or directory") and also I
think there should be a debug message with most (if not all) ERROR
messages - to give developers a string that is guaranteed to be NOT
translated.

But it's true that in case of file operations, strerror() messages are
OK. Added.

>> In server/tools/sss_userdel.c:
>> +        if (ret != EOK) {
>> +            switch (ret) {
>> +                case EPERM:
>> +                    ERROR("Not removing home dir - not owned by user
>> \n");
>> +                    break;
>> +
>> +                default:
>> +                    ERROR("Cannot remove homedir\n");
>> +                    ret = EXIT_FAILURE;
>> +                    goto fini;
>> +            }
>> +        }
>> Same as above.
>>
>> In server/tools/tools_util.c:
>> In is_owner():
>> +        DEBUG(1, ("Cannot stat %s: %d\n", path, ret));
>> Use %s and strerror(ret) instead please.
>>
>> >From remove_mail_spool():
>> +        switch (ret) {
>> +            case 0:
>> +                break;
>> +            case -1:
>> +                DEBUG(3, ("%s not owned by %d, not removing\n",
>> +                          spool_file, uid));
>> +                ret = EACCES;
>> +            default:
>> +                goto fail;
>> +        }
>> Per [1], please add a /* FALLTHROUGH */ comment after ret = EACCESS.
>>
>> +        DEBUG(1, ("Cannot remove() the spool file\n"));
>> Please add ": %s" and strerror(ret).
>>

All the above comments should be fixed.

>> In remove_homedir():
>> +    ret = remove_mail_spool(mem_ctx, maildir, username, uid, force);
>> +    if (ret != EOK) {
>> +        DEBUG(1, ("Cannot remove user's mail spool"));
>> +        /* Should this be fatal? I don't think so. Maybe convert to
>> ERROR? */
>> +    }
>> The DEBUG statement is missing a \n. I think you should ignore the
>> return value (so this shouldn't be fatal) and not print anything.
>> Rather, I would convert the DEBUG() inside of remove_mail_spool() to an
>> ERROR().
>>
>> +        DEBUG(1, ("Cannot remove homedir based on %s: %d\n",
>> +                  homedir, ret));
>> Use strerror() and ERROR(). "based on " is probably redundant.
>>

OK, here's my problem with ERROR -- the call prints directly to stderr
and these functions like remove_homedir et al. are used also from python
bindings where instead of printing the error you want to throw an
exception. I honestly don't know a very good way to solve it, I even
toyed with the idea of passing out an error string if ret != EOK.

>> For the selinux related stuff.. Is selinux_file_context() really
>> necessary? I don't know about selinux that much, but I'd expect that
>> when you create a new file it will possess the right selinux context
>> automatically. 

It will posses the right *default* selinux context automatically. But
some files (for example ~/public_html) must have different contexts than
default files created in home directory, selinux_file_context() sets it.

What the routines do roughly equals to calling restorecon <fullpath>
after creating files or directories.

This can be trivially tested by configuring SSSD --without-selinux,
adding public_html to /etc/skel and then running sss_useradd.

>> Also, I think it would be nice to only use HAVE_SELINUX
>> when defining the functions, like this:
>>
>> #ifdef HAVE_FOO
>> int fun1(int a, int b, int c)
>> {
>>     /* Code here. */
>>     return EOK;
>> }
>>
>> void fun2(int a)
>> {
>>     /* Code here. */
>> }
>> #else /* !HAVE_FOO */
>> #define fun1(a, b, c) EOK
>> #define fun2(a) ((void)0)
>> #endif /* HAVE_FOO */
>>
>> You then don't have to use HAVE_FOO every time you need fun1() or
>> fun2(). But in our case, you would also have to make a simple wrapper
>> (or better yet, a simple define) around setfscreatecon() to avoid using
>> HAVE_FOO for some functions and not for others.
>>

done, thanks

>> BTW, in create_mail_spool(), you don't reset the create context with
>> setfscreatecon(NULL) as you do in create_homedir(). Is this intentional?
>>
>> In create_homedir() you set selinux file context but if mkdir(), chown()
>> or chmod() fails, you won't reset it. If my understanding of what
>> selinux_file_context() does is correct, this could be a potential
>> security hazard, since the context will be used for the next created
>> file.
>>

both resets were fixed

>> In util/files.c:
>> I think that almost all DEBUG macros here should really be ERROR macros.

See above for DEBUG vs. ERROR

>> Furthermore, please consider making wrappers for the common functions
>> that would do the error checking and use the ERROR() macro for errors. I
>> would suggest that you do this for at least functions opendir(),
>> lstat(), closedir(), rmdir(), mkdir(), chown(), chmod(), utimes(),
>> symlink(), mknod(), open(), fchown(), fchmod() and close().
> 
> There might be more functions that I missed.
> 
> It would be nice to have these wrappers somewhere in a separate file in
> util/. They should have a prefix like x, sss_, or something similar.
> Example:
> 
> int xmkdir(const char *pathname, mode_t mode)
> {
>     int ret;
> 
>     ret = mkdir(pathname, mode);
>     if (ret != 0) {
>         ret = errno;
>         ERROR("Failed to create directory %s: %s", pathname,
>               strerror(ret));
>     }
>     return ret;
> }
> 
> When we have these wrappers it is much easier to write and read code
> since all the unimportant stuff is already taken care of. The resulting
> binary will also won't be so big because of all those strings. And most
> importantly, you don't have to duplicate the code (and potentially
> bugs).
> If you decide to follow this approach, I'd also recommend using the
> functions like this:
> 

Dunno, I generally prefer to just check the return value than wrappers,
you can nicely see where exactly it failed with the DEBUG. A macro would
solve this, though...is there a general consensus on what to use?

> ret = xmkdir(path, mode);
> if (ret != 0) goto fail;
> 
> IIRC Simo and Stephen shorten if's in this way often, it's a nice way to
> not take the attention away from the calls themselves.
> 
>> In copy_dir():
>> You don't check value returned from mkdir().
>     ret = chown(dst, (uid == - 1) ? statp->st_uid : (uid_t) uid,
>                      (gid == - 1) ? statp->st_gid : (gid_t) gid);
> Sorry for nitpicking, but please no space between '-' and '1'. Also, I
> might be wrong but I think uid_t and gid_t are unsigned types. Since you
> have a cast there, I'm guessing you wanted to define them as signed?
> 

Yeah..after some thinking I decided that the "no uid set" check was not
neccessary and just removed it..

> Perhaps specifying that mt needs to be array of 2 members would be nice
> (same as utimes()).
> 

done

> I just noticed that in a lot of functions in files.c you don't give much
> attention to freeing memory if an error occurs. I also noticed that this
> is ok since those are static functions and the caller takes care of
> freeing the whole temporary context at the end. 

Both copy_tree and remove_tree are recursive, the context can be freed
only in the top level function.

> However, at least a
> comment for each function that doesn't care about memory because of this
> should be added.
> 

OK, added comments

> Same with setting selinux_file_context(). Although here we should maybe
> reset the context anyway, just to be sure since it might be a source of
> security issues.
> 
> In copy_symlink():
> Same issue with uid/gid signedness as in copy_dir().
> 
> In copy_special():
> Same as above.
> 
> In copy_file():
> Same as above. Hm, you use this code pretty often, maybe a macro would
> be appropriate?
> 

Removed from all three

> +    while ((cnt = read(ifd, buf, sizeof(buf))) > 0) {
> +        if (cnt == -1) {
> +            ret = errno;
> +            DEBUG(1, ("Cannot read() from source file '%s': [%d][%
> s].\n",
> +                      dst, ret, strerror(ret)));
> +            goto fail;
> +        }
> Move the if block after the loop, otherwise it will never execute.
> 

Good catch.

> +        if (write(ofd, buf, (size_t)cnt) != cnt) {
> +            ret = errno;
> You have to be able to handle cases when write returns something > -1
> but < cnt (write could have been interrupted by a signal handler, etc.).

Fixed

> Also, I'm not sure if we shouldn't handle EAGAIN cases for both read and
> write. I don't know if a blocking I/O is the default on all platforms.
> 
> +    close(ifd);
> +    ifd = -1;
> Error handling here is missing.
> 

Fixed

> In copy_entry():
> +        ret = copy_symlink(cctx, src, dst, &sb, mt, uid, gid);
> +        if (ret != EOK) {
> +            DEBUG(1, ("Cannot copy symlink '%s' to '%s': [%d][%s]\n",
> +                      src, dst, ret, strerror(ret)));
> +        }
> +        return ret;
> 
> Suggestion: move the error message into copy_symlink() and just do
> "return copy_symlink();". Same with copy_dir(), copy_special(),
> copy_file().
> 

Done. I think the error messages were extra since the copy_XXX()
functions all have their error reporting.

> Martin
> 


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkrfU0cACgkQHsardTLnvCVQCACfb/bwZZcD4RJD6NYzK8HK6Ko7
2ywAoJfJo1T1sBl43ZX8nfMt3s6H24Uc
=63F3
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-User-home-directories-management.patch
Type: text/x-patch
Size: 83568 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20091021/39d2df0d/attachment.bin>


More information about the sssd-devel mailing list