Bug 491767

Updated SRPM URL: http://nalin.fedorapeople.org/nss-ldapd-0.6.8-0.1.src.rpm

Some quick notes:

- ldconfig does not need to be required since it's part of glibc.

- Change requirement of /sbin/chkconfig to chkconfig (package).

- .so files should be in devel package?

(In reply to comment #2)
> Some quick notes:
> 
> - ldconfig does not need to be required since it's part of glibc.

You're sure that won't get us into a situation where the package gets installed before the binary's there?
 
> - Change requirement of /sbin/chkconfig to chkconfig (package).

Ok, will do.

> - .so files should be in devel package?  

There aren't any header files, and right now it would create a conflict with nss_ldap to include it, so it's disabled.  Does it make sense to package a symlink by itself in a subpackage?

(In reply to comment #3)
> (In reply to comment #2)
> > Some quick notes:
> > 
> > - ldconfig does not need to be required since it's part of glibc.
> 
> You're sure that won't get us into a situation where the package gets installed
> before the binary's there?

Unless there is dependency loop between glibc and this package,
this must not happen (and if there is such dependency loop,
perhaps it is a bug on this package)

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Some quick notes:
> > > 
> > > - ldconfig does not need to be required since it's part of glibc.
> > 
> > You're sure that won't get us into a situation where the package gets installed
> > before the binary's there?
> 
> Unless there is dependency loop between glibc and this package,
> this must not happen (and if there is such dependency loop,
> perhaps it is a bug on this package)  

I'm afraid I don't follow.  Can you elaborate?  I was under the impression that without any explicit ordering information, RPM would be free to install them in any order.

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Some quick notes:
> > > > 
> > > > - ldconfig does not need to be required since it's part of glibc.
> > > 
> > > You're sure that won't get us into a situation where the package gets installed
> > > before the binary's there?
> > 
> > Unless there is dependency loop between glibc and this package,
> > this must not happen (and if there is such dependency loop,
> > perhaps it is a bug on this package)  
> 
> I'm afraid I don't follow.  Can you elaborate?  I was under the impression that
> without any explicit ordering information, RPM would be free to install them in
> any order.  

No, If so reviewers would already file a bug against rpm. see
(for now I cannot find more "proper" URL)
http://markmail.org/message/hwihrwnis5ikd34f
and Requires are automatically added by libraries' dependencies
detected by rpmbuild itself.

If the %post was in the form "%post -p /sbin/ldconfig", then the dependency on /sbin/ldconfig would include flags indicating that it was required for the %post script to operate correctly.  Since the script is a bit more involved, that information is lost unless we flag it correctly.  I really think it needs to be there.

So that dependency is already included in library dependency.

Which ones?  The flags that accompany auto-generated shared library deps only note that they were auto-generated.

I desperately need a respite from nss-ldap, so I'll take this.  If you have any hints on testing it properly I'd be happy to hear them.

(In reply to comment #10)
> I desperately need a respite from nss-ldap, so I'll take this.  If you have any
> hints on testing it properly I'd be happy to hear them.  

Hopefully the interesting parts of /etc/ldap.conf get pulled in automatically the first time the package gets installed.  After that, you may need to chkconfig it on and start it if system-config-authentication didn't record that you're using LDAP for naming information.

I've been remiss in not getting back to this sooner.  I did build and install it, but unfortinately after a reboot I was stuck without working uid lookups.  nslcd seemed to be running properly at the time.  So I guess there's more debugging to be done, but in the meantime I can go ahead and review the packaging.

First, therpmlint complaints:
  nss-ldapd.x86_64: W: no-version-in-last-changelog
Indeed, the most recent changelog is missing a version.  

  nss-ldapd.x86_64: E: non-readable /etc/nss-ldapd.conf 0600
This is obviously a security-related file and needs to be hidden.

  nss-ldapd.x86_64: W: missing-lsb-keyword Required-Stop in 
   /etc/rc.d/init.d/nslcd
That seems to be bogus; Required-Stop is optional.

  nss-ldapd.x86_64: W: incoherent-subsys /etc/rc.d/init.d/nslcd $prog
  nss-ldapd.x86_64: W: incoherent-subsys /etc/rc.d/init.d/nslcd $prog
  nss-ldapd.x86_64: W: incoherent-subsys /etc/rc.d/init.d/nslcd $prog
rpmlint can't comprehend most initscripts.  These are bogus.

  nss-ldapd.x86_64: W: incoherent-init-script-name nslcd
One does wonder why the daemon isn't just called "nss-ldapd", but I guess that would make sense.  Instead the daemon is "nslcd" so it does make sense for the initscript to carry that name as well.


Any reason for not just using a release of "1%{?dist}"?  This doesn't seem to be a prerelease package so your release number should be a positive integer.  If you're trying to make the initial import of the package have release 1 then it should be OK although I don't really understand why it would matter.

I'll admit to not comprehending the dependency on pam_ldap.so, but only because I don't really know what you intend to do with nss_ldap in the future.  I'm guessing that this package really doesn't have any need for pam_ldap, and you're just trying to make sure that it stays around in the case that nss-ldapd starts obsoleting nss_ldap.  I'm curious as to whether that's really necessary or if you're just doing some CYA.

There's a test suite present, but it seems to require an existing ldap server already loaded with test data and it requires the daemon to be running, which probably rules out running it at build time.

The scriptlets are pretty complex and somewhat scary.  I note that installing this prints 'USELDAP=yes' to the console, which it probably shouldn't.  I suppose the egrep call needs -q or a redirect.  Other than that, though, they seem to work well enough.  However, there are some issues with things that are supported with nss_ldap that are deprecated or not supported with nss-ldapd and I wonder if it's worth worrying about migrating them?

Starting nslcd:
nslcd: /etc/nss-ldapd.conf:130: option tls_checkpeer is deprecated (and will be removed in an upcoming release), use tls_reqcert instead
nslcd: /etc/nss-ldapd.conf:131: option tls_cacertfile is currently untested (please report any successes)

I think something's off with the groupadd stuff:
  getent group nslcd > /dev/null || /usr/sbin/groupadd -r -g 55 ldap
Shouldn't it try to add "nslcd" instead of "ldap"?  Also, why does this need to have a specific numbered account?  Shouldn't any low UID suffice?

* source files match upstream.  sha256sum:
   9e1e44a2dcce2851deb8a402a8aabc5163f2bf26f4476109b3dbab7a230a54ac  
   nss-ldapd-0.6.8.tar.gz
* package meets naming guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
   config(nss-ldapd) = 0.6.8-0.fc11.1
   libnss_ldap.so.2()(64bit)
   libnss_ldap.so.2(EXPORTED)(64bit)
   nss-ldapd = 0.6.8-0.fc11.1
   nss-ldapd(x86-64) = 0.6.8-0.fc11.1
  =
   /bin/sh
?  /lib64/security/pam_ldap.so
   /sbin/ldconfig
   chkconfig
   config(nss-ldapd) = 0.6.8-0.fc11.1
   grep
   initscripts
   ld-linux-x86-64.so.2()(64bit)
   ld-linux-x86-64.so.2(GLIBC_2.3)(64bit)
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libgcc_s.so.1(GCC_3.3.1)(64bit)
   libgssapi_krb5.so.2()(64bit)
   libgssapi_krb5.so.2(gssapi_krb5_2_MIT)(64bit)
   liblber-2.4.so.2()(64bit)
   libldap_r-2.4.so.2()(64bit)
   libsasl2.so.2()(64bit)
   sed

* shared libraries installed; ldconfig called properly.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.

(In reply to comment #12)
> First, the rpmlint complaints:
>   nss-ldapd.x86_64: W: no-version-in-last-changelog
> Indeed, the most recent changelog is missing a version.  

Fixed.

> Any reason for not just using a release of "1%{?dist}"?  This doesn't seem to
> be a prerelease package so your release number should be a positive integer. 
> If you're trying to make the initial import of the package have release 1 then
> it should be OK although I don't really understand why it would matter.

Good point.  Fixed.

> I'll admit to not comprehending the dependency on pam_ldap.so, but only because
> I don't really know what you intend to do with nss_ldap in the future.  I'm
> guessing that this package really doesn't have any need for pam_ldap, and
> you're just trying to make sure that it stays around in the case that nss-ldapd
> starts obsoleting nss_ldap.  I'm curious as to whether that's really necessary
> or if you're just doing some CYA.

Nope, that's it --  the F12 branch splits nss_ldap and pam_ldap into separate binary packages, but keeps them in the same source package (for now, at least).

> The scriptlets are pretty complex and somewhat scary.  I note that installing
> this prints 'USELDAP=yes' to the console, which it probably shouldn't.  I
> suppose the egrep call needs -q or a redirect.  Other than that, though, they
> seem to work well enough.  However, there are some issues with things that are
> supported with nss_ldap that are deprecated or not supported with nss-ldapd and
> I wonder if it's worth worrying about migrating them?

The logic's copying any options that start with tls_ or ssl wholesale, and not doing anything smart about converting or dropping specific settings.  I think that the settings which don't port over cleanly are important enough that triggering the error is, possibly perversely, a behavior worth keeping.

> Starting nslcd:
> nslcd: /etc/nss-ldapd.conf:130: option tls_checkpeer is deprecated (and will be
> removed in an upcoming release), use tls_reqcert instead
> nslcd: /etc/nss-ldapd.conf:131: option tls_cacertfile is currently untested
> (please report any successes)
> 
> I think something's off with the groupadd stuff:
>   getent group nslcd > /dev/null || /usr/sbin/groupadd -r -g 55 ldap
> Shouldn't it try to add "nslcd" instead of "ldap"?  Also, why does this need to
> have a specific numbered account?  Shouldn't any low UID suffice?

It would, but I started the ball rolling on getting a new one (see bug #491899), and while that netted a UID, we end up sharing the 'ldap' group with the openldap package.  The inconsistency was me missing half of correcting it when that happened, thanks for catching it!  Fixed.
 
> ?  /lib64/security/pam_ldap.so

Requiring pam_ldap by package name (F12) won't guarantee that you get one that matches your arch on multilib boxes, so it's being required as /%{_lib}/security/pam_ldap.so, to ensure thatwe get the pam_ldap.so that matches the arch of the libnss_ldap.so.2 that this binary package is supplying.  We've had weird problems both composing trees and with things not always working with multilib and plugins (I'm thinking of cyrus-sasl, but PAM modules have similar problems), and I'm trying to avoid that.

Spec updated, new source package built using it.

Thanks!

Short on time today, but one quick comment:

> Requiring pam_ldap by package name (F12) won't guarantee that you get one that
> matches your arch on multilib boxes, so it's being required as
> /%{_lib}/security/pam_ldap.so, to ensure thatwe get the pam_ldap.so that
> matches the arch of the libnss_ldap.so.2 that this binary package is supplying.

There's a different way to do this in F11 and perhaps F10 as well: %{_isa}.  You'll see packages providing things like "nss_ldap(x86-64) = 264-2.fc11" now, so you can have a dependency like
  Requires: nss_ldap%{_isa}
and it should do the right thing.  I've no idea whether it makes any difference for what you're trying to do, though.

(In reply to comment #14)
> Short on time today, but one quick comment:
> 
> There's a different way to do this in F11 and perhaps F10 as well: %{_isa}. 
> You'll see packages providing things like "nss_ldap(x86-64) = 264-2.fc11" now,
> so you can have a dependency like
>   Requires: nss_ldap%{_isa}
> and it should do the right thing.  I've no idea whether it makes any difference
> for what you're trying to do, though.  

Hmm, on my devel box, I do seem to be seeing an autoprovides on %{name}%{_isa} in other packages, which means it should work.  So yes, that would probably be better.

Well, I figured out that my problems getting this to work simply go away with 'setenforce 0'.  Here are the complaints I see while running in permissive mode:

type=1400 audit(1240256724.128:55): avc:  denied  { write } for  pid=1712 comm="nscd" name="socket" dev=dm-4 ino=409614 scontext=system_u:system_r:nscd_t:s0 tcontext=system_u:object_r:var_run_t:s0 tclass=sock_file

type=1400 audit(1240256724.134:56): avc:  denied  { connectto } for  pid=1712 comm="nscd" path="/var/run/nslcd/socket" scontext=system_u:system_r:nscd_t:s0 tcontext=system_u:system_r:initrc_t:s0 tclass=unix_stream_socket

The daemon started fine, but it seems that nothing could talk to it.  I guess some policy tweaks will be needed before this gets to the point of being useful.

BTW, does Simo know you're packaging this for inclusion?  I thought SSSD was supposed to do the same thing in a different way.

(In reply to comment #16)
> Well, I figured out that my problems getting this to work simply go away with
> 'setenforce 0'.  Here are the complaints I see while running in permissive
> mode:
> 
> type=1400 audit(1240256724.128:55): avc:  denied  { write } for  pid=1712
> comm="nscd" name="socket" dev=dm-4 ino=409614
> scontext=system_u:system_r:nscd_t:s0 tcontext=system_u:object_r:var_run_t:s0
> tclass=sock_file
> 
> type=1400 audit(1240256724.134:56): avc:  denied  { connectto } for  pid=1712
> comm="nscd" path="/var/run/nslcd/socket" scontext=system_u:system_r:nscd_t:s0
> tcontext=system_u:system_r:initrc_t:s0 tclass=unix_stream_socket
> 
> The daemon started fine, but it seems that nothing could talk to it.  I guess
> some policy tweaks will be needed before this gets to the point of being
> useful.

Well, it can't talk to nscd, and nscd can't talk to it.  I'm having trouble reproducing the case where this causes things to fail completely, but temporarily stopping nscd should take these out of the picture.  (Until we get a policy for it, the daemon's running as initrc_t, which is effectively unconfined, so it shouldn't have difficulties itself.  BTW which policy version do you have installed?)

> BTW, does Simo know you're packaging this for inclusion?  I thought SSSD was
> supposed to do the same thing in a different way.  

I'm pretty sure, yes.  It's pretty clear that SSSD won't replace nss_ldap or its successors for 100% of cases.

Hmm, actually, nslcd can use nscd, but not the other way around.

I'm running whatever's in rawhide currently:
  selinux-policy-3.6.12-4.fc11.noarch
  selinux-policy-targeted-3.6.12-4.fc11.noarch

Well, at least I've found that the caching works across reboots.  After logging in with setenforce 0, I can reboot the machine (which resets selinux to enforcing) and still log in.  But I can't resolve any other users.

And indeed, stopping nscd does get things working, but of course nscd caches more than uid/gid lookups.

BTW, do you know if this will cache autofs lookups as well?

Finally, to packaging issues: You fixed the minor issues I had; personally I dont' care one way or the other about the /lib64/security/pam_ldap.so dependency.  However, one issue concerns me:
  /lib64/libnss_ldap.so.2
  /usr/lib64/libnss_ldap-264.so
  /usr/lib64/libnss_ldap.so
  /usr/lib64/libnss_ldap.so.2
This brings up a couple of issues:

Does the library really need to live in the root directory?  Generally we try not to install things there unless they're absolutely required that early in the boot process (or for recovery).  I know it conveniently avoids a conflict in this case, but I'm wondering if it's just done that way because of the conflict or if there's another reason.

Unfortunately now I'm seeing other strange behavior.  I found that, even though I'm currently logged in, a second login fails unless I start nscd.  But that causes things like sudo to complain that I'm not in the passwd file.  Only running setenforce 0 gets things working properly.

(In reply to comment #19)
> I'm running whatever's in rawhide currently:
>   selinux-policy-3.6.12-4.fc11.noarch
>   selinux-policy-targeted-3.6.12-4.fc11.noarch
> 
> Well, at least I've found that the caching works across reboots.  After logging
> in with setenforce 0, I can reboot the machine (which resets selinux to
> enforcing) and still log in.  But I can't resolve any other users.

Yeah, we're going to have to get that sorted before it gets into any repositories.
 
> And indeed, stopping nscd does get things working, but of course nscd caches
> more than uid/gid lookups.

> BTW, do you know if this will cache autofs lookups as well?

It does not; autofs doesn't use libc's nsswitch subsystem to look up automount information (there's no interface for one, and for such an interface to be useful right away you'd have to add frontend the functions to libc, and then a backend for it to the various nsswitch modules, and by doing so duplicate a lot of code that you need to keep in the automounter until the older versions of libc that don't provide it go away, so in the end you haven't gained much).

> Finally, to packaging issues: You fixed the minor issues I had; personally I
> don't care one way or the other about the /lib64/security/pam_ldap.so
> dependency.  However, one issue concerns me:
>   /lib64/libnss_ldap.so.2
>   /usr/lib64/libnss_ldap-264.so
>   /usr/lib64/libnss_ldap.so
>   /usr/lib64/libnss_ldap.so.2
> This brings up a couple of issues:
> 
> Does the library really need to live in the root directory?  Generally we try
> not to install things there unless they're absolutely required that early in
> the boot process (or for recovery).  I know it conveniently avoids a conflict
> in this case, but I'm wondering if it's just done that way because of the
> conflict or if there's another reason.  

It doesn't need to live in /%{_lib}, but by convention nsswitch modules, following largely from what glibc does with its own modules, have been put there anyway.  In practice, any location in the shared library path will do.  The main reason to move one elsewhere is admitting that it has dependencies that will never be satisfiable without a working /usr.

In this case, though, it avoids having to deal with file conflicts or working something out with alternatives (which I actually tried doing, but trying to select the "right" one without requiring manual intervention didn't lend itself to any elegant solutions).

> It does not; autofs doesn't use libc's nsswitch subsystem to look up automount
> information

I guess the "automount:" line in nsswitch.conf is confusing.  Lack of caching of autofs lookups is the other half of the problem (or at least my problem).  Completely off topic here, though.

> It doesn't need to live in /%{_lib}, but by convention nsswitch modules,
> following largely from what glibc does with its own modules, have been put
> there anyway.

Oh, good point.  I guess nss_ldap is the one that's out of sorts, placing its libraries under /usr/lib instead.

> In this case, though, it avoids having to deal with file conflicts or working
> something out with alternatives (which I actually tried doing, but trying to
> select the "right" one without requiring manual intervention didn't lend itself
> to any elegant solutions).  

Indeed, I can't imagine how you would do this with alternatives.

So where do we go from here?  I think that from a packaging standpoint this is good, but without support from the selinux policy it's not as useful and the interactions with nscd are problematic (although it seems that at least some of the problems I'm seeing are due to nscd's negative caching).

(In reply to comment #22)
> > It doesn't need to live in /%{_lib}, but by convention nsswitch modules,
> > following largely from what glibc does with its own modules, have been put
> > there anyway.
> 
> Oh, good point.  I guess nss_ldap is the one that's out of sorts, placing its
> libraries under /usr/lib instead.

That's as much about not being able to link with various static libraries any longer, and the shared versions of those libraries living in /usr, as anything else.

> > In this case, though, it avoids having to deal with file conflicts or working
> > something out with alternatives (which I actually tried doing, but trying to
> > select the "right" one without requiring manual intervention didn't lend itself
> > to any elegant solutions).  
> 
> Indeed, I can't imagine how you would do this with alternatives.

Well, the idea was to hook runlevel changes (you can do that sort of thing with upstart, at least I thought you could) and call alternatives to select one or the other depending on whether the daemon was enabled at all for any runlevel, not that it worked right.

> So where do we go from here?  I think that from a packaging standpoint this is
> good, but without support from the selinux policy it's not as useful and the
> interactions with nscd are problematic (although it seems that at least some of
> the problems I'm seeing are due to nscd's negative caching).  

If you can leave aside the no-policy-for-it problem while the rest of the packaging review continues, I can take a first stab at getting a policy together and then impose on dwalsh to work on fixing it.  I'm okay with leaving this bug open until the policy's sorted out.

Unfortunately I completely ran out of time to do any reviewing for some time.  Getting back to this, I see that the only real issues I had with this past comment #13 were in actually getting it working (which cleared up with selinux turned off).  I've been running it on a couple of test servers, and while I haven't really tested it under failure conditions it at least seems to work OK.

Can you simply add a note in %description or in a README.Fedora file or something which warns people not to try this with selinux?  There's no hard requirement that selinux be supported before a package is imported, though with something that has the potential to lock people out of their machines I really think it's worth making a prominent warning.

Created attachment 348333 [details]
policy changes

I think that with the policy changes (exercising Dan's polgen.py along the way), I think we'll have it sorted soon.  Ok to get going on CVS?

I didn't propose waiting for policy; I only suggested that the selinux issues be documented so that people (or at least those who read documentation) don't inadvertently hose their systems.  I still don't see why you wouldn't want to do that, but I suppose if the policy issues are being worked out then there's no reason not to push things forward.

APPROVED

Did you intend to make a CVS request?  http://fedoraproject.org/wiki/CVS_admin_requests

(In reply to comment #28)
> Did you intend to make a CVS request? 

Actually, yeah.

New Package CVS Request
=======================
Package Name: nss-ldapd
Short Description: An nsswitch module which uses directory servers
Owners: nalin
Branches: devel
InitialCC:

CVS done.

This is built and in rawhide, so  I see no reason for this ticket to stay open.