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?
>+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.
>+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.
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@(a)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.
>
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.
> }
>
> void *wbcAllocateMemory(size_t nelem, size_t elsize,
> LS
new version attached.
bye,
Sumit