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
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 ..."
From 676cf42d5749c9a76f145968c9e0afa6438ba700 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@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
Makefile.am | 33 +- configure.ac | 1 + contrib/sssd.spec.in | 26 + src/sss_client/libwbclient/libwbclient.h | 46 + src/sss_client/libwbclient/wbc_err_internal.h | 44 + src/sss_client/libwbclient/wbc_guid.c | 100 ++ src/sss_client/libwbclient/wbc_idmap_common.c | 89 ++ src/sss_client/libwbclient/wbc_idmap_sssd.c | 204 ++++ src/sss_client/libwbclient/wbc_pam_sssd.c | 151 +++ src/sss_client/libwbclient/wbc_pwd_sssd.c | 659 ++++++++++++ src/sss_client/libwbclient/wbc_sid_common.c | 199 ++++ src/sss_client/libwbclient/wbc_sid_sssd.c | 276 +++++ src/sss_client/libwbclient/wbc_sssd_internal.h | 41 + src/sss_client/libwbclient/wbc_util_common.c | 97 ++ src/sss_client/libwbclient/wbc_util_sssd.c | 160 +++ src/sss_client/libwbclient/wbclient.exports | 90 ++ src/sss_client/libwbclient/wbclient.h | 1372 ++++++++++++++++++++++++ src/sss_client/libwbclient/wbclient.pc.in | 11 + src/sss_client/libwbclient/wbclient_common.c | 178 +++ src/sss_client/libwbclient/wbclient_internal.h | 44 + src/sss_client/libwbclient/wbclient_sssd.c | 40 + 21 files changed, 3859 insertions(+), 2 deletions(-) create mode 100644 src/sss_client/libwbclient/libwbclient.h create mode 100644 src/sss_client/libwbclient/wbc_err_internal.h create mode 100644 src/sss_client/libwbclient/wbc_guid.c create mode 100644 src/sss_client/libwbclient/wbc_idmap_common.c create mode 100644 src/sss_client/libwbclient/wbc_idmap_sssd.c create mode 100644 src/sss_client/libwbclient/wbc_pam_sssd.c create mode 100644 src/sss_client/libwbclient/wbc_pwd_sssd.c create mode 100644 src/sss_client/libwbclient/wbc_sid_common.c create mode 100644 src/sss_client/libwbclient/wbc_sid_sssd.c create mode 100644 src/sss_client/libwbclient/wbc_sssd_internal.h create mode 100644 src/sss_client/libwbclient/wbc_util_common.c create mode 100644 src/sss_client/libwbclient/wbc_util_sssd.c create mode 100644 src/sss_client/libwbclient/wbclient.exports create mode 100644 src/sss_client/libwbclient/wbclient.h create mode 100644 src/sss_client/libwbclient/wbclient.pc.in create mode 100644 src/sss_client/libwbclient/wbclient_common.c create mode 100644 src/sss_client/libwbclient/wbclient_internal.h create mode 100644 src/sss_client/libwbclient/wbclient_sssd.c
diff --git a/Makefile.am b/Makefile.am index e359286..adb981b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -574,6 +574,10 @@ dist_noinst_HEADERS = \ src/tests/cmocka/common_mock_sysdb_objects.h \ src/sss_client/ssh/sss_ssh_client.h \ src/sss_client/sudo/sss_sudo.h \
- src/sss_client/libwbclient/libwbclient.h \
- src/sss_client/libwbclient/wbc_err_internal.h \
- src/sss_client/libwbclient/wbclient_internal.h \
- src/sss_client/libwbclient/wbc_sssd_internal.h \ src/lib/idmap/sss_idmap_private.h \ src/lib/sifp/sss_sifp_private.h
@@ -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.
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
+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.
+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?
if BUILD_IFP lib_LTLIBRARIES += libsss_simpleifp.la diff --git a/configure.ac b/configure.ac index 3865421..d20cb9c 100644 --- a/configure.ac +++ b/configure.ac @@ -323,6 +323,7 @@ AC_CONFIG_FILES([Makefile contrib/sssd.spec src/examples/rwtab src/doxy.config src/sss_client/sudo/sss_sudo.doxy src/sss_client/idmap/sss_nss_idmap.pc src/sss_client/idmap/sss_nss_idmap.doxy
src/sss_client/libwbclient/wbclient.pc src/lib/sifp/sss_simpleifp.pc src/lib/sifp/sss_simpleifp.doxy src/config/setup.py
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index 82d8e89..d781936 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -427,6 +427,22 @@ Requires: libsss_simpleifp = %{version}-%{release} %description -n libsss_simpleifp-devel Provides library that simplifies D-Bus API for the SSSD InfoPipe responder.
+%package libwbclient +Summary: The SSSD libwbclient implementation +Group: Applications/System +License: GPLv3+ and LGPLv3+
+%description libwbclient +The SSSD libwbclient implementation.
+%package libwbclient-devel +Summary: Development libraries for the SSSD libwbclient implementation +Group: Development/Libraries +License: GPLv3+ and LGPLv3+
+%description libwbclient-devel +Development libraries for the SSSD libwbclient implementation.
%prep %setup -q -n %{name}-%{version}
@@ -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. Do we need conflicts or something different? Do we want to have different name for package?
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
+%files libwbclient-devel +%defattr(-,root,root,-) +%{_includedir}/wbclient.h +%{_libdir}/libwbclient.so +%{_libdir}/pkgconfig/wbclient.pc
%if (0%{?use_systemd} == 1) # systemd %post common diff --git a/src/sss_client/libwbclient/libwbclient.h b/src/sss_client/libwbclient/libwbclient.h new file mode 100644 index 0000000..79d9be2 --- /dev/null +++ b/src/sss_client/libwbclient/libwbclient.h @@ -0,0 +1,46 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API
- Copyright (C) Gerald (Jerry) Carter 2007
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
+:set number
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#ifndef _LIBWBCLIENT_H +#define _LIBWBCLIENT_H
+#include <stdint.h> +#include <stdbool.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h>
+/* Super header including necessary public and private header files
- for building the wbclient library. __DO NOT__ define anything
- in this file. Only include other headers. */
+/* Public headers */
+#include "wbclient.h"
+/* Private headers */
+#include "wbc_err_internal.h" +#include "wbclient_internal.h"
+#endif /* _LIBWBCLIENT_H */ diff --git a/src/sss_client/libwbclient/wbc_idmap_sssd.c b/src/sss_client/libwbclient/wbc_idmap_sssd.c new file mode 100644 index 0000000..430de0c --- /dev/null +++ b/src/sss_client/libwbclient/wbc_idmap_sssd.c @@ -0,0 +1,204 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API - SSSD version
- Copyright (C) Sumit Bose sbose@redhat.com 2014
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+/* Required Headers */ +#include <sss_nss_idmap.h>
We will not include external header file. it would work, but it can be confusing.
+#include "libwbclient.h" +#include "wbc_sssd_internal.h"
diff --git a/src/sss_client/libwbclient/wbc_pam_sssd.c b/src/sss_client/libwbclient/wbc_pam_sssd.c new file mode 100644 index 0000000..0bd27ce --- /dev/null +++ b/src/sss_client/libwbclient/wbc_pam_sssd.c @@ -0,0 +1,151 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API - SSSD version
- Copyright (C) Sumit Bose sbose@redhat.com 2014
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+/* Required Headers */ +#include "libwbclient.h" +#include "wbc_sssd_internal.h"
+/* Authenticate a username/password pair */ +wbcErr wbcAuthenticateUser(const char *username,
const char *password)
+{
- wbcErr wbc_status = WBC_ERR_SUCCESS;
- struct wbcAuthUserParams params;
- memset(¶ms, 0, sizeof(params));
Could you use "= {0,}" for initialisation. I saw it on another place in libwbclient code.
- params.account_name = username;
- params.level = WBC_AUTH_USER_LEVEL_PLAIN;
- params.password.plaintext = password;
- wbc_status = wbcAuthenticateUserEx(¶ms, NULL, NULL);
- return wbc_status;
+}
//snip
+/* Change a password for a user */ +wbcErr wbcChangeUserPassword(const char *username,
const char *old_password,
const char *new_password)
+{
- wbcErr wbc_status = WBC_ERR_SUCCESS;
- struct wbcChangePasswordParams params;
- memset(¶ms, 0, sizeof(params));
the same here
- params.account_name = username;
- params.level = WBC_CHANGE_PASSWORD_LEVEL_PLAIN;
- params.old_password.plaintext = old_password;
- params.new_password.plaintext = new_password;
- wbc_status = wbcChangeUserPasswordEx(¶ms, NULL, NULL, NULL);
- return wbc_status;
+}
+/* Logon a User */ +wbcErr wbcLogonUser(const struct wbcLogonUserParams *params,
struct wbcLogonUserInfo **info,
struct wbcAuthErrorInfo **error,
struct wbcUserPasswordPolicyInfo **policy)
+{
- WBC_SSSD_NOT_IMPLEMENTED;
+} diff --git a/src/sss_client/libwbclient/wbc_pwd_sssd.c b/src/sss_client/libwbclient/wbc_pwd_sssd.c new file mode 100644 index 0000000..8f4f375 --- /dev/null +++ b/src/sss_client/libwbclient/wbc_pwd_sssd.c @@ -0,0 +1,659 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API - SSSD version
- Copyright (C) Sumit Bose sbose@redhat.com 2014
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/ +/* Required Headers */
//snip
+/* Fill in a struct passwd* for a domain user based on username */ +wbcErr wbcGetgrnam(const char *name, struct group **grp) +{
- struct group lgrp;
- enum nss_status status;
- char *newbuffer = NULL;
- char *buffer = NULL;
- size_t buflen = 0;
- wbcErr wbc_status;
- int nss_errno;
- if (ctx == NULL && !open_libnss_sss()) {
return WBC_ERR_NSS_ERROR;
- }
- if (name == NULL || grp == NULL) {
return WBC_ERR_INVALID_PARAM;
- }
- 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
newbuffer = realloc(buffer, buflen);
if (newbuffer == NULL) {
free(buffer);
return WBC_ERR_NO_MEMORY;
}
buffer = newbuffer;
memset(grp, 0, sizeof(struct group));
status = ctx->getgrnam_r(name, &lgrp, buffer, buflen, &nss_errno);
wbc_status = nss_to_wbc(status);
if (WBC_ERROR_IS_OK(wbc_status)) {
wbc_status = copy_grp(&lgrp, grp);
}
- } while (status == NSS_STATUS_TRYAGAIN && nss_errno == ERANGE \
&& buflen < MAX_BUFSIZE);
- free(buffer);
- return wbc_status;
+}
//snip
+/* Return the unix group array belonging to the given user */ +wbcErr wbcGetGroups(const char *account,
uint32_t *num_groups,
gid_t **_groups)
+{
- wbcErr wbc_status;
- enum nss_status status;
- struct passwd *pwd;
- long int gr_size = 0;
- long int start = 0;
- gid_t *gids = NULL;
- int nss_errno;
- wbc_status = wbcGetpwnam(account, &pwd);
- if (!WBC_ERROR_IS_OK(wbc_status)) {
return wbc_status;
- }
- gr_size = DEFAULT_BUFSIZE;
- gids = calloc(gr_size, sizeof(gid_t));
- if (gids == NULL) {
wbc_status = WBC_ERR_NO_MEMORY;
goto done;
- }
- /* nss modules may skip the primary group when we pass it in so always
* add it in advance */
- gids[0] = pwd->pw_gid;
- start++;
- 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
- }
- *_groups = gids;
- *num_groups = start;
- wbc_status = WBC_ERR_SUCCESS;
+done:
- wbcFreeMemory(pwd);
- return wbc_status;
+} diff --git a/src/sss_client/libwbclient/wbc_sid_sssd.c b/src/sss_client/libwbclient/wbc_sid_sssd.c new file mode 100644 index 0000000..699e1d3 --- /dev/null +++ b/src/sss_client/libwbclient/wbc_sid_sssd.c @@ -0,0 +1,276 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API - SSSD version
- Copyright (C) Sumit Bose sbose@redhat.com 2014
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+/* 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.
+#include <stdio.h>
+#include <errno.h>
+#include <sss_nss_idmap.h>
s/<sss_nss_idmap.h>/"sss_nss_idmap.h"/ This is the same situation as in wbc_idmap_sssd.c
+#include "libwbclient.h" +#include "wbc_sssd_internal.h"
+static int sss_id_type_to_wbcSidType(enum sss_id_type sss_type,
enum wbcSidType *name_type)
+{
- switch (sss_type) {
- case SSS_ID_TYPE_NOT_SPECIFIED:
*name_type = WBC_SID_NAME_USE_NONE;
break;
//snip
+/* Convert a SID to a domain and name */ +wbcErr wbcLookupSid(const struct wbcDomainSid *sid,
char **pdomain,
char **pname,
enum wbcSidType *pname_type)
+{
- char *str_sid;
- char *fq_name = NULL;
- enum sss_id_type type;
- int ret;
- char *p;
- wbcErr wbc_status;
- wbc_status = wbcSidToString(sid, &str_sid);
- if (!WBC_ERROR_IS_OK(wbc_status)) {
return wbc_status;
- }
- ret = sss_nss_getnamebysid(str_sid, &fq_name, &type);
- wbcFreeMemory(str_sid);
- if (ret != 0) {
return WBC_ERR_UNKNOWN_FAILURE;
- }
- ret = sss_id_type_to_wbcSidType(type, pname_type);
- if (ret != 0) {
wbc_status = WBC_ERR_UNKNOWN_FAILURE;
goto done;
- }
- /* TODO: it would be nice to have a sss_nss_getnamebysid() call which
- return name and domain separately. */
- p = strchr(fq_name, '@');
- if (p == NULL) {
wbc_status = WBC_ERR_UNKNOWN_FAILURE;
goto done;
- }
- *p = '\0';
- *pname = wbcStrDup(fq_name);
- if (*pname == NULL) {
wbc_status = WBC_ERR_NO_MEMORY;
goto 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.
- }
- wbc_status = WBC_ERR_SUCCESS;
+done:
- free(fq_name);
- return wbc_status;
+}
diff --git a/src/sss_client/libwbclient/wbc_sssd_internal.h b/src/sss_client/libwbclient/wbc_sssd_internal.h new file mode 100644 index 0000000..e20de48 --- /dev/null +++ b/src/sss_client/libwbclient/wbc_sssd_internal.h @@ -0,0 +1,41 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API - SSSD version
- Copyright (C) Sumit Bose sbose@redhat.com 2014
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#ifndef _WBC_SSSD_INTERNAL_H +#define _WBC_SSSD_INTERNAL_H
+#include <syslog.h>
+#include "libwbclient.h"
+#if defined(DEVELOPER) +#define WBC_SSSD_DEV_LOG syslog(LOG_DEBUG, "libwbclient_sssd: %s", __FUNCTION__);
Would _FILE__, __LINE__ be useful here?
+#else +#define WBC_SSSD_DEV_LOG +#endif
+#define WBC_SSSD_NOT_IMPLEMENTED \
- do { \
WBC_SSSD_DEV_LOG; \
return WBC_ERR_NOT_IMPLEMENTED; \
- } while(0)
+#endif /* _WBC_SSSD_INTERNAL_H */
src/sss_client/libwbclient/wbclient.exports WBCLIENT_0.9 { global: wbcGetpwuid; wbcLogoffUser; wbcSidToStringBuf; wbcLogonUser; wbcGetgrgid; wbcSetGidMapping; wbcQueryGidToSid; wbcListTrusts; wbcGetGroups; wbcDomainInfo; wbcSidToGid; wbcLookupRids; wbcCredentialCache; wbcDcInfo; wbcAuthenticateUserEx; wbcGetpwent; wbcGetSidAliases; wbcGetDisplayName; wbcAllocateUid; wbcSidToUid; wbcChangeTrustCredentials; wbcGetpwsid; wbcPingDc; wbcAllocateStringArray; wbcErrorString; wbcStringToGuid; wbcStrDup; wbcGetgrnam; wbcGetgrlist; wbcListUsers; wbcRemoveUidMapping; wbcLookupDomainController; wbcRemoveGidMapping; wbcSidTypeString; wbcAllocateMemory; wbcInterfaceDetails; wbcCheckTrustCredentials; wbcListGroups; wbcLookupUserSids; wbcResolveWinsByName; wbcSetpwent; wbcSetUidHwm; wbcSidsToUnixIds; wbcQuerySidToGid; wbcChangeUserPasswordEx; wbcPing; wbcQueryUidToSid; wbcEndpwent; wbcLibraryDetails; wbcSetgrent; wbcLookupName; wbcChangeUserPassword; wbcSetGidHwm; wbcAddNamedBlob; wbcGuidToString; wbcLookupSids; wbcRequestResponsePriv; wbcAllocateGid; wbcFreeMemory; wbcResolveWinsByIP; wbcRequestResponse; wbcStringToSid; wbcLookupSid; wbcCredentialSave; wbcGidToSid; wbcQuerySidToUid; 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
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*); }
void *wbcAllocateMemory(size_t nelem, size_t elsize,
LS
On (23/07/14 00:58), 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 ..."
From 676cf42d5749c9a76f145968c9e0afa6438ba700 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@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
Makefile.am | 33 +- configure.ac | 1 + contrib/sssd.spec.in | 26 + src/sss_client/libwbclient/libwbclient.h | 46 + src/sss_client/libwbclient/wbc_err_internal.h | 44 + src/sss_client/libwbclient/wbc_guid.c | 100 ++ src/sss_client/libwbclient/wbc_idmap_common.c | 89 ++ src/sss_client/libwbclient/wbc_idmap_sssd.c | 204 ++++ src/sss_client/libwbclient/wbc_pam_sssd.c | 151 +++ src/sss_client/libwbclient/wbc_pwd_sssd.c | 659 ++++++++++++ src/sss_client/libwbclient/wbc_sid_common.c | 199 ++++ src/sss_client/libwbclient/wbc_sid_sssd.c | 276 +++++ src/sss_client/libwbclient/wbc_sssd_internal.h | 41 + src/sss_client/libwbclient/wbc_util_common.c | 97 ++ src/sss_client/libwbclient/wbc_util_sssd.c | 160 +++ src/sss_client/libwbclient/wbclient.exports | 90 ++ src/sss_client/libwbclient/wbclient.h | 1372 ++++++++++++++++++++++++ src/sss_client/libwbclient/wbclient.pc.in | 11 + src/sss_client/libwbclient/wbclient_common.c | 178 +++ src/sss_client/libwbclient/wbclient_internal.h | 44 + src/sss_client/libwbclient/wbclient_sssd.c | 40 + 21 files changed, 3859 insertions(+), 2 deletions(-) create mode 100644 src/sss_client/libwbclient/libwbclient.h create mode 100644 src/sss_client/libwbclient/wbc_err_internal.h create mode 100644 src/sss_client/libwbclient/wbc_guid.c create mode 100644 src/sss_client/libwbclient/wbc_idmap_common.c create mode 100644 src/sss_client/libwbclient/wbc_idmap_sssd.c create mode 100644 src/sss_client/libwbclient/wbc_pam_sssd.c create mode 100644 src/sss_client/libwbclient/wbc_pwd_sssd.c create mode 100644 src/sss_client/libwbclient/wbc_sid_common.c create mode 100644 src/sss_client/libwbclient/wbc_sid_sssd.c create mode 100644 src/sss_client/libwbclient/wbc_sssd_internal.h create mode 100644 src/sss_client/libwbclient/wbc_util_common.c create mode 100644 src/sss_client/libwbclient/wbc_util_sssd.c create mode 100644 src/sss_client/libwbclient/wbclient.exports create mode 100644 src/sss_client/libwbclient/wbclient.h create mode 100644 src/sss_client/libwbclient/wbclient.pc.in create mode 100644 src/sss_client/libwbclient/wbclient_common.c create mode 100644 src/sss_client/libwbclient/wbclient_internal.h create mode 100644 src/sss_client/libwbclient/wbclient_sssd.c
diff --git a/Makefile.am b/Makefile.am index e359286..adb981b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -574,6 +574,10 @@ dist_noinst_HEADERS = \ src/tests/cmocka/common_mock_sysdb_objects.h \ src/sss_client/ssh/sss_ssh_client.h \ src/sss_client/sudo/sss_sudo.h \
- src/sss_client/libwbclient/libwbclient.h \
- src/sss_client/libwbclient/wbc_err_internal.h \
- src/sss_client/libwbclient/wbclient_internal.h \
- src/sss_client/libwbclient/wbc_sssd_internal.h \ src/lib/idmap/sss_idmap_private.h \ src/lib/sifp/sss_sifp_private.h
@@ -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.
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
+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.
+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?
if BUILD_IFP lib_LTLIBRARIES += libsss_simpleifp.la diff --git a/configure.ac b/configure.ac index 3865421..d20cb9c 100644 --- a/configure.ac +++ b/configure.ac @@ -323,6 +323,7 @@ AC_CONFIG_FILES([Makefile contrib/sssd.spec src/examples/rwtab src/doxy.config src/sss_client/sudo/sss_sudo.doxy src/sss_client/idmap/sss_nss_idmap.pc src/sss_client/idmap/sss_nss_idmap.doxy
src/sss_client/libwbclient/wbclient.pc src/lib/sifp/sss_simpleifp.pc src/lib/sifp/sss_simpleifp.doxy src/config/setup.py
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index 82d8e89..d781936 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -427,6 +427,22 @@ Requires: libsss_simpleifp = %{version}-%{release} %description -n libsss_simpleifp-devel Provides library that simplifies D-Bus API for the SSSD InfoPipe responder.
+%package libwbclient +Summary: The SSSD libwbclient implementation +Group: Applications/System +License: GPLv3+ and LGPLv3+
+%description libwbclient +The SSSD libwbclient implementation.
+%package libwbclient-devel +Summary: Development libraries for the SSSD libwbclient implementation +Group: Development/Libraries +License: GPLv3+ and LGPLv3+
+%description libwbclient-devel +Development libraries for the SSSD libwbclient implementation.
%prep %setup -q -n %{name}-%{version}
@@ -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. Do we need conflicts or something different? Do we want to have different name for package?
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
+%files libwbclient-devel +%defattr(-,root,root,-) +%{_includedir}/wbclient.h +%{_libdir}/libwbclient.so +%{_libdir}/pkgconfig/wbclient.pc
%if (0%{?use_systemd} == 1) # systemd %post common diff --git a/src/sss_client/libwbclient/libwbclient.h b/src/sss_client/libwbclient/libwbclient.h new file mode 100644 index 0000000..79d9be2 --- /dev/null +++ b/src/sss_client/libwbclient/libwbclient.h @@ -0,0 +1,46 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API
- Copyright (C) Gerald (Jerry) Carter 2007
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
+:set number
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#ifndef _LIBWBCLIENT_H +#define _LIBWBCLIENT_H
+#include <stdint.h> +#include <stdbool.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h>
+/* Super header including necessary public and private header files
- for building the wbclient library. __DO NOT__ define anything
- in this file. Only include other headers. */
+/* Public headers */
+#include "wbclient.h"
+/* Private headers */
+#include "wbc_err_internal.h" +#include "wbclient_internal.h"
+#endif /* _LIBWBCLIENT_H */ diff --git a/src/sss_client/libwbclient/wbc_idmap_sssd.c b/src/sss_client/libwbclient/wbc_idmap_sssd.c new file mode 100644 index 0000000..430de0c --- /dev/null +++ b/src/sss_client/libwbclient/wbc_idmap_sssd.c @@ -0,0 +1,204 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API - SSSD version
- Copyright (C) Sumit Bose sbose@redhat.com 2014
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+/* Required Headers */ +#include <sss_nss_idmap.h>
We will not include external header file. it would work, but it can be confusing.
+#include "libwbclient.h" +#include "wbc_sssd_internal.h"
diff --git a/src/sss_client/libwbclient/wbc_pam_sssd.c b/src/sss_client/libwbclient/wbc_pam_sssd.c new file mode 100644 index 0000000..0bd27ce --- /dev/null +++ b/src/sss_client/libwbclient/wbc_pam_sssd.c @@ -0,0 +1,151 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API - SSSD version
- Copyright (C) Sumit Bose sbose@redhat.com 2014
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+/* Required Headers */ +#include "libwbclient.h" +#include "wbc_sssd_internal.h"
+/* Authenticate a username/password pair */ +wbcErr wbcAuthenticateUser(const char *username,
const char *password)
+{
- wbcErr wbc_status = WBC_ERR_SUCCESS;
- struct wbcAuthUserParams params;
- memset(¶ms, 0, sizeof(params));
Could you use "= {0,}" for initialisation. I saw it on another place in libwbclient code.
- params.account_name = username;
- params.level = WBC_AUTH_USER_LEVEL_PLAIN;
- params.password.plaintext = password;
- wbc_status = wbcAuthenticateUserEx(¶ms, NULL, NULL);
- return wbc_status;
+}
//snip
+/* Change a password for a user */ +wbcErr wbcChangeUserPassword(const char *username,
const char *old_password,
const char *new_password)
+{
- wbcErr wbc_status = WBC_ERR_SUCCESS;
- struct wbcChangePasswordParams params;
- memset(¶ms, 0, sizeof(params));
the same here
- params.account_name = username;
- params.level = WBC_CHANGE_PASSWORD_LEVEL_PLAIN;
- params.old_password.plaintext = old_password;
- params.new_password.plaintext = new_password;
- wbc_status = wbcChangeUserPasswordEx(¶ms, NULL, NULL, NULL);
- return wbc_status;
+}
+/* Logon a User */ +wbcErr wbcLogonUser(const struct wbcLogonUserParams *params,
struct wbcLogonUserInfo **info,
struct wbcAuthErrorInfo **error,
struct wbcUserPasswordPolicyInfo **policy)
+{
- WBC_SSSD_NOT_IMPLEMENTED;
+} diff --git a/src/sss_client/libwbclient/wbc_pwd_sssd.c b/src/sss_client/libwbclient/wbc_pwd_sssd.c new file mode 100644 index 0000000..8f4f375 --- /dev/null +++ b/src/sss_client/libwbclient/wbc_pwd_sssd.c @@ -0,0 +1,659 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API - SSSD version
- Copyright (C) Sumit Bose sbose@redhat.com 2014
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/ +/* Required Headers */
//snip
+/* Fill in a struct passwd* for a domain user based on username */ +wbcErr wbcGetgrnam(const char *name, struct group **grp) +{
- struct group lgrp;
- enum nss_status status;
- char *newbuffer = NULL;
- char *buffer = NULL;
- size_t buflen = 0;
- wbcErr wbc_status;
- int nss_errno;
- if (ctx == NULL && !open_libnss_sss()) {
return WBC_ERR_NSS_ERROR;
- }
- if (name == NULL || grp == NULL) {
return WBC_ERR_INVALID_PARAM;
- }
- 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
newbuffer = realloc(buffer, buflen);
if (newbuffer == NULL) {
free(buffer);
return WBC_ERR_NO_MEMORY;
}
buffer = newbuffer;
memset(grp, 0, sizeof(struct group));
status = ctx->getgrnam_r(name, &lgrp, buffer, buflen, &nss_errno);
wbc_status = nss_to_wbc(status);
if (WBC_ERROR_IS_OK(wbc_status)) {
wbc_status = copy_grp(&lgrp, grp);
}
- } while (status == NSS_STATUS_TRYAGAIN && nss_errno == ERANGE \
&& buflen < MAX_BUFSIZE);
- free(buffer);
- return wbc_status;
+}
//snip
+/* Return the unix group array belonging to the given user */ +wbcErr wbcGetGroups(const char *account,
uint32_t *num_groups,
gid_t **_groups)
+{
- wbcErr wbc_status;
- enum nss_status status;
- struct passwd *pwd;
- long int gr_size = 0;
- long int start = 0;
- gid_t *gids = NULL;
- int nss_errno;
- wbc_status = wbcGetpwnam(account, &pwd);
- if (!WBC_ERROR_IS_OK(wbc_status)) {
return wbc_status;
- }
- gr_size = DEFAULT_BUFSIZE;
- gids = calloc(gr_size, sizeof(gid_t));
- if (gids == NULL) {
wbc_status = WBC_ERR_NO_MEMORY;
goto done;
- }
- /* nss modules may skip the primary group when we pass it in so always
* add it in advance */
- gids[0] = pwd->pw_gid;
- start++;
- 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
- }
- *_groups = gids;
- *num_groups = start;
- wbc_status = WBC_ERR_SUCCESS;
+done:
- wbcFreeMemory(pwd);
- return wbc_status;
+} diff --git a/src/sss_client/libwbclient/wbc_sid_sssd.c b/src/sss_client/libwbclient/wbc_sid_sssd.c new file mode 100644 index 0000000..699e1d3 --- /dev/null +++ b/src/sss_client/libwbclient/wbc_sid_sssd.c @@ -0,0 +1,276 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API - SSSD version
- Copyright (C) Sumit Bose sbose@redhat.com 2014
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+/* 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.
+#include <stdio.h>
+#include <errno.h>
+#include <sss_nss_idmap.h>
s/<sss_nss_idmap.h>/"sss_nss_idmap.h"/ This is the same situation as in wbc_idmap_sssd.c
+#include "libwbclient.h" +#include "wbc_sssd_internal.h"
+static int sss_id_type_to_wbcSidType(enum sss_id_type sss_type,
enum wbcSidType *name_type)
+{
- switch (sss_type) {
- case SSS_ID_TYPE_NOT_SPECIFIED:
*name_type = WBC_SID_NAME_USE_NONE;
break;
//snip
+/* Convert a SID to a domain and name */ +wbcErr wbcLookupSid(const struct wbcDomainSid *sid,
char **pdomain,
char **pname,
enum wbcSidType *pname_type)
+{
- char *str_sid;
- char *fq_name = NULL;
- enum sss_id_type type;
- int ret;
- char *p;
- wbcErr wbc_status;
- wbc_status = wbcSidToString(sid, &str_sid);
- if (!WBC_ERROR_IS_OK(wbc_status)) {
return wbc_status;
- }
- ret = sss_nss_getnamebysid(str_sid, &fq_name, &type);
- wbcFreeMemory(str_sid);
- if (ret != 0) {
return WBC_ERR_UNKNOWN_FAILURE;
- }
- ret = sss_id_type_to_wbcSidType(type, pname_type);
- if (ret != 0) {
wbc_status = WBC_ERR_UNKNOWN_FAILURE;
goto done;
- }
- /* TODO: it would be nice to have a sss_nss_getnamebysid() call which
- return name and domain separately. */
- p = strchr(fq_name, '@');
- if (p == NULL) {
wbc_status = WBC_ERR_UNKNOWN_FAILURE;
goto done;
- }
- *p = '\0';
- *pname = wbcStrDup(fq_name);
- if (*pname == NULL) {
wbc_status = WBC_ERR_NO_MEMORY;
goto 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.
- }
- wbc_status = WBC_ERR_SUCCESS;
+done:
- free(fq_name);
- return wbc_status;
+}
diff --git a/src/sss_client/libwbclient/wbc_sssd_internal.h b/src/sss_client/libwbclient/wbc_sssd_internal.h new file mode 100644 index 0000000..e20de48 --- /dev/null +++ b/src/sss_client/libwbclient/wbc_sssd_internal.h @@ -0,0 +1,41 @@ +/*
- Unix SMB/CIFS implementation.
- Winbind client API - SSSD version
- Copyright (C) Sumit Bose sbose@redhat.com 2014
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Library General Public License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#ifndef _WBC_SSSD_INTERNAL_H +#define _WBC_SSSD_INTERNAL_H
+#include <syslog.h>
+#include "libwbclient.h"
+#if defined(DEVELOPER) +#define WBC_SSSD_DEV_LOG syslog(LOG_DEBUG, "libwbclient_sssd: %s", __FUNCTION__);
Would _FILE__, __LINE__ be useful here?
+#else +#define WBC_SSSD_DEV_LOG +#endif
+#define WBC_SSSD_NOT_IMPLEMENTED \
- do { \
WBC_SSSD_DEV_LOG; \
return WBC_ERR_NOT_IMPLEMENTED; \
- } while(0)
+#endif /* _WBC_SSSD_INTERNAL_H */
src/sss_client/libwbclient/wbclient.exports WBCLIENT_0.9 { global: wbcGetpwuid; wbcLogoffUser; wbcSidToStringBuf; wbcLogonUser; wbcGetgrgid; wbcSetGidMapping; wbcQueryGidToSid; wbcListTrusts; wbcGetGroups; wbcDomainInfo; wbcSidToGid; wbcLookupRids; wbcCredentialCache; wbcDcInfo; wbcAuthenticateUserEx; wbcGetpwent; wbcGetSidAliases; wbcGetDisplayName; wbcAllocateUid; wbcSidToUid; wbcChangeTrustCredentials; wbcGetpwsid; wbcPingDc; wbcAllocateStringArray; wbcErrorString; wbcStringToGuid; wbcStrDup; wbcGetgrnam; wbcGetgrlist; wbcListUsers; wbcRemoveUidMapping; wbcLookupDomainController; wbcRemoveGidMapping; wbcSidTypeString; wbcAllocateMemory; wbcInterfaceDetails; wbcCheckTrustCredentials; wbcListGroups; wbcLookupUserSids; wbcResolveWinsByName; wbcSetpwent; wbcSetUidHwm; wbcSidsToUnixIds; wbcQuerySidToGid; wbcChangeUserPasswordEx; wbcPing; wbcQueryUidToSid; wbcEndpwent; wbcLibraryDetails; wbcSetgrent; wbcLookupName; wbcChangeUserPassword; wbcSetGidHwm; wbcAddNamedBlob; wbcGuidToString; wbcLookupSids; wbcRequestResponsePriv; wbcAllocateGid; wbcFreeMemory; wbcResolveWinsByIP; wbcRequestResponse; wbcStringToSid; wbcLookupSid; wbcCredentialSave; wbcGidToSid; wbcQuerySidToUid; 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
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*);
}
void *wbcAllocateMemory(size_t nelem, size_t elsize,
And one more think. Could you also add new library to dlopen-test?
LS
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@@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
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 sbose@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
On Wed, Jul 23, 2014 at 02:02:16PM +0200, Lukas Slebodnik wrote:
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? :-)
I admit I haven't read the patches at all yet, so I'm just replying based on this e-mail conversation, but does winbind require that the filename is libwbclient.so? Can't we make the file called something else?
On Wed, Jul 23, 2014 at 03:16:03PM +0200, Jakub Hrozek wrote:
On Wed, Jul 23, 2014 at 02:02:16PM +0200, Lukas Slebodnik wrote:
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? :-)
I admit I haven't read the patches at all yet, so I'm just replying based on this e-mail conversation, but does winbind require that the filename is libwbclient.so? Can't we make the file called something else?
No because the binaries using libwbclient like wbinfo or smbd are linked against this name.
There are two items to consider here. First about the dependency chain. At least in the Fedora/RHEL samba spec files the samba package version is hardcoded in 'Requires: libwbclient = %{samba_depver}' i.e. only the samba implementation can fulfil this requirement.
The second is the conflict of the filenames which in theory can be removed by manually de-installing the offending package which currently won't work because of the dependency chain mentioned above.
Since on Fedora and RHEL a changes in samba packaging are needed either way I would recommend to use /sbin/alternatives to manage libwbclient.so.* . This requires that both SSSD and samba rename the libraries [*], e.g. libwbclient_sssd.so* and libwbclient_samba.so*, and let /sbin/alternatives manage libwbclient.so*. For the files in *-devel that same is needed. The cifs-client plugin is already managed in a similar way in the sssd spec file.
bye, Sumit
[*] Please note that in samba the library should be renamed only for packaging after 'make install' was run because otherwise the binaries will be linked against the new name and we are back to square 1. I would recommend to do the same in SSSD.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (23/07/14 15:51), Sumit Bose wrote:
On Wed, Jul 23, 2014 at 03:16:03PM +0200, Jakub Hrozek wrote:
On Wed, Jul 23, 2014 at 02:02:16PM +0200, Lukas Slebodnik wrote:
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? :-)
I admit I haven't read the patches at all yet, so I'm just replying based on this e-mail conversation, but does winbind require that the filename is libwbclient.so? Can't we make the file called something else?
No because the binaries using libwbclient like wbinfo or smbd are linked against this name.
There are two items to consider here. First about the dependency chain. At least in the Fedora/RHEL samba spec files the samba package version is hardcoded in 'Requires: libwbclient = %{samba_depver}' i.e. only the samba implementation can fulfil this requirement.
If we fix this strict requires yum swap will help.
[root@host ~]# yum swap -- remove sssd-libwbclient -- install libwbclient install sssd-libwbclient Loaded plugins: auto-update-debuginfo, langpacks Resolving Dependencies
//snip
Dependencies Resolved
================================================================================ Package Arch Version Repository Size ================================================================================ Installing: sssd-libwbclient x86_64 1.12.1-0.20140723.1323.gitcd61aff.fc20 localrepo 14 k Removing: libwbclient x86_64 2:4.1.9-3.fc20 installed 58 k Removing for dependencies: freeipa-client x86_64 3.3.5-1.fc20 installed 330 k libsmbclient x86_64 2:4.1.9-3.fc20 installed 158 k libsmbclient-devel x86_64 2:4.1.9-3.fc20 installed 121 k python-smbc x86_64 1.0.13-8.fc20 @fedora 59 k samba-client x86_64 2:4.1.9-3.fc20 installed 1.4 M samba-common x86_64 2:4.1.9-3.fc20 installed 1.7 M samba-devel x86_64 2:4.1.9-3.fc20 installed 1.4 M samba-libs x86_64 2:4.1.9-3.fc20 installed 16 M sssd x86_64 1.12.1-0.20140722.1109.git5debc61.fc20 installed 34 k sssd-ad x86_64 1.12.1-0.20140722.1109.git5debc61.fc20 installed 414 k sssd-common-pac x86_64 1.12.1-0.20140722.1109.git5debc61.fc20 installed 135 k sssd-ipa x86_64 1.12.1-0.20140722.1109.git5debc61.fc20 installed 344 k
Transaction Summary ================================================================================ Install 1 Package Remove 1 Package (+12 Dependent packages)
Total download size: 14 k Is this ok [y/d/N]: N
LS
On (23/07/14 17:42), Lukas Slebodnik wrote:
On (23/07/14 15:51), Sumit Bose wrote:
On Wed, Jul 23, 2014 at 03:16:03PM +0200, Jakub Hrozek wrote:
On Wed, Jul 23, 2014 at 02:02:16PM +0200, Lukas Slebodnik wrote:
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? :-)
I admit I haven't read the patches at all yet, so I'm just replying based on this e-mail conversation, but does winbind require that the filename is libwbclient.so? Can't we make the file called something else?
No because the binaries using libwbclient like wbinfo or smbd are linked against this name.
There are two items to consider here. First about the dependency chain. At least in the Fedora/RHEL samba spec files the samba package version is hardcoded in 'Requires: libwbclient = %{samba_depver}' i.e. only the samba implementation can fulfil this requirement.
If we fix this strict requires yum swap will help.
[root@host ~]# yum swap -- remove sssd-libwbclient -- install libwbclient install sssd-libwbclient
copy&paste problem it should hve been: yum swap -- remove libwbclient -- install sssd-libwbclient
Loaded plugins: auto-update-debuginfo, langpacks Resolving Dependencies
//snip
Dependencies Resolved
================================================================================ Package Arch Version Repository Size ================================================================================ Installing: sssd-libwbclient x86_64 1.12.1-0.20140723.1323.gitcd61aff.fc20 localrepo 14 k Removing: libwbclient x86_64 2:4.1.9-3.fc20 installed 58 k Removing for dependencies: freeipa-client x86_64 3.3.5-1.fc20 installed 330 k libsmbclient x86_64 2:4.1.9-3.fc20 installed 158 k libsmbclient-devel x86_64 2:4.1.9-3.fc20 installed 121 k python-smbc x86_64 1.0.13-8.fc20 @fedora 59 k samba-client x86_64 2:4.1.9-3.fc20 installed 1.4 M samba-common x86_64 2:4.1.9-3.fc20 installed 1.7 M samba-devel x86_64 2:4.1.9-3.fc20 installed 1.4 M samba-libs x86_64 2:4.1.9-3.fc20 installed 16 M sssd x86_64 1.12.1-0.20140722.1109.git5debc61.fc20 installed 34 k sssd-ad x86_64 1.12.1-0.20140722.1109.git5debc61.fc20 installed 414 k sssd-common-pac x86_64 1.12.1-0.20140722.1109.git5debc61.fc20 installed 135 k sssd-ipa x86_64 1.12.1-0.20140722.1109.git5debc61.fc20 installed 344 k
Transaction Summary
Install 1 Package Remove 1 Package (+12 Dependent packages)
Total download size: 14 k Is this ok [y/d/N]: N
LS
On Wed, Jul 23, 2014 at 03:51:31PM +0200, Sumit Bose wrote:
I admit I haven't read the patches at all yet, so I'm just replying based on this e-mail conversation, but does winbind require that the filename is libwbclient.so? Can't we make the file called something else?
No because the binaries using libwbclient like wbinfo or smbd are linked against this name.
Ah, OK, for some reason I thought the library is dlopened. Then I agree with the alternatives.
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 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.
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.
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 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?
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..
In wbclient.pc.in can you mention this is wbclient API using SSSD?
That's all I have. The code looks very solid and clean overall -- thanks!
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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, Aug 20, 2014 at 05:37:51PM +0200, Sumit Bose wrote:
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.
and now with the patch ...
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote:
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.
For some reason I thought that asprintf allocates on the stack like alloca and didn't see the call to free. In this case, I don't think the check is needed.
I'm sorry I made you do the work.
The rest looks fine to me.
On Thu, Aug 21, 2014 at 01:27:58PM +0200, Jakub Hrozek wrote:
On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote:
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.
For some reason I thought that asprintf allocates on the stack like alloca and didn't see the call to free. In this case, I don't think the check is needed.
I think the NULL checks still make sense and if you agree I would keep the others just as sanity checks.
bye, Sumit
I'm sorry I made you do the work.
The rest looks fine to me. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Aug 21, 2014 at 01:38:02PM +0200, Sumit Bose wrote:
On Thu, Aug 21, 2014 at 01:27:58PM +0200, Jakub Hrozek wrote:
On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote:
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.
For some reason I thought that asprintf allocates on the stack like alloca and didn't see the call to free. In this case, I don't think the check is needed.
I think the NULL checks still make sense and if you agree I would keep the others just as sanity checks.
bye, Sumit
OK, the additional checks don't hurt.
I tested the latest version of the patch and all the interface entry points I could test work fine.
As discussed with Sumit, winbind supports the name@domain notation. Because it might be useful to support that from sssd, too, I filed https://fedorahosted.org/sssd/ticket/2414 to track that.
ACK to the patch.
On Thu, Aug 21, 2014 at 05:51:58PM +0200, Jakub Hrozek wrote:
On Thu, Aug 21, 2014 at 01:38:02PM +0200, Sumit Bose wrote:
On Thu, Aug 21, 2014 at 01:27:58PM +0200, Jakub Hrozek wrote:
On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote:
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.
For some reason I thought that asprintf allocates on the stack like alloca and didn't see the call to free. In this case, I don't think the check is needed.
I think the NULL checks still make sense and if you agree I would keep the others just as sanity checks.
bye, Sumit
OK, the additional checks don't hurt.
I tested the latest version of the patch and all the interface entry points I could test work fine.
As discussed with Sumit, winbind supports the name@domain notation. Because it might be useful to support that from sssd, too, I filed https://fedorahosted.org/sssd/ticket/2414 to track that.
ACK to the patch.
* master: 885386b7e3f1c3e74b354576b98a092b0835d64e
On (21/08/14 13:38), Sumit Bose wrote:
On Thu, Aug 21, 2014 at 01:27:58PM +0200, Jakub Hrozek wrote:
On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote:
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.
For some reason I thought that asprintf allocates on the stack like alloca and didn't see the call to free. In this case, I don't think the check is needed.
I think the NULL checks still make sense and if you agree I would keep the others just as sanity checks.
libwbclient is installed by default with make install and there isn't way how to disable this behaviour. Should we add possibility to build (or install) library optionaly? It might help package maintainers.
LS
On Wed, Aug 27, 2014 at 11:31:27AM +0200, Lukas Slebodnik wrote:
On (21/08/14 13:38), Sumit Bose wrote:
On Thu, Aug 21, 2014 at 01:27:58PM +0200, Jakub Hrozek wrote:
On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote:
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.
For some reason I thought that asprintf allocates on the stack like alloca and didn't see the call to free. In this case, I don't think the check is needed.
I think the NULL checks still make sense and if you agree I would keep the others just as sanity checks.
libwbclient is installed by default with make install and there isn't way how to disable this behaviour. Should we add possibility to build (or install) library optionaly? It might help package maintainers.
Do you think attached patch would work?
bye, Sumit
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (27/08/14 18:33), Sumit Bose wrote:
On Wed, Aug 27, 2014 at 11:31:27AM +0200, Lukas Slebodnik wrote:
On (21/08/14 13:38), Sumit Bose wrote:
On Thu, Aug 21, 2014 at 01:27:58PM +0200, Jakub Hrozek wrote:
On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote:
> 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.
For some reason I thought that asprintf allocates on the stack like alloca and didn't see the call to free. In this case, I don't think the check is needed.
I think the NULL checks still make sense and if you agree I would keep the others just as sanity checks.
libwbclient is installed by default with make install and there isn't way how to disable this behaviour. Should we add possibility to build (or install) library optionaly? It might help package maintainers.
Do you think attached patch would work?
Yes, thank you
From b92a817a5b984216f85ceb89f60e554a50d1c2e5 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 27 Aug 2014 18:27:07 +0200 Subject: [PATCH] libwbclient: make build optional
Makefile.am | 5 ++++- configure.ac | 1 + src/conf_macros.m4 | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 1b183d0..dd0ddb3 100644 --- a/Makefile.am +++ b/Makefile.am @@ -733,9 +733,12 @@ endif # BUILD_IFP lib_LTLIBRARIES = libipa_hbac.la \ libsss_idmap.la \ libsss_nss_idmap.la \
libwbclient.la \ $(NULL)
+if BUILD_LIBWBCLIENT +lib_LTLIBRARIES += libwbclient.la +endif #BUILD_LIBWBCLIENT
pkgconfig_DATA += src/providers/ipa/ipa_hbac.pc libipa_hbac_la_DEPENDENCIES = src/providers/ipa/ipa_hbac.exports libipa_hbac_la_SOURCES = \
Almost ACK.
wbclient.pc should not be installed if sssd is built without libwbclient.
@@ -770,7 +770,11 @@ libsss_nss_idmap_la_LDFLAGS = \
dist_noinst_DATA += src/sss_client/idmap/sss_nss_idmap.exports
+ +if BUILD_LIBWBCLIENT pkgconfig_DATA += src/sss_client/libwbclient/wbclient.pc +endif #BUILD_LIBWBCLIENT + EXTRA_libwbclient_la_DEPENDENCIES = src/sss_client/libwbclient/wbclient.exports libwbclient_la_SOURCES = \ src/sss_client/libwbclient/wbc_guid.c \
LS
On Thu, Aug 28, 2014 at 09:42:50AM +0200, Lukas Slebodnik wrote:
On (27/08/14 18:33), Sumit Bose wrote:
On Wed, Aug 27, 2014 at 11:31:27AM +0200, Lukas Slebodnik wrote:
On (21/08/14 13:38), Sumit Bose wrote:
On Thu, Aug 21, 2014 at 01:27:58PM +0200, Jakub Hrozek wrote:
On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote:
> > 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.
For some reason I thought that asprintf allocates on the stack like alloca and didn't see the call to free. In this case, I don't think the check is needed.
I think the NULL checks still make sense and if you agree I would keep the others just as sanity checks.
libwbclient is installed by default with make install and there isn't way how to disable this behaviour. Should we add possibility to build (or install) library optionaly? It might help package maintainers.
Do you think attached patch would work?
Yes, thank you
From b92a817a5b984216f85ceb89f60e554a50d1c2e5 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 27 Aug 2014 18:27:07 +0200 Subject: [PATCH] libwbclient: make build optional
Makefile.am | 5 ++++- configure.ac | 1 + src/conf_macros.m4 | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 1b183d0..dd0ddb3 100644 --- a/Makefile.am +++ b/Makefile.am @@ -733,9 +733,12 @@ endif # BUILD_IFP lib_LTLIBRARIES = libipa_hbac.la \ libsss_idmap.la \ libsss_nss_idmap.la \
libwbclient.la \ $(NULL)
+if BUILD_LIBWBCLIENT +lib_LTLIBRARIES += libwbclient.la +endif #BUILD_LIBWBCLIENT
pkgconfig_DATA += src/providers/ipa/ipa_hbac.pc libipa_hbac_la_DEPENDENCIES = src/providers/ipa/ipa_hbac.exports libipa_hbac_la_SOURCES = \
Almost ACK.
wbclient.pc should not be installed if sssd is built without libwbclient.
good catch, folded in, new version attached.
bye, Sumit
@@ -770,7 +770,11 @@ libsss_nss_idmap_la_LDFLAGS = \
dist_noinst_DATA += src/sss_client/idmap/sss_nss_idmap.exports
+if BUILD_LIBWBCLIENT pkgconfig_DATA += src/sss_client/libwbclient/wbclient.pc +endif #BUILD_LIBWBCLIENT
EXTRA_libwbclient_la_DEPENDENCIES = src/sss_client/libwbclient/wbclient.exports libwbclient_la_SOURCES = \ src/sss_client/libwbclient/wbc_guid.c \
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (29/08/14 11:14), Sumit Bose wrote:
On Thu, Aug 28, 2014 at 09:42:50AM +0200, Lukas Slebodnik wrote:
On (27/08/14 18:33), Sumit Bose wrote:
On Wed, Aug 27, 2014 at 11:31:27AM +0200, Lukas Slebodnik wrote:
On (21/08/14 13:38), Sumit Bose wrote:
On Thu, Aug 21, 2014 at 01:27:58PM +0200, Jakub Hrozek wrote:
On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote: > > > 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.
For some reason I thought that asprintf allocates on the stack like alloca and didn't see the call to free. In this case, I don't think the check is needed.
I think the NULL checks still make sense and if you agree I would keep the others just as sanity checks.
libwbclient is installed by default with make install and there isn't way how to disable this behaviour. Should we add possibility to build (or install) library optionaly? It might help package maintainers.
Do you think attached patch would work?
Yes, thank you
From b92a817a5b984216f85ceb89f60e554a50d1c2e5 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 27 Aug 2014 18:27:07 +0200 Subject: [PATCH] libwbclient: make build optional
Makefile.am | 5 ++++- configure.ac | 1 + src/conf_macros.m4 | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 1b183d0..dd0ddb3 100644 --- a/Makefile.am +++ b/Makefile.am @@ -733,9 +733,12 @@ endif # BUILD_IFP lib_LTLIBRARIES = libipa_hbac.la \ libsss_idmap.la \ libsss_nss_idmap.la \
libwbclient.la \ $(NULL)
+if BUILD_LIBWBCLIENT +lib_LTLIBRARIES += libwbclient.la +endif #BUILD_LIBWBCLIENT
pkgconfig_DATA += src/providers/ipa/ipa_hbac.pc libipa_hbac_la_DEPENDENCIES = src/providers/ipa/ipa_hbac.exports libipa_hbac_la_SOURCES = \
Almost ACK.
wbclient.pc should not be installed if sssd is built without libwbclient.
good catch, folded in, new version attached.
bye, Sumit
@@ -770,7 +770,11 @@ libsss_nss_idmap_la_LDFLAGS = \
dist_noinst_DATA += src/sss_client/idmap/sss_nss_idmap.exports
+if BUILD_LIBWBCLIENT pkgconfig_DATA += src/sss_client/libwbclient/wbclient.pc +endif #BUILD_LIBWBCLIENT
EXTRA_libwbclient_la_DEPENDENCIES = src/sss_client/libwbclient/wbclient.exports libwbclient_la_SOURCES = \ src/sss_client/libwbclient/wbc_guid.c \
LS
From 8c1de23bb1ffc739340931a02452af55fa1af516 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 27 Aug 2014 18:27:07 +0200 Subject: [PATCH] libwbclient: make build optional
Makefile.am | 8 +++++++- configure.ac | 1 + src/conf_macros.m4 | 16 ++++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 1b183d0..cf46135 100644 --- a/Makefile.am +++ b/Makefile.am
I am so sorry. First time, I used grep "libwbclient", therefore I didn't see that there is also wbclient.h and It should not be installed without libwbclient.
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/libwbclient/wbclient.h \ ^^^^^^^^^^^ $(NULL)
LS
On Fri, Aug 29, 2014 at 11:34:37AM +0200, Lukas Slebodnik wrote:
On (29/08/14 11:14), Sumit Bose wrote:
On Thu, Aug 28, 2014 at 09:42:50AM +0200, Lukas Slebodnik wrote:
On (27/08/14 18:33), Sumit Bose wrote:
On Wed, Aug 27, 2014 at 11:31:27AM +0200, Lukas Slebodnik wrote:
On (21/08/14 13:38), Sumit Bose wrote:
On Thu, Aug 21, 2014 at 01:27:58PM +0200, Jakub Hrozek wrote: > On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote: > > > > 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. > > For some reason I thought that asprintf allocates on the stack like > alloca and didn't see the call to free. In this case, I don't think the > check is needed.
I think the NULL checks still make sense and if you agree I would keep the others just as sanity checks.
libwbclient is installed by default with make install and there isn't way how to disable this behaviour. Should we add possibility to build (or install) library optionaly? It might help package maintainers.
Do you think attached patch would work?
Yes, thank you
From b92a817a5b984216f85ceb89f60e554a50d1c2e5 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 27 Aug 2014 18:27:07 +0200 Subject: [PATCH] libwbclient: make build optional
Makefile.am | 5 ++++- configure.ac | 1 + src/conf_macros.m4 | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 1b183d0..dd0ddb3 100644 --- a/Makefile.am +++ b/Makefile.am @@ -733,9 +733,12 @@ endif # BUILD_IFP lib_LTLIBRARIES = libipa_hbac.la \ libsss_idmap.la \ libsss_nss_idmap.la \
libwbclient.la \ $(NULL)
+if BUILD_LIBWBCLIENT +lib_LTLIBRARIES += libwbclient.la +endif #BUILD_LIBWBCLIENT
pkgconfig_DATA += src/providers/ipa/ipa_hbac.pc libipa_hbac_la_DEPENDENCIES = src/providers/ipa/ipa_hbac.exports libipa_hbac_la_SOURCES = \
Almost ACK.
wbclient.pc should not be installed if sssd is built without libwbclient.
good catch, folded in, new version attached.
bye, Sumit
@@ -770,7 +770,11 @@ libsss_nss_idmap_la_LDFLAGS = \
dist_noinst_DATA += src/sss_client/idmap/sss_nss_idmap.exports
+if BUILD_LIBWBCLIENT pkgconfig_DATA += src/sss_client/libwbclient/wbclient.pc +endif #BUILD_LIBWBCLIENT
EXTRA_libwbclient_la_DEPENDENCIES = src/sss_client/libwbclient/wbclient.exports libwbclient_la_SOURCES = \ src/sss_client/libwbclient/wbc_guid.c \
LS
From 8c1de23bb1ffc739340931a02452af55fa1af516 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 27 Aug 2014 18:27:07 +0200 Subject: [PATCH] libwbclient: make build optional
Makefile.am | 8 +++++++- configure.ac | 1 + src/conf_macros.m4 | 16 ++++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 1b183d0..cf46135 100644 --- a/Makefile.am +++ b/Makefile.am
I am so sorry. First time, I used grep "libwbclient", therefore I didn't see that there is also wbclient.h and It should not be installed without libwbclient.
np, I should have thought of this before. Hopefully I moved everything needed in the if block. New version attached.
bye, Sumit
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/libwbclient/wbclient.h \ ^^^^^^^^^^^ $(NULL)
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (02/09/14 13:02), Sumit Bose wrote:
On Fri, Aug 29, 2014 at 11:34:37AM +0200, Lukas Slebodnik wrote:
On (29/08/14 11:14), Sumit Bose wrote:
On Thu, Aug 28, 2014 at 09:42:50AM +0200, Lukas Slebodnik wrote:
On (27/08/14 18:33), Sumit Bose wrote:
On Wed, Aug 27, 2014 at 11:31:27AM +0200, Lukas Slebodnik wrote:
On (21/08/14 13:38), Sumit Bose wrote: >On Thu, Aug 21, 2014 at 01:27:58PM +0200, Jakub Hrozek wrote: >> On Wed, Aug 20, 2014 at 05:38:56PM +0200, Sumit Bose wrote: >> > > > 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. >> >> For some reason I thought that asprintf allocates on the stack like >> alloca and didn't see the call to free. In this case, I don't think the >> check is needed. > >I think the NULL checks still make sense and if you agree I would keep >the others just as sanity checks. >
libwbclient is installed by default with make install and there isn't way how to disable this behaviour. Should we add possibility to build (or install) library optionaly? It might help package maintainers.
Do you think attached patch would work?
Yes, thank you
From b92a817a5b984216f85ceb89f60e554a50d1c2e5 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 27 Aug 2014 18:27:07 +0200 Subject: [PATCH] libwbclient: make build optional
Makefile.am | 5 ++++- configure.ac | 1 + src/conf_macros.m4 | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 1b183d0..dd0ddb3 100644 --- a/Makefile.am +++ b/Makefile.am @@ -733,9 +733,12 @@ endif # BUILD_IFP lib_LTLIBRARIES = libipa_hbac.la \ libsss_idmap.la \ libsss_nss_idmap.la \
libwbclient.la \ $(NULL)
+if BUILD_LIBWBCLIENT +lib_LTLIBRARIES += libwbclient.la +endif #BUILD_LIBWBCLIENT
pkgconfig_DATA += src/providers/ipa/ipa_hbac.pc libipa_hbac_la_DEPENDENCIES = src/providers/ipa/ipa_hbac.exports libipa_hbac_la_SOURCES = \
Almost ACK.
wbclient.pc should not be installed if sssd is built without libwbclient.
good catch, folded in, new version attached.
bye, Sumit
@@ -770,7 +770,11 @@ libsss_nss_idmap_la_LDFLAGS = \
dist_noinst_DATA += src/sss_client/idmap/sss_nss_idmap.exports
+if BUILD_LIBWBCLIENT pkgconfig_DATA += src/sss_client/libwbclient/wbclient.pc +endif #BUILD_LIBWBCLIENT
EXTRA_libwbclient_la_DEPENDENCIES = src/sss_client/libwbclient/wbclient.exports libwbclient_la_SOURCES = \ src/sss_client/libwbclient/wbc_guid.c \
LS
From 8c1de23bb1ffc739340931a02452af55fa1af516 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 27 Aug 2014 18:27:07 +0200 Subject: [PATCH] libwbclient: make build optional
Makefile.am | 8 +++++++- configure.ac | 1 + src/conf_macros.m4 | 16 ++++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 1b183d0..cf46135 100644 --- a/Makefile.am +++ b/Makefile.am
I am so sorry. First time, I used grep "libwbclient", therefore I didn't see that there is also wbclient.h and It should not be installed without libwbclient.
np, I should have thought of this before. Hopefully I moved everything needed in the if block. New version attached.
Works fine.
ACK
LS
sssd-devel@lists.fedorahosted.org