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@(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.
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 <sbose(a)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