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

[Rob Crittenden]

Simo Sorce wrote:
> 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.
>

pushed 75-77 to master and ipa-3-0

rob