[SSSD] [PATCHES] Support for ghost users

Jan Zeleny jzeleny at redhat.com
Wed May 2 18:44:20 UTC 2012


Updated patch set in the attachment

Stephen Gallagher <sgallagh at redhat.com> wrote:
> On Wed, 2012-04-25 at 15:41 +0200, Jan Zelený wrote:
> > Ok, here is the updated patch set.
> 
> 117: Ack
> 
> 
> 118: Nack
> 
> In sdap_fill_memberships(), please do not ever store HASH_* value in
> 'ret'. Create a new variable (I suggest 'hret'). 'ret' is assumed to be
> of type errno_t in pretty much all of our code and I don't want to
> conflate the two.

Well, I have one of my questions about the code in other parts of SSSD 
explained :) Fixed

> It's probably academic, but ghostel->values[k].length should probably be
> strlen()+1, since the buffer technically includes the NULL-terminator.
> In practice, it will likely never matter.

You are right. Academic or not, it should be +1. Fixed

> I don't understand why you look up SYSDB_MEMBER a second time if
> ghostel->num_values == 0. Why would it have changed?

That's actually pretty nasty part of the sysdb code. The thing is that 
sysdb_attrs_get_el() creates an element if it isn't found in element array. 
That means the element array is reallocated. In such case the original 
SYSDB_MEMBER element would still be there (even though the array could have 
been actually moved elsewhere) for some time but then it disappears as the 
space is reallocated for something else. That leads to "wonderful" heisenbugs, 
it took me two days to get to the bottom of this.

To sum up: ghoster->num_values == 0 might be indicating that the array of 
elements has been reallocated. In that case we need to find the SYSDB_MEMBER 
again so we don't have a pointer to possibly invalid memory.

> In sdap_save_group() you assign the ghost name to
> el->values[el->num_values].data but you don't set its length.

Fixed

> 119: Ack
> 
> 
> 120: Nack
> I know it's not strictly the fault of this patch, but could you please
> fix the ldb_transaction logic in this function. Right now, it won't
> cancel if the commit fails.

I'd prefer to fix this in a separate patch since it is on six more places in 
the code (if I understand correctly what did you mean).

> Please allocate 'msg' on tmp_ctx. Right now it will not be freed if any
> of the ldb_msg_add_* functions fail.

Ah, copy-paste error. Fixed

> Please rename 'el' to 'alias_el' so it's clear what it's being used for
> in the cleanout code. A few more comments there would be handy too, for
> future maintainers.

Done and done

> 121: Looks good, but I'd much prefer if you would add some comments. The
> memberOf plugin is very complex code.

Done

> 122: Nack
> 
> I disagree with your assertion in the comment that this is rare. In many
> (maybe most) existing environments, there will be fake users in the DB.

I thought that fake users are resolved ASAP, perhaps this basic assumption was 
wrong?

> Please do the search within the transaction, not before it.

Done

> Please rename 'memberof' to 'memberof_el' so it's clear it contains all
> of them.

Done

> We know that 'name' is a single-valued attribute. Just use
> ldb_msg_find_attr_as_string() and make 'name' a 'const char *'. We
> should also ensure that it's non-NULL.

Done

> Use ldb_dn_validate() on the msg->dn, just in case the memberOf
> reference is broken. In that case, we should just log a message, ignore
> it and hope for the best.

Done

> Please prefer talloc_zfree() for variables that get reused (such as in a
> loop). Specifically, please talloc_zfree(msg) at the end of the deletion
> loop.

I original left it as is because it is immediatelly assigned to in the 
beginning of the loop. Nevertheless it is fixed now.

> 123: Nack
> I don't see the point of this patch (and I think it's guaranteeing
> duplicates). Unless I completely misread Patch 121, you've already got
> the memberOf plugin creating memberUID attributes for ghost users. So
> with this patch, we're adding all memberUID values as group members, and
> then going and adding the ghost users a second time. That doesn't make
> any sense. Please tell me if I've misinterpreted something here.

Actually no, there are no duplicates. I tried to explain in the commit 
message. The object that contains users in the ghost attribute doesn't contain 
them in memberuid attribute. I didn't want to have redundant information in 
the object. That's why we need to look for both attributes.

> 124: Ack
> 
> 
> 125: Ack
> 
> 
> 126: Ack

Thanks for the review.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jzeleny-117-3-ghost-members-add-the-ghost-attribute-to-sysdb.patch
Type: text/x-patch
Size: 1070 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/519785c0/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jzeleny-118-3-ghost-members-support-in-ldap-provider.patch
Type: text/x-patch
Size: 31884 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/519785c0/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jzeleny-119-3-ghost-members-support-in-proxy-provider.patch
Type: text/x-patch
Size: 2733 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/519785c0/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jzeleny-120-3-ghost-members-modifications-in-sysdb.patch
Type: text/x-patch
Size: 7377 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/519785c0/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jzeleny-121-3-ghost-members-modifications-in-memberof-plugin.patch
Type: text/x-patch
Size: 11348 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/519785c0/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jzeleny-122-3-ghost-members-sysdb-upgrade-routine.patch
Type: text/x-patch
Size: 6423 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/519785c0/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jzeleny-123-3-ghost-members-nss-responder-changes.patch
Type: text/x-patch
Size: 10026 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/519785c0/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jzeleny-124-3-ghost-members-removed-sdap_check_aliases.patch
Type: text/x-patch
Size: 7610 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/519785c0/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jzeleny-125-3-ghost-members-modified-sss_groupshow.patch
Type: text/x-patch
Size: 3005 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/519785c0/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jzeleny-126-2-ghost-members-various-small-changes.patch
Type: text/x-patch
Size: 3536 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/519785c0/attachment-0009.bin>


More information about the sssd-devel mailing list