[SSSD] [PATCH] Split becoming user into a separate module (with tests!)

Lukas Slebodnik lslebodn at redhat.com
Wed Oct 8 15:46:38 UTC 2014


On (06/10/14 15:46), Jakub Hrozek wrote:
>Hi,
>
>attached are more patches that are related to running sssd as a
>non-root. I split the functions to become a different user into a
>separate module and provided unit tests using nss_wrapper and
>uid_wrapper.
>
>There is a separate Makefile.am for the cwrap-based tests, because it's
>not easy (read: possible without a wrapper script) to run tests with a
>custom environment. Some build systems, like cmake, allow that, but
>autotools does not.

>From 565aca30c666cb696ccb5c6a9426144276c7dce9 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek at redhat.com>
>Date: Sat, 26 Jul 2014 12:46:26 +0200
>Subject: [PATCH 1/3] UTIL: Move become_user outside krb5 tree
>
>In order for several other SSSD processes to run as a non-root user, we
>need to move the functions to become another user to a shared space in
>our source tree.
>---
> Makefile.am                                          | 20 ++++++++++++--------
> src/providers/krb5/krb5_utils.h                      |  8 --------
> .../krb5/krb5_become_user.c => util/become_user.c}   |  1 -
> src/util/util.h                                      |  9 +++++++++
> 4 files changed, 21 insertions(+), 17 deletions(-)
> rename src/{providers/krb5/krb5_become_user.c => util/become_user.c} (99%)
>
>diff --git a/Makefile.am b/Makefile.am
>index eb0e649437f465d3354a0c063a29c65203824506..ac3f26cad6e1d0a5ac8979ce13286bc896c62b62 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -1286,13 +1286,14 @@ strtonum_tests_LDADD = \
> krb5_utils_tests_SOURCES = \
>     src/tests/krb5_utils-tests.c \
>     src/providers/krb5/krb5_utils.c \
>-    src/providers/krb5/krb5_become_user.c \
>     src/providers/krb5/krb5_common.c \
>     src/util/sss_krb5.c \
>     src/providers/data_provider_fo.c \
>     src/providers/data_provider_opts.c \
>     src/providers/data_provider_callbacks.c \
>-    $(SSSD_FAILOVER_OBJ)
>+    src/util/become_user.c \
>+    $(SSSD_FAILOVER_OBJ) \
>+    $(NULL)
Does it make sense to add NULL at the end?
You put new file before SSSD_FAILOVER_OBJ.
If you think that it make sense I will not insist on change.

> krb5_utils_tests_CFLAGS = \
>     $(AM_CFLAGS) \
>     $(KRB5_CFLAGS) \
>@@ -1567,13 +1568,14 @@ krb5_child_test_SOURCES = \
>     src/tests/krb5_child-test.c \
>     src/providers/krb5/krb5_utils.c \
>     src/providers/krb5/krb5_child_handler.c \
>-    src/providers/krb5/krb5_become_user.c \
>     src/providers/krb5/krb5_common.c \
>     src/util/sss_krb5.c \
>     src/providers/data_provider_fo.c \
>     src/providers/data_provider_opts.c \
>     src/providers/data_provider_callbacks.c \
>-    $(SSSD_FAILOVER_OBJ)
>+    src/util/become_user.c \
>+    $(SSSD_FAILOVER_OBJ) \
>+    $(NULL)
The same as previous one.

> krb5_child_test_CFLAGS = \
>     $(AM_CFLAGS) \
>     -DKRB5_CHILD_DIR=\"$(builddir)\" \
>@@ -2243,7 +2245,6 @@ libsss_ad_common_la_LIBADD = \
> 
> libsss_krb5_common_la_SOURCES = \
>     src/providers/krb5/krb5_utils.c \
>-    src/providers/krb5/krb5_become_user.c \
>     src/providers/krb5/krb5_delayed_online_authentication.c \
>     src/providers/krb5/krb5_renew_tgt.c \
>     src/providers/krb5/krb5_wait_queue.c \
>@@ -2252,7 +2253,9 @@ libsss_krb5_common_la_SOURCES = \
>     src/providers/krb5/krb5_access.c \
>     src/providers/krb5/krb5_child_handler.c \
>     src/providers/krb5/krb5_init_shared.c \
>-    src/util/sss_krb5.c
>+    src/util/sss_krb5.c \
>+    src/util/become_user.c \
>+    $(NULL)
> libsss_krb5_common_la_CFLAGS = \
>     $(KRB5_CFLAGS)
> libsss_krb5_common_la_LIBADD = \
>@@ -2432,7 +2435,6 @@ libsss_ad_la_LDFLAGS = \
>     -module
> 
> krb5_child_SOURCES = \
>-    src/providers/krb5/krb5_become_user.c \
>     src/providers/krb5/krb5_child.c \
>     src/providers/dp_pam_data_util.c \
>     src/util/user_info_msg.c \
>@@ -2441,7 +2443,9 @@ krb5_child_SOURCES = \
>     src/util/authtok.c \
>     src/util/util.c \
>     src/util/signal.c \
>-    src/sss_client/common.c
>+    src/util/become_user.c \
>+    src/sss_client/common.c \
>+    $(NULL)
> krb5_child_CFLAGS = \
>     $(AM_CFLAGS) \
>     $(POPT_CFLAGS) \
>diff --git a/src/providers/krb5/krb5_utils.h b/src/providers/krb5/krb5_utils.h
//snip
ack to other changes.

