The mail is getting too big, I'm going to trim the acked patches.
On Wed, May 02, 2012 at 02:26:38PM -0400, Stephen Gallagher wrote:
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
>
ldap_group_objectsid seems to be still missing from src/config/SSSDConfig.py
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.
Yes, 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()
>
> 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.
Ack
> > > 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.
Ack
> The rest of the review wil follow, I just didn't want to add
one more round to
> the review process.
Thanks
>
0005: No change, ack remains
0006: No change, ack remains
0007: No change, ack remains
0008: No change, ack remains
0009: No change, ack remains
0010: No change, ack remains
0011: No change, ack remains
> > > 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.
Wasn't that bug just a matter of /proc not representing the data
correctly? On my system, both uid_t and gid_t eventually resolve to
unsigned int (through a __U32_TYPE constant).
> > > 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)
> >
I acked patch #21 in the other thread.
Ack to patch #13 now
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.
> >
0015: No change, ack remains
> > > 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.
> >
That was fixed, ack
0017: No change, ack remains
0018: No change, ack remains
0019: No change, ack remains
> > > 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.
Ack