[pkg] CurveDNS - review

[Lukas Schwaighofer]

Hi Stéphane,

On Wed, 28 Jun 2017 14:05:57 +0200
Stéphane Neveu <stefneveu at gmail.com> wrote:

> 2017-06-27 23:00 GMT+02:00 Lukas Schwaighofer
> <lukas at schwaighofer.name>:
> > In the postinst script you tried the following:
> >
> > DNSPUBKEY=`echo $OUTPUT | awk '/DNS public key:/ {print $4}'`
> > HEXPUBKEY=`echo $OUTPUT | awk '/Hex public key:/ {print $4}'`
> > HEXSECRETKEY=`echo $OUTPUT | awk '/Hex secret key:/ {print $4}'`
> >
> > This is a good way of extracting the information (much better than
> > what you did afterwards).  The reason why it didn't work is because
> > you need to quote "$OUTPUT", otherwise the shell performs what is
> > called word splitting.
> >
> > Please change the lines again to be similar to:
> > DNSPUBKEY=`echo "$OUTPUT" | awk '/DNS public key:/ {print $4}'`
> >  
> 
> Corrected, may I ask you to explain me why it is a better solution ?

It's better because it's more explicit (much easier for someone reading
the script to see you are indeed extracting the correct information).
Also it's less likely to break if the output of curvedns-keygen changes
slightly (e.g. keys returned in a different order).


> > [regarding the init script]
> > The environment should work fine.  Have a look at the DAEMON_ARGS…
> >  
> 
> Yes the environment is well sourced but actually "$@" is empty ($*
> also empty) in the wrapper-script, whereas echoing every variables
> works well...

The init script is, in this case, quite difficult to get right.  To
save some time I've made the required adjustments in git, I'll explain
each of my changes here:

* I've added the path to the wrapper script into a separate variable
  including a comment why we've made such a wrapper script
* lsb-base >= 3.2-14 is even part of oldoldstable, so I've dropped the
  version comment.
* We need to source the defaults file (`. /etc/default/$NAME`) so we can
  pass the command line arguments to the daemon later.  (Not needed
  when you do that in the wrapper, see below.)
* do_start: I've adjusted the command to start the daemon:
  - `--exec` needs to be passed the path to the actual daemon
    executable (the executable of the long-running process); that's
    because --exec is part of the "matching options" to look if the
    process already exists (and the wrapper will have long died)
  - added `--startas $DAEMON_WRAPPER`, which is what start-stop-daemon
    will actually execute
  - curvedns doesn't fork and doesn't create a pidfile, so we let
    start-stop-daemon do that for us (by adding `--make-pidfile` and
    `--background`); this will also take care of the Ctrl+C problem you
    had
  - pass the correct arguments (ip port remoteip remoteport) to the
    wrapper script (this is what "$@" in the wrapper will expand to,
    so they are then passed to the real daemon in the same way)
* do_stop cleanup: curvedns does not fork or create child processes,
  removing unnecessary comments and the second call to start-stop-daemon
* curvedns has no reload functionality, thus we completely remove the
  do_reload shell function
* pass the pidfile to status_of_proc now that we have it :)
* some more cleanup towards the end of the script

I have also reverted your change to the wrapper script and added "$@"
again for the commandline arguments. I think it's better to do as much
as possible in the init script itself (like passing the correct
arguments) and only do what we cannot do (setting up the environment)
in the wrapper script.


Please check each of the changes and get back to me if there's
something you do not understand.


I have a few more TODOs for you:
* debian/curvedns.postinst cleanup:
  - remove the `chmod` of the wrapper script
  - drop `set +x` once you are done debugging
* debian/curvedns.dirs: drop `/usr/lib/curvedns` (created by dh_install
  when it installs the wrapper script)
* debian/watch: "Debian" needs to be lower case (otherwise this is
  interpreted as a version and `uscan -v` reports a warning)
* enable pristine-tar in your debian/gbp.conf, download the original
  upstream tarball and commit (+push) the delta files into the
  pristine-tar branch
* debian/control: drop the version in the lsb-base dependency (even
  oldoldstable is recent enough as noted above)
* debian/copyright:
  - The curvedns software is licensed as BSD-2-clause (which you also
    specified).  However, as license text for the BSD-2-clause license,
    you have given the text of the MIT license (!).
  - I would make the Copyright line as follows:
      Copyright: 2010, CurveDNS Project.
    There are no other mentions of names/entities directly listed as
    copyright holders in any of the files…
  - The nacl software is public domain, you should add a separate
    section for nacl/* in the debian/copyright file (you can probably
    copy the license text for that from the nacl source package)
* consistent order of sourcing environment files: both curvedns.service
  and the wrapper script should take the environment files in the same
  order so we have consistent behavior between the different service
  managers.  I would suggest the following order:
    1. /var/lib/curvedns/numeric_uid_gid
    2. /etc/default/curvedns
    3. /etc/curvedns/curvedns_private_key.hex
  That allows overriding the UID/GID in /etc/default/curvedns if
  someone desires (but does not encourage them to write the secret key
  into this world readable file).



General feedback:  Reviewing your work gets easier if you do not push
your trial-and-error steps as commits.  You can do that by either
always amending the topmost commit until you are satisfied (use
`git commit --amend`) or to squash the try-and-error commits together
just before pushing.

However, be aware that you should never amend commits or squash
something that was already pushed to alioth.

Regards
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/20170628/60868e5e/attachment-0001.sig>