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

Mon Apr 2 07:27:07 UTC 2012

On Fri, 2012-03-30 at 09:05 -0400, Rob Crittenden wrote:
> 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

Ok. Your patch 993 is still useful and fixes the creation of UPGs, we
just need to remove nonposix->posix conversion and make sure non-posix
users are migrated successfully as non-posix users.

Martin