On Fri, Apr 27, 2012 at 01:47:51PM -0400, Stephen Gallagher wrote: > These patches are built atop Sumit's recent patch "Allow different SID > representations in libidmap". I added the manpage as a single patch near > the end because it was just too much trouble to do it piecemeal > throughout the set. This patch series went through numerous iterations, > so some of the patches may be slightly out of order. Please review as a > whole. The patches themselves are separated primarily the way they are > to make reviewing easier. Some have notes below to guide the reviewer to > changes that may have been revised later but were nontrivial to rewrite > history for. > > Patch 0001: Add objectSID config option > This patch adds an option to specify the objectSID attribute on the LDAP > server, for use when performing ID-mapping
Ack > > Patch 0002: Add option to enable id-mapping Ack > > Patch 0003: Add sysdb routines for storing ID maps in the cache Nack, you forgot to free tmp_ctx in sysdb_idmap_store_mapping() > > Patch 0004: Add helper routines to the LDAP provider for ID mapping > This contains the meat of the ID-mapping algorithm. See the manpage in > Patch 0020 for full details. > Mostly ack, but in sdap_idmap_add_domain I would prefer to check for rangesize > 0 before dividing with its value. > Patch 0005: Add options for configuring range settings > Ack > Patch 0006: LDAP: Initialize ID mapping when configured > Ack > Patch 0007: Do ID-mapped lookups for users > Note: this patch contains a bit of code that has been refactored by the > end of the patchset (specifically it references binary_to_dom_sid() and > dom_sid_to_string()). I didn't bother rewriting this particular bit of > history because it's replaced entirely by Patch 0016 anyway (which uses > the correct functions from Sumit's patch). > The diff looked good, so ack, but I actually ended up reviewing the sdap_save_user code as a whole. > Patch 0008: Add an autorid compatibility mode > Autorid assigns slices as first-come-first-served. This will force the > LDAP ID-mapping to behave the same way. Ack > > Patch 0009: Add a feature to guarantee that a single "default" domain is > always assigned to slice 0, regardless of hashing. This can be used to > extend autorid compatibility mode. > Ack > Patch 0010: Helper routine to get the domain SID portion from a user or > group objectSID > Ack > Patch 0011: Allow us to auto-provision a new domain when we see one for > the first time. > Note: this gets turned into its own function in Patch 0018 for reuse in > groups and initgroups. Looks good, full review done as part of patch 18 > > Patch 0012: Look up mapped users by UID UID and GID values are 32bit unsigned values in general, right? Wouldn't it be better to use strtouint32() and not strtoint32(), then to avoid errors when a very large ID is requested? The strto(u)int32 functions also set errno to 0 themselves, no need to do it manually. > > Patch 0013: Look up mapped groups by name > Ack for the changes that actually made it into sdap_save_group(). I noticed that even though the basic filters for group lookups have been changed to not include SDAP_AT_GROUP_GID, the enumeration filters still do include it. Does id-mapped enumeration work? (I haven't tested the patches yet, I'll do it while waiting for the next revision) > Patch 0014: Look up mapped groups by GID > Same comment about strtoint32 vs. strtouint32 as I had for patch #12 applies here, too. Otherwise looks good. > Patch 0015: Map the user's primaryGID. ActiveDirectory stores only the > RID of the primary group, so we extract the domain SID from the user SID > and then convert the group ID from that. Ack > > Patch 0016: Common routine to convert an LDAP blob of the objectSID into > a UNIX ID. > Ack, but please fix the compound bracket indentation in sdap_add_incomplete_groups - it's line 155 when all patches are applied. > Patch 0017: Hack to ensure that uidNumber and gidNumber are not deleted > when we save users and groups. Ack > > Patch 0018: Convert the auto-provisioning code into a common routine, > rather than copying it everywhere. > Ack > Patch 0019: Handle cases where we have an unmappable ID (such as > special-case SIDs) for a group membership. In these cases, we'll treat > the groups as non-POSIX so we can safely continue. > Ack > Patch 0020: (Hopefully) comprehensive manpage on the ID-mapping feature. > It's separated into its own include file so that it will be possible to > import it into the manpage for the AD provider when that is available. + components that represent the Active Directory domain identity and + the relative identifier (ID) of the user or group object. Shouldn't the sentence say "RID" in the brackets? Otherwise Ack _______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
