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

[Jakub Hrozek]

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.

[Lukas Slebodnik]

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 
> 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 
> 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 
> 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

[Lukas Slebodnik]

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 
> > 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 
> > 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 
> > 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

[Jakub Hrozek]

On Thu, Oct 09, 2014 at 05:07:27PM +0200, Lukas Slebodnik wrote:
> 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 
> > > 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.

I'm not sure what you propose as a better way. I'm not opposed to a
change, but I'm not sure what shall I change.

I added the conditional into specfile. I also squashed in your changes
(thanks!) and new patches are attached.

[Lukas Slebodnik]

On (09/10/14 20:12), Jakub Hrozek wrote:
> On Thu, Oct 09, 2014 at 05:07:27PM +0200, Lukas Slebodnik wrote:
> > 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 
> > > > 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.
>
> I'm not sure what you propose as a better way. I'm not opposed to a
> change, but I'm not sure what shall I change.
>
> I added the conditional into specfile. I also squashed in your changes
> (thanks!) and new patches are attached.

I thought that idea of NULL is to avoid changing two lines if files is added to
the end. The file "src/util/become_user.c" was added before variable
SSSD_FAILOVER_OBJ, but it isn't important. We can be consistent and add NULL
also after variable SSSD_FAILOVER_OBJ.

Patches works well. make distcheck is fixed. nitpicks were addressed as well.

ACK

LS

[Jakub Hrozek]

On Fri, Oct 10, 2014 at 12:10:16PM +0200, Lukas Slebodnik wrote:
> Patches works well. make distcheck is fixed. nitpicks were addressed as well.
>
> ACK
>
> LS

Thank you for the review. All patches were pushed to master:
* 428db8a58c0c149d5efccc6d788f70916c1d34d7
* 40b2be4f4312470044cdef460b02b66003f5c85f
* 9df7cddb68c61ef4e0397c196604999c68f4be0d