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

Stephen Gallagher sgallagh at redhat.com
Wed May 18 12:47:45 UTC 2011


On Wed, 2011-05-18 at 11:22 +0200, Jakub Hrozek wrote:
> On 05/13/2011 02:54 PM, Stephen Gallagher wrote:
> > On Fri, 2011-05-13 at 11:14 +0200, Jakub Hrozek wrote:
> >> On 05/10/2011 01:35 PM, Stephen Gallagher wrote:
> >>> On Tue, 2011-05-10 at 10:58 +0200, Jakub Hrozek wrote:
> >>>> On 05/09/2011 07:42 PM, Stephen Gallagher wrote:
> >>>>> On Mon, 2011-05-09 at 10:45 +0200, Jakub Hrozek wrote:
> >>>>>> On 05/06/2011 04:05 PM, Stephen Gallagher wrote:
> >>>>>>> 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.
> >>>>>>>
> >>>>>>
> >>>>>> This is done in the attached patch, but I don't agree with the change.
> >>>>>>
> >>>>>> It differs from the current behaviour and moreover allowed_shells is
> >>>>>> unset in the default setting, so the change would affect anyone running
> >>>>>> with defaults.
> >>>>>>
> >>>>>
> >>>>> Well, that wouldn't really have been a change in behavior, since if the
> >>>>> shell didn't exist on the system, the login program or SSH would still
> >>>>> be denying them. 
> >>>>
> >>>> Oh, I though the user would get /bin/sh..
> >>>>
> >>>> In this case I agree with you..thank you for explaining.
> >>>
> >>>
> >>> I was thinking about this more last night, and I think in the end that
> >>> you were probably right with how you did this the first time. While it's
> >>> true that both 'login' and 'sshd' will just reject the user (actually,
> >>> they'll try to exec the shell, get an error from exec and then bail
> >>> out), it's possible that other mechanisms like rlogin or telnet or
> >>> similar might have their own ways of behaving (implementing their own
> >>> fallback, for example).
> >>>
> >>> So I think that we probably need to just punt if we don't have an
> >>> allowed_shells option and return the LDAP value.
> >>>
> >>> Sorry for going back and forth on this so many times.
> >>>
> >>>
> >>
> >> No problem :-)
> >>
> >> New patches attached. They also change the label "fail" to "done" in the
> >> two functions that expand templates.
> > 
> > Please don't squash unrelated changes into patches. The label change
> > should have been separate. 
> 
> Didn't we use to say that trivial changes such as fixing typos can be
> squashed? Or does that only apply when we're changing code around that area?
> 

This is a bit of an edge case. Fixing a typo in a piece of code you're
working with is fine to squash in. Making the same change to a
half-dozen similar places is probably grounds for separating into its
own patch, just for ease of code review.

The general rule I try to follow is: one patch per logical change. This
way a reviewer can pick up a patch and know that, while there may be a
five patch series, each individual patch should be relatively easy to
review and lead logically to the next.

> > Much the same way that this patch should have
> > been three, one for shells, one for homedir and one for GID.
> > 
> > I just realized that I have one more nack for this. While I think the
> > shell overrides are fine, homedir and GID need to be configurable
> > per-domain (especially GID). Configuring GID globally for all domains
> > just doesn't make sense. Homedir I think should probably be settable in
> > either place (similar to how we do filter_users in either [NSS] or
> > per-domain)
> > 
> > Nack.
> > 
> 
> OK, I've changed the patches and also split them into three. I think it
> actually makes sense now that the options are quite different,
> previously it was just three new NSS options.

Patch 0001: Ack

Patch 0002: Ack

Patch 0003: Nack. One last minor change. I realized that
nss_get_etc_shells() could be vulnerable to a race-condition attack. If
an administrator accidentally had the wrong permissions set
on /etc/shells, then it would be possible for an attacker to monitor
that file for accesses with inotify and replace its contents between the
initial read for length and the second read for contents. Thus, a buffer
overrun could be exploited.

I think it would be wiser for us to do buffered talloc_realloc()'s (say,
five at a time) and run through the getusershell() loop only once.
Probably putting an upper limit of, say 50 shells just to be safe.



-------------- 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/20110518/27f06fc1/attachment.sig>


More information about the sssd-devel mailing list