On Fri, Apr 27, 2012 at 01:47:51PM -0400, Stephen Gallagher wrote:
These patches are built atop Sumit's recent patch "Allow
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
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
Patch 0002: Add option to enable id-mapping
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
Patch 0006: LDAP: Initialize ID mapping when configured
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.
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.
Patch 0010: Helper routine to get the domain SID portion from a user
Patch 0011: Allow us to auto-provision a new domain when we see one
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
RID of the primary group, so we extract the domain SID from the user SID
and then convert the group ID from that.
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
when we save users and groups.
Patch 0018: Convert the auto-provisioning code into a common routine,
rather than copying it everywhere.
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.
Patch 0020: (Hopefully) comprehensive manpage on the ID-mapping
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?