On (23/07/14 12:34), Sumit Bose wrote:
>On Wed, Jul 23, 2014 at 12:58:56AM +0200, Lukas Slebodnik wrote:
>> On (22/07/14 10:51), Sumit Bose wrote:
>> >Hi,
>> >
>> >this patch contains the libwbclient implementation for SSSD to allow
>> >Samba file-servers and utilities to use SSSD instead of winbind to map
>> >SIDs, names and POSIX IDs. The related ticket is
>> >https://fedorahosted.org/sssd/ticket/1588.
>> >
>> >The SSSD specific calls can be found in files with '_sssd' as part of
>> >the name, the other files are mainly taken from the original Samba
>> >sources because they contain API calls which are independent of the
>> >backend. I have made some minor modification to meet the SSSD coding
>> >style but tried to avoid major changes to make a diff to the original
>> >version more easy. If major issue are found during review I think it
>> >would be a good idea to try to bring the changes back to samba as well.
>> >
>> >In wbc_pwd_sssd.c instead of linking in the related sources of the NSS
>> >client I dlopen-ed libnss_sss.so.2 to have more flexibility about where
>> >and how to build the library. I you think this is too much overhead I'd
>> >be happy to change the code to use the NSS client call directly.
>> >
>> >The patch does not contain unit test because the Samba source code
>> >already contains some basic tests. I'll try to work with the samba
>> >package maintainer to make this code in a samba-devel or samba-test
>> >package available so that it can be used by SSSD as well.
>> >
>> >Basic functional test can be run manually with wbinfo, e.g.:
>> >
>> >$ /usr/bin/wbinfo -n 'AD18\Administrator'
>> >S-1-5-21-3090815309-2627318493-3395719201-500 SID_USER (1)
>> >$ /usr/bin/wbinfo -S S-1-5-21-3090815309-2627318493-3395719201-500
>> >1670800500
>> >
>> >bye,
>> >Sumit
>>
>> It is quite big patch. I think some files are copied from samba.
>> I read just patches with header "Author: sbose ..."
>
>Thank you for the fast review.
>
>>
>> >@@ -713,7 +717,7 @@ libsss_config_la_LDFLAGS = \
>> > endif # BUILD_CONFIG_LIB
>> > endif # BUILD_IFP
>> >
>> >-lib_LTLIBRARIES = libipa_hbac.la libsss_idmap.la libsss_nss_idmap.la
>> >+lib_LTLIBRARIES = libipa_hbac.la libsss_idmap.la libsss_nss_idmap.la
>> >libwbclient.la
>> Could you put each library to separate line and use $(NULL) at the end.
>
>done
>
>>
>> > pkgconfig_DATA += src/providers/ipa/ipa_hbac.pc
>> > libipa_hbac_la_DEPENDENCIES = src/providers/ipa/ipa_hbac.exports
>> > libipa_hbac_la_SOURCES = \
>> >@@ -753,11 +757,36 @@ libsss_nss_idmap_la_LDFLAGS = \
>> >
>> > dist_noinst_DATA += src/sss_client/idmap/sss_nss_idmap.exports
>> >
>> >+pkgconfig_DATA += src/sss_client/libwbclient/wbclient.pc
>> >+libwbclient_la_DEPENDENCIES = src/sss_client/libwbclient/wbclient.exports
>> Use prefix EXTRA_, otherwise parallel build will fail.
>> CCLD libwbclient.la
>> libtool: link: cannot find the library `libsss_nss_idmap.la' or unhandled
>> argument `libsss_nss_idmap.la'
>> make[2]: *** [libwbclient.la] Error 1
>
>done, I wonder if this should be done for the other *.exports
>DEPENDNECIES as well?
Dependencies are automatically detected from variable libname_LIBADD. In your
case, this variable contain local library libsss_nss_idmap.la. Without prefix
EXTRA_ dependencies are overridden and libsss_nss_idmap was not build before
libwbclient. Other libraries in sssd are independent and prefix EXTRA_ isn't
needed.
>
>>
>> >+libwbclient_la_SOURCES = \
>> >+ src/sss_client/libwbclient/wbc_guid.c \
>> >+ src/sss_client/libwbclient/wbc_idmap_common.c \
>> >+ src/sss_client/libwbclient/wbc_idmap_sssd.c \
>> >+ src/sss_client/libwbclient/wbclient_common.c \
>> >+ src/sss_client/libwbclient/wbclient_sssd.c \
>> >+ src/sss_client/libwbclient/wbc_pam_sssd.c \
>> >+ src/sss_client/libwbclient/wbc_pwd_sssd.c \
>> >+ src/sss_client/libwbclient/wbc_sid_common.c \
>> >+ src/sss_client/libwbclient/wbc_sid_sssd.c \
>> >+ src/sss_client/libwbclient/wbc_sssd_internal.h \
>> >+ src/sss_client/libwbclient/wbc_util_common.c \
>> >+ src/sss_client/libwbclient/wbc_util_sssd.c
>> There was expectation that libsss_nss_idmap.so is external library and header
>> files are installed. Compilation failed on machine without installed header
>> files for libsss_nss_idmap. We should use header files from source directory.
>> e.g.
>> libwbclient_la_CPPFLAGS = \
>> -I$(srcdir)/src/sss_client/idmap/ \
>> $(AM_CPPFLAGS)
>>
>> or directly adding this directory to the AM_CPPFLAGS.
>
>ah, sorry, I've build the library outside of the SSSD tree for some
>time, looks like this is a left-over.
>
>I have changed the #include in the related files and added the full path
>there as we do with other internal includes as well. I think this might
>be even safer because with adding -I... to the compiler flags there is
>still a chance that the header file from the system is used and if this
>from a different version we might fail.
I agree.
>> >+libwbclient_la_LIBADD = \
>> >+ libsss_nss_idmap.la \
>> >+ $(CLIENT_LIBS)
>> >+libwbclient_la_LDFLAGS = \
>> >+
>> >-Wl,--version-script,$(srcdir)/src/sss_client/libwbclient/wbclient.exports \
>> >+ -version-info 11:0:11
>> >+
>> >+dist_noinst_DATA += src/sss_client/libwbclient/wbclient.exports
>> >+
>> >
>> > include_HEADERS = \
>> > src/providers/ipa/ipa_hbac.h \
>> > src/lib/idmap/sss_idmap.h \
>> >- src/sss_client/idmap/sss_nss_idmap.h
>> >+ src/sss_client/idmap/sss_nss_idmap.h \
>> >+ src/sss_client/libwbclient/wbclient.h
>> Could you add $(NULL) as well?
>
>done
>
>>
>> > if BUILD_IFP
>> > lib_LTLIBRARIES += libsss_simpleifp.la
>> >diff --git a/configure.ac b/configure.ac
>> >index 3865421..d20cb9c 100644
>> >
>
>> >@@ -821,6 +837,16 @@ rm -rf $RPM_BUILD_ROOT
>> > %defattr(-,root,root,-)
>> > %{python_sitearch}/pyhbac.so
>> >
>> >+%files libwbclient
>> >+%defattr(-,root,root,-)
>> >+%{_libdir}/libwbclient.so.*
>> This library will be installed to the default libdir; the same as
>> libwbclient.so from libwbclient from samba.
>
>This is intentional because only the SSSD or Samba version should be
>installed.
>
>> Do we need conflicts or something different? Do we want to have different
>> name
>> for package?
>
>I think the two packages will conflict automatically and no additional
>options are needed here (I'm not an expert here, please correct me if
>I'm wrong).
>
>If distribution do not like the conflict schemes like /sbin/alternatives
>can be used, but this will be distribution specific and requires changes
>in the samba package as well, so I think it is not suitable for the
>upstream spec file.
>
I realised that libwbclient will have prefix sssd-. I didn't test it at frst
time, because macro "%files" does not have argument -n.
The problem is that it will cause problems with installation sssd.
sssd will require sssd-libwbclient
sssd-ad(gpo) -> libsmbclient -> samba-common -> libwbclient
There is conflict which we need to solve.
[root@host ~]# dnf install -y sssd-libwbclient
Dependencies resolved.
===============================================================================
Package Arch Version Repository
Size
===============================================================================
Installing:
sssd-libwbclient
x86_64 1.12.1-0.20140723.1323.gitcd61aff.mit.fc20 localrepo 14 k
Transaction Summary
===============================================================================
Install 1 Package
Total size: 14 k
Installed size: 27 k
Downloading Packages:
Running transaction check
Transaction check succeeded.
Running transaction test
Error: Transaction check error:
file /usr/lib64/libwbclient.so.0 from install of
sssd-libwbclient-1.12.1-0.fc20.x86_64 conflicts with file from package
libwbclient-2:4.1.9-3.fc20.x86_64
Error Summary
-------------
Some more experinced fedora package maintaner should help us wit this.
Jakub? :-)
>> sh-4.2$ rpm -ql libwbclient-4.1.9-3.fc20.x86_64.rpm
>> /usr/lib64/libwbclient.so.0
>> /usr/lib64/libwbclient.so.0.11
>> /usr/lib64/samba/libwinbind-client.so
>>
>> >+
>
>
>> >+
>> >+/* Required Headers */
>> >+#include <sss_nss_idmap.h>
>> We will not include external header file. it would work, but it can be
>> confusing.
>
>done, I've change it to
>#include "sss_client/idmap/sss_nss_idmap.h"
>(see above)
>
>>
>> >+
>> >+
>
>
>> >+ memset(¶ms, 0, sizeof(params));
>> Could you use "= {0,}" for initialisation.
>> I saw it on another place in libwbclient code.
>
>done
>
>>
>> >+
>> >+ memset(¶ms, 0, sizeof(params));
>> the same here
>
>done
>
>>
>
>
>
>> >+
>> >+ do {
>> >+ if (buflen == 0) {
>> >+ buflen = DEFAULT_BUFSIZE;
>> >+ } else {
>> >+ buflen *= 2;
>> >+ }
>> I think next linkes look better:
>> buflen = DEFAULT_BUFSIZE / 2;
>> do {
>> buflen *= 2;
>>
>> The same in function wbcGetgrgid and wbcGetgrent
>
>done, please note that I have introduced DEFAULT_BUFSIZE_HALF to avoid
>the division.
>
>>
>
>
>> >+ status = ctx->initgroups_dyn(pwd->pw_name, pwd->pw_gid, &start,
>> >+ &gr_size, &gids, -1, &nss_errno);
>> >+ wbc_status = nss_to_wbc(status);
>> >+ if (!WBC_ERROR_IS_OK(wbc_status)) {
>> >+ goto done;
>> //alocated memory in gids will be lost in done section
>
>fixed
>
>> >+ }
>> >+/* Required Headers */
>> >+#define _GNU_SOURCE /* for asprintf */
>> Could you include "config.h" instaead of using _GNU_SOURCE?
>> All necessary macros for extensions should be detected in config.h.
>
>done
>
>>
>
>> >+
>> >+ *pdomain = wbcStrDup(p + 1);
>> >+ if (*pdomain == NULL) {
>> >+ wbc_status = WBC_ERR_NO_MEMORY;
>> >+ goto done;
>> there would be memory leak, because *pname is not freed.
>
>fixed
>
>> >+ }
>
>
>> > wbcEndgrent;
>> > wbcGetgrent;
>> > wbcAuthenticateUser;
>> > wbcGetpwnam;
>> > wbcLookupDomainControllerEx;
>> > wbcLogoffUserEx;
>> > wbcSetUidMapping;
>> > wbcSidToString;
>> > wbcUidToSid;
>> >};
>> >
>> >WBCLIENT_0.10 {
>> > global:
>> > wbcPingDc2;
>> >} WBCLIENT_0.9;
>> >
>> >WBCLIENT_0.11 {
>> > global:
>> > wbc*;
>> > local:
>> > *;
>> ^^^^^^
>> In most libraries, local: is in 1st version. (@see libraries in
>> systemd:-)
>> >};
>>
>> I checked original libwbclient library and the last version does not have any
>> functions. It can be empty e.g. "WBCLIENT_0.11 {};"
>> sh-4.2$ readelf --dyn-syms /usr/lib64/libwbclient.so.0 | grep WBCLIENT_0.1
>> 30: 0000000000003890 272 FUNC GLOBAL DEFAULT 12
>> wbcPingDc2@@WBCLIENT_0.10
>> 61: 0000000000000000 0 OBJECT GLOBAL DEFAULT ABS WBCLIENT_0.10
>> 68: 0000000000000000 0 OBJECT GLOBAL DEFAULT ABS WBCLIENT_0.11
>>
>
>I copied the file that is used to build the Samba version of the library
>and I would prefer to keep it unchanged to avoid issues. This file is
>auto-generated during the samba build process based on ABI definitions.
>I guess this is the reason why it looks a bit odd.
I agree.
If this generated file is copied from samba then we chould not change it.
I found only wbclient-*.sigs files in samba git (nsswitch/libwbclient/ABI/),
but I dind't build samba.
>
>>
>>
>>
>> There is cast-align warning
>> ../sssd/src/sss_client/libwbclient/wbclient_common.c:85:12: error: cast from
>> 'char *' to
>> 'struct wbcMemPrefix *' increases required alignment from 1 to 8
>> [-Werror,-Wcast-align]
>> return (struct wbcMemPrefix *)(((char *)ptr) - wbcPrefixLen());
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 1 error generated.
>>
>> Next diff should fix problem. We needn't include util_safealign if you don't
>> want, because this file is from samba.
>>
>> --- a/src/sss_client/libwbclient/wbclient_common.c
>> +++ b/src/sss_client/libwbclient/wbclient_common.c
>> @@ -23,6 +23,7 @@
>> /* Required Headers */
>>
>> #include "libwbclient.h"
>> +#include "util/util_safealign.h"
>>
>> /** @brief Translate an error value into a string
>> *
>> @@ -82,7 +83,7 @@ static size_t wbcPrefixLen(void)
>>
>> static struct wbcMemPrefix *wbcMemToPrefix(void *ptr)
>> {
>> - return (struct wbcMemPrefix *)(((char *)ptr) - wbcPrefixLen());
>> + return DISCARD_ALIGN(((char *)ptr) - wbcPrefixLen(), struct
>> wbcMemPrefix*);
>
>I've added a cast to (void *) instead of using the macro, I hope this
>will fix the warning.
>
Problem fixed. Do we want to send patch to samba upstream to keep files the
same?
>> }
>>
>> void *wbcAllocateMemory(size_t nelem, size_t elsize,
>>
>> LS
>
>new version attached.
>
>bye,
>Sumit
>From 3a0976abbca796a163a855d90c146b04d811c1ae Mon Sep 17 00:00:00 2001
>From: Sumit Bose <sb...@redhat.com>
>Date: Tue, 15 Jul 2014 18:13:24 +0200
>Subject: [PATCH] libwbclient: SSSD implementation
>
>This patch implements the libwbclient API for Samba daemons and
>utilities. The main purpose is to map Active Directory users and groups
>identified by their SID to POSIX users and groups identified by their
>POSIX UIDs and GIDs respectively.
>
>The API is not fully implemented because SSSD does not support some AD
>features like WINS or NTLM. Additionally this implementation has its
>focus on the file-server use case and hence does not implement some
>features which might be needed for a domain controller use case.
>
>Some API calls are generic and independent of the backend like e.g.
>converting binary SIDs and GUIDs into a string representation and back
>or memory allocation and deallocation. These parts are taken from the
>original Samba sources together with copyright and authors. Files
>with'_sssd' as part of the name contain the SSSD related calls.
>
>Resolves: https://fedorahosted.org/sssd/ticket/1588
>---
Memory leaks were fixed, build issues and nitpicks as well. We need to just
solve packaging.
Patch LGTM. I know there was some review on samba-technical, but it would be
good to have ACK from more experienced samba developer in CC (Alexander, Simo)
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
