[Bug 491767] Review Request: nss-ldapd - An nsswitch module which uses directory servers

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=491767





--- Comment #12 from Jason Tibbitts <tibbs at math.uh.edu>  2009-04-16 15:37:44 EDT ---
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.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.