[pkg] CurveDNS - review

[Lukas Schwaighofer]

Hi Stéphane,

On Tue, 20 Jun 2017 13:58:20 +0200
Stéphane Neveu <stefneveu at gmail.com> wrote:
> May I ask you to review my first package : CurveDNS ?
> It's on alioth...

Thanks for your work!  I had a look at your package. Disclaimer: I'm a
fairly new contributor myself, doing a review mostly to learn something
myself and hopefully take some load off the DDs.


I noticed the source includes (and compiles+uses) the nacl library.
While that library is already packaged in Debian, it's unfortunately
not available as shared object.

I don't really know how to proceed here, none of our options look 
particularly promising:
* Keep the copy of the nacl library: this means that in case of security
  fixes in the nacl library, these need to be applied to your package 
  separately.
* Build-Depend on libnacl-dev and use the provided `libnacl.a` for
  linking:  This means that after a security update your package "only" 
  needs to be recompiled.  But an incompatible ABI change in libnacl
  might break your package without warning…
* Convince the maintainer of nacl to provide it as shared library 
  (upstream does not support this, so this may involve a lot of work and
  may not be doable): This would be what we want, security patches can 
  affect you directly and ABI compatibility can be handled somewhat 
  gracefully.

I hope someone here can provide us with guidance what to do.


Some possible problems I've noticed when going through the files:
* To what extend does the software need daemontools?  Can't systemd or 
  sysvinit run it also?  This part creates a lot of complexity (also in 
  the maintainer scripts) and I would like to know if we really need
  this.
  I also don't understand why we need to start multilog in ExecStartPost
  in the systemd unit file (not present in the init script).  Can't we 
  let curvedns log to journal?  Then we can also drop the whole
  envuidgid stuff and select the correct user in the unit file directly.
* I think with your hardening_support.patch, only the CPPFLAGS from
  `dpkg-buildflags` are applied and not the CFLAGS or LDFLAGS…
* debian/curvedns.config: don't you need to db_get in order to be able
  to use $RET (it's commented out)?
* debian/curvedns.{postrm,prerm}: you should not do something like

      if not_my_phase; then
          exit 0
      fi
      #commands here

  because that will prevent #DEBHELPER# later to execute what it may 
  need to in other phases.  Instead do something like:

      if my_phase; then
          #commands here
      fi

* Suggestions for "beautification":
  - debian/control: make whitespace consistent, don't mix tabs and
    spaces
  - debian/copyright: link to format could be https :)
  - debian/{dirs,install,manpages}: rename to debian/curvedns.{...} for
    consistency
  - permissions in the repository for the maintainer scripts are
    inconsistent (only some of them have execute permission)


Have a nice evening
Lukas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-security-team/attachments/20170620/78480225/attachment.sig>