[SSSD] [PATCH] Add options to override GID, homedir and shell

Stephen Gallagher sgallagh at redhat.com
Fri May 6 14:05:05 UTC 2011


On Fri, 2011-05-06 at 13:56 +0200, Jakub Hrozek wrote:
> [PATCH 1/2] Add a confdb_get_uint32_t convenience function
> A convenience function to fetch the gid override from confdb
> 

Ack

> [PATCH 2/2] Add options to override GID, homedir and shell
> The functionality to override the shell is a little different than
> described in the ticket -- it was adjusted after a discussion on #sssd
> on IRC.

Nack.

In the future, please provide separate patches for logically different
features (this patch contains three different changes).

The home directory template should also support UID (I mentioned this in
ticket 742). We should cover all our bases.

Please reorder the manpage entry for allowed_shells. It should be clear
that the order of evaluation is:
 1. If it's in /etc/shells, use it.
 2. If it's NOT in /etc/shells, but it is in allowed_shells, use the
value of shell_fallback.
 3. If it's NOT in /etc/shells and it's not in allowed_shells, it
becomes nologin

We should also make it clear that having an empty string for shell is
passed as-is to glibc for it to make the decision.


In expand_homedir_template(), you allocate memory for "copy" that you
never free. Please create a tmp_ctx for copy and free it on all exits
(success or failure). It would be preferable to also allocate "result" 
on tmp_ctx and only talloc_steal() it onto mem_ctx at the end of
successful execution.


get_shell_override() should not shortcut out if there are no allowed
shells. It should still check /etc/shells because if it's not listed, we
want to convert it to nologin.

Also, get_shell_override() isn't handling the empty shell special case,
which should be passed back as-is.

Finally, getusershell() is an expensive system call. I'd prefer if we
only did so once and cached it in the nss_ctx, rather than invoking it
for every user request. I think it's reasonable to require the SSSD to
be restarted if /etc/shells changes. (Might want to put that in the
manpage though.)



-------------- 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/20110506/de32ac9b/attachment.sig>


More information about the sssd-devel mailing list