[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