[pkg] CurveDNS - review

[Stéphane Neveu]

Hi Lukas,


2017-06-27 0:28 GMT+02:00 Lukas Schwaighofer <lukas at schwaighofer.name>:
> 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!

Ok I'll try to do so, but for the chroot one, I know that chroots may
be provided directly by systemd now. So I'm not sure if it's still
useful.

>   - 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
>
Done.

> * 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.
>

Yes done. Just tell me if not enough in curvedns.README.Debian

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

Deleted.

>
> * 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)
>

Ok done.

> * 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
>

Ok done.

>   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.
>

Done.

> * 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 "$@"
>

Still not sure to understand... do you mean that my curvedns.init is
just calling that new script to start curvedns without
stop-start-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.
>

So I removed the entire env directory and now everything is saved
under /etc/curvedns (back again :)
pub key+hex key + priv key


> * 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. :)
>

OK great I'll test that asap.

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

Thank you Lukas,

Stephane