[pkg] CurveDNS - review

[Lukas Schwaighofer]

Hi Stéphane,

I've looked at your package more closely now. Suggestions follow below.

* patches:
  - chroot.patch: I see you have taken that from a pull request.  As you
    don't currently use it you should drop it; it will be added with an
    update if upstream accepts the request
  - spelling.patch: you should create a pull request to get it merged
    upstream!
  - I think libsodium.patch and makefile.patch could be merged into one
    single patch as it's a single objective: compile against libsodium
    and not against the source included nacl library

* Documentation:
  - You should document that the Debian version of curvedns is linked
    against libsodium (which is a quite, but for our case I'd say
    justified, deviation from upstream).  My suggestion would be to
    write this in the debian/curvedns.README.Debian file (which will
    then be installed to /usr/share/doc/curvedns/).
  - curvedns-keygen man page: The section "RUNNING CurveDNS" is outdated
    now that we've dropped daemontools
  - curvedns man page:  The last paragraph of the description seems
    plain wrong: the arguments are not part of /etc/default/curvedns and
    there is no logic at all regarding KEYS_GENERATED
  - Upstream's README.md and INSTALL.md should probably be installed to
    /usr/share/doc.  See dh_installdocs(1) on how to do that.

* because we dropped daemontools, /var/log/curvedns is now no longer
  used; there is no need to create that directory any more.

* adduser in postinst: create group as well
  it's free and separates it more strongly from other processes running
  as nogroup (simply add `--group` to your adduser invocation and drop
  the `--ingroup ...` part)

* UID/GID handling:
  I would propose not adding UID and GID to /etc/default/curvedns.
  These should not really be toched by the user.  Instead I would
  generate a suitable file in postinst that you can source in addition
  to /etc/default/curvedns. I'll give you an example here that shows
  how to do that with a here document (easier than printf):

        cat <<EOF >/var/lib/curvedns/numeric_uid_gid
UID=$(id -u curvedns)
GID=$(id -g curvedns)
EOF

  Then you can remove everything from postinst that writes to
  /etc/default/curvedns and instead create all this configuration
  statically in curvedns.default.  That's important as it fixes a bug:
  When calling `dpkg-reconfigure curvedns`, the whole part from postinst
  is appended once more.  Don't forget to clean up that file in postrm.

* init script (debian/curvedns.init):
  the environment variables will not be available to the executed daemon
  and there is no straight forward way I'm aware of to pass environment
  variables through start-stop-daemon.  The `env` binary can help in
  some cases, but is not powerful enough to read the variables from a
  file.  One option would be to write a wrapper script and install it to
  installed to /usr/lib/curvedns/ that does something like

        #!/bin/sh
        set -o allexport
        . /path/to/sourcefile1
        . /path/to/sourcefile2
        exec real-daemon "$@"

* debconf and FQDN
  Asking for the FQDN is wrong because
    - you're not really asking for an FQDN (I don't see upstream calling
      it that either)
    - you don't need that information to start the software, in fact you
      don't need to configure what you called FQDN at all as the
      generated key is independent from the entered name (also covered
      by the FAQ of upstream's INSTALL.md file)
  So you should completely drop the debconf logic and don't ask any
  questions during the installation.

  Instead you should simply use curvedns-keygen (without parameters) in
  postinst and parse the output.  The curvedns daemon only needs the
  Hex secret key to run (you can write that to the same file as
  previously).  However, in order to set up the zone for which curvedns
  is run as forwarder, the admin will need at least the DNS public key
  later to set up the NS record of his zone.  You need to preserve that
  information (probably a file in /etc/curvedns/ is still the best
  place, even though it's not a configuration for curvedns).

  Make sure the key cration in postinst is only performed once (i.e.  if
  the key exists it should not be overwritten).  Additionally you should
  document somewhere (e.g. in README.Debian) how to perform a key
  rollover.

* Check if the instructions for how to curvedns interacts with other DNS
  components are sufficient:
  - interaction with the authoritative nameserver
  - NS set (+ possibly glue records) installed in the parent zone; the
    DNS public key is needed for that
  In case you have not already, I'd suggest you try setting up the
  complete system to test it. dqcache is packaged in Debian, so you
  also have a DNScurve capable resolver ("recursive nameserver")
  allowing you to check if everything actually works. :)


I hope everything I wrote is understandable.  If you have any questions
or disagree with something let me know.

Good night
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/20170627/5892f113/attachment.sig>