Re: RFS: zkt -- A tool to manage keys and signatures for DNSSEC-zones
On 2013-03-11 09:57 , Paul Wise wrote:
> Here is a review:
Thanks, very useful stuff!
Various things where already fixed, others have been fixed. See inline
for the specifics and the major question of maybe splitting the repo
into a 'fixes' and 'debian' one, where the latter is just the debian
package and has the patches from the former against the upstream release.
1.1.2g should be up on mentors in a few moments, it is uploaded, just
needs to be processed there.
> You seem to have sent this RFS bug to debian-mentors instead of
> submit@bugs.d.o, please read this:
That was done because there is an existing bug[1] and I wanted to avoid
creating a new bug, which then had to be duped etc.
[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=481898
> http://wiki.debian.org/Mentors/BTS
>
> I would suggest that a non-native package would be more appropriate
> here, please change debian/source/format to 3.0 (quilt) and change the
> version number to 1.1.2c-1
The official/original upstream is at http://www.hznet.de/dns/zkt/
and that is 1.1.2.
The 1.1.2 has various issues which are resolved in my git repo, from
which the package is currently built. Hence the letters for showing the
difference from the upstream as they are not 'official' releases.
But from the point of view of the repo, it is a native version of the
package.
If your argument is "you should supply the fixes as quilt patches", then
my question becomes: How would one go at best keeping the changed
version in a git repo, so that it can easily be tracked what those
changes are (hence git) and how they are applied and keeping the patches
in sync with the ones that need to be extracted? I guess it would result
in two git repos, one for the 'patched version' and one for the Debian
package.
I think that it is less visible what has been changed if I produce a
diff against the release version, and include that as a quilt. Though,
actually thinking about it, it should be doable by using format-patch.
Should I go the way of two repos, one for zkt, the current one, and
split off the Debian parts into a separate one and put the patches in there?
> The upstream build system should probably install the manual pages,
> please ask upstream about that.
But the Debian way is easy, sleek and very clear about what happens...
If changing it, at the moment one would have to call 'make install-man'
to get them, which requires a dh_override, or is there a nicer way?
Currently I don't install any 'docs' either, I'll see which ones to pick
for that.
> The comments in debian/rules are not needed.
Done:
https://github.com/massar/zkt/commit/aee92e1fa8f31bba284fb22ca2a3ef345906199e
https://github.com/massar/zkt/commit/d40718c65a0e5b985568bebae3b0cc9477ae5b7f
> No need for both --with autotools_dev and --with autoreconf in
> debian/rules, please drop autotools_dev.
Done.
https://github.com/massar/zkt/commit/de9db92c74aed52e2f75ce1898db22a3c628bc8a
> The Homepage link is broken.
Already resolved in 1.1.2e (note that there is a 1.1.2f too and now a
1.1.2g)
> Standards-Version is out of date, please check the upgrading list and
> update anything needed.
Also updated already:
https://github.com/massar/zkt/commit/2e4df8f05c83cf1acacc4d2068e349663173660a
> http://www.debian.org/doc/debian-policy/upgrading-checklist
>
> I would suggest running wrap-and-sort -sa.
Over which files / with which tool?
> The source package is missing a watch file.
Of course, this as it is, at the moment, a native package.
Lintian would complain if it was not. If we change it to a quilt'ed
package then the watch could simply be the SF style one.
> There are some RFCs in doc/ and RFCs tend to be non-free, please
> remove them from the source package. Even if they were free, they
> should be packaged separately if at all.
The source package is the original upstream tarball, that is why they
are included. These would not be there for a quilted version.
> doc/KeyRollover.ps is a generated file, please ensure that it is
> recreated at build time.
Thanks for noticing this, it will be gone from the repo even then.
> The debian/copyright file doesn't properly document all of the
> copyright holders.
I think it does, but I also think you where looking at an older version.
Indeed the copyright owners of the RFC/drafts are not included, but
these might not appear there anyway if we go the quilt route.
> soaserial.c contains an unsafe use of system() inside the #ifdef
> SOA_TEST, the popen calls don't look nice either.
The system call would be bad indeed, it is just testing code, hence the
ifdef SOA_TEST that is never defined and thus never used, just there for
testing. As such that should be fine IMHO.
While the popen calls indeed "don't look nice" they don't seem harmful
to me and actually are part of the core functionality of the code.
They properly use snprintf to do some minor length checking, thus the
primary thing that is missing there is the checking of the return codes.
The code would need a big rewrite to support passing back error codes in
the cases it goes wrong, I think that is out of scope for packaging for
the time being as the code would divert a lot from upstream. This is
IMHO thus something to be fixed there. That is unless we stick to the
'native' packaging.
Security wise there is no big issue though as no elevation of privileges
is possible as the code is all run as the same user.
If one is able to run this tool, and thus has access to your DNSSEC keys
and update the DNS files it writes, you have bigger problems already...
In the case that the snprintf()'s actually fail silently as the strings
printed are too long (which is likely a rarish case with the larger
static buffers used) the commands being executed would fail, which would
signal errors and thus it would become visible.
> Automatic checks:
>
> http://wiki.debian.org/HowToPackageForDebian#Check_points_for_any_package
Why is that long list of checks not in Lintian? ;)
Very useful tests!
> gcc:
>
> tcap.c:266:12: warning: 'tc_printattr' defined but not used [-Wunused-function]
> tcap.c:282:12: warning: 'tc_color' defined but not used [-Wunused-function]
> tcap.c:254:12: warning: 'put' defined but not used [-Wunused-function]
Interesting, I did not get these. The dependency was for ncurses was
missing. Resolved.
https://github.com/massar/zkt/commit/d7213309c4b35cf02232696d334155a676b63b60
That is what one gets for not having an empty buildroot with minimal
packages.
> lintian:
>
> W: zkt source: package-needs-versioned-debhelper-build-depends 9
> W: zkt source: missing-license-paragraph-in-dep5-copyright
> bsd-3-clause (paragraph at line 6)
> W: zkt source: out-of-date-standards-version 3.9.3 (current is 3.9.4)
> W: zkt: latest-debian-changelog-entry-without-new-date
> I: zkt: extended-description-is-probably-too-short
These are all fixed already in the 1.1.2e that is on mentors.
> cme:
>
> Configuration item 'Files:"*" License short_name' has a wrong value:
> license BSD-3-clause is not declared in main License section. Expected
As above, already fixed in 1.1.2e
> blhc:
>
> CFLAGS missing (-fPIE)
> LDFLAGS missing (-fPIE -pie -Wl,-z,now)
Would it not be debhelper's job to add these?
Note that debhelper 9 is used and that should enable all the hardening
options for that architecture.
> cppcheck:
>
> [dki.c:73]: (error) Memory leak: dkp
Fixed:
https://github.com/massar/zkt/commit/482f779c8c260fd0be27d0fa8d11409ce4d2726a
> [zkt-soaserial.c:123]: (error) Width 255 given in format string (no.
> 1) is larger than destination buffer 'master[255]', use %254s to
> prevent overflowing it.
> [zkt-soaserial.c:125]: (error) Width 255 given in format string (no.
> 1) is larger than destination buffer 'master[255]', use %254s to
> prevent overflowing it.
https://github.com/massar/zkt/commit/3af5f7f92b9ab8db3b6b6f4a6f284f98b31d0b22
> [zkt-soaserial.c:130]: (error) Resource leak: fp
https://github.com/massar/zkt/commit/a315b1d0453c648206924a77fa1d0daad1bcd59f
> bfbtester:
>
> zkt-conf crashes a lot under bfbtester, please look at this.
It calls abort() on line 235, which looks like a segfault.
Resolved by showing the unknown option instead:
https://github.com/massar/zkt/commit/531aec434a776676dec5c3ba7198310856ae4cc8
> checkbashisms:
>
> script ./distribute.sh does not appear to have a #! interpreter line;
> you may get strange results
> script ./examples/flat/dist.sh does not appear to have a #! interpreter line;
> you may get strange results
> script ./examples/views/viewtest.sh does not appear to have a #!
> interpreter line;
> you may get strange results
https://github.com/massar/zkt/commit/1611429f89c902e348e8fe0c7a09164ce8508521
> fdupes:
>
> ./examples/views/intern/example.net/keyset-example.net.
> ./examples/views/intern/keyset-example.net.
>
> ./examples/views/extern/keyset-example.net.
> ./examples/views/extern/example.net/keyset-example.net.
>
> ./examples/hierarchical/de/example.de/sub.example.de/parent-sub.example.de.
> ./examples/hierarchical/de/example.de/keyset-sub.example.de.
>
> ./examples/hierarchical/de/keyset-example.de.
> ./examples/hierarchical/de/example.de/keyset-example.de.
Well, they are examples that is why the might be similar.
Thanks for the comments, hoping to receive more.
Greets,
Jeroen
Reply to: