Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Mon, Jul 15, 2013 at 08:30:03PM +0300, Alexander Bokovoy wrote: > Hi! > > Attached please find two patches against slapi-nis 0.47 to serve trusted > domain users and groups to old clients. FreeIPA master needs to be > enabled with this, see my patch 0108 (on freeipa-devel@). > > The patches add both lookup and PAM-based authentication bind for the > users returned by SSSD lookup. > > Here is the logic: > > 0. Configuration is performed by setting > >schema-compat-lookup-sssd: >schema-compat-sssd-min-id: > > in corresponding schema-compat plugin tree (cn=users and cn=groups). > > If schema-compat-sssd-min-id is not set, it will default to 1000. It is > used to filter out attempts to fetch system users (<1000 on Fedora by > default). > > 1. On query, we parse query filter to identify what type of request is > this: user or group lookup and then issue getpwnam_r()/getgrnam_r() and > getsidbyid() for libsss_nss_idmap to fetch all needed information. > > SSSD caches these requests they should be relatively fast. > > 2. Once we served the request, it is cached in schema-compat cache map. > The entry in the cache is currently not expired explicitly but I'm > working on expiring it on wrong authentication -- if PAM stack returns a > response telling there is no such user. > > 3. Authentication bind for cached entries is done via PAM service > 'system-auth'. If HBAC rule 'allow_all' is disabled in FreeIPA, one > needs to create a rule with service 'system-auth' and allow all users to > access it on IPA masters. Since system-auth is never used explicitly by > any application (it is always included through PAM stack and only > top-level PAM service is used to drive the HBAC ruleset), there is no > problem. > > PAM authentication code is taken from pam_passthru DS plugin. We cannot > use it unchanged because pam_passthru expects that LDAP entry will exist > in DS, while it is not true for these synthetic entries representing > trusted domain users. > > On Fedora one needs pam-devel and libsss_nss_idmap-devel to build the > plugin with new functionality. > > -- > / Alexander Bokovoy Hi, so far I've only built the code, testing is next. So far I have some comments about the first patch, see inline: > >From d3433f2033015724fc6580c00d89627afbc06c1d Mon Sep 17 00:00:00 2001 > From: Alexander Bokovoy > Date: Mon, 15 Jul 2013 14:18:52 +0300 > Subject: [PATCH 1/2] configure: add configure checks for sss_idmap and define > attribute to lookup sssd > > If schema compat plugin configuration has 'schema-compat-lookup-sssd: > user|group' > then schema compat plugin will perform lookups of users/groups that were not > found > in the main store using getpwnam_r()/getgrnam_r() and libsss_idmap library. > > This is special case to support legacy clients. Schema compat plugin in the > case is assumed to be running on FreeIPA master configured with trusts against > Active Directory and SSSD configure as ipa_server_mode = True. > > Additionally, such entries are added to schema compat plugin's map cache and > can > be used for authentication purposes. They will use PAM authentication > pass-through > to system-auth service. > --- > configure.ac| 48 > src/Makefile.am | 6 ++ > 2 files changed, 54 insertions(+) > > diff --git a/configure.ac b/configure.ac > index 8d7cbe1..4a47d36 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -309,6 +309,47 @@ AC_SUBST(ASYNCNS_CFLAGS) > AC_SUBST(ASYNCNS_LIBS) > fi > > +AC_ARG_WITH(sss_nss_idmap, > + AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]), > + use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO) > +if pkg-config sss_nss_idmap 2> /dev/null ; then > + if test x$use_sss_nss_idmap != xno ; then > + AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have > libsss_nss_idmap.]) > + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) > + else > + SSS_NSS_IDMAP_CFLAGS= > + SSS_NSS_IDMAP_LIBS= > + fi > +else > + if test $use_sss_idmap = yes ; then > + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) > + else > + SSS_NSS_IDMAP_CFLAGS= > + SSS_NSS_IDMAP_LIBS= > + fi > +fi > +AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x]) > +AC_SUBST(SSS_NSS_IDMAP_CFLAGS) > +AC_SUBST(SSS_NSS_IDMAP_LIBS) > + > +if x$SSS_NSS_IDMAP_LIBS != x ; then I think you should use test or square brackets here, otherwise I'm seeing: checking for SSS_NSS_IDMAP... yes ./configure: line 12952: x-lsss_nss_idmap: command not found > + AC_CHECK_HEADERS(pam.h) I don't see any pam.h in pam-devel RPM. In SSSD we look for security/pam_appl.h > + if test x$ac_cv_header_pam_h = xno ; then > + use_pam=yes > + else > + use_pam=no > + fi > + > + if test $use_pam = yes ; then > + PAM_CFLAGS= > + PAM_LIBS=-lpam > + els
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
Hi!
On Tue, 16 Jul 2013, Jakub Hrozek wrote:
+AC_ARG_WITH(sss_nss_idmap,
+ AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]),
+ use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO)
+if pkg-config sss_nss_idmap 2> /dev/null ; then
+ if test x$use_sss_nss_idmap != xno ; then
+ AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have
libsss_nss_idmap.])
+ PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap)
+ else
+ SSS_NSS_IDMAP_CFLAGS=
+ SSS_NSS_IDMAP_LIBS=
+ fi
+else
+ if test $use_sss_idmap = yes ; then
+ PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap)
+ else
+ SSS_NSS_IDMAP_CFLAGS=
+ SSS_NSS_IDMAP_LIBS=
+ fi
+fi
+AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x])
+AC_SUBST(SSS_NSS_IDMAP_CFLAGS)
+AC_SUBST(SSS_NSS_IDMAP_LIBS)
+
+if x$SSS_NSS_IDMAP_LIBS != x ; then
I think you should use test or square brackets here, otherwise I'm
seeing:
checking for SSS_NSS_IDMAP... yes
./configure: line 12952: x-lsss_nss_idmap: command not found
Thanks, added 'test ...'
+ AC_CHECK_HEADERS(pam.h)
I don't see any pam.h in pam-devel RPM. In SSSD we look for
security/pam_appl.h
Right. Fixed.
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -51,6 +51,7 @@ nisserver_plugin_la_LIBADD = $(LDAP_LIBS) $(RUNTIME_LIBS)
$(TIRPC_LIBS) $(LIBWRA
schemacompat_plugin_la_SOURCES = \
back-sch.c \
+ back-sch.h \
This file is only added in the second patch, so maybe the whole
Makefile.am hunk should be moved there.
Yes, moved to the second patch.
New patchset is attached.
--
/ Alexander Bokovoy
>From 3e413ce8f71f28b9fd58eb6cd391fd4c3e9cc7fd Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy
Date: Tue, 16 Jul 2013 13:17:03 +0300
Subject: [PATCH 1/2] configure: add configure checks for sss_idmap and define
attribute to lookup sssd
If schema compat plugin configuration has 'schema-compat-lookup-sssd:
user|group'
then schema compat plugin will perform lookups of users/groups that were not
found
in the main store using getpwnam_r()/getgrnam_r() and libsss_idmap library.
This is special case to support legacy clients. Schema compat plugin in the
case is assumed to be running on FreeIPA master configured with trusts against
Active Directory and SSSD configure as ipa_server_mode = True.
Additionally, such entries are added to schema compat plugin's map cache and can
be used for authentication purposes. They will use PAM authentication
pass-through
to system-auth service.
---
configure.ac | 48
1 file changed, 48 insertions(+)
diff --git a/configure.ac b/configure.ac
index 8d7cbe1..aac52e2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -309,6 +309,47 @@ AC_SUBST(ASYNCNS_CFLAGS)
AC_SUBST(ASYNCNS_LIBS)
fi
+AC_ARG_WITH(sss_nss_idmap,
+ AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]),
+ use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO)
+if pkg-config sss_nss_idmap 2> /dev/null ; then
+ if test x$use_sss_nss_idmap != xno ; then
+ AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have
libsss_nss_idmap.])
+ PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap)
+ else
+ SSS_NSS_IDMAP_CFLAGS=
+ SSS_NSS_IDMAP_LIBS=
+ fi
+else
+ if test $use_sss_idmap = yes ; then
+ PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap)
+ else
+ SSS_NSS_IDMAP_CFLAGS=
+ SSS_NSS_IDMAP_LIBS=
+ fi
+fi
+AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x])
+AC_SUBST(SSS_NSS_IDMAP_CFLAGS)
+AC_SUBST(SSS_NSS_IDMAP_LIBS)
+
+if test x$SSS_NSS_IDMAP_LIBS != x ; then
+ AC_CHECK_HEADERS(security/pam_appl.h)
+ if test x$ac_cv_header_pam_h = xno ; then
+ use_pam=yes
+ else
+ use_pam=no
+ fi
+
+ if test $use_pam = yes ; then
+ PAM_CFLAGS=
+ PAM_LIBS=-lpam
+ else
+ AC_ERROR([ not found and it is required
for SSSD mode])
+ fi
+ AC_SUBST(PAM_CFLAGS)
+ AC_SUBST(PAM_LIBS)
+fi
+
mylibdir=`eval echo "$libdir" | sed "s,NONE,${ac_default_prefix},g"`
mylibdir=`eval echo "$mylibdir" | sed "s,NONE,${ac_prefix},g"`
case "$server" in
@@ -401,6 +442,13 @@
AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_RDN_ATTR,"$rdnattr",
attrattr=schema-compat-entry-attribute
AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_ATTR_ATTR,"$attrattr",
[Define to name of the attribute which is used to specify
attributes to be used when constructing entries.])
+sssdattr=schema-compat-lookup-sssd
+AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_SSSD_ATTR,"$sssdattr",
+ [Define to name of the attribute which dictates whether or
not SSSD on FreeIPA master is consulted about trusted domains' users.])
+sssdminidattr=schema
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Tue, Jul 16, 2013 at 01:23:41PM +0300, Alexander Bokovoy wrote: > Hi! > > On Tue, 16 Jul 2013, Jakub Hrozek wrote: > >>+AC_ARG_WITH(sss_nss_idmap, > >>+ AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]), > >>+ use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO) > >>+if pkg-config sss_nss_idmap 2> /dev/null ; then > >>+ if test x$use_sss_nss_idmap != xno ; then > >>+ AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have > >>libsss_nss_idmap.]) > >>+ PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) > >>+ else > >>+ SSS_NSS_IDMAP_CFLAGS= > >>+ SSS_NSS_IDMAP_LIBS= > >>+ fi > >>+else > >>+ if test $use_sss_idmap = yes ; then > >>+ PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) > >>+ else > >>+ SSS_NSS_IDMAP_CFLAGS= > >>+ SSS_NSS_IDMAP_LIBS= > >>+ fi > >>+fi > >>+AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x]) > >>+AC_SUBST(SSS_NSS_IDMAP_CFLAGS) > >>+AC_SUBST(SSS_NSS_IDMAP_LIBS) > >>+ > >>+if x$SSS_NSS_IDMAP_LIBS != x ; then > > > >I think you should use test or square brackets here, otherwise I'm > >seeing: > > > >checking for SSS_NSS_IDMAP... yes > >./configure: line 12952: x-lsss_nss_idmap: command not found > Thanks, added 'test ...' > > > > >>+ AC_CHECK_HEADERS(pam.h) > > > >I don't see any pam.h in pam-devel RPM. In SSSD we look for > >security/pam_appl.h > Right. Fixed. > > >>--- a/src/Makefile.am > >>+++ b/src/Makefile.am > >>@@ -51,6 +51,7 @@ nisserver_plugin_la_LIBADD = $(LDAP_LIBS) $(RUNTIME_LIBS) > >>$(TIRPC_LIBS) $(LIBWRA > >> > >> schemacompat_plugin_la_SOURCES = \ > >>back-sch.c \ > >>+ back-sch.h \ > > > >This file is only added in the second patch, so maybe the whole > >Makefile.am hunk should be moved there. > Yes, moved to the second patch. > > New patchset is attached. > -- > / Alexander Bokovoy Thanks for the changes, one more comment about patch #1: > +if test x$SSS_NSS_IDMAP_LIBS != x ; then > + AC_CHECK_HEADERS(security/pam_appl.h) > + if test x$ac_cv_header_pam_h = xno ; then I think you need to check for ac_cv_header_security_pam_appl_h here. Also the condition seems reversed to me, it was complaining about missing PAM-devel even when I had one. After changing the test to use ac_cv_header_security_pam_appl_h the and reverting the condition configure seems to have worked correctly for me. Here is the git diff I made: diff --git a/configure.ac b/configure.ac index aac52e2..085f51d 100644 --- a/configure.ac +++ b/configure.ac @@ -334,7 +334,7 @@ AC_SUBST(SSS_NSS_IDMAP_LIBS) if test x$SSS_NSS_IDMAP_LIBS != x ; then AC_CHECK_HEADERS(security/pam_appl.h) - if test x$ac_cv_header_pam_h = xno ; then + if test x$ac_cv_header_security_pam_appl_h = xyes ; then use_pam=yes else use_pam=no ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Tue, 16 Jul 2013, Jakub Hrozek wrote: On Tue, Jul 16, 2013 at 01:23:41PM +0300, Alexander Bokovoy wrote: Hi! On Tue, 16 Jul 2013, Jakub Hrozek wrote: >>+AC_ARG_WITH(sss_nss_idmap, >>+ AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]), >>+ use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO) >>+if pkg-config sss_nss_idmap 2> /dev/null ; then >>+ if test x$use_sss_nss_idmap != xno ; then >>+ AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have libsss_nss_idmap.]) >>+ PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) >>+ else >>+ SSS_NSS_IDMAP_CFLAGS= >>+ SSS_NSS_IDMAP_LIBS= >>+ fi >>+else >>+ if test $use_sss_idmap = yes ; then >>+ PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) >>+ else >>+ SSS_NSS_IDMAP_CFLAGS= >>+ SSS_NSS_IDMAP_LIBS= >>+ fi >>+fi >>+AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x]) >>+AC_SUBST(SSS_NSS_IDMAP_CFLAGS) >>+AC_SUBST(SSS_NSS_IDMAP_LIBS) >>+ >>+if x$SSS_NSS_IDMAP_LIBS != x ; then > >I think you should use test or square brackets here, otherwise I'm >seeing: > >checking for SSS_NSS_IDMAP... yes >./configure: line 12952: x-lsss_nss_idmap: command not found Thanks, added 'test ...' > >>+ AC_CHECK_HEADERS(pam.h) > >I don't see any pam.h in pam-devel RPM. In SSSD we look for >security/pam_appl.h Right. Fixed. >>--- a/src/Makefile.am >>+++ b/src/Makefile.am >>@@ -51,6 +51,7 @@ nisserver_plugin_la_LIBADD = $(LDAP_LIBS) $(RUNTIME_LIBS) $(TIRPC_LIBS) $(LIBWRA >> >> schemacompat_plugin_la_SOURCES = \ >>back-sch.c \ >>+ back-sch.h \ > >This file is only added in the second patch, so maybe the whole >Makefile.am hunk should be moved there. Yes, moved to the second patch. New patchset is attached. -- / Alexander Bokovoy Thanks for the changes, one more comment about patch #1: +if test x$SSS_NSS_IDMAP_LIBS != x ; then + AC_CHECK_HEADERS(security/pam_appl.h) + if test x$ac_cv_header_pam_h = xno ; then I think you need to check for ac_cv_header_security_pam_appl_h here. Also the condition seems reversed to me, it was complaining about missing PAM-devel even when I had one. After changing the test to use ac_cv_header_security_pam_appl_h the and reverting the condition configure seems to have worked correctly for me. Here is the git diff I made: diff --git a/configure.ac b/configure.ac index aac52e2..085f51d 100644 --- a/configure.ac +++ b/configure.ac @@ -334,7 +334,7 @@ AC_SUBST(SSS_NSS_IDMAP_LIBS) if test x$SSS_NSS_IDMAP_LIBS != x ; then AC_CHECK_HEADERS(security/pam_appl.h) - if test x$ac_cv_header_pam_h = xno ; then + if test x$ac_cv_header_security_pam_appl_h = xyes ; then use_pam=yes else use_pam=no Thanks! I was too fast :) New patchset is attached, resending all patches to make easier tracking. -- / Alexander Bokovoy >From 6909b33edbfb30292645f0998b60a4ffb38891dd Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy Date: Tue, 16 Jul 2013 13:17:03 +0300 Subject: [PATCH 1/2] configure: add configure checks for sss_idmap and define attribute to lookup sssd If schema compat plugin configuration has 'schema-compat-lookup-sssd: user|group' then schema compat plugin will perform lookups of users/groups that were not found in the main store using getpwnam_r()/getgrnam_r() and libsss_idmap library. This is special case to support legacy clients. Schema compat plugin in the case is assumed to be running on FreeIPA master configured with trusts against Active Directory and SSSD configure as ipa_server_mode = True. Additionally, such entries are added to schema compat plugin's map cache and can be used for authentication purposes. They will use PAM authentication pass-through to system-auth service. --- configure.ac | 48 1 file changed, 48 insertions(+) diff --git a/configure.ac b/configure.ac index 8d7cbe1..085f51d 100644 --- a/configure.ac +++ b/configure.ac @@ -309,6 +309,47 @@ AC_SUBST(ASYNCNS_CFLAGS) AC_SUBST(ASYNCNS_LIBS) fi +AC_ARG_WITH(sss_nss_idmap, + AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]), + use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO) +if pkg-config sss_nss_idmap 2> /dev/null ; then + if test x$use_sss_nss_idmap != xno ; then + AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have libsss_nss_idmap.]) + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) + else + SSS_NSS_IDMAP_CFLAGS= + SSS_NSS_IDMAP_LIBS= + fi +else + if test $use_sss_idmap = yes ; then + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) + else + SSS_NSS_IDMAP_CFLAGS= + SSS_NSS_IDMAP_LIBS= + fi +fi +AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x]) +AC_SUBST(SSS_NSS_IDMAP
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Tue, Jul 16, 2013 at 03:33:49PM +0300, Alexander Bokovoy wrote: > On Tue, 16 Jul 2013, Jakub Hrozek wrote: > >On Tue, Jul 16, 2013 at 01:23:41PM +0300, Alexander Bokovoy wrote: > >>Hi! > >> > >>On Tue, 16 Jul 2013, Jakub Hrozek wrote: > +AC_ARG_WITH(sss_nss_idmap, > + AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]), > + use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO) > +if pkg-config sss_nss_idmap 2> /dev/null ; then > + if test x$use_sss_nss_idmap != xno ; then > + AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have > libsss_nss_idmap.]) > + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) > + else > + SSS_NSS_IDMAP_CFLAGS= > + SSS_NSS_IDMAP_LIBS= > + fi > +else > + if test $use_sss_idmap = yes ; then > + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) > + else > + SSS_NSS_IDMAP_CFLAGS= > + SSS_NSS_IDMAP_LIBS= > + fi > +fi > +AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x]) > +AC_SUBST(SSS_NSS_IDMAP_CFLAGS) > +AC_SUBST(SSS_NSS_IDMAP_LIBS) > + > +if x$SSS_NSS_IDMAP_LIBS != x ; then > >>> > >>>I think you should use test or square brackets here, otherwise I'm > >>>seeing: > >>> > >>>checking for SSS_NSS_IDMAP... yes > >>>./configure: line 12952: x-lsss_nss_idmap: command not found > >>Thanks, added 'test ...' > >> > >>> > + AC_CHECK_HEADERS(pam.h) > >>> > >>>I don't see any pam.h in pam-devel RPM. In SSSD we look for > >>>security/pam_appl.h > >>Right. Fixed. > >> > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -51,6 +51,7 @@ nisserver_plugin_la_LIBADD = $(LDAP_LIBS) > $(RUNTIME_LIBS) $(TIRPC_LIBS) $(LIBWRA > > schemacompat_plugin_la_SOURCES = \ > back-sch.c \ > + back-sch.h \ > >>> > >>>This file is only added in the second patch, so maybe the whole > >>>Makefile.am hunk should be moved there. > >>Yes, moved to the second patch. > >> > >>New patchset is attached. > >>-- > >>/ Alexander Bokovoy > > > >Thanks for the changes, one more comment about patch #1: > > > >>+if test x$SSS_NSS_IDMAP_LIBS != x ; then > >>+ AC_CHECK_HEADERS(security/pam_appl.h) > >>+ if test x$ac_cv_header_pam_h = xno ; then > > > >I think you need to check for ac_cv_header_security_pam_appl_h here. > >Also the condition seems reversed to me, it was complaining about > >missing PAM-devel even when I had one. > > > >After changing the test to use ac_cv_header_security_pam_appl_h the > >and reverting the condition configure seems to have worked correctly for me. > > > >Here is the git diff I made: > >diff --git a/configure.ac b/configure.ac > >index aac52e2..085f51d 100644 > >--- a/configure.ac > >+++ b/configure.ac > >@@ -334,7 +334,7 @@ AC_SUBST(SSS_NSS_IDMAP_LIBS) > > > >if test x$SSS_NSS_IDMAP_LIBS != x ; then > > AC_CHECK_HEADERS(security/pam_appl.h) > >- if test x$ac_cv_header_pam_h = xno ; then > >+ if test x$ac_cv_header_security_pam_appl_h = xyes ; then > > use_pam=yes > > else > > use_pam=no > Thanks! I was too fast :) > > New patchset is attached, resending all patches to make easier tracking. > > -- > / Alexander Bokovoy > >From 6909b33edbfb30292645f0998b60a4ffb38891dd Mon Sep 17 00:00:00 2001 > From: Alexander Bokovoy > Date: Tue, 16 Jul 2013 13:17:03 +0300 > Subject: [PATCH 1/2] configure: add configure checks for sss_idmap and define > attribute to lookup sssd The first patch looks good to me now, so Ack. Although I suspect Nalin would like to check it out as well prior to commiting it. I haven't dug deep into patch #2 yet, but user and group lookups work fine with the patch applied atop slapi-nis git master. User authentication doesn't work fine due to an issue that is being discussed separately. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
Apologies for the delay. On Mon, Jul 15, 2013 at 08:30:03PM +0300, Alexander Bokovoy wrote: > Here is the logic: > > 0. Configuration is performed by setting > >schema-compat-lookup-sssd: >schema-compat-sssd-min-id: > > in corresponding schema-compat plugin tree (cn=users and cn=groups). > > If schema-compat-sssd-min-id is not set, it will default to 1000. It is > used to filter out attempts to fetch system users (<1000 on Fedora by > default). > > 1. On query, we parse query filter to identify what type of request is > this: user or group lookup and then issue getpwnam_r()/getgrnam_r() and > getsidbyid() for libsss_nss_idmap to fetch all needed information. > > SSSD caches these requests they should be relatively fast. > > 2. Once we served the request, it is cached in schema-compat cache map. > The entry in the cache is currently not expired explicitly but I'm > working on expiring it on wrong authentication -- if PAM stack returns a > response telling there is no such user. > > 3. Authentication bind for cached entries is done via PAM service > 'system-auth'. If HBAC rule 'allow_all' is disabled in FreeIPA, one > needs to create a rule with service 'system-auth' and allow all users to > access it on IPA masters. Since system-auth is never used explicitly by > any application (it is always included through PAM stack and only > top-level PAM service is used to drive the HBAC ruleset), there is no > problem. > > PAM authentication code is taken from pam_passthru DS plugin. We cannot > use it unchanged because pam_passthru expects that LDAP entry will exist > in DS, while it is not true for these synthetic entries representing > trusted domain users. > > On Fedora one needs pam-devel and libsss_nss_idmap-devel to build the > plugin with new functionality. The bits about how to configure this facility need to be in the documentation somewhere. Right now there is none being added, and no new self-tests. > diff --git a/configure.ac b/configure.ac > index 8d7cbe1..4a47d36 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -309,6 +309,47 @@ AC_SUBST(ASYNCNS_CFLAGS) > AC_SUBST(ASYNCNS_LIBS) > fi > > +AC_ARG_WITH(sss_nss_idmap, > + AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]), > + use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO) > +if pkg-config sss_nss_idmap 2> /dev/null ; then > + if test x$use_sss_nss_idmap != xno ; then > + AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have > libsss_nss_idmap.]) > + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) > + else > + SSS_NSS_IDMAP_CFLAGS= > + SSS_NSS_IDMAP_LIBS= > + fi > +else > + if test $use_sss_idmap = yes ; then Should this reference to $use_sss_idmap be referring to $use_sss_nss_idmap instead? > + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) > + else > + SSS_NSS_IDMAP_CFLAGS= > + SSS_NSS_IDMAP_LIBS= > + fi > +fi > +AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x]) I suspect this'll need to quote SSS_NSS_IDMAP_LIBS here, in case its value ever starts to include whitespace. > +if x$SSS_NSS_IDMAP_LIBS != x ; then Likewise here. > + AC_CHECK_HEADERS(pam.h) > + if test x$ac_cv_header_pam_h = xno ; then > + use_pam=yes > + else > + use_pam=no > + fi > + > + if test $use_pam = yes ; then > + PAM_CFLAGS= > + PAM_LIBS=-lpam > + else > + AC_ERROR([ not found and it is required for SSSD mode]) > + fi > + AC_SUBST(PAM_CFLAGS) > + AC_SUBST(PAM_LIBS) > +fi Jakub already noted that this should be checking for . > @@ -401,6 +442,13 @@ > AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_RDN_ATTR,"$rdnattr", > attrattr=schema-compat-entry-attribute > AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_ATTR_ATTR,"$attrattr", > [Define to name of the attribute which is used to specify > attributes to be used when constructing entries.]) > +sssdattr=schema-compat-lookup-sssd > +AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_SSSD_ATTR,"$sssdattr", > +[Define to name of the attribute which dictates whether or > not SSSD on FreeIPA master is consulted about trusted domains' users.]) Is this a boolean attribute? > diff --git a/src/back-sch-pam.c b/src/back-sch-pam.c > new file mode 100644 > index 000..3266261 > --- /dev/null > +++ b/src/back-sch-pam.c > @@ -0,0 +1,361 @@ > +/** BEGIN COPYRIGHT BLOCK > + * This Program is free software; you can redistribute it and/or modify it > under > + * the terms of the GNU General Public License as published by the Free > Software > + * Foundation; version 2 of the License. > + * > + * This Program 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 General Public Lic
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Tue, 23 Jul 2013, Nalin Dahyabhai wrote: Apologies for the delay. Thanks for the review! One short comment -- PAM code is from PAM pass-through plugin from 389-ds. That's the reason why its code doesn't follow slapi-nis way and why it has that license. I tried to keep it mostly intact to share changes but looking at git log it gets roughly two commits per year so maybe it is better to rework it completely. I'll address other comments and will send updated version for the review today. This was my first sizable SLAPI code so errors are inevitable. On Mon, Jul 15, 2013 at 08:30:03PM +0300, Alexander Bokovoy wrote: Here is the logic: 0. Configuration is performed by setting schema-compat-lookup-sssd: schema-compat-sssd-min-id: in corresponding schema-compat plugin tree (cn=users and cn=groups). If schema-compat-sssd-min-id is not set, it will default to 1000. It is used to filter out attempts to fetch system users (<1000 on Fedora by default). 1. On query, we parse query filter to identify what type of request is this: user or group lookup and then issue getpwnam_r()/getgrnam_r() and getsidbyid() for libsss_nss_idmap to fetch all needed information. SSSD caches these requests they should be relatively fast. 2. Once we served the request, it is cached in schema-compat cache map. The entry in the cache is currently not expired explicitly but I'm working on expiring it on wrong authentication -- if PAM stack returns a response telling there is no such user. 3. Authentication bind for cached entries is done via PAM service 'system-auth'. If HBAC rule 'allow_all' is disabled in FreeIPA, one needs to create a rule with service 'system-auth' and allow all users to access it on IPA masters. Since system-auth is never used explicitly by any application (it is always included through PAM stack and only top-level PAM service is used to drive the HBAC ruleset), there is no problem. PAM authentication code is taken from pam_passthru DS plugin. We cannot use it unchanged because pam_passthru expects that LDAP entry will exist in DS, while it is not true for these synthetic entries representing trusted domain users. On Fedora one needs pam-devel and libsss_nss_idmap-devel to build the plugin with new functionality. The bits about how to configure this facility need to be in the documentation somewhere. Right now there is none being added, and no new self-tests. diff --git a/configure.ac b/configure.ac index 8d7cbe1..4a47d36 100644 --- a/configure.ac +++ b/configure.ac @@ -309,6 +309,47 @@ AC_SUBST(ASYNCNS_CFLAGS) AC_SUBST(ASYNCNS_LIBS) fi +AC_ARG_WITH(sss_nss_idmap, + AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]), + use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO) +if pkg-config sss_nss_idmap 2> /dev/null ; then + if test x$use_sss_nss_idmap != xno ; then + AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have libsss_nss_idmap.]) + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) + else + SSS_NSS_IDMAP_CFLAGS= + SSS_NSS_IDMAP_LIBS= + fi +else + if test $use_sss_idmap = yes ; then Should this reference to $use_sss_idmap be referring to $use_sss_nss_idmap instead? + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) + else + SSS_NSS_IDMAP_CFLAGS= + SSS_NSS_IDMAP_LIBS= + fi +fi +AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x]) I suspect this'll need to quote SSS_NSS_IDMAP_LIBS here, in case its value ever starts to include whitespace. +if x$SSS_NSS_IDMAP_LIBS != x ; then Likewise here. + AC_CHECK_HEADERS(pam.h) + if test x$ac_cv_header_pam_h = xno ; then + use_pam=yes + else + use_pam=no + fi + + if test $use_pam = yes ; then + PAM_CFLAGS= + PAM_LIBS=-lpam + else + AC_ERROR([ not found and it is required for SSSD mode]) + fi + AC_SUBST(PAM_CFLAGS) + AC_SUBST(PAM_LIBS) +fi Jakub already noted that this should be checking for . @@ -401,6 +442,13 @@ AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_RDN_ATTR,"$rdnattr", attrattr=schema-compat-entry-attribute AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_ATTR_ATTR,"$attrattr", [Define to name of the attribute which is used to specify attributes to be used when constructing entries.]) +sssdattr=schema-compat-lookup-sssd +AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_SSSD_ATTR,"$sssdattr", + [Define to name of the attribute which dictates whether or not SSSD on FreeIPA master is consulted about trusted domains' users.]) Is this a boolean attribute? diff --git a/src/back-sch-pam.c b/src/back-sch-pam.c new file mode 100644 index 000..3266261 --- /dev/null +++ b/src/back-sch-pam.c @@ -0,0 +1,361 @@ +/** BEGIN COPYRIGHT BLOCK + * This Program is free software; you can redistri
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Tue, Jul 23, 2013 at 10:15:47AM +0300, Alexander Bokovoy wrote:
> On Tue, 23 Jul 2013, Nalin Dahyabhai wrote:
> >Apologies for the delay.
> Thanks for the review!
>
> One short comment -- PAM code is from PAM pass-through plugin from
> 389-ds. That's the reason why its code doesn't follow slapi-nis way and
> why it has that license. I tried to keep it mostly intact to share
> changes but looking at git log it gets roughly two commits per year so
> maybe it is better to rework it completely.
That'd be my preference. Other than knowing how to map specific
PAM error codes to LDAP-level errors, there doesn't seem to be a lot of
magic that needs to be preserved in there.
> I'll address other comments and will send updated version for the
> review today. This was my first sizable SLAPI code so errors are
> inevitable.
No worries. I think there's already a lot in there that's right.
I've been thinking about the patch some more, and I need to revise a
couple of my comments.
[snip]
> >>+ slapi_entry_add_string(entry,
> >>+ "uid", user_name);
> >
> >If is_uid is true, this is a numeric string. Intentional?
Given that group entries are being constructed using group member
information, which is always login names, I guess it isn't.
[snip]
> >>+ for (i=0; grp.gr_mem[i]; i++) {
> >>+ slapi_entry_add_string(entry, "memberUid",
> >>+ slapi_ch_smprintf("uid=%s,%s", grp.gr_mem[i],
> >>sdn));
> >>+ }
> >
> >The "memberUid" attribute doesn't typically contain DNs. Did you mean
> >to use "member" here? Or to just use the user login name for the value?
This probably wants to set "memberUid" to the grp.gr_mem element's
value, because if we construct a "member" DN and expect the plugin's
configured logic to dereference it and pull out the UID value, and the
plugin attempts to read the entry with that DN by doing a search with
scope=base to find the entry, I don't think it'll trigger the logic that
would create that entry in the cache.
That, and in compat trees we're generally in the business of unrolling
group memberships anyway.
Cheers,
Nalin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
Hi Nalin, On Tue, 23 Jul 2013, Nalin Dahyabhai wrote: On Tue, Jul 23, 2013 at 10:15:47AM +0300, Alexander Bokovoy wrote: On Tue, 23 Jul 2013, Nalin Dahyabhai wrote: >Apologies for the delay. Thanks for the review! One short comment -- PAM code is from PAM pass-through plugin from 389-ds. That's the reason why its code doesn't follow slapi-nis way and why it has that license. I tried to keep it mostly intact to share changes but looking at git log it gets roughly two commits per year so maybe it is better to rework it completely. That'd be my preference. Other than knowing how to map specific PAM error codes to LDAP-level errors, there doesn't seem to be a lot of magic that needs to be preserved in there. I'll address other comments and will send updated version for the review today. This was my first sizable SLAPI code so errors are inevitable. No worries. I think there's already a lot in there that's right. I think I'm ready now with my patches. :) At http://fedorapeople.org/cgit/abbra/public_git/slapi-nis.git/log/?h=slapi-nis-ad you can find twelve patches that form support for the trusted domain users and groups lookups via SSSD and their authentication via PAM stack. The code no longer is experiencing deadlocks as any potential access that could cause contention on the slapi-nis map cache lock is eliminated. I implemented lookups to all needed functions, including working getgrouplist()/initgroups(), getgrnam_r(), getpwnam_r(), getgrgid_r(), getpwuid_r(). SIDs are also looked up through libsss_nss_idmap library provided by SSSD. Authentication is handled for both IPA and trusted domain users. The former case requires some specific handling of the SLAPI_BIND_TARGET_SDN to rewrite it to the original entry's DN. As result successful bind looks like this in the dirsrv logs: [31/Jul/2013:15:49:03 +0300] conn=15 fd=79 slot=79 SSL connection from 192.168.111.216 to 192.168.111.216 [31/Jul/2013:15:49:03 +0300] conn=15 SSL 256-bit AES [31/Jul/2013:15:49:03 +0300] conn=15 op=0 BIND dn="uid=admin,cn=users,cn=compat,dc=example,dc=com" method=128 version=3 [31/Jul/2013:15:49:03 +0300] conn=15 op=1 SRCH base="dc=example,dc=com" scope=2 filter="(uid=foobar)" attrs=ALL [31/Jul/2013:15:49:03 +0300] conn=15 op=0 RESULT err=0 tag=97 nentries=0 etime=0 dn="uid=admin,cn=users,cn=accounts,dc=example,dc=com" [31/Jul/2013:15:49:03 +0300] conn=15 op=1 RESULT err=0 tag=101 nentries=1 etime=0 [31/Jul/2013:15:49:03 +0300] conn=15 op=2 UNBIND [31/Jul/2013:15:49:03 +0300] conn=15 op=2 fd=79 closed - U1 -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Wed, Jul 31, 2013 at 03:53:21PM +0300, Alexander Bokovoy wrote: > Authentication is handled for both IPA and trusted domain users. The > former case requires some specific handling of the SLAPI_BIND_TARGET_SDN > to rewrite it to the original entry's DN. As result successful bind > looks like this in the dirsrv logs: > [31/Jul/2013:15:49:03 +0300] conn=15 fd=79 slot=79 SSL connection from > 192.168.111.216 to 192.168.111.216 > [31/Jul/2013:15:49:03 +0300] conn=15 SSL 256-bit AES > [31/Jul/2013:15:49:03 +0300] conn=15 op=0 BIND > dn="uid=admin,cn=users,cn=compat,dc=example,dc=com" method=128 version=3 > [31/Jul/2013:15:49:03 +0300] conn=15 op=1 SRCH base="dc=example,dc=com" > scope=2 filter="(uid=foobar)" attrs=ALL > [31/Jul/2013:15:49:03 +0300] conn=15 op=0 RESULT err=0 tag=97 nentries=0 > etime=0 dn="uid=admin,cn=users,cn=accounts,dc=example,dc=com" The RESULT dn being different from the BIND dn is easy to miss, but it should prevent possible problems where a given user can be bound to more than one DN, depending on where they started, so I guess it's fine. On to specific patches: HEAD~11 looks fine. HEAD~10: * Add internal whitespace when computing the value to pass to slapi_ch_malloc(). * Break the declaration and initialization of "str" into two lines. * Use '\0' to terminate str instead of 0. * In the new comment in format.h, NULL should be NUL. HEAD~9 looks fine. HEAD~8: * In configure.in, drop the hunk that changes the version number that we pass to AC_INIT. * In configure.in, one test compares x$use_sss_nss_idmap != xno, while one shortly after compares $use_sss_nss_idmap = yes. If $use_sss_nss_idmap can be empty, then the second test needs to be ready for that. * The help text still refers to SSSD specifically, when the code doesn't enforce or guarantee that SSSD's involved when performing nsswitch lookups or PAM authentication. HEAD~7: * Fix formatting of wrapped lines when calling backend_shr_get_vattr_str(). * Check for errors when converting the configured sssd_min_id to a number by switching from atol() to strtol() or strtoul(). HEAD~6: * Drop extra whitespace after the type of the "name" member when defining struct backend_search_filter_config. * backend_search_filter_has_cn_uid() allocates config->name using slapi_ch_malloc(), but it is later freed by free(). * backend_retrieve_user_entry_from_sssd(): fix line wrapping in function signature. * backend_retrieve_user_entry_from_sssd() needs to handle ERANGE errors. * backend_retrieve_user_entry_from_sssd(): avoid using slapi_entry_attr_set_int(), which will treat an unsigned 32-bit with the high bit set as a negative value * backend_retrieve_user_entry_from_sssd(): check for zero-length pw_shell value, and avoid adding it as a loginShell value, since that'd be invalid * backend_retrieve_user_entry_from_sssd() adds "ipaNTUserAttrs" as an objectClass in the temporary entry, but the value isn't copied into the cache. What purpose does it serve? * backend_retrieve_user_entry_from_sssd(): extra whitespace when constructing the new entry's DN. * backend_retrieve_user_entry_from_sssd(): memory leak from not freeing the result of slapi_escape_filter_value(). * backend_retrieve_group_entry_from_sssd(): fix line wrapping in function signature. * backend_retrieve_group_entry_from_sssd() needs to handle ERANGE errors. * backend_retrieve_group_entry_from_sssd(): extra whitespace when constructing the new entry's DN. * backend_retrieve_group_entry_from_sssd(): memory leak from not freeing the result of slapi_escape_filter_value(). * backend_retrieve_group_entry_from_sssd() adds "ipaNTGroupAttrs" as an objectClass in the temporary entry, but the value isn't copied into the cache. What purpose does it serve? * backend_retrieve_group_list_from_sssd() needs to handle ERANGE errors. * backend_retrieve_group_entry_from_sssd(): avoid using slapi_entry_attr_set_int(), which will treat an unsigned 32-bit with the high bit set as a negative value * backend_retrieve_group_list_from_sssd(): check for NULL result from realloc(). * backend_search_sssd(): fix line wrapping for call to slapi_filter_apply(). * backend_search_sssd(): use strtol() or strtoul() to convert a value to a number, and check for errors. * backend_search_sssd(): staged->container_sdn/map_group/map_set are allocated with slapi_ch_strdup() but freed with free() later. * backend_retrieve_from_sssd(): check for NULL result from malloc(). * backend_retrieve_from_sssd(): fix line wrapping for calls to backend_retrieve_user/group_entry_from_sssd. HEAD~5: * free_pam_response() uses slapi_ch_free() to free memory that was probably allocated with malloc(). * pam_conv_func(): remove extra whitespace in call to slapi_pblock_get() * pam_conv_func(): use malloc() instead of slapi_ch_calloc() to allocate replies, because plugins tend to be use free() to free them. * pam_conv_func(): replace if/else-if/else-if/else-if stacks with a switc
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
Hi Nalin! Thanks for the review. On Thu, 01 Aug 2013, Nalin Dahyabhai wrote: On Wed, Jul 31, 2013 at 03:53:21PM +0300, Alexander Bokovoy wrote: Authentication is handled for both IPA and trusted domain users. The former case requires some specific handling of the SLAPI_BIND_TARGET_SDN to rewrite it to the original entry's DN. As result successful bind looks like this in the dirsrv logs: [31/Jul/2013:15:49:03 +0300] conn=15 fd=79 slot=79 SSL connection from 192.168.111.216 to 192.168.111.216 [31/Jul/2013:15:49:03 +0300] conn=15 SSL 256-bit AES [31/Jul/2013:15:49:03 +0300] conn=15 op=0 BIND dn="uid=admin,cn=users,cn=compat,dc=example,dc=com" method=128 version=3 [31/Jul/2013:15:49:03 +0300] conn=15 op=1 SRCH base="dc=example,dc=com" scope=2 filter="(uid=foobar)" attrs=ALL [31/Jul/2013:15:49:03 +0300] conn=15 op=0 RESULT err=0 tag=97 nentries=0 etime=0 dn="uid=admin,cn=users,cn=accounts,dc=example,dc=com" The RESULT dn being different from the BIND dn is easy to miss, but it should prevent possible problems where a given user can be bound to more than one DN, depending on where they started, so I guess it's fine. Yes. Frankly, I don't see any other way to handle this authentication correctly. On to specific patches: HEAD~11 looks fine. HEAD~10: * Add internal whitespace when computing the value to pass to slapi_ch_malloc(). * Break the declaration and initialization of "str" into two lines. * Use '\0' to terminate str instead of 0. * In the new comment in format.h, NULL should be NUL. done HEAD~9 looks fine. HEAD~8: * In configure.in, drop the hunk that changes the version number that we pass to AC_INIT. * In configure.in, one test compares x$use_sss_nss_idmap != xno, while one shortly after compares $use_sss_nss_idmap = yes. If $use_sss_nss_idmap can be empty, then the second test needs to be ready for that. * The help text still refers to SSSD specifically, when the code doesn't enforce or guarantee that SSSD's involved when performing nsswitch lookups or PAM authentication. The whole setup really makes sense only when SSSD is in use. Aside from that, the whole setup is triggered only if we find libsss_nss_idmap library which is provided by SSSD. Yes, it is used for an optional adding of the SID but linking is explicit. Due to these reasons I has left the text there and added small mentioning of nsswitch interface. HEAD~7: * Fix formatting of wrapped lines when calling backend_shr_get_vattr_str(). * Check for errors when converting the configured sssd_min_id to a number by switching from atol() to strtol() or strtoul(). done. HEAD~6: * Drop extra whitespace after the type of the "name" member when defining struct backend_search_filter_config. * backend_search_filter_has_cn_uid() allocates config->name using slapi_ch_malloc(), but it is later freed by free(). * backend_retrieve_user_entry_from_sssd(): fix line wrapping in function signature. * backend_retrieve_user_entry_from_sssd() needs to handle ERANGE errors. * backend_retrieve_user_entry_from_sssd(): avoid using slapi_entry_attr_set_int(), which will treat an unsigned 32-bit with the high bit set as a negative value * backend_retrieve_user_entry_from_sssd(): check for zero-length pw_shell value, and avoid adding it as a loginShell value, since that'd be invalid all done. * backend_retrieve_user_entry_from_sssd() adds "ipaNTUserAttrs" as an objectClass in the temporary entry, but the value isn't copied into the cache. What purpose does it serve? I removed this part since we are anyway using extensibleObject for the generated entry. * backend_retrieve_user_entry_from_sssd(): extra whitespace when constructing the new entry's DN. * backend_retrieve_user_entry_from_sssd(): memory leak from not freeing the result of slapi_escape_filter_value(). * backend_retrieve_group_entry_from_sssd(): fix line wrapping in function signature. * backend_retrieve_group_entry_from_sssd() needs to handle ERANGE errors. * backend_retrieve_group_entry_from_sssd(): extra whitespace when constructing the new entry's DN. * backend_retrieve_group_entry_from_sssd(): memory leak from not freeing the result of slapi_escape_filter_value(). all done * backend_retrieve_group_entry_from_sssd() adds "ipaNTGroupAttrs" as an objectClass in the temporary entry, but the value isn't copied into the cache. What purpose does it serve? I removed this part since we are anyway using extensibleObject for the generated entry * backend_retrieve_group_list_from_sssd() needs to handle ERANGE errors. * backend_retrieve_group_entry_from_sssd(): avoid using slapi_entry_attr_set_int(), which will treat an unsigned 32-bit with the high bit set as a negative value * backend_retrieve_group_list_from_sssd(): check for NULL result from realloc(). * backend_search_sssd(): fix line wrapping for call to slapi_filter_apply(). * backend_search_sssd(): use strtol() or strtoul() to convert a value to a number, and check for errors. * backend_search_sssd(
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Fri, 02 Aug 2013, Alexander Bokovoy wrote: Hi Nalin! Thanks for the review. On Thu, 01 Aug 2013, Nalin Dahyabhai wrote: On Wed, Jul 31, 2013 at 03:53:21PM +0300, Alexander Bokovoy wrote: Authentication is handled for both IPA and trusted domain users. The former case requires some specific handling of the SLAPI_BIND_TARGET_SDN to rewrite it to the original entry's DN. As result successful bind looks like this in the dirsrv logs: [31/Jul/2013:15:49:03 +0300] conn=15 fd=79 slot=79 SSL connection from 192.168.111.216 to 192.168.111.216 [31/Jul/2013:15:49:03 +0300] conn=15 SSL 256-bit AES [31/Jul/2013:15:49:03 +0300] conn=15 op=0 BIND dn="uid=admin,cn=users,cn=compat,dc=example,dc=com" method=128 version=3 [31/Jul/2013:15:49:03 +0300] conn=15 op=1 SRCH base="dc=example,dc=com" scope=2 filter="(uid=foobar)" attrs=ALL [31/Jul/2013:15:49:03 +0300] conn=15 op=0 RESULT err=0 tag=97 nentries=0 etime=0 dn="uid=admin,cn=users,cn=accounts,dc=example,dc=com" The RESULT dn being different from the BIND dn is easy to miss, but it should prevent possible problems where a given user can be bound to more than one DN, depending on where they started, so I guess it's fine. Yes. Frankly, I don't see any other way to handle this authentication correctly. On to specific patches: HEAD~11 looks fine. HEAD~10: * Add internal whitespace when computing the value to pass to slapi_ch_malloc(). * Break the declaration and initialization of "str" into two lines. * Use '\0' to terminate str instead of 0. * In the new comment in format.h, NULL should be NUL. done HEAD~9 looks fine. HEAD~8: * In configure.in, drop the hunk that changes the version number that we pass to AC_INIT. * In configure.in, one test compares x$use_sss_nss_idmap != xno, while one shortly after compares $use_sss_nss_idmap = yes. If $use_sss_nss_idmap can be empty, then the second test needs to be ready for that. * The help text still refers to SSSD specifically, when the code doesn't enforce or guarantee that SSSD's involved when performing nsswitch lookups or PAM authentication. The whole setup really makes sense only when SSSD is in use. Aside from that, the whole setup is triggered only if we find libsss_nss_idmap library which is provided by SSSD. Yes, it is used for an optional adding of the SID but linking is explicit. Due to these reasons I has left the text there and added small mentioning of nsswitch interface. HEAD~7: * Fix formatting of wrapped lines when calling backend_shr_get_vattr_str(). * Check for errors when converting the configured sssd_min_id to a number by switching from atol() to strtol() or strtoul(). done. HEAD~6: * Drop extra whitespace after the type of the "name" member when defining struct backend_search_filter_config. * backend_search_filter_has_cn_uid() allocates config->name using slapi_ch_malloc(), but it is later freed by free(). * backend_retrieve_user_entry_from_sssd(): fix line wrapping in function signature. * backend_retrieve_user_entry_from_sssd() needs to handle ERANGE errors. * backend_retrieve_user_entry_from_sssd(): avoid using slapi_entry_attr_set_int(), which will treat an unsigned 32-bit with the high bit set as a negative value * backend_retrieve_user_entry_from_sssd(): check for zero-length pw_shell value, and avoid adding it as a loginShell value, since that'd be invalid all done. * backend_retrieve_user_entry_from_sssd() adds "ipaNTUserAttrs" as an objectClass in the temporary entry, but the value isn't copied into the cache. What purpose does it serve? I removed this part since we are anyway using extensibleObject for the generated entry. * backend_retrieve_user_entry_from_sssd(): extra whitespace when constructing the new entry's DN. * backend_retrieve_user_entry_from_sssd(): memory leak from not freeing the result of slapi_escape_filter_value(). * backend_retrieve_group_entry_from_sssd(): fix line wrapping in function signature. * backend_retrieve_group_entry_from_sssd() needs to handle ERANGE errors. * backend_retrieve_group_entry_from_sssd(): extra whitespace when constructing the new entry's DN. * backend_retrieve_group_entry_from_sssd(): memory leak from not freeing the result of slapi_escape_filter_value(). all done * backend_retrieve_group_entry_from_sssd() adds "ipaNTGroupAttrs" as an objectClass in the temporary entry, but the value isn't copied into the cache. What purpose does it serve? I removed this part since we are anyway using extensibleObject for the generated entry * backend_retrieve_group_list_from_sssd() needs to handle ERANGE errors. * backend_retrieve_group_entry_from_sssd(): avoid using slapi_entry_attr_set_int(), which will treat an unsigned 32-bit with the high bit set as a negative value * backend_retrieve_group_list_from_sssd(): check for NULL result from realloc(). * backend_search_sssd(): fix line wrapping for call to slapi_filter_apply(). * backend_search_sssd(): use strtol() or strtoul() to convert a value to a number, and check for errors. * ba
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
Crikey, that was fast. On Fri, Aug 02, 2013 at 04:44:33PM +0300, Alexander Bokovoy wrote: > >On Thu, 01 Aug 2013, Nalin Dahyabhai wrote: > >>HEAD~10: > >>* Add internal whitespace when computing the value to pass to > >>slapi_ch_malloc(). > >>* Break the declaration and initialization of "str" into two lines. > >>* Use '\0' to terminate str instead of 0. > >>* In the new comment in format.h, NULL should be NUL. > >done Ok, good. > >>HEAD~8: > >>* In configure.in, drop the hunk that changes the version number that we > >>pass to AC_INIT. > >>* In configure.in, one test compares x$use_sss_nss_idmap != xno, while > >>one shortly after compares $use_sss_nss_idmap = yes. If > >>$use_sss_nss_idmap can be empty, then the second test needs to be > >>ready for that. Good. > >>* The help text still refers to SSSD specifically, when the code doesn't > >>enforce or guarantee that SSSD's involved when performing nsswitch > >>lookups or PAM authentication. > > > >The whole setup really makes sense only when SSSD is in use. Aside from > >that, the whole setup is triggered only if we find libsss_nss_idmap > >library which is provided by SSSD. Yes, it is used for an optional > >adding of the SID but linking is explicit. Yeah, but why is all of that required? If we want to be able to gateway whatever's going on at the server, the ipaNTSecurityIdentifier lookup is already optional at runtime. Making that part optional at compile-time would make the rest of the code (at least the nsswitch parts) easier to self-test. > >>HEAD~7: > >>* Fix formatting of wrapped lines when calling backend_shr_get_vattr_str(). > >>* Check for errors when converting the configured sssd_min_id to a number > >>by switching from atol() to strtol() or strtoul(). > >done. Looks good, except that the comment in the checking block implies that it's imposing a lower limit on ret.sssd_min_id, when it's reassigning the default in case of a parsing error. If the value parses as valid, it appears that a value lower than 1000 would be accepted. > >>HEAD~6: > >>* Drop extra whitespace after the type of the "name" member when > >>defining struct backend_search_filter_config. > >>* backend_search_filter_has_cn_uid() allocates config->name using > >>slapi_ch_malloc(), but it is later freed by free(). > >>* backend_retrieve_user_entry_from_sssd(): fix line wrapping in > >>function signature. > >>* backend_retrieve_user_entry_from_sssd() needs to handle ERANGE errors. > >>* backend_retrieve_user_entry_from_sssd(): avoid using > >>slapi_entry_attr_set_int(), which will treat an unsigned 32-bit > >>with the high bit set as a negative value > >>* backend_retrieve_user_entry_from_sssd(): check for zero-length > >>pw_shell value, and avoid adding it as a loginShell value, since > >>that'd be invalid > >all done. That last one's just a NULL check now. Please add a check for a zero-length value, which would also be skipped. Otherwise, looks good. > >>* backend_retrieve_user_entry_from_sssd() adds "ipaNTUserAttrs" as an > >>objectClass in the temporary entry, but the value isn't copied into > >>the cache. What purpose does it serve? > >I removed this part since we are anyway using extensibleObject for the > >generated entry. Makes sense. > >>* backend_retrieve_user_entry_from_sssd(): extra whitespace when > >>constructing the new entry's DN. Both of these still have an extra space after the assignment operator. > >>* backend_retrieve_user_entry_from_sssd(): memory leak from not freeing > >>the result of slapi_escape_filter_value(). > >>* backend_retrieve_group_entry_from_sssd(): fix line wrapping in > >>function signature. > >>* backend_retrieve_group_entry_from_sssd() needs to handle ERANGE errors. > >>* backend_retrieve_group_entry_from_sssd(): extra whitespace when > >>constructing the new entry's DN. > >>* backend_retrieve_group_entry_from_sssd(): memory leak from not freeing > >>the result of slapi_escape_filter_value(). > >all done Otherwise, looks good. > >>* backend_retrieve_group_list_from_sssd() needs to handle ERANGE errors. > >>* backend_retrieve_group_entry_from_sssd(): avoid using > >>slapi_entry_attr_set_int(), which will treat an unsigned 32-bit > >>with the high bit set as a negative value > >>* backend_retrieve_group_list_from_sssd(): check for NULL result from > >>realloc(). Leaks are possible if realloc() fails for grouplist or entries. > >>* backend_search_sssd(): fix line wrapping for call to > >>slapi_filter_apply(). > >>* backend_search_sssd(): use strtol() or strtoul() to convert a value to > >>a number, and check for errors. > >>* backend_search_sssd(): staged->container_sdn/map_group/map_set are > >>allocated with slapi_ch_strdup() but freed with free() later. > >>* backend_retrieve_from_sssd(): check for NULL result from malloc(). > >>* backend_retrieve_from_sssd(): fix line wrapping for calls to > >>backend_retrieve_user/group_entry_from_sssd. > >all done Otherwise, looks good. > >>HEAD~5: > >>* free_pam_response() us
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Sun, 04 Aug 2013, Nalin Dahyabhai wrote: >>* The help text still refers to SSSD specifically, when the code doesn't >>enforce or guarantee that SSSD's involved when performing nsswitch >>lookups or PAM authentication. > >The whole setup really makes sense only when SSSD is in use. Aside from >that, the whole setup is triggered only if we find libsss_nss_idmap >library which is provided by SSSD. Yes, it is used for an optional >adding of the SID but linking is explicit. Yeah, but why is all of that required? If we want to be able to gateway whatever's going on at the server, the ipaNTSecurityIdentifier lookup is already optional at runtime. Making that part optional at compile-time would make the rest of the code (at least the nsswitch parts) easier to self-test. OK, fair enough. I did use of libsss_nss_idmap optional. For tests I think we need to involve nsswrapper here to make sure of a predictable testing. I've added: --with-nsswitch use nsswitch API to look up users and groupsnot found in the LDAP --with-sss-nss-idmapuse libsss_nss_idmap to discover SIDs. Requires --with-nsswitch as well --with-pam use PAM API to authenticate users not found in the LDAP. Requires --with-nsswitch as well And also split PAM use -- if --with-nsswitch is provided, you may optionally disable use of PAM. I also moved src/back-sch-sssd.c to src/back-sch-nss.c to reflect that and renamed SSSD references to NSSWITCH. >>HEAD~7: >>* Fix formatting of wrapped lines when calling backend_shr_get_vattr_str(). >>* Check for errors when converting the configured sssd_min_id to a number >>by switching from atol() to strtol() or strtoul(). >done. Looks good, except that the comment in the checking block implies that it's imposing a lower limit on ret.sssd_min_id, when it's reassigning the default in case of a parsing error. If the value parses as valid, it appears that a value lower than 1000 would be accepted. That's OK because it is a configuration option. If you want to have it set to something like 500, so be it -- not all distributions enforce 1000 as their defaults. >>HEAD~6: >>* Drop extra whitespace after the type of the "name" member when >>defining struct backend_search_filter_config. >>* backend_search_filter_has_cn_uid() allocates config->name using >>slapi_ch_malloc(), but it is later freed by free(). >>* backend_retrieve_user_entry_from_sssd(): fix line wrapping in >>function signature. >>* backend_retrieve_user_entry_from_sssd() needs to handle ERANGE errors. >>* backend_retrieve_user_entry_from_sssd(): avoid using >>slapi_entry_attr_set_int(), which will treat an unsigned 32-bit >>with the high bit set as a negative value >>* backend_retrieve_user_entry_from_sssd(): check for zero-length >>pw_shell value, and avoid adding it as a loginShell value, since >>that'd be invalid >all done. That last one's just a NULL check now. Please add a check for a zero-length value, which would also be skipped. Otherwise, looks good. Done. >>* backend_retrieve_user_entry_from_sssd(): extra whitespace when >>constructing the new entry's DN. Both of these still have an extra space after the assignment operator. Done >>* backend_retrieve_group_list_from_sssd() needs to handle ERANGE errors. >>* backend_retrieve_group_entry_from_sssd(): avoid using >>slapi_entry_attr_set_int(), which will treat an unsigned 32-bit >>with the high bit set as a negative value >>* backend_retrieve_group_list_from_sssd(): check for NULL result from >>realloc(). Leaks are possible if realloc() fails for grouplist or entries. I've used a temporary variable to realloc() into and then only change 'entries' if its value is not NULL. This should handle the case. >>HEAD~5: >>* free_pam_response() uses slapi_ch_free() to free memory that was >>probably allocated with malloc(). >>* pam_conv_func(): remove extra whitespace in call to slapi_pblock_get() >>* pam_conv_func(): use malloc() instead of slapi_ch_calloc() to allocate >>replies, because plugins tend to be use free() to free them. >>* pam_conv_func(): replace if/else-if/else-if/else-if stacks with a >>switch() statement. >>* do_pam_auth(): fix line wrapping in function signature. >>* do_pam_auth(): comments suggest that it's just entered or left a >>critical section, when the function's been called in one. >>* do_pam_auth(): replace if/else-if/else-if/else-if stacks with a >>switch() statement. >>* do_pam_auth(): fix line wrapping when calling PR_smprintf(). Missed one, right after the done: label. Done. >>* backend_search_cb() now appears to be freeing the closest-match >>as part of a conditional clause, right before it would unconditionally >>do so. >removed the duplicate. My original intent in moving that code was to >avoid freeing closest-match right before we are printing it with >slapi_log_error() and then sending the result with send_ldap_result(): >https://git
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Mon, Aug 05, 2013 at 03:45:06PM +0300, Alexander Bokovoy wrote: > OK, fair enough. I did use of libsss_nss_idmap optional. For tests I > think we need to involve nsswrapper here to make sure of a predictable > testing. > > I've added: > > --with-nsswitch use nsswitch API to look up users and groupsnot > found in the LDAP > --with-sss-nss-idmapuse libsss_nss_idmap to discover SIDs. Requires > --with-nsswitch as well > --with-pam use PAM API to authenticate users not found in the > LDAP. Requires --with-nsswitch as well > > And also split PAM use -- if --with-nsswitch is provided, you may > optionally disable use of PAM. I like where this is going, but the configure logic looks a bit off. In HEAD~9, if configure is passed --without-nsswitch, $use_nsswitch will be set to "no", so the conditional USE_NSSWITCH will be enabled. The USE_PAM conditional's based on $use_nsswitch instead of $use_pam, which looks like a copy/paste error. The subsequent call to check the contents of $USE_NSSWITCH should probably be changed to check $use_nsswitch, as automake conditionals don't set variables in configure that are named after the conditionals. Likewise for $USE_PAM and $use_pam. > I also moved src/back-sch-sssd.c to src/back-sch-nss.c to reflect that > and renamed SSSD references to NSSWITCH. Ok. Some line wrapping adjustments appear to still be needed. > HEAD~7: > * Fix formatting of wrapped lines when calling > backend_shr_get_vattr_str(). > * Check for errors when converting the configured sssd_min_id to a number > by switching from atol() to strtol() or strtoul(). > >>>done. > > > >Looks good, except that the comment in the checking block implies that > >it's imposing a lower limit on ret.sssd_min_id, when it's reassigning > >the default in case of a parsing error. If the value parses as valid, > >it appears that a value lower than 1000 would be accepted. > That's OK because it is a configuration option. If you want to have it set > to something like 500, so be it -- not all distributions enforce 1000 > as their defaults. That's fine by me. Please correct the comment to reflect the intent. > HEAD~6: > * Drop extra whitespace after the type of the "name" member when > defining struct backend_search_filter_config. > * backend_search_filter_has_cn_uid() allocates config->name using > slapi_ch_malloc(), but it is later freed by free(). > * backend_retrieve_user_entry_from_sssd(): fix line wrapping in > function signature. > * backend_retrieve_user_entry_from_sssd() needs to handle ERANGE errors. > * backend_retrieve_user_entry_from_sssd(): avoid using > slapi_entry_attr_set_int(), which will treat an unsigned 32-bit > with the high bit set as a negative value > * backend_retrieve_user_entry_from_sssd(): check for zero-length > pw_shell value, and avoid adding it as a loginShell value, since > that'd be invalid > >>>all done. > > > >That last one's just a NULL check now. Please add a check for a > >zero-length value, which would also be skipped. Otherwise, looks good. > Done. Good. One thing I just noticed: in backend_search_filter_has_cn_uid(), is there a guarantee that bval->bv_val is NUL-terminated when it's being passed to strcasecmp()? If not, it could cause problems later. > * backend_retrieve_group_list_from_sssd(): check for NULL result from > realloc(). > > > >Leaks are possible if realloc() fails for grouplist or entries. > I've used a temporary variable to realloc() into and then only change > 'entries' if its value is not NULL. This should handle the case. I see it being handled for 'entries' now, but not where it's being done for 'grouplist'. > * backend_search_cb() now appears to be freeing the closest-match > as part of a conditional clause, right before it would unconditionally > do so. > >>>removed the duplicate. My original intent in moving that code was to > >>>avoid freeing closest-match right before we are printing it with > >>>slapi_log_error() and then sending the result with send_ldap_result(): > >>>https://git.fedorahosted.org/cgit/slapi-nis.git/tree/src/back-sch.c#n1219 > >>> > >>>I can revert this part back if you wish but I think there is an error in > >>>the original code. > > > >The current intent there is to avoid sending a closest-match DN when > >we're returning entries as part of the result, to only send such a value > >as part of the result if we're sending back an LDAP_NO_SUCH_OBJECT > >error. > My main issue with this is that for the case when we found the entry we > still show the debug message with 'closest match = (null)'. I think it > is misleading at least. Oh, that's the error you were referring to. Yeah, we should fix that. > >>... and I did fix couple more comments received from Sumit: > >> > >>1. switched to use usigned long for sssd_min_id >
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Mon, 05 Aug 2013, Nalin Dahyabhai wrote:
On Mon, Aug 05, 2013 at 03:45:06PM +0300, Alexander Bokovoy wrote:
OK, fair enough. I did use of libsss_nss_idmap optional. For tests I
think we need to involve nsswrapper here to make sure of a predictable
testing.
I've added:
--with-nsswitch use nsswitch API to look up users and groupsnot
found in the LDAP
--with-sss-nss-idmapuse libsss_nss_idmap to discover SIDs. Requires
--with-nsswitch as well
--with-pam use PAM API to authenticate users not found in the
LDAP. Requires --with-nsswitch as well
And also split PAM use -- if --with-nsswitch is provided, you may
optionally disable use of PAM.
I like where this is going, but the configure logic looks a bit off.
In HEAD~9, if configure is passed --without-nsswitch, $use_nsswitch will
be set to "no", so the conditional USE_NSSWITCH will be enabled. The
USE_PAM conditional's based on $use_nsswitch instead of $use_pam, which
looks like a copy/paste error.
The subsequent call to check the contents of $USE_NSSWITCH should
probably be changed to check $use_nsswitch, as automake conditionals
don't set variables in configure that are named after the conditionals.
Likewise for $USE_PAM and $use_pam.
Thanks, fixed.
I also moved src/back-sch-sssd.c to src/back-sch-nss.c to reflect that
and renamed SSSD references to NSSWITCH.
Ok. Some line wrapping adjustments appear to still be needed.
Yes, fixed.
>Looks good, except that the comment in the checking block implies that
>it's imposing a lower limit on ret.sssd_min_id, when it's reassigning
>the default in case of a parsing error. If the value parses as valid,
>it appears that a value lower than 1000 would be accepted.
That's OK because it is a configuration option. If you want to have it set
to something like 500, so be it -- not all distributions enforce 1000
as their defaults.
That's fine by me. Please correct the comment to reflect the intent.
I've added one more sentence. Below is the full comment, reformatted by
my mail client:
/* Make sure we don't return system users/groups by limiting lower bound on
searches.
* If config value cannot be parsed or not specified, default to 1000.
* It is OK to specify something lower in the config as some
* Linux distributions force lower limit to 500 */
HEAD~6:
* Drop extra whitespace after the type of the "name" member when
defining struct backend_search_filter_config.
* backend_search_filter_has_cn_uid() allocates config->name using
slapi_ch_malloc(), but it is later freed by free().
* backend_retrieve_user_entry_from_sssd(): fix line wrapping in
function signature.
* backend_retrieve_user_entry_from_sssd() needs to handle ERANGE errors.
* backend_retrieve_user_entry_from_sssd(): avoid using
slapi_entry_attr_set_int(), which will treat an unsigned 32-bit
with the high bit set as a negative value
* backend_retrieve_user_entry_from_sssd(): check for zero-length
pw_shell value, and avoid adding it as a loginShell value, since
that'd be invalid
>>>all done.
>
>That last one's just a NULL check now. Please add a check for a
>zero-length value, which would also be skipped. Otherwise, looks good.
Done.
Good. One thing I just noticed: in backend_search_filter_has_cn_uid(),
is there a guarantee that bval->bv_val is NUL-terminated when it's being
passed to strcasecmp()? If not, it could cause problems later.
I've switched to use strncasecmp() with bval->bv_len but in reality
slapd manages it as a NUL-terminated string:
$ git grep f_avvalue
filter.c:ptr = slapi_filter_sprintf(fmt, f->f_avtype, ESC_NEXT_VAL,
f->f_avvalue.bv_val );
filter.c: if (strchr (f->f_avvalue.bv_val, '.')) {
filter.c: char*ocname = oc_find_name(
f->f_avvalue.bv_val );
filter.c:
slapi_ch_free((void**)&f->f_avvalue.bv_val );
filter.c: f->f_avvalue.bv_val =
ocname;
filter.c: f->f_avvalue.bv_len =
strlen ( f->f_avvalue.bv_val );
filter.c: *bval = &f->f_avvalue;
filter.c: if ( 0 == strcasecmp ( f->f_avvalue.bv_val,
SLAPI_ATTR_VALUE_TOMBSTONE)) {
filter.c: if ( 0 == strcasecmp ( f->f_avvalue.bv_val,
"---")) {
slap.h:#define f_avvaluef_un.f_un_ava.ava_value
str2filter.c: f->f_avvalue.bv_val = unqstr;
str2filter.c: f->f_avvalue.bv_len = len2;
str2filter.c: f->f_avvalue.bv_val = slapi_ch_strdup ( value );
str2filter.c: f->f_avvalue.bv_len = strlen ( f->f_avvalue.bv_val );
subentry.c: if ( 0 == strcasecmp ( f->f_avvalue.bv_val, "ldapsubentry")) {
Using strncasecmp() doesn't really hurt.
I've used a temporary variable to realloc() into and then only ch
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On 08/05/2013 02:45 PM, Alexander Bokovoy wrote: > On Sun, 04 Aug 2013, Nalin Dahyabhai wrote: >>> >>* The help text still refers to SSSD specifically, when the code doesn't >>> >>enforce or guarantee that SSSD's involved when performing nsswitch >>> >>lookups or PAM authentication. >>> > >>> >The whole setup really makes sense only when SSSD is in use. Aside from >>> >that, the whole setup is triggered only if we find libsss_nss_idmap >>> >library which is provided by SSSD. Yes, it is used for an optional >>> >adding of the SID but linking is explicit. >> >> Yeah, but why is all of that required? If we want to be able to gateway >> whatever's going on at the server, the ipaNTSecurityIdentifier lookup is >> already optional at runtime. Making that part optional at compile-time >> would make the rest of the code (at least the nsswitch parts) easier to >> self-test. > OK, fair enough. I did use of libsss_nss_idmap optional. For tests I > think we need to involve nsswrapper here to make sure of a predictable > testing. > > I've added: > > --with-nsswitch use nsswitch API to look up users and groupsnot > found in the LDAP > --with-sss-nss-idmapuse libsss_nss_idmap to discover SIDs. Requires > --with-nsswitch as well > --with-pam use PAM API to authenticate users not found in the > LDAP. Requires --with-nsswitch as well > > And also split PAM use -- if --with-nsswitch is provided, you may > optionally disable use of PAM. > > I also moved src/back-sch-sssd.c to src/back-sch-nss.c to reflect that > and renamed SSSD references to NSSWITCH. > >>> >>HEAD~7: >>> >>* Fix formatting of wrapped lines when calling >>> >>backend_shr_get_vattr_str(). >>> >>* Check for errors when converting the configured sssd_min_id to a number >>> >>by switching from atol() to strtol() or strtoul(). >>> >done. >> >> Looks good, except that the comment in the checking block implies that >> it's imposing a lower limit on ret.sssd_min_id, when it's reassigning >> the default in case of a parsing error. If the value parses as valid, >> it appears that a value lower than 1000 would be accepted. > That's OK because it is a configuration option. If you want to have it set > to something like 500, so be it -- not all distributions enforce 1000 > as their defaults. > >>> >>HEAD~6: >>> >>* Drop extra whitespace after the type of the "name" member when >>> >>defining struct backend_search_filter_config. >>> >>* backend_search_filter_has_cn_uid() allocates config->name using >>> >>slapi_ch_malloc(), but it is later freed by free(). >>> >>* backend_retrieve_user_entry_from_sssd(): fix line wrapping in >>> >>function signature. >>> >>* backend_retrieve_user_entry_from_sssd() needs to handle ERANGE errors. >>> >>* backend_retrieve_user_entry_from_sssd(): avoid using >>> >>slapi_entry_attr_set_int(), which will treat an unsigned 32-bit >>> >>with the high bit set as a negative value >>> >>* backend_retrieve_user_entry_from_sssd(): check for zero-length >>> >>pw_shell value, and avoid adding it as a loginShell value, since >>> >>that'd be invalid >>> >all done. >> >> That last one's just a NULL check now. Please add a check for a >> zero-length value, which would also be skipped. Otherwise, looks good. > Done. > >>> >>* backend_retrieve_user_entry_from_sssd(): extra whitespace when >>> >>constructing the new entry's DN. >> >> Both of these still have an extra space after the assignment operator. > Done > > >>> >>* backend_retrieve_group_list_from_sssd() needs to handle ERANGE errors. >>> >>* backend_retrieve_group_entry_from_sssd(): avoid using >>> >>slapi_entry_attr_set_int(), which will treat an unsigned 32-bit >>> >>with the high bit set as a negative value >>> >>* backend_retrieve_group_list_from_sssd(): check for NULL result from >>> >>realloc(). >> >> Leaks are possible if realloc() fails for grouplist or entries. > I've used a temporary variable to realloc() into and then only change > 'entries' if its value is not NULL. This should handle the case. > >>> >>HEAD~5: >>> >>* free_pam_response() uses slapi_ch_free() to free memory that was >>> >>probably allocated with malloc(). >>> >>* pam_conv_func(): remove extra whitespace in call to slapi_pblock_get() >>> >>* pam_conv_func(): use malloc() instead of slapi_ch_calloc() to allocate >>> >>replies, because plugins tend to be use free() to free them. >>> >>* pam_conv_func(): replace if/else-if/else-if/else-if stacks with a >>> >>switch() statement. >>> >>* do_pam_auth(): fix line wrapping in function signature. >>> >>* do_pam_auth(): comments suggest that it's just entered or left a >>> >>critical section, when the function's been called in one. >>> >>* do_pam_auth(): replace if/else-if/else-if/else-if stacks with a >>> >>switch() statement. >>> >>* do_pam_auth(): fix line wrapping when calling PR_smprintf(). >> >> Missed one, right after the done: label. > Done. > >>> >>* back
