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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel