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

Tue Apr 3 07:10:19 UTC 2012

On Mon, 2012-04-02 at 15:18 -0400, Rob Crittenden wrote:
> Martin Kosek wrote:
> > 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
> >
> 
> I decided to not migrate non-POSIX users at all. They are a-typical in 
> IPA and won't show at all using our tools so I don't see the point in 
> migrating them. non-POSIX users in IPA are typically stored in 
> cn=sysconfig rather than cn=users.
> 
> rob

ACK. Pushed to master, ipa-2-2.

Martin