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

Stephen Gallagher sgallagh at redhat.com
Wed May 2 18:26:38 UTC 2012


On Wed, 2012-05-02 at 16:21 +0200, Jan Zelený wrote:
> > 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
> 

Fixed. Thanks for catching that. I'd added the user version but not the
group one.

> > > Patch 0002: Add option to enable id-mapping
> > 
> > Ack
> 
> Nack, please add ldap_id_mapping to man page
> 

As noted elsewhere, this is covered in patch 0020.

> > > 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
> 
> 

Good catches. Fixed both.

> 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
> 

I think this is okay here in general. The only time this functionality
should ever be used is if we're populating it with known data, which
will therefore be the canonical capitalization. If it's different from
what's there, we'll update it.

> > > 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.
> 

Added checks for rangesize <= 0, for upper being lower than the lower
bound and for the difference being less than a rangesize.

> The rest of the review wil follow, I just didn't want to add one more round to 
> the review process.

Thanks

> 
> > > 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.
> > 

That's probably the right approach.

> > > 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.
> > 

The errno thing isn't harmful, so I'm not going to bother with it. It
actually turns out that id_t (as well as uid_t and gid_t) are actually
*signed* values, not unsigned. This has actually caused us issues
elsewhere before. See https://fedorahosted.org/sssd/ticket/1216 for
details.

> > > 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)
> > 

Eep. Thanks, I had completely forgotten about enumeration. I'm going to
add that support as new patches atop these. Let's get these in first,
please.

> > > 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

Yes, thanks. Fixed.


New patches attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-LDAP-Add-objectSID-config-option.patch
Type: text/x-patch
Size: 11049 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-LDAP-Add-id-mapping-option.patch
Type: text/x-patch
Size: 4722 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-SYSDB-Add-sysdb-routines-for-ID-mapping.patch
Type: text/x-patch
Size: 12887 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-LDAP-Add-helper-routines-for-ID-mapping.patch
Type: text/x-patch
Size: 13561 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-LDAP-Add-ID-mapping-range-settings.patch
Type: text/x-patch
Size: 4838 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-LDAP-Initialize-ID-mapping-when-configured.patch
Type: text/x-patch
Size: 1898 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-LDAP-Enable-looking-up-ID-mapped-users-by-name.patch
Type: text/x-patch
Size: 4861 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-LDAP-Add-autorid-compatibility-mode.patch
Type: text/x-patch
Size: 5921 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-LDAP-Allow-setting-a-default-domain-for-id-mapping-s.patch
Type: text/x-patch
Size: 7014 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-LDAP-Add-routine-to-extract-domain-SID-from-an-objec.patch
Type: text/x-patch
Size: 3749 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0011-LDAP-Allow-automatically-provisioning-a-domain-and-r.patch
Type: text/x-patch
Size: 3862 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0012-LDAP-Enable-looking-up-id-mapped-users-by-UID.patch
Type: text/x-patch
Size: 3419 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0013-LDAP-Allow-looking-up-ID-mapped-groups-by-name.patch
Type: text/x-patch
Size: 8895 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0014-LDAP-Enable-looking-up-id-mapped-groups-by-GID.patch
Type: text/x-patch
Size: 3432 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0015-LDAP-Map-the-user-s-primaryGroupID.patch
Type: text/x-patch
Size: 10517 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0016-LDAP-Add-helper-routine-to-convert-LDAP-blob-to-SID-.patch
Type: text/x-patch
Size: 17088 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0017-LDAP-Do-not-remove-uidNumber-and-gidNumber-attribute.patch
Type: text/x-patch
Size: 2476 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0018-LDAP-Add-helper-function-to-map-IDs.patch
Type: text/x-patch
Size: 13954 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0017.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0019-LDAP-Treat-groups-with-unmappable-SIDs-as-non-POSIX-.patch
Type: text/x-patch
Size: 2208 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0018.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0020-MAN-Add-manpage-for-ID-mapping.patch
Type: text/x-patch
Size: 12904 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment-0019.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/32a6cec8/attachment.sig>


More information about the sssd-devel mailing list