>From afc346f2c8735656256cd8136e38f636923cd8ed Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek at redhat.com>
>Date: Sat, 26 Jul 2014 14:58:58 +0200
>Subject: [PATCH 2/3] BUILD: Detect nss_wrapper and uid_wrapper during
> configure
>
>Unit testing the utilities to become another user requires the use of
>the cwrap libraries. This patch augments our build system with macros to
>detect the nss_wrapper and and uid_wrapper libraries.

ACK

>From b962ad8b9fb4aa120e12a37c874f907a68abfc8e Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek at redhat.com>
>Date: Sun, 27 Jul 2014 14:44:24 +0200
>Subject: [PATCH 3/3] TESTS: Add a test to change user IDs
>
>---
> Makefile.am                         |  11 +++
> configure.ac                        |   2 +-
> contrib/sssd.spec.in                |   2 +
> src/man/Makefile.am                 |   2 +
> src/tests/cwrap/Makefile.am         |  45 +++++++++++
> src/tests/cwrap/cwrap_test_setup.sh |  17 ++++
> src/tests/cwrap/group               |   1 +
> src/tests/cwrap/passwd              |   1 +
> src/tests/cwrap/test_become_user.c  | 156 ++++++++++++++++++++++++++++++++++++
> 9 files changed, 236 insertions(+), 1 deletion(-)
> create mode 100644 src/tests/cwrap/Makefile.am
> create mode 100755 src/tests/cwrap/cwrap_test_setup.sh
> create mode 100644 src/tests/cwrap/group
> create mode 100644 src/tests/cwrap/passwd
> create mode 100644 src/tests/cwrap/test_become_user.c
>
>diff --git a/Makefile.am b/Makefile.am
>index ac3f26cad6e1d0a5ac8979ce13286bc896c62b62..91cda02ca57209d33a378f302a150f897974ff25 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -19,6 +19,16 @@ if HAVE_MANPAGES
> SUBDIRS += src/man
> endif
> 
>+# The cwrap tests require the wrapper libraries and are written
>+# using cmocka
>+if HAVE_CMOCKA
>+if HAVE_NSS_WRAPPER
>+if HAVE_UID_WRAPPER
>+SUBDIRS += src/tests/cwrap
>+endif # HAVE_UID_WRAPPER
>+endif # HAVE_NSS_WRAPPER
>+endif # HAVE_CMOCKA
>+
If wrappers are not installed you can change directory to src/tests/cwrap
and make check will fail. This "if statements" should be also in cwrap Makefile
for ckeck_PROGRAMS.

> # Some old versions of automake don't define builddir
> builddir ?= .
> 
>@@ -2829,6 +2839,7 @@ endif
> CLEANFILES = *.X */*.X */*/*.X
> 
> tests: all $(check_PROGRAMS)
>+	(cd src/tests/cwrap && $(MAKE) $(AM_MAKEFLAGS) $@) || exit 1;
> 
> 
> # RPM-related tasks
>diff --git a/configure.ac b/configure.ac
>index 1edf4cb19e390ced2043353341d321425652da71..660ea8d373272917d2d6ceeafdccf5dddbe493bc 100644
>--- a/configure.ac
>+++ b/configure.ac
>@@ -335,7 +335,7 @@ AC_SUBST([abs_builddir], $abs_build_dir)
> 
> AC_CONFIG_FILES([Makefile contrib/sssd.spec src/examples/rwtab src/doxy.config
>                  src/sysv/sssd src/sysv/gentoo/sssd src/sysv/SUSE/sssd
>-                 po/Makefile.in src/man/Makefile
>+                 po/Makefile.in src/man/Makefile src/tests/cwrap/Makefile
>                  src/providers/ipa/ipa_hbac.pc src/providers/ipa/ipa_hbac.doxy
>                  src/lib/idmap/sss_idmap.pc src/lib/idmap/sss_idmap.doxy
>                  src/sss_client/sudo/sss_sudo.doxy
>diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
>index 611730c1928b5bbfaa2aee50967226c5824bbca0..4c11bab24eb9eb0c94ee865cc1fc975180aa2ee2 100644
>--- a/contrib/sssd.spec.in
>+++ b/contrib/sssd.spec.in
>@@ -113,6 +113,8 @@ BuildRequires: glib2-devel
> BuildRequires: selinux-policy-targeted
> %if 0%{?fedora}
> BuildRequires: libcmocka-devel
>+BuildRequires: uid_wrapper
>+BuildRequires: nss_wrapper

{nss,uid}_wrapper are not in fedora 19. On the other hand, new ding-libs isn't
either. Do we want to be correct and add proper condition for wrappers.

BTW you can also add wrappers into additionals dependencies for rhel distros in
CI (contrib/ci/deps.sh)
The wrappers are in epel6 and epel7
https://admin.fedoraproject.org/pkgdb/package/uid_wrapper/
https://admin.fedoraproject.org/pkgdb/package/nss_wrapper/

> %endif
> %if (0%{?fedora} || 0%{?rhel} >= 7)
> BuildRequires: libnl3-devel
>diff --git a/src/man/Makefile.am b/src/man/Makefile.am
>index a92ac0d1e62c4ef96e347d47ccf427f94a60fea2..f8e8f81af964fa7084300745ead80635739c3a47 100644
>--- a/src/man/Makefile.am
>+++ b/src/man/Makefile.am
>@@ -218,3 +218,5 @@ uninstall-local-yes:
> 				man_MANS="$$manpages"; \
> 		fi \
> 	done
>+
>+tests:
This change is not necessary with current version of recursive target "tests".
>diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am
>new file mode 100644
>index 0000000000000000000000000000000000000000..3bf2ff9f89ea49870f3676fd9b255a8094d47770
>--- /dev/null
>+++ b/src/tests/cwrap/Makefile.am
>@@ -0,0 +1,45 @@
>+
>+AM_CPPFLAGS = \
>+    -Wall \
>+    -I$(top_srcdir)/src \
>+    -I. \
>+    $(DBUS_CFLAGS) \
>+    $(NULL)
>+
>+AM_TESTS_ENVIRONMENT = \
>+    SSSD_SRCDIR=$(top_srcdir) \
>+	. $(top_srcdir)/src/tests/cwrap/cwrap_test_setup.sh; \
>+    $(NULL)
>+
>+check_PROGRAMS = \
>+	become_user-tests \
>+	$(NULL)
>+
>+check_LTLIBRARIES = \
>+    libsss_test_common.la
>+
>+TESTS = $(check_PROGRAMS)
>+
>+libsss_test_common_la_SOURCES = \
>+    ../common_tev.c \
>+    ../common_dom.c \
>+    ../leak_check.c \
>+    ../common.c
>+libsss_test_common_la_LIBADD = \
>+    $(TALLOC_LIBS) \
>+    $(TEVENT_LIBS)
>+
>+become_user_tests_SOURCES = \
>+    test_become_user.c \
>+    $(NULL)
>+become_user_tests_CFLAGS = \
>+    $(AM_CFLAGS) \
>+    $(NULL)
>+become_user_tests_LDADD = \
>+    $(POPT_LIBS) \
>+    $(CMOCKA_LIBS) \
>+    $(abs_top_builddir)/libsss_debug.la \
>+    libsss_test_common.la \
>+    $(NULL)
>+
>+tests: $(check_PROGRAMS)
>diff --git a/src/tests/cwrap/cwrap_test_setup.sh b/src/tests/cwrap/cwrap_test_setup.sh
>new file mode 100755
>index 0000000000000000000000000000000000000000..d5ddffb8ee81509e7b43183ed6e0671258366fe3
This script is not added in any makefile, therefore make distcheck failed.
The same with src/tests/cwrap/group src/tests/cwrap/passwd

make distcheck failed few times for me.
    * I had to change order of executing makefile
        (add "." to SUBDIRS before directory "src/tests/cwrap")
    * add missing script to tarball (makefile)
    * add missing tests files (passwd,group) to tarball (makefile)
    * modify paths in cwrap_test_setup to absolute paths.

Attaching diff which fixes make distcheck failures.

LS
-------------- next part --------------
diff --git a/Makefile.am b/Makefile.am
index 91cda02..ee6e39b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -19,15 +19,7 @@ if HAVE_MANPAGES
 SUBDIRS += src/man
 endif
 
-# The cwrap tests require the wrapper libraries and are written
-# using cmocka
-if HAVE_CMOCKA
-if HAVE_NSS_WRAPPER
-if HAVE_UID_WRAPPER
-SUBDIRS += src/tests/cwrap
-endif # HAVE_UID_WRAPPER
-endif # HAVE_NSS_WRAPPER
-endif # HAVE_CMOCKA
+SUBDIRS += . src/tests/cwrap
 
 # Some old versions of automake don't define builddir
 builddir ?= .
diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am
index 3bf2ff9..ad78b64 100644
--- a/src/tests/cwrap/Makefile.am
+++ b/src/tests/cwrap/Makefile.am
@@ -1,4 +1,3 @@
-
 AM_CPPFLAGS = \
     -Wall \
     -I$(top_srcdir)/src \
@@ -7,28 +6,32 @@ AM_CPPFLAGS = \
     $(NULL)
 
 AM_TESTS_ENVIRONMENT = \
-    SSSD_SRCDIR=$(top_srcdir) \
-	. $(top_srcdir)/src/tests/cwrap/cwrap_test_setup.sh; \
+    CWRAP_TEST_SRCDIR=$(abs_srcdir) \
+    . $(srcdir)/cwrap_test_setup.sh; \
+    $(NULL)
+
+dist_noinst_SCRIPTS = \
+    cwrap_test_setup.sh \
     $(NULL)
 
-check_PROGRAMS = \
-	become_user-tests \
-	$(NULL)
+dist_noinst_DATA = \
+    group \
+    passwd \
+    $(NULL)
 
-check_LTLIBRARIES = \
-    libsss_test_common.la
+check_PROGRAMS =
+if HAVE_CMOCKA
+if HAVE_NSS_WRAPPER
+if HAVE_UID_WRAPPER
+check_PROGRAMS += \
+    become_user-tests \
+    $(NULL)
+endif # HAVE_UID_WRAPPER
+endif # HAVE_NSS_WRAPPER
+endif # HAVE_CMOCKA
 
 TESTS = $(check_PROGRAMS)
 
-libsss_test_common_la_SOURCES = \
-    ../common_tev.c \
-    ../common_dom.c \
-    ../leak_check.c \
-    ../common.c
-libsss_test_common_la_LIBADD = \
-    $(TALLOC_LIBS) \
-    $(TEVENT_LIBS)
-
 become_user_tests_SOURCES = \
     test_become_user.c \
     $(NULL)
@@ -39,7 +42,7 @@ become_user_tests_LDADD = \
     $(POPT_LIBS) \
     $(CMOCKA_LIBS) \
     $(abs_top_builddir)/libsss_debug.la \
-    libsss_test_common.la \
+    $(abs_top_builddir)/libsss_test_common.la \
     $(NULL)
 
 tests: $(check_PROGRAMS)
diff --git a/src/tests/cwrap/cwrap_test_setup.sh b/src/tests/cwrap/cwrap_test_setup.sh
index d5ddffb..0d35cb7 100755
--- a/src/tests/cwrap/cwrap_test_setup.sh
+++ b/src/tests/cwrap/cwrap_test_setup.sh
@@ -11,7 +11,7 @@ if [ -z $nss_wrapper -o -z $uid_wrapper ]; then
 fi
 
 export LD_PRELOAD="$nss_wrapper $uid_wrapper"
-export NSS_WRAPPER_PASSWD=$SSSD_SRCDIR/src/tests/cwrap/passwd
-export NSS_WRAPPER_GROUP=$SSSD_SRCDIR/src/tests/cwrap/group
+export NSS_WRAPPER_PASSWD=$CWRAP_TEST_SRCDIR/passwd
+export NSS_WRAPPER_GROUP=$CWRAP_TEST_SRCDIR/group
 export UID_WRAPPER=1
 export UID_WRAPPER_ROOT=1


More information about the sssd-devel mailing list