> 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
Nack, please add ldap_group_objectsid to man page and API definition > > Patch 0002: Add option to enable id-mapping > > Ack Nack, please add ldap_id_mapping to man page > > Patch 0003: Add sysdb routines for storing ID maps in the cache > > Nack, you forgot to free tmp_ctx in sysdb_idmap_store_mapping() Line 254 in the patch: if (old_slice == -1) - you forgot to set the ret value I'm not sure about name comparison on line 282 in the patch. Since it is related to windows, I suppose it should be case insensitive > > 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. To avoid problems, I think all three values (idmap_lower, idmap_upper and rangesize) should be checked for sanity. The rest of the review wil follow, I just didn't want to add one more round to the review process. Jan > > 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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
