[pkg] CurveDNS - review
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
Reply to: