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