[pkg] CurveDNS - review

[Lukas Schwaighofer]

Hi Stéphane,

On Wed, 21 Jun 2017 13:43:43 +0200
Stéphane Neveu <stefneveu at gmail.com> wrote:
> > 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.
> > (...)
> 
> Yes, I don't know how to deal with that really. Should I ask on
> mentors mailing list ?
> I'm not sure to be able to convince the maintainer.

I did some digging and found libsodium. From the description:

  "Sodium is a portable, cross-compatible, installable, packageable
   fork of NaCl, with a compatible API"

It's packaged in Debian and is available as shared library.  Do you
think it's feasible to link curvedns against that?  A quick search
suggests that others have done that already successfully…


> > * debian/curvedns.config: don't you need to db_get in order to be
> > able to use $RET (it's commented out)?  
> 
> Suprisingly, it does not work with db_get whereas it does when
> commented out... did I missed something ? yes probably.

What do you mean with "does not work".  What is happening and what are
you expecting to happen.  Is the question being asked?  What is stored
in debconf?

Without the `db_get` your $RET will probably contain "ok" from
`db_go`.  Did you check what $RET actually contains?


Apart from that problem, I the logic of you config script still
needs some work:
* How important is it to correctly get the fqdn?
* What happens if the user does not enter an fqdn and `curvedns-keygen`
  is not run?  Does the software still function as normal?
  - if the user needs to edit /etc/default/curvedns anyways, could we
    configure the FQDN there?
* What happens if the user enters something illegal as fqdn (e.g.
  something containing spaces)?  Does curvedns-keygen exit uncleanly?
  Can we handle that case and re-prompt if need be?
  - You should quote "$RET" in the curvedns-keygen command


> > * 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  
> 
> Thank you, it's done.

It's not yet done in curvedns.postinst.

In curvedns.{postrm,prerm} your logic is now wrong.  E.g. in
curvedns.postrm you have

  if [ x"$1" != "xpurge" ]; then
    # purge commands here
  fi

but now you actually want

  if [ x"$1" = "xpurge" ]; then
    # purge commands here
  fi

You also need to negate your first if in curvedns.prerm accordingly
(and it shouldn't be indented).

> >   - debian/copyright: link to format could be https :)  
> 
> Well the tls certificate doesn't seem to be valid :/
> https://curvedns.on2it.net/

Yes, I've noticed, but the same does not apply for the "format" link:
https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
works fine.

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/20170621/17a45530/attachment-0001.sig>