[Freeipa-devel] [PATCH] 993 disable UPG for migration

Fri Mar 30 13:05:26 UTC 2012

Martin Kosek wrote:
> On Thu, 2012-03-29 at 11:27 -0400, Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> On Wed, 2012-03-28 at 17:28 -0400, Rob Crittenden wrote:
>>>> Martin Kosek wrote:
>>>>> On Thu, 2012-03-22 at 15:21 -0400, Rob Crittenden wrote:
>>>>>> We don't want to create private groups automatically for migrated users,
>>>>>> there could be namespace overlap (and race conditions prevent us from
>>>>>> trying to check in advance).
>>>>>>
>>>>>> Check the sanity of groups in general, warn if the group for the
>>>>>> gidnumber doesn't exist at least on the remote server.
>>>>>>
>>>>>> Fill in the user's shell as well.
>>>>>>
>>>>>> This will migrate users that are non-POSIX on the remote server.
>>>>>>
>>>>>> rob
>>>>>
>>>>> This patch fixes the problem of creating UPGs for migrated users, but
>>>>> there are several parts which confused me.
>>>>>
>>>>> 1) Confusing defaults
>>>>>
>>>>> +    if 'def_group_gid' in ctx:
>>>>> +        entry_attrs.setdefault('gidnumber', ctx['def_group_gid'])
>>>>>
>>>>> This statement seems redundant, because the account either is posix and
>>>>> has both uidnumber and gidnumber or it is non-posix and does not have
>>>>> any.
>>>>>
>>>>> Now, we don't touch gidNumber for posix user, and non-posix users are
>>>>> made posix with this statement:
>>>>>
>>>>> +    # migrated user is not already POSIX, make it so
>>>>> +    if 'uidnumber' not in entry_attrs:
>>>>> +        entry_attrs['uidnumber'] = entry_attrs['gidnumber'] = [999]
>>>>>
>>>>>
>>>>> 2) Missing UPG
>>>>> When UPG is disabled, the following statement will result in a user with
>>>>> a GID pointing to non-existent group.
>>>>>
>>>>> +    # migrated user is not already POSIX, make it so
>>>>> +    if 'uidnumber' not in entry_attrs:
>>>>> +        entry_attrs['uidnumber'] = entry_attrs['gidnumber'] = [999]
>>>>>
>>>>> We may want to run ldap.has_upg() and report a add this user to "failed
>>>>> users" with appropriate error.
>>>>>
>>>>> 3) Check for GID
>>>>> The patch implements a check if a group with the gidNumber exists on a
>>>>> remote LDAP server and the result is a warning:
>>>>>
>>>>> -            (g_dn, g_attrs) = ldap.get_entry(ctx['def_group_dn'], ['gidnumber'])
>>>>> +            (remote_dn, remote_entry) = ds_ldap.find_entry_by_attr(
>>>>> +                'gidnumber', entry_attrs['gidnumber'][0], 'posixgroup', [''],
>>>>> +                 search_bases['group']
>>>>> +            )
>>>>>
>>>>> I have few (minor-ish) questions there:
>>>>> a) Is the warning in a apache log enough? Maybe it should be included in
>>>>> migrate-ds output.
>>>>> b) This will be a one more remote LDAP call for every user, we may want
>>>>> to optimize it with something like that:
>>>>>
>>>>> valid_gids = []
>>>>> if user.gidnumber not in valid_gids:
>>>>>       run the check in remote LDAP
>>>>>       valid_gids.append(user.gidnumber)
>>>>>
>>>>> That would save us 999 LDAP queries for 1000 migrated with the same
>>>>> primary group.
>>>>>
>>>>> 4) Extraneous Warning:
>>>>> When non-posix user is migrated and thus we make it a posix user, we
>>>>> still produce a warning for non-existent group:
>>>>>
>>>>> [Fri Mar 23 04:21:36 2012] [error] ipa: WARNING: Migrated user's GID
>>>>> number 999 does not point to a known group.
>>>>>
>>>>> 5) Extraneous LDAP call
>>>>>
>>>>> Isn't the following call to LDAP to get a description redundant? We
>>>>> already have the description in entry_attrs.
>>>>>
>>>>> +    (dn, desc_attr) = ldap.get_entry(dn, ['description'])
>>>>> +    entry_attrs.update(desc_attr)
>>>>> +    if 'description' in entry_attrs and NO_UPG_MAGIC in
>>>>> entry_attrs['description']:
>>>>>
>>>>>
>>>>> Martin
>>>>>
>>>>
>>>> I think this covers your concerns.
>>>>
>>>> I can't do anything but log warnings at this point in order to maintain
>>>> backwards compatibility. I looked into returning a warning entry and it
>>>> blew up in validate_output() on older clients.
>>>>
>>>> rob
>>>>
>>>
>>> This patch is much better and covers my previous concerns. I just find
>>> an issue with UPG. It is not created for non-posix users when UPGs are
>>> enabled:
>>>
>>> # echo "Secret123" | ipa migrate-ds ldap://ldap.example.com
>>> --with-compat --base-dn="dc=greyoak,dc=com"
>>> -----------
>>> migrate-ds:
>>> -----------
>>> Migrated:
>>>     user: darcee_leeson, ayaz_kreiger, mnonposix, mollee_weisenberg
>>>     group: ipagroup
>>> Failed user:
>>> Failed group:
>>> ----------
>>> Passwords have been migrated in pre-hashed format.
>>> IPA is unable to generate Kerberos keys unless provided
>>> with clear text passwords. All migrated users need to
>>> login at https://your.domain/ipa/migration/ before they
>>> can use their Kerberos accounts.
>>>
>>> # ipa user-show mnonposix
>>>     User login: mnonposix
>>>     First name: Mister
>>>     Last name: Nonposix
>>>     Home directory: /home/mnonposix
>>>     Login shell: /bin/sh
>>>     UID: 328000195
>>>     GID: 328000195
>>>     Org. Unit: Product Testing
>>>     Job Title: Test User
>>>     Account disabled: False
>>>     Password: True
>>>     Member of groups: ipausers
>>>     Kerberos keys available: False
>>>
>>> # ipa group-show mnonposix
>>> ipa: ERROR: mnonposix: group not found
>>
>> Yes, I was always disabling UPG. I now allow it when migrating a
>> non-POSIX user.
>>
>>> I am also thinking if we need to ask if UPG is enabled for every
>>> migrated user - every ldap.has_upg() call means one query to host LDAP.
>>> Maybe we should ask just in the beginning and store the setting in
>>> "ctx['upg_enabled']. I don't think that anyone needs to switch UPG
>>> status during migration process.
>>
>> I agree, nice optimization.
>>
>> rob
>
> This patch is OK, everything worked as expected. We just now need to
> specify if we want a special option/flag to enable non-posix ->  posix
> user conversion. If not, then ACK.
>
> Martin
>

Simo and I had a brief discussion about this in IRC and I'm going to 
punt on non-POSIX user conversion for now. There could be specific 
reasons for this, like having a shared user (nss_ldap) or some sort of 
system user that they don't to be a full POSIX user.

Adding a new option this late in 2.2 doesn't seem like a good idea so 
I'll patch it to simply fail non-POSIX users for now. I've opened a 
ticket to optionally allow them to be converted.

rob