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

Lukas Slebodnik lslebodn at redhat.com
Thu Oct 9 15:07:27 UTC 2014


On (08/10/14 17:46), Lukas Slebodnik wrote:
>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

The diff from previous mail did not work with rhel6.

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/man/Makefile.am b/src/man/Makefile.am
index f8e8f81..a92ac0d 100644
--- a/src/man/Makefile.am
+++ b/src/man/Makefile.am
@@ -218,5 +218,3 @@ uninstall-local-yes:
 				man_MANS="$$manpages"; \
 		fi \
 	done
-
-tests:
diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am
index 3bf2ff9..34aec92 100644
--- a/src/tests/cwrap/Makefile.am
+++ b/src/tests/cwrap/Makefile.am
@@ -1,4 +1,3 @@
-
 AM_CPPFLAGS = \
     -Wall \
     -I$(top_srcdir)/src \
@@ -6,28 +5,31 @@ AM_CPPFLAGS = \
     $(DBUS_CFLAGS) \
     $(NULL)
 
-AM_TESTS_ENVIRONMENT = \
-    SSSD_SRCDIR=$(top_srcdir) \
-	. $(top_srcdir)/src/tests/cwrap/cwrap_test_setup.sh; \
+TESTS_ENVIRONMENT = \
+    CWRAP_TEST_SRCDIR=$(abs_srcdir) \
+    . $(srcdir)/cwrap_test_setup.sh; \
+    $(AUX_TESTS_ENVIRONMENT) \
     $(NULL)
 
-check_PROGRAMS = \
-	become_user-tests \
-	$(NULL)
+dist_noinst_SCRIPTS = \
+    cwrap_test_setup.sh \
+    $(NULL)
 
-check_LTLIBRARIES = \
-    libsss_test_common.la
+dist_noinst_DATA = \
+    group \
+    passwd \
+    $(NULL)
 
-TESTS = $(check_PROGRAMS)
+check_PROGRAMS =
+if HAVE_CMOCKA
+if HAVE_NSS_WRAPPER
+if HAVE_UID_WRAPPER
+check_PROGRAMS += become_user-tests
+endif # HAVE_UID_WRAPPER
+endif # HAVE_NSS_WRAPPER
+endif # HAVE_CMOCKA
 
-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)
+TESTS = $(check_PROGRAMS)
 
 become_user_tests_SOURCES = \
     test_become_user.c \
@@ -39,7 +41,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