[SSSD] [PATCH] libwbclient: SSSD implementation

Sumit Bose sbose at redhat.com
Wed Aug 20 15:37:51 UTC 2014


On Sun, Aug 17, 2014 at 08:43:25PM +0200, Jakub Hrozek wrote:
> On Wed, Jul 23, 2014 at 12:34:32PM +0200, Sumit Bose wrote:
> > new version attached.
> > 
> > bye,
> > Sumit
> 
> I admit I haven't reviewed the filed copied from Winbind too closely,
> I just quickly read them to know what's going on in these modules. In the
> review, I mostly compared the sssd implementation to the winbind one.
> 
> Do you expect the files copied from Samba to change? Most of the files I
> check in the Samba tree didn't change since 2012 or even 2010 so I
> expect we're safe..

I think so, too.

> 
> I think the dlopen to open the nss_sss module is perfectly fine, in fact
> I prefer it over linking.  That's how the module is used by the glibc
> after all.
> 
> A question -- in sssd's copy_group_entry() you only create the gr_mem
> array when there are some members. Winbind seems to always create an
> array which contains just a NULL sentinel if the group has no members.
> Does that matter? If not, let's leave the code as-is, I just wanted to
> check.

I changed it so that an array with a single NULL is returned.

> 
> In nss_to_wbc(), would it make sense to also translate
> NSS_STATUS_UNAVAIL to WBC_ERR_WINBIND_NOT_AVAILABLE ? Sure, there is no
> winbind, so the return code might better be called something like
> WBC_ERR_DATA_SOURCE_NOT_AVAILABLE or similar, but my question is more if
> handling the NSS_STATUS_UNAVAIL NSS code is expected.

done

> 
> wbcGetgrnam and wbcGetgrnam have wrong comments, but this trivial bug is
> also in Samba.
> 
> Are you sure the asprintf() call in wbcLookupName is safe? Could this
> enable someone to trash the stack with a long enough name?

I added some checks to prevent this.

> 
> I agree with the comment in wbcLookupSid() about returning the name and
> domain separately. Do you want to file a ticket? Alternatively, we can
> make a function that parses a FQDN into a (name,domain) tuple public if
> you expect this functionality to be needed elsewhere?

But this would require parsing sssd.conf as well because in theory the
full name format can be changed. So I would prefer to return them
separately.

> 
> sss_client/libwbclient/wbc_sssd_internal.h seems to use a DEVELOPER
> macro, do you expect this macro to be enabled with modifying CFLAGS?
> It's OK to leave it there I guess, I'm not aware of any similar macro in
> SSSD..

With this flag calls to un-implemented calls are written to syslog. I
found this option useful during development and since not all calls are
implemented it might become useful later as well, this is why I kept it.

> 
> In wbclient.pc.in can you mention this is wbclient API using SSSD?

done

> 
> That's all I have. The code looks very solid and clean overall --
> thanks!

Thank you for the review, new version attached.

bye,
Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list