[SSSD] [PATCHES] Add support for ID-mapping with Active Directory domains

Jan Zelený jzeleny at redhat.com
Wed May 2 14:21:16 UTC 2012


> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/26486fab/attachment.sig>


More information about the sssd-devel mailing list