[Freeipa-devel] [PATCH] 75-78 Add fallback group

[Simo Sorce]

On Fri, 2012-10-05 at 19:02 +0200, Sumit Bose wrote:
> On Fri, Oct 05, 2012 at 06:34:25PM +0200, Sumit Bose wrote:
> > On Fri, Oct 05, 2012 at 09:45:58AM -0400, Simo Sorce wrote:
> > > On Fri, 2012-10-05 at 16:27 +0300, Alexander Bokovoy wrote:
> > > > On Tue, 02 Oct 2012, Simo Sorce wrote:
> > > > >On Tue, 2012-10-02 at 21:29 +0200, Sumit Bose wrote:
> > > > >> Hi,
> > > > >>
> > > > >> this patch should fix https://fedorahosted.org/freeipa/ticket/2955 by
> > > > >> adding a fallback group as described in comment 2 of the ticket in
> > > > >> ipa-adtrust-install.
> > > > >>
> > > > >> If you prefer to use a different kind of group I can change the patch
> > > > >> accordingly.
> > > > Patch works for me, so ACK except the group name.
> > > > 
> > > > >Yes I think we should use a more natural group name. In my recent
> > > > >testing I have been using the name 'Trust Users' that pairs well with
> > > > >another group we create called 'Trust Admins'. But I am open to
> > > > >suggestions on a better name, 'Domain Users' may be better if we really
> > > > >want to associate the wellknown SID to this group.
> > > > I'm fine with 'Trust Users'.
> > > > 
> > > > >On the SID side I wonder if using the wellknown 'Domain Users' SID is
> > > > >the right thing to do. I do not see any special reasons why it shouldn't
> > > > >but I also do not have any special reason why we should.
> > > > >Anyone can think of any pros/cons of doing that ?
> > > > Since it only has special meaning within the same domain and we are not
> > > > using it for anything, it should be fine.
> > > 
> > > Well it has a special meaning for samba servers ... we may have a few in
> > > the IPA domain.
> > > I think using that SID is fine but I think then the name 'Trust Users'
> > > could be confusing to users looking at permissions.
> > > 
> > > We also need to make the group a posix group btw, because samba uses the
> > > Primary Group SID for users and if it can't be resolved to uid/gids it
> > > will probably prevent authentication.
> > > 
> > > What about calling it 'Default SMB Group' or similar ?
> > > 
> > > Simo.
> > 
> > ok new version attached. The group is now a Posix with an
> > automatically assigned SID called 'default_smb_group' and in the
> > protected list.
> > 
> > bye,
> > Sumit
> > > 
> > > -- 
> > > Simo Sorce * Red Hat, Inc * New York
> > > 
> 
> I found a copy-and-paste issue in the first patch, new version attached.
> 
> bye,
> Sumit

0075:

I think we should default_smb_group -> 'default smb group',
Default_SMB_Group -> 'Default SMB Group' etc... not sure why we
shouldn't use spaces here ? 'Trust Admins' has spaces we should be
consistent and use the same conventions everywhere for builtin groups.

The comment in __add_admin_sids() is now wrong, we do not associate the
wellknown 'Domain Users' sid to the fallback group. The second phrase of
the docstring should simply be dropped.

The description attribute of the group_add command in
__add_fallback_group() should use 'users' (plural) not 'user' I think.

0076:

Ack

0077:

I am going to Ack this patch but please open a ticket to fix edtection
of UPGs. It's not enough to have uidnumber to match the pgid gidnumber
to tell that the group is actually a upg. In cases where admins decide
to override the DNA plugin and user their own uid/gid allocation, they
may have users that use a real group as primary group (potentially with
the same name of the user too). We will need to verify we can use the
secondary rid range and to mangle the group name if that happens, but we
shouldn't fallback to the primary group fallback I think.

However I do not think this is should hold back the patchset, so Ack.

Nitpicks faillback -> fallback in get_fallback_group_sid()

0078:

Nack, I think you had in mind the fact that the group we were using had
no posix IDs, but that should never be the case, samba needs the primary
group to be a posix group.
So I think we should just drop this patch, we do create the
Default_SMB_Group as a posix group.


Simo.

-- 
Simo Sorce * Red Hat, Inc * New